Jeff Moyer
2019-Apr-12 13:12 UTC
[PATCH v5 1/6] libnvdimm: nd_region flush callback support
Jan Kara <jack at suse.cz> writes:> On Thu 11-04-19 07:51:48, Dan Williams wrote: >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta at redhat.com> wrote: >> > + } else { >> > + if (nd_region->flush(nd_region)) >> > + rc = -EIO; >> >> Given the common case wants to be fast and synchronous I think we >> should try to avoid retpoline overhead by default. So something like >> this: >> >> if (nd_region->flush == generic_nvdimm_flush) >> rc = generic_nvdimm_flush(...); > > 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. -Jeff
Pankaj Gupta
2019-Apr-18 06:27 UTC
[Qemu-devel] [PATCH v5 1/6] libnvdimm: nd_region flush callback support
Hello, Thank you for the suggestions on this.> > > On Thu 11-04-19 07:51:48, Dan Williams wrote: > >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta at redhat.com> wrote: > >> > + } else { > >> > + if (nd_region->flush(nd_region)) > >> > + rc = -EIO; > >> > >> Given the common case wants to be fast and synchronous I think we > >> should try to avoid retpoline overhead by default. So something like > >> this: > >> > >> if (nd_region->flush == generic_nvdimm_flush) > >> rc = generic_nvdimm_flush(...); > > > > 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.Can we add the optimization after this version is merged. Best regards, Pankaj> > -Jeff > >
Dan Williams
2019-Apr-18 16:05 UTC
[PATCH v5 1/6] libnvdimm: nd_region flush callback support
On Fri, Apr 12, 2019 at 6:12 AM Jeff Moyer <jmoyer at redhat.com> wrote:> > Jan Kara <jack at suse.cz> writes: > > > On Thu 11-04-19 07:51:48, Dan Williams wrote: > >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta at redhat.com> wrote: > >> > + } else { > >> > + if (nd_region->flush(nd_region)) > >> > + rc = -EIO; > >> > >> Given the common case wants to be fast and synchronous I think we > >> should try to avoid retpoline overhead by default. So something like > >> this: > >> > >> if (nd_region->flush == generic_nvdimm_flush) > >> rc = generic_nvdimm_flush(...); > > > > 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/
Jeff Moyer
2019-Apr-18 16:10 UTC
[PATCH v5 1/6] libnvdimm: nd_region flush callback support
Dan Williams <dan.j.williams at intel.com> writes:> On Fri, Apr 12, 2019 at 6:12 AM Jeff Moyer <jmoyer at redhat.com> wrote: >> >> Jan Kara <jack at suse.cz> writes: >> >> > On Thu 11-04-19 07:51:48, Dan Williams wrote: >> >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta at redhat.com> wrote: >> >> > + } else { >> >> > + if (nd_region->flush(nd_region)) >> >> > + rc = -EIO; >> >> >> >> Given the common case wants to be fast and synchronous I think we >> >> should try to avoid retpoline overhead by default. So something like >> >> this: >> >> >> >> if (nd_region->flush == generic_nvdimm_flush) >> >> rc = generic_nvdimm_flush(...); >> > >> > 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/The technique is fine, but that doesn't mean it should be applied everywhere. Is *this* code path really going to benefit from the optimization? -Jeff
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.
Possibly Parallel 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