Christoph Hellwig
2019-Apr-18 16:18 UTC
[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.
Dan Williams
2019-Apr-18 18:14 UTC
[PATCH v5 1/6] libnvdimm: nd_region flush callback support
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.
Jeff Moyer
2019-Apr-22 15:51 UTC
[PATCH v5 1/6] libnvdimm: nd_region flush callback support
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
Maybe Matching Threads
- [PATCH v5 1/6] libnvdimm: nd_region flush callback support
- [PATCH v5 1/6] libnvdimm: nd_region flush callback support
- [PATCH v5 1/6] libnvdimm: nd_region flush callback support
- [PATCH v5 1/6] libnvdimm: nd_region flush callback support
- [PATCH v5 1/6] libnvdimm: nd_region flush callback support