Linux Virtualization - Apr 2019 - [PATCH v5 1/6] libnvdimm: nd_region flush callback support

On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams
wrote:
> > > I'd either add a comment about avoiding retpoline overhead
here or just
> > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that
people don't
> > > get confused by the code.
> >
> > Isn't this premature optimization?  I really don't like adding
things
> > like this without some numbers to show it's worth it.
> 
> I don't think it's premature given this optimization technique is
> already being deployed elsewhere, see:
> 
> https://lwn.net/Articles/774347/
For one this one was backed by numbers, and second after feedback
from Linux we switched to the NULL pointer check instead.

On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch at infradead.org>
wrote:
>
> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
> > > > I'd either add a comment about avoiding retpoline
overhead here or just
> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so
that people don't
> > > > get confused by the code.
> > >
> > > Isn't this premature optimization?  I really don't like
adding things
> > > like this without some numbers to show it's worth it.
> >
> > I don't think it's premature given this optimization technique
is
> > already being deployed elsewhere, see:
> >
> > https://lwn.net/Articles/774347/
>
> For one this one was backed by numbers, and second after feedback
> from Linux we switched to the NULL pointer check instead.
Ok I should have noticed the switch to NULL pointer check. However,
the question still stands do we want everyone to run numbers to
justify this optimization, or make it a new common kernel coding
practice to do:

    if (!object->op)
        generic_op(object);
    else
        object->op(object);

...in hot paths? I agree with not doing premature optimization in
principle, but this hack is minimally intrusive from a readability
perspective similar to likely()/unlikely() usage which also don't come
with numbers on a per patch basis.

Dan Williams <dan.j.williams at intel.com> writes:
> On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch at
infradead.org> wrote:
>>
>> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
>> > > > I'd either add a comment about avoiding retpoline
overhead here or just
>> > > > make ->flush == NULL mean generic_nvdimm_flush().
Just so that people don't
>> > > > get confused by the code.
>> > >
>> > > Isn't this premature optimization?  I really don't
like adding things
>> > > like this without some numbers to show it's worth it.
>> >
>> > I don't think it's premature given this optimization
technique is
>> > already being deployed elsewhere, see:
>> >
>> > https://lwn.net/Articles/774347/
>>
>> For one this one was backed by numbers, and second after feedback
>> from Linux we switched to the NULL pointer check instead.
>
> Ok I should have noticed the switch to NULL pointer check. However,
> the question still stands do we want everyone to run numbers to
> justify this optimization, or make it a new common kernel coding
> practice to do:
>
>     if (!object->op)
>         generic_op(object);
>     else
>         object->op(object);
>
> ...in hot paths?
I don't think nvdimm_flush is a hot path.  Numbers of some
representative workload would prove one of us right.

-Jeff