* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers [not found] ` <20181220180651.4879-2-ntsironis@arrikto.com> @ 2019-02-28 21:32 ` Mike Snitzer 2019-02-28 21:34 ` Mike Snitzer ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Mike Snitzer @ 2019-02-28 21:32 UTC (permalink / raw) To: Paul E. McKenney, hch, Nikos Tsironis Cc: agk, dm-devel, mpatocka, iliastsi, linux-kernel On Thu, Dec 20 2018 at 1:06pm -0500, Nikos Tsironis <ntsironis@arrikto.com> wrote: > Add hlist_bl_add_before/behind helpers to add an element before/after an > existing element in a bl_list. > > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> > --- > include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h > index 3fc2cc57ba1b..2fd918e5fd48 100644 > --- a/include/linux/list_bl.h > +++ b/include/linux/list_bl.h > @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, > hlist_bl_set_first(h, n); > } > > +static inline void hlist_bl_add_before(struct hlist_bl_node *n, > + struct hlist_bl_node *next) > +{ > + struct hlist_bl_node **pprev = next->pprev; > + > + n->pprev = pprev; > + n->next = next; > + next->pprev = &n->next; > + > + /* pprev may be `first`, so be careful not to lose the lock bit */ > + WRITE_ONCE(*pprev, > + (struct hlist_bl_node *) > + ((unsigned long)n | > + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); > +} > + > +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, > + struct hlist_bl_node *prev) > +{ > + n->next = prev->next; > + n->pprev = &prev->next; > + WRITE_ONCE(prev->next, n); > + > + if (n->next) > + n->next->pprev = &n->next; > +} > + > static inline void __hlist_bl_del(struct hlist_bl_node *n) > { > struct hlist_bl_node *next = n->next; > -- > 2.11.0 Hi Paul and Christoph, You've added your Signed-off-by to include/linux/list_bl.h commits in the past. I'm not sure how this proposed patch should be handled. These new hlist_bl_add_{before,behind} changes are a prereq for dm-snapshot changes that Nikos has proposed, please see: https://patchwork.kernel.org/patch/10739265/ Any assistance/review you, or others on LKML, might be able to provide would be appreciated. Thanks, Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-02-28 21:32 ` [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers Mike Snitzer @ 2019-02-28 21:34 ` Mike Snitzer 2019-03-11 18:16 ` Christoph Hellwig 2019-03-13 23:48 ` Paul E. McKenney 2 siblings, 0 replies; 15+ messages in thread From: Mike Snitzer @ 2019-02-28 21:34 UTC (permalink / raw) To: Paul E. McKenney, hch, Nikos Tsironis Cc: agk, dm-devel, mpatocka, iliastsi, linux-kernel On Thu, Feb 28 2019 at 4:32pm -0500, Mike Snitzer <snitzer@redhat.com> wrote: > On Thu, Dec 20 2018 at 1:06pm -0500, > Nikos Tsironis <ntsironis@arrikto.com> wrote: > > > Add hlist_bl_add_before/behind helpers to add an element before/after an > > existing element in a bl_list. > > > > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > > Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> > > --- > > include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h > > index 3fc2cc57ba1b..2fd918e5fd48 100644 > > --- a/include/linux/list_bl.h > > +++ b/include/linux/list_bl.h > > @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, > > hlist_bl_set_first(h, n); > > } > > > > +static inline void hlist_bl_add_before(struct hlist_bl_node *n, > > + struct hlist_bl_node *next) > > +{ > > + struct hlist_bl_node **pprev = next->pprev; > > + > > + n->pprev = pprev; > > + n->next = next; > > + next->pprev = &n->next; > > + > > + /* pprev may be `first`, so be careful not to lose the lock bit */ > > + WRITE_ONCE(*pprev, > > + (struct hlist_bl_node *) > > + ((unsigned long)n | > > + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); > > +} > > + > > +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, > > + struct hlist_bl_node *prev) > > +{ > > + n->next = prev->next; > > + n->pprev = &prev->next; > > + WRITE_ONCE(prev->next, n); > > + > > + if (n->next) > > + n->next->pprev = &n->next; > > +} > > + > > static inline void __hlist_bl_del(struct hlist_bl_node *n) > > { > > struct hlist_bl_node *next = n->next; > > -- > > 2.11.0 > > Hi Paul and Christoph, > > You've added your Signed-off-by to include/linux/list_bl.h commits in > the past. I'm not sure how this proposed patch should be handled. > > These new hlist_bl_add_{before,behind} changes are a prereq for > dm-snapshot changes that Nikos has proposed, please see: > https://patchwork.kernel.org/patch/10739265/ > > Any assistance/review you, or others on LKML, might be able to provide > would be appreciated. I should've clarified that: I'm asking for the purpose of getting these changes staged for Linux 5.2. Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-02-28 21:32 ` [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers Mike Snitzer 2019-02-28 21:34 ` Mike Snitzer @ 2019-03-11 18:16 ` Christoph Hellwig 2019-03-11 22:13 ` Paul E. McKenney 2019-03-13 23:48 ` Paul E. McKenney 2 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2019-03-11 18:16 UTC (permalink / raw) To: Mike Snitzer Cc: Paul E. McKenney, hch, Nikos Tsironis, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > Hi Paul and Christoph, > > You've added your Signed-off-by to include/linux/list_bl.h commits in > the past. I'm not sure how this proposed patch should be handled. > > These new hlist_bl_add_{before,behind} changes are a prereq for > dm-snapshot changes that Nikos has proposed, please see: > https://patchwork.kernel.org/patch/10739265/ > > Any assistance/review you, or others on LKML, might be able to provide > would be appreciated. I just killed two helpers. That being said assuming that we only rely on the next pointer for the lockless traversals the changes look fine to me, but the code might be beyond my paygrade.. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-11 18:16 ` Christoph Hellwig @ 2019-03-11 22:13 ` Paul E. McKenney 2019-03-11 22:43 ` Mike Snitzer 0 siblings, 1 reply; 15+ messages in thread From: Paul E. McKenney @ 2019-03-11 22:13 UTC (permalink / raw) To: Christoph Hellwig Cc: Mike Snitzer, Nikos Tsironis, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Mon, Mar 11, 2019 at 11:16:08AM -0700, Christoph Hellwig wrote: > On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > > Hi Paul and Christoph, > > > > You've added your Signed-off-by to include/linux/list_bl.h commits in > > the past. I'm not sure how this proposed patch should be handled. > > > > These new hlist_bl_add_{before,behind} changes are a prereq for > > dm-snapshot changes that Nikos has proposed, please see: > > https://patchwork.kernel.org/patch/10739265/ > > > > Any assistance/review you, or others on LKML, might be able to provide > > would be appreciated. > > I just killed two helpers. That being said assuming that we only > rely on the next pointer for the lockless traversals the changes look > fine to me, but the code might be beyond my paygrade.. First, apologies for being slow on this one. Second, were the two helpers hlist_bl_add_{before,behind}()? If so, I guess there is not much point in me looking them over. Though perhaps I should be looking something else over? Thanx, Paul ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-11 22:13 ` Paul E. McKenney @ 2019-03-11 22:43 ` Mike Snitzer 2019-03-14 0:25 ` Paul E. McKenney 0 siblings, 1 reply; 15+ messages in thread From: Mike Snitzer @ 2019-03-11 22:43 UTC (permalink / raw) To: Paul E. McKenney Cc: Christoph Hellwig, Nikos Tsironis, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Mon, Mar 11 2019 at 6:13pm -0400, Paul E. McKenney <paulmck@linux.ibm.com> wrote: > On Mon, Mar 11, 2019 at 11:16:08AM -0700, Christoph Hellwig wrote: > > On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > > > Hi Paul and Christoph, > > > > > > You've added your Signed-off-by to include/linux/list_bl.h commits in > > > the past. I'm not sure how this proposed patch should be handled. > > > > > > These new hlist_bl_add_{before,behind} changes are a prereq for > > > dm-snapshot changes that Nikos has proposed, please see: > > > https://patchwork.kernel.org/patch/10739265/ > > > > > > Any assistance/review you, or others on LKML, might be able to provide > > > would be appreciated. > > > > I just killed two helpers. That being said assuming that we only > > rely on the next pointer for the lockless traversals the changes look > > fine to me, but the code might be beyond my paygrade.. > > First, apologies for being slow on this one. No problem. > Second, were the two helpers hlist_bl_add_{before,behind}()? If so, I > guess there is not much point in me looking them over. Though perhaps > I should be looking something else over? No, think Christoph was referring to his commit 1879fd6a26571fd4e8e1f from 2011. Anyway, I'd like you to look over this new proposed patch that introduces hlist_bl_add_{before,behind}(), please see: https://patchwork.kernel.org/patch/10835713/ If you're happy with the patch, and can provide your Reviewed-by or Acked-by, I'll then pick it up as a prereq for the broader dm-snapshot changes that Nikos has provided. Thanks! Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-11 22:43 ` Mike Snitzer @ 2019-03-14 0:25 ` Paul E. McKenney 0 siblings, 0 replies; 15+ messages in thread From: Paul E. McKenney @ 2019-03-14 0:25 UTC (permalink / raw) To: Mike Snitzer Cc: Christoph Hellwig, Nikos Tsironis, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Mon, Mar 11, 2019 at 06:43:33PM -0400, Mike Snitzer wrote: > On Mon, Mar 11 2019 at 6:13pm -0400, > Paul E. McKenney <paulmck@linux.ibm.com> wrote: > > > On Mon, Mar 11, 2019 at 11:16:08AM -0700, Christoph Hellwig wrote: > > > On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > > > > Hi Paul and Christoph, > > > > > > > > You've added your Signed-off-by to include/linux/list_bl.h commits in > > > > the past. I'm not sure how this proposed patch should be handled. > > > > > > > > These new hlist_bl_add_{before,behind} changes are a prereq for > > > > dm-snapshot changes that Nikos has proposed, please see: > > > > https://patchwork.kernel.org/patch/10739265/ > > > > > > > > Any assistance/review you, or others on LKML, might be able to provide > > > > would be appreciated. > > > > > > I just killed two helpers. That being said assuming that we only > > > rely on the next pointer for the lockless traversals the changes look > > > fine to me, but the code might be beyond my paygrade.. > > > > First, apologies for being slow on this one. > > No problem. > > > Second, were the two helpers hlist_bl_add_{before,behind}()? If so, I > > guess there is not much point in me looking them over. Though perhaps > > I should be looking something else over? > > No, think Christoph was referring to his commit 1879fd6a26571fd4e8e1f > from 2011. > > Anyway, I'd like you to look over this new proposed patch that > introduces hlist_bl_add_{before,behind}(), please see: > https://patchwork.kernel.org/patch/10835713/ > > If you're happy with the patch, and can provide your Reviewed-by or > Acked-by, I'll then pick it up as a prereq for the broader dm-snapshot > changes that Nikos has provided. I replied to the version earlier in this email thread. Looks close, but a couple of questions. Thanx, Paul ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-02-28 21:32 ` [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers Mike Snitzer 2019-02-28 21:34 ` Mike Snitzer 2019-03-11 18:16 ` Christoph Hellwig @ 2019-03-13 23:48 ` Paul E. McKenney 2019-03-14 0:30 ` Mike Snitzer 2 siblings, 1 reply; 15+ messages in thread From: Paul E. McKenney @ 2019-03-13 23:48 UTC (permalink / raw) To: Mike Snitzer Cc: hch, Nikos Tsironis, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > On Thu, Dec 20 2018 at 1:06pm -0500, > Nikos Tsironis <ntsironis@arrikto.com> wrote: > > > Add hlist_bl_add_before/behind helpers to add an element before/after an > > existing element in a bl_list. > > > > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > > Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> > > --- > > include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h > > index 3fc2cc57ba1b..2fd918e5fd48 100644 > > --- a/include/linux/list_bl.h > > +++ b/include/linux/list_bl.h > > @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, > > hlist_bl_set_first(h, n); > > } > > > > +static inline void hlist_bl_add_before(struct hlist_bl_node *n, > > + struct hlist_bl_node *next) > > +{ > > + struct hlist_bl_node **pprev = next->pprev; > > + > > + n->pprev = pprev; > > + n->next = next; > > + next->pprev = &n->next; > > + > > + /* pprev may be `first`, so be careful not to lose the lock bit */ > > + WRITE_ONCE(*pprev, > > + (struct hlist_bl_node *) > > + ((unsigned long)n | > > + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); A nit, but use of uintptr_t shrinks things a bit: + (struct hlist_bl_node *) + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); I am not too concerned about this, though. The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain the corresponding READ_ONCE()) correct? > > +} > > + > > +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, > > + struct hlist_bl_node *prev) > > +{ > > + n->next = prev->next; > > + n->pprev = &prev->next; > > + WRITE_ONCE(prev->next, n); I don't see what this WRITE_ONCE() is interacting with. The traversals use plain C-language reads, and hlist_bl_empty() can't get here. All uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. (Perhaps it should be removed? Or is there some anticipated use?) I don't believe that the WRITE_ONCE() is needed. What am I missing? Other than that, looks good. Thanx, Paul > > + > > + if (n->next) > > + n->next->pprev = &n->next; > > +} > > + > > static inline void __hlist_bl_del(struct hlist_bl_node *n) > > { > > struct hlist_bl_node *next = n->next; > > -- > > 2.11.0 > > Hi Paul and Christoph, > > You've added your Signed-off-by to include/linux/list_bl.h commits in > the past. I'm not sure how this proposed patch should be handled. > > These new hlist_bl_add_{before,behind} changes are a prereq for > dm-snapshot changes that Nikos has proposed, please see: > https://patchwork.kernel.org/patch/10739265/ > > Any assistance/review you, or others on LKML, might be able to provide > would be appreciated. > > Thanks, > Mike > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-13 23:48 ` Paul E. McKenney @ 2019-03-14 0:30 ` Mike Snitzer 2019-03-14 13:28 ` [dm-devel] " Nikos Tsironis 0 siblings, 1 reply; 15+ messages in thread From: Mike Snitzer @ 2019-03-14 0:30 UTC (permalink / raw) To: Paul E. McKenney, Nikos Tsironis Cc: hch, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Wed, Mar 13 2019 at 7:48pm -0400, Paul E. McKenney <paulmck@linux.ibm.com> wrote: > On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > > On Thu, Dec 20 2018 at 1:06pm -0500, > > Nikos Tsironis <ntsironis@arrikto.com> wrote: > > > > > Add hlist_bl_add_before/behind helpers to add an element before/after an > > > existing element in a bl_list. > > > > > > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > > > Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> > > > --- > > > include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ > > > 1 file changed, 27 insertions(+) > > > > > > diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h > > > index 3fc2cc57ba1b..2fd918e5fd48 100644 > > > --- a/include/linux/list_bl.h > > > +++ b/include/linux/list_bl.h > > > @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, > > > hlist_bl_set_first(h, n); > > > } > > > > > > +static inline void hlist_bl_add_before(struct hlist_bl_node *n, > > > + struct hlist_bl_node *next) > > > +{ > > > + struct hlist_bl_node **pprev = next->pprev; > > > + > > > + n->pprev = pprev; > > > + n->next = next; > > > + next->pprev = &n->next; > > > + > > > + /* pprev may be `first`, so be careful not to lose the lock bit */ > > > + WRITE_ONCE(*pprev, > > > + (struct hlist_bl_node *) > > > + ((unsigned long)n | > > > + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); > > A nit, but use of uintptr_t shrinks things a bit: > > + (struct hlist_bl_node *) > + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); > > I am not too concerned about this, though. I'm fine with folding in your suggestion. > The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain > the corresponding READ_ONCE()) correct? Correct. > > > +} > > > + > > > +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, > > > + struct hlist_bl_node *prev) > > > +{ > > > + n->next = prev->next; > > > + n->pprev = &prev->next; > > > + WRITE_ONCE(prev->next, n); > > I don't see what this WRITE_ONCE() is interacting with. The traversals > use plain C-language reads, and hlist_bl_empty() can't get here. All > uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting > the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. > (Perhaps it should be removed? Or is there some anticipated use?) > > I don't believe that the WRITE_ONCE() is needed. What am I missing? > > Other than that, looks good. > > Thanx, Paul > I'd imagine it was just born out of symmetry with hlist_bl_add_before() and/or caution. But let's see what Nikos has to say. Thanks, Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-14 0:30 ` Mike Snitzer @ 2019-03-14 13:28 ` Nikos Tsironis 2019-03-14 14:07 ` Paul E. McKenney 0 siblings, 1 reply; 15+ messages in thread From: Nikos Tsironis @ 2019-03-14 13:28 UTC (permalink / raw) To: Mike Snitzer, Paul E. McKenney Cc: hch, agk, dm-devel, mpatocka, iliastsi, linux-kernel On 3/14/19 2:30 AM, Mike Snitzer wrote: > On Wed, Mar 13 2019 at 7:48pm -0400, > Paul E. McKenney <paulmck@linux.ibm.com> wrote: > Hi Paul, Thanks a lot for your feedback! >> On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: >>> On Thu, Dec 20 2018 at 1:06pm -0500, >>> Nikos Tsironis <ntsironis@arrikto.com> wrote: >>> >>>> Add hlist_bl_add_before/behind helpers to add an element before/after an >>>> existing element in a bl_list. >>>> >>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> >>>> Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> >>>> --- >>>> include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ >>>> 1 file changed, 27 insertions(+) >>>> >>>> diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h >>>> index 3fc2cc57ba1b..2fd918e5fd48 100644 >>>> --- a/include/linux/list_bl.h >>>> +++ b/include/linux/list_bl.h >>>> @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, >>>> hlist_bl_set_first(h, n); >>>> } >>>> >>>> +static inline void hlist_bl_add_before(struct hlist_bl_node *n, >>>> + struct hlist_bl_node *next) >>>> +{ >>>> + struct hlist_bl_node **pprev = next->pprev; >>>> + >>>> + n->pprev = pprev; >>>> + n->next = next; >>>> + next->pprev = &n->next; >>>> + >>>> + /* pprev may be `first`, so be careful not to lose the lock bit */ >>>> + WRITE_ONCE(*pprev, >>>> + (struct hlist_bl_node *) >>>> + ((unsigned long)n | >>>> + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); >> >> A nit, but use of uintptr_t shrinks things a bit: >> >> + (struct hlist_bl_node *) >> + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); >> >> I am not too concerned about this, though. > > I'm fine with folding in your suggestion. > Indeed, this looks better. >> The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain >> the corresponding READ_ONCE()) correct? > > Correct. Yes that's correct. > >>>> +} >>>> + >>>> +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, >>>> + struct hlist_bl_node *prev) >>>> +{ >>>> + n->next = prev->next; >>>> + n->pprev = &prev->next; >>>> + WRITE_ONCE(prev->next, n); >> >> I don't see what this WRITE_ONCE() is interacting with. The traversals >> use plain C-language reads, and hlist_bl_empty() can't get here. All >> uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting >> the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. >> (Perhaps it should be removed? Or is there some anticipated use?) I am using hlist_bl_for_each_entry_safe() in this proposed patch for dm-snapshot: https://patchwork.kernel.org/patch/10835709/ >> >> I don't believe that the WRITE_ONCE() is needed. What am I missing? >> >> Other than that, looks good. >> >> Thanx, Paul >> > > I'd imagine it was just born out of symmetry with hlist_bl_add_before() > and/or caution. But let's see what Nikos has to say. I also don't believe that this WRITE_SAME() is needed. But, looking at hlist_add_behind() in include/linux/list.h, which, if I am not missing something, is used in the same way as hlist_bl_add_behind(), it also uses WRITE_ONCE() to update prev->next: static inline void hlist_add_behind(struct hlist_node *n, struct hlist_node *prev) { n->next = prev->next; WRITE_ONCE(prev->next, n); n->pprev = &prev->next; if (n->next) n->next->pprev = &n->next; } Could it be the case that the WRITE_ONCE() in hlist_add_behind() is also not needed? This WRITE_ONCE() was introduced by commit 1c97be677f72b3 ("list: Use WRITE_ONCE() when adding to lists and hlists"). But, since I am not an expert in lockless programming, I opted to be on the safe side and followed the example of hlist_add_behind(). That said, I will follow up with a new version of the patch removing the WRITE_ONCE() and using uintptr_t instead of unsigned long. Thanks, Nikos ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-14 13:28 ` [dm-devel] " Nikos Tsironis @ 2019-03-14 14:07 ` Paul E. McKenney 2019-03-14 15:03 ` Paul E. McKenney 2019-03-14 17:01 ` Nikos Tsironis 0 siblings, 2 replies; 15+ messages in thread From: Paul E. McKenney @ 2019-03-14 14:07 UTC (permalink / raw) To: Nikos Tsironis Cc: Mike Snitzer, hch, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Thu, Mar 14, 2019 at 03:28:23PM +0200, Nikos Tsironis wrote: > On 3/14/19 2:30 AM, Mike Snitzer wrote: > > On Wed, Mar 13 2019 at 7:48pm -0400, > > Paul E. McKenney <paulmck@linux.ibm.com> wrote: > > > Hi Paul, > > Thanks a lot for your feedback! NP, and apologies for the delay. > >> On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > >>> On Thu, Dec 20 2018 at 1:06pm -0500, > >>> Nikos Tsironis <ntsironis@arrikto.com> wrote: > >>> > >>>> Add hlist_bl_add_before/behind helpers to add an element before/after an > >>>> existing element in a bl_list. > >>>> > >>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > >>>> Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> > >>>> --- > >>>> include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ > >>>> 1 file changed, 27 insertions(+) > >>>> > >>>> diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h > >>>> index 3fc2cc57ba1b..2fd918e5fd48 100644 > >>>> --- a/include/linux/list_bl.h > >>>> +++ b/include/linux/list_bl.h > >>>> @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, > >>>> hlist_bl_set_first(h, n); > >>>> } > >>>> > >>>> +static inline void hlist_bl_add_before(struct hlist_bl_node *n, > >>>> + struct hlist_bl_node *next) > >>>> +{ > >>>> + struct hlist_bl_node **pprev = next->pprev; > >>>> + > >>>> + n->pprev = pprev; > >>>> + n->next = next; > >>>> + next->pprev = &n->next; > >>>> + > >>>> + /* pprev may be `first`, so be careful not to lose the lock bit */ > >>>> + WRITE_ONCE(*pprev, > >>>> + (struct hlist_bl_node *) > >>>> + ((unsigned long)n | > >>>> + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); > >> > >> A nit, but use of uintptr_t shrinks things a bit: > >> > >> + (struct hlist_bl_node *) > >> + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); > >> > >> I am not too concerned about this, though. > > > > I'm fine with folding in your suggestion. > > Indeed, this looks better. > > >> The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain > >> the corresponding READ_ONCE()) correct? > > > > Correct. > > Yes that's correct. > > >>>> +} > >>>> + > >>>> +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, > >>>> + struct hlist_bl_node *prev) > >>>> +{ > >>>> + n->next = prev->next; > >>>> + n->pprev = &prev->next; > >>>> + WRITE_ONCE(prev->next, n); > >> > >> I don't see what this WRITE_ONCE() is interacting with. The traversals > >> use plain C-language reads, and hlist_bl_empty() can't get here. All > >> uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting > >> the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. > >> (Perhaps it should be removed? Or is there some anticipated use?) > > I am using hlist_bl_for_each_entry_safe() in this proposed patch for > dm-snapshot: https://patchwork.kernel.org/patch/10835709/ Probably should keep it, then. ;-) > >> > >> I don't believe that the WRITE_ONCE() is needed. What am I missing? > >> > >> Other than that, looks good. > >> > >> Thanx, Paul > >> > > > > I'd imagine it was just born out of symmetry with hlist_bl_add_before() > > and/or caution. But let's see what Nikos has to say. > > I also don't believe that this WRITE_SAME() is needed. But, looking at > hlist_add_behind() in include/linux/list.h, which, if I am not missing > something, is used in the same way as hlist_bl_add_behind(), it also > uses WRITE_ONCE() to update prev->next: > > static inline void hlist_add_behind(struct hlist_node *n, > struct hlist_node *prev) > { > n->next = prev->next; > WRITE_ONCE(prev->next, n); > n->pprev = &prev->next; > > if (n->next) > n->next->pprev = &n->next; > } > > Could it be the case that the WRITE_ONCE() in hlist_add_behind() is also > not needed? This WRITE_ONCE() was introduced by commit 1c97be677f72b3 > ("list: Use WRITE_ONCE() when adding to lists and hlists"). Looks like I have no one to blame but myself! Would you like to remove that as part of your patch series? > But, since I am not an expert in lockless programming, I opted to be on > the safe side and followed the example of hlist_add_behind(). > > That said, I will follow up with a new version of the patch removing the > WRITE_ONCE() and using uintptr_t instead of unsigned long. Sounds good! Thanx, Paul ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-14 14:07 ` Paul E. McKenney @ 2019-03-14 15:03 ` Paul E. McKenney 2019-03-17 11:52 ` Nikos Tsironis 2019-03-14 17:01 ` Nikos Tsironis 1 sibling, 1 reply; 15+ messages in thread From: Paul E. McKenney @ 2019-03-14 15:03 UTC (permalink / raw) To: Nikos Tsironis Cc: Mike Snitzer, hch, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Thu, Mar 14, 2019 at 07:07:50AM -0700, Paul E. McKenney wrote: > On Thu, Mar 14, 2019 at 03:28:23PM +0200, Nikos Tsironis wrote: > > On 3/14/19 2:30 AM, Mike Snitzer wrote: > > > On Wed, Mar 13 2019 at 7:48pm -0400, > > > Paul E. McKenney <paulmck@linux.ibm.com> wrote: > > > > > Hi Paul, > > > > Thanks a lot for your feedback! > > NP, and apologies for the delay. > > > >> On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > > >>> On Thu, Dec 20 2018 at 1:06pm -0500, > > >>> Nikos Tsironis <ntsironis@arrikto.com> wrote: > > >>> > > >>>> Add hlist_bl_add_before/behind helpers to add an element before/after an > > >>>> existing element in a bl_list. > > >>>> > > >>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > > >>>> Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> > > >>>> --- > > >>>> include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ > > >>>> 1 file changed, 27 insertions(+) > > >>>> > > >>>> diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h > > >>>> index 3fc2cc57ba1b..2fd918e5fd48 100644 > > >>>> --- a/include/linux/list_bl.h > > >>>> +++ b/include/linux/list_bl.h > > >>>> @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, > > >>>> hlist_bl_set_first(h, n); > > >>>> } > > >>>> > > >>>> +static inline void hlist_bl_add_before(struct hlist_bl_node *n, > > >>>> + struct hlist_bl_node *next) > > >>>> +{ > > >>>> + struct hlist_bl_node **pprev = next->pprev; > > >>>> + > > >>>> + n->pprev = pprev; > > >>>> + n->next = next; > > >>>> + next->pprev = &n->next; > > >>>> + > > >>>> + /* pprev may be `first`, so be careful not to lose the lock bit */ > > >>>> + WRITE_ONCE(*pprev, > > >>>> + (struct hlist_bl_node *) > > >>>> + ((unsigned long)n | > > >>>> + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); > > >> > > >> A nit, but use of uintptr_t shrinks things a bit: > > >> > > >> + (struct hlist_bl_node *) > > >> + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); > > >> > > >> I am not too concerned about this, though. > > > > > > I'm fine with folding in your suggestion. > > > > Indeed, this looks better. > > > > >> The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain > > >> the corresponding READ_ONCE()) correct? > > > > > > Correct. > > > > Yes that's correct. > > > > >>>> +} > > >>>> + > > >>>> +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, > > >>>> + struct hlist_bl_node *prev) > > >>>> +{ > > >>>> + n->next = prev->next; > > >>>> + n->pprev = &prev->next; > > >>>> + WRITE_ONCE(prev->next, n); > > >> > > >> I don't see what this WRITE_ONCE() is interacting with. The traversals > > >> use plain C-language reads, and hlist_bl_empty() can't get here. All > > >> uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting > > >> the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. > > >> (Perhaps it should be removed? Or is there some anticipated use?) > > > > I am using hlist_bl_for_each_entry_safe() in this proposed patch for > > dm-snapshot: https://patchwork.kernel.org/patch/10835709/ > > Probably should keep it, then. ;-) > > > >> > > >> I don't believe that the WRITE_ONCE() is needed. What am I missing? > > >> > > >> Other than that, looks good. > > >> > > >> Thanx, Paul > > >> > > > > > > I'd imagine it was just born out of symmetry with hlist_bl_add_before() > > > and/or caution. But let's see what Nikos has to say. > > > > I also don't believe that this WRITE_SAME() is needed. But, looking at > > hlist_add_behind() in include/linux/list.h, which, if I am not missing > > something, is used in the same way as hlist_bl_add_behind(), it also > > uses WRITE_ONCE() to update prev->next: > > > > static inline void hlist_add_behind(struct hlist_node *n, > > struct hlist_node *prev) > > { > > n->next = prev->next; > > WRITE_ONCE(prev->next, n); > > n->pprev = &prev->next; > > > > if (n->next) > > n->next->pprev = &n->next; > > } > > > > Could it be the case that the WRITE_ONCE() in hlist_add_behind() is also > > not needed? This WRITE_ONCE() was introduced by commit 1c97be677f72b3 > > ("list: Use WRITE_ONCE() when adding to lists and hlists"). > > Looks like I have no one to blame but myself! > > Would you like to remove that as part of your patch series? > > > But, since I am not an expert in lockless programming, I opted to be on > > the safe side and followed the example of hlist_add_behind(). > > > > That said, I will follow up with a new version of the patch removing the > > WRITE_ONCE() and using uintptr_t instead of unsigned long. > > Sounds good! Oh, and of course intptr_t is one character shorter than uintptr_t, and looks to work just as well in this context. ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-14 15:03 ` Paul E. McKenney @ 2019-03-17 11:52 ` Nikos Tsironis 2019-03-18 17:16 ` Paul E. McKenney 0 siblings, 1 reply; 15+ messages in thread From: Nikos Tsironis @ 2019-03-17 11:52 UTC (permalink / raw) To: paulmck Cc: Mike Snitzer, hch, agk, dm-devel, mpatocka, iliastsi, linux-kernel On 3/14/19 5:03 PM, Paul E. McKenney wrote: > On Thu, Mar 14, 2019 at 07:07:50AM -0700, Paul E. McKenney wrote: >> On Thu, Mar 14, 2019 at 03:28:23PM +0200, Nikos Tsironis wrote: >>> On 3/14/19 2:30 AM, Mike Snitzer wrote: >>>> On Wed, Mar 13 2019 at 7:48pm -0400, >>>> Paul E. McKenney <paulmck@linux.ibm.com> wrote: >>>> >>> Hi Paul, >>> >>> Thanks a lot for your feedback! >> >> NP, and apologies for the delay. >> >>>>> On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: >>>>>> On Thu, Dec 20 2018 at 1:06pm -0500, >>>>>> Nikos Tsironis <ntsironis@arrikto.com> wrote: >>>>>> >>>>>>> Add hlist_bl_add_before/behind helpers to add an element before/after an >>>>>>> existing element in a bl_list. >>>>>>> >>>>>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> >>>>>>> Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> >>>>>>> --- >>>>>>> include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ >>>>>>> 1 file changed, 27 insertions(+) >>>>>>> >>>>>>> diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h >>>>>>> index 3fc2cc57ba1b..2fd918e5fd48 100644 >>>>>>> --- a/include/linux/list_bl.h >>>>>>> +++ b/include/linux/list_bl.h >>>>>>> @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, >>>>>>> hlist_bl_set_first(h, n); >>>>>>> } >>>>>>> >>>>>>> +static inline void hlist_bl_add_before(struct hlist_bl_node *n, >>>>>>> + struct hlist_bl_node *next) >>>>>>> +{ >>>>>>> + struct hlist_bl_node **pprev = next->pprev; >>>>>>> + >>>>>>> + n->pprev = pprev; >>>>>>> + n->next = next; >>>>>>> + next->pprev = &n->next; >>>>>>> + >>>>>>> + /* pprev may be `first`, so be careful not to lose the lock bit */ >>>>>>> + WRITE_ONCE(*pprev, >>>>>>> + (struct hlist_bl_node *) >>>>>>> + ((unsigned long)n | >>>>>>> + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); >>>>> >>>>> A nit, but use of uintptr_t shrinks things a bit: >>>>> >>>>> + (struct hlist_bl_node *) >>>>> + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); >>>>> >>>>> I am not too concerned about this, though. >>>> >>>> I'm fine with folding in your suggestion. >>> >>> Indeed, this looks better. >>> >>>>> The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain >>>>> the corresponding READ_ONCE()) correct? >>>> >>>> Correct. >>> >>> Yes that's correct. >>> >>>>>>> +} >>>>>>> + >>>>>>> +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, >>>>>>> + struct hlist_bl_node *prev) >>>>>>> +{ >>>>>>> + n->next = prev->next; >>>>>>> + n->pprev = &prev->next; >>>>>>> + WRITE_ONCE(prev->next, n); >>>>> >>>>> I don't see what this WRITE_ONCE() is interacting with. The traversals >>>>> use plain C-language reads, and hlist_bl_empty() can't get here. All >>>>> uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting >>>>> the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. >>>>> (Perhaps it should be removed? Or is there some anticipated use?) >>> >>> I am using hlist_bl_for_each_entry_safe() in this proposed patch for >>> dm-snapshot: https://patchwork.kernel.org/patch/10835709/ >> >> Probably should keep it, then. ;-) >> >>>>> >>>>> I don't believe that the WRITE_ONCE() is needed. What am I missing? >>>>> >>>>> Other than that, looks good. >>>>> >>>>> Thanx, Paul >>>>> >>>> >>>> I'd imagine it was just born out of symmetry with hlist_bl_add_before() >>>> and/or caution. But let's see what Nikos has to say. >>> >>> I also don't believe that this WRITE_SAME() is needed. But, looking at >>> hlist_add_behind() in include/linux/list.h, which, if I am not missing >>> something, is used in the same way as hlist_bl_add_behind(), it also >>> uses WRITE_ONCE() to update prev->next: >>> >>> static inline void hlist_add_behind(struct hlist_node *n, >>> struct hlist_node *prev) >>> { >>> n->next = prev->next; >>> WRITE_ONCE(prev->next, n); >>> n->pprev = &prev->next; >>> >>> if (n->next) >>> n->next->pprev = &n->next; >>> } >>> >>> Could it be the case that the WRITE_ONCE() in hlist_add_behind() is also >>> not needed? This WRITE_ONCE() was introduced by commit 1c97be677f72b3 >>> ("list: Use WRITE_ONCE() when adding to lists and hlists"). >> >> Looks like I have no one to blame but myself! >> >> Would you like to remove that as part of your patch series? >> >>> But, since I am not an expert in lockless programming, I opted to be on >>> the safe side and followed the example of hlist_add_behind(). >>> >>> That said, I will follow up with a new version of the patch removing the >>> WRITE_ONCE() and using uintptr_t instead of unsigned long. >> >> Sounds good! > > Oh, and of course intptr_t is one character shorter than uintptr_t, and > looks to work just as well in this context. ;-) > > Thanx, Paul > Hi Paul, Sorry for the late reply. intptr_t seems to be defined only in a header file under arch/mips, so I will stick to uintptr_t. Thanks, Nikos ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-17 11:52 ` Nikos Tsironis @ 2019-03-18 17:16 ` Paul E. McKenney 2019-03-20 20:25 ` Nikos Tsironis 0 siblings, 1 reply; 15+ messages in thread From: Paul E. McKenney @ 2019-03-18 17:16 UTC (permalink / raw) To: Nikos Tsironis Cc: Mike Snitzer, hch, agk, dm-devel, mpatocka, iliastsi, linux-kernel On Sun, Mar 17, 2019 at 01:52:50PM +0200, Nikos Tsironis wrote: > On 3/14/19 5:03 PM, Paul E. McKenney wrote: > > On Thu, Mar 14, 2019 at 07:07:50AM -0700, Paul E. McKenney wrote: > >> On Thu, Mar 14, 2019 at 03:28:23PM +0200, Nikos Tsironis wrote: > >>> On 3/14/19 2:30 AM, Mike Snitzer wrote: > >>>> On Wed, Mar 13 2019 at 7:48pm -0400, > >>>> Paul E. McKenney <paulmck@linux.ibm.com> wrote: > >>>> > >>> Hi Paul, > >>> > >>> Thanks a lot for your feedback! > >> > >> NP, and apologies for the delay. > >> > >>>>> On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: > >>>>>> On Thu, Dec 20 2018 at 1:06pm -0500, > >>>>>> Nikos Tsironis <ntsironis@arrikto.com> wrote: > >>>>>> > >>>>>>> Add hlist_bl_add_before/behind helpers to add an element before/after an > >>>>>>> existing element in a bl_list. > >>>>>>> > >>>>>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > >>>>>>> Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> > >>>>>>> --- > >>>>>>> include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ > >>>>>>> 1 file changed, 27 insertions(+) > >>>>>>> > >>>>>>> diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h > >>>>>>> index 3fc2cc57ba1b..2fd918e5fd48 100644 > >>>>>>> --- a/include/linux/list_bl.h > >>>>>>> +++ b/include/linux/list_bl.h > >>>>>>> @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, > >>>>>>> hlist_bl_set_first(h, n); > >>>>>>> } > >>>>>>> > >>>>>>> +static inline void hlist_bl_add_before(struct hlist_bl_node *n, > >>>>>>> + struct hlist_bl_node *next) > >>>>>>> +{ > >>>>>>> + struct hlist_bl_node **pprev = next->pprev; > >>>>>>> + > >>>>>>> + n->pprev = pprev; > >>>>>>> + n->next = next; > >>>>>>> + next->pprev = &n->next; > >>>>>>> + > >>>>>>> + /* pprev may be `first`, so be careful not to lose the lock bit */ > >>>>>>> + WRITE_ONCE(*pprev, > >>>>>>> + (struct hlist_bl_node *) > >>>>>>> + ((unsigned long)n | > >>>>>>> + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); > >>>>> > >>>>> A nit, but use of uintptr_t shrinks things a bit: > >>>>> > >>>>> + (struct hlist_bl_node *) > >>>>> + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); > >>>>> > >>>>> I am not too concerned about this, though. > >>>> > >>>> I'm fine with folding in your suggestion. > >>> > >>> Indeed, this looks better. > >>> > >>>>> The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain > >>>>> the corresponding READ_ONCE()) correct? > >>>> > >>>> Correct. > >>> > >>> Yes that's correct. > >>> > >>>>>>> +} > >>>>>>> + > >>>>>>> +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, > >>>>>>> + struct hlist_bl_node *prev) > >>>>>>> +{ > >>>>>>> + n->next = prev->next; > >>>>>>> + n->pprev = &prev->next; > >>>>>>> + WRITE_ONCE(prev->next, n); > >>>>> > >>>>> I don't see what this WRITE_ONCE() is interacting with. The traversals > >>>>> use plain C-language reads, and hlist_bl_empty() can't get here. All > >>>>> uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting > >>>>> the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. > >>>>> (Perhaps it should be removed? Or is there some anticipated use?) > >>> > >>> I am using hlist_bl_for_each_entry_safe() in this proposed patch for > >>> dm-snapshot: https://patchwork.kernel.org/patch/10835709/ > >> > >> Probably should keep it, then. ;-) > >> > >>>>> > >>>>> I don't believe that the WRITE_ONCE() is needed. What am I missing? > >>>>> > >>>>> Other than that, looks good. > >>>>> > >>>>> Thanx, Paul > >>>>> > >>>> > >>>> I'd imagine it was just born out of symmetry with hlist_bl_add_before() > >>>> and/or caution. But let's see what Nikos has to say. > >>> > >>> I also don't believe that this WRITE_SAME() is needed. But, looking at > >>> hlist_add_behind() in include/linux/list.h, which, if I am not missing > >>> something, is used in the same way as hlist_bl_add_behind(), it also > >>> uses WRITE_ONCE() to update prev->next: > >>> > >>> static inline void hlist_add_behind(struct hlist_node *n, > >>> struct hlist_node *prev) > >>> { > >>> n->next = prev->next; > >>> WRITE_ONCE(prev->next, n); > >>> n->pprev = &prev->next; > >>> > >>> if (n->next) > >>> n->next->pprev = &n->next; > >>> } > >>> > >>> Could it be the case that the WRITE_ONCE() in hlist_add_behind() is also > >>> not needed? This WRITE_ONCE() was introduced by commit 1c97be677f72b3 > >>> ("list: Use WRITE_ONCE() when adding to lists and hlists"). > >> > >> Looks like I have no one to blame but myself! > >> > >> Would you like to remove that as part of your patch series? > >> > >>> But, since I am not an expert in lockless programming, I opted to be on > >>> the safe side and followed the example of hlist_add_behind(). > >>> > >>> That said, I will follow up with a new version of the patch removing the > >>> WRITE_ONCE() and using uintptr_t instead of unsigned long. > >> > >> Sounds good! > > > > Oh, and of course intptr_t is one character shorter than uintptr_t, and > > looks to work just as well in this context. ;-) > > > > Thanx, Paul > > > > > Hi Paul, > > Sorry for the late reply. > > intptr_t seems to be defined only in a header file under arch/mips, so I > will stick to uintptr_t. Ah, apologies for the misdirection! Hmmm... Maybe intptr_t should be added alongside uintptr_t? Saving a character is saving a character. ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-18 17:16 ` Paul E. McKenney @ 2019-03-20 20:25 ` Nikos Tsironis 0 siblings, 0 replies; 15+ messages in thread From: Nikos Tsironis @ 2019-03-20 20:25 UTC (permalink / raw) To: paulmck Cc: Mike Snitzer, hch, agk, dm-devel, mpatocka, iliastsi, linux-kernel On 3/18/19 7:16 PM, Paul E. McKenney wrote: > On Sun, Mar 17, 2019 at 01:52:50PM +0200, Nikos Tsironis wrote: >> On 3/14/19 5:03 PM, Paul E. McKenney wrote: >>> On Thu, Mar 14, 2019 at 07:07:50AM -0700, Paul E. McKenney wrote: >>>> On Thu, Mar 14, 2019 at 03:28:23PM +0200, Nikos Tsironis wrote: >>>>> On 3/14/19 2:30 AM, Mike Snitzer wrote: >>>>>> On Wed, Mar 13 2019 at 7:48pm -0400, >>>>>> Paul E. McKenney <paulmck@linux.ibm.com> wrote: >>>>>> >>>>> Hi Paul, >>>>> >>>>> Thanks a lot for your feedback! >>>> >>>> NP, and apologies for the delay. >>>> >>>>>>> On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: >>>>>>>> On Thu, Dec 20 2018 at 1:06pm -0500, >>>>>>>> Nikos Tsironis <ntsironis@arrikto.com> wrote: >>>>>>>> >>>>>>>>> Add hlist_bl_add_before/behind helpers to add an element before/after an >>>>>>>>> existing element in a bl_list. >>>>>>>>> >>>>>>>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> >>>>>>>>> Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> >>>>>>>>> --- >>>>>>>>> include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ >>>>>>>>> 1 file changed, 27 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h >>>>>>>>> index 3fc2cc57ba1b..2fd918e5fd48 100644 >>>>>>>>> --- a/include/linux/list_bl.h >>>>>>>>> +++ b/include/linux/list_bl.h >>>>>>>>> @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, >>>>>>>>> hlist_bl_set_first(h, n); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static inline void hlist_bl_add_before(struct hlist_bl_node *n, >>>>>>>>> + struct hlist_bl_node *next) >>>>>>>>> +{ >>>>>>>>> + struct hlist_bl_node **pprev = next->pprev; >>>>>>>>> + >>>>>>>>> + n->pprev = pprev; >>>>>>>>> + n->next = next; >>>>>>>>> + next->pprev = &n->next; >>>>>>>>> + >>>>>>>>> + /* pprev may be `first`, so be careful not to lose the lock bit */ >>>>>>>>> + WRITE_ONCE(*pprev, >>>>>>>>> + (struct hlist_bl_node *) >>>>>>>>> + ((unsigned long)n | >>>>>>>>> + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); >>>>>>> >>>>>>> A nit, but use of uintptr_t shrinks things a bit: >>>>>>> >>>>>>> + (struct hlist_bl_node *) >>>>>>> + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); >>>>>>> >>>>>>> I am not too concerned about this, though. >>>>>> >>>>>> I'm fine with folding in your suggestion. >>>>> >>>>> Indeed, this looks better. >>>>> >>>>>>> The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain >>>>>>> the corresponding READ_ONCE()) correct? >>>>>> >>>>>> Correct. >>>>> >>>>> Yes that's correct. >>>>> >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, >>>>>>>>> + struct hlist_bl_node *prev) >>>>>>>>> +{ >>>>>>>>> + n->next = prev->next; >>>>>>>>> + n->pprev = &prev->next; >>>>>>>>> + WRITE_ONCE(prev->next, n); >>>>>>> >>>>>>> I don't see what this WRITE_ONCE() is interacting with. The traversals >>>>>>> use plain C-language reads, and hlist_bl_empty() can't get here. All >>>>>>> uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting >>>>>>> the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. >>>>>>> (Perhaps it should be removed? Or is there some anticipated use?) >>>>> >>>>> I am using hlist_bl_for_each_entry_safe() in this proposed patch for >>>>> dm-snapshot: https://patchwork.kernel.org/patch/10835709/ >>>> >>>> Probably should keep it, then. ;-) >>>> >>>>>>> >>>>>>> I don't believe that the WRITE_ONCE() is needed. What am I missing? >>>>>>> >>>>>>> Other than that, looks good. >>>>>>> >>>>>>> Thanx, Paul >>>>>>> >>>>>> >>>>>> I'd imagine it was just born out of symmetry with hlist_bl_add_before() >>>>>> and/or caution. But let's see what Nikos has to say. >>>>> >>>>> I also don't believe that this WRITE_SAME() is needed. But, looking at >>>>> hlist_add_behind() in include/linux/list.h, which, if I am not missing >>>>> something, is used in the same way as hlist_bl_add_behind(), it also >>>>> uses WRITE_ONCE() to update prev->next: >>>>> >>>>> static inline void hlist_add_behind(struct hlist_node *n, >>>>> struct hlist_node *prev) >>>>> { >>>>> n->next = prev->next; >>>>> WRITE_ONCE(prev->next, n); >>>>> n->pprev = &prev->next; >>>>> >>>>> if (n->next) >>>>> n->next->pprev = &n->next; >>>>> } >>>>> >>>>> Could it be the case that the WRITE_ONCE() in hlist_add_behind() is also >>>>> not needed? This WRITE_ONCE() was introduced by commit 1c97be677f72b3 >>>>> ("list: Use WRITE_ONCE() when adding to lists and hlists"). >>>> >>>> Looks like I have no one to blame but myself! >>>> >>>> Would you like to remove that as part of your patch series? >>>> >>>>> But, since I am not an expert in lockless programming, I opted to be on >>>>> the safe side and followed the example of hlist_add_behind(). >>>>> >>>>> That said, I will follow up with a new version of the patch removing the >>>>> WRITE_ONCE() and using uintptr_t instead of unsigned long. >>>> >>>> Sounds good! >>> >>> Oh, and of course intptr_t is one character shorter than uintptr_t, and >>> looks to work just as well in this context. ;-) >>> >>> Thanx, Paul >>> >> >> >> Hi Paul, >> >> Sorry for the late reply. >> >> intptr_t seems to be defined only in a header file under arch/mips, so I >> will stick to uintptr_t. > > Ah, apologies for the misdirection! Hmmm... Maybe intptr_t should be > added alongside uintptr_t? Saving a character is saving a character. ;-) > > Thanx, Paul > I will follow up with an unrelated patch to add intptr_t to include/linux/types.h, so that it is available in subsequent patches throughout the kernel. Nikos ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers 2019-03-14 14:07 ` Paul E. McKenney 2019-03-14 15:03 ` Paul E. McKenney @ 2019-03-14 17:01 ` Nikos Tsironis 1 sibling, 0 replies; 15+ messages in thread From: Nikos Tsironis @ 2019-03-14 17:01 UTC (permalink / raw) To: paulmck Cc: Mike Snitzer, linux-kernel, iliastsi, hch, dm-devel, mpatocka, agk On 3/14/19 4:07 PM, Paul E. McKenney wrote: > On Thu, Mar 14, 2019 at 03:28:23PM +0200, Nikos Tsironis wrote: >> On 3/14/19 2:30 AM, Mike Snitzer wrote: >>> On Wed, Mar 13 2019 at 7:48pm -0400, >>> Paul E. McKenney <paulmck@linux.ibm.com> wrote: >>> >> Hi Paul, >> >> Thanks a lot for your feedback! > > NP, and apologies for the delay. > >>>> On Thu, Feb 28, 2019 at 04:32:02PM -0500, Mike Snitzer wrote: >>>>> On Thu, Dec 20 2018 at 1:06pm -0500, >>>>> Nikos Tsironis <ntsironis@arrikto.com> wrote: >>>>> >>>>>> Add hlist_bl_add_before/behind helpers to add an element before/after an >>>>>> existing element in a bl_list. >>>>>> >>>>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> >>>>>> Signed-off-by: Ilias Tsitsimpis <iliastsi@arrikto.com> >>>>>> --- >>>>>> include/linux/list_bl.h | 27 +++++++++++++++++++++++++++ >>>>>> 1 file changed, 27 insertions(+) >>>>>> >>>>>> diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h >>>>>> index 3fc2cc57ba1b..2fd918e5fd48 100644 >>>>>> --- a/include/linux/list_bl.h >>>>>> +++ b/include/linux/list_bl.h >>>>>> @@ -86,6 +86,33 @@ static inline void hlist_bl_add_head(struct hlist_bl_node *n, >>>>>> hlist_bl_set_first(h, n); >>>>>> } >>>>>> >>>>>> +static inline void hlist_bl_add_before(struct hlist_bl_node *n, >>>>>> + struct hlist_bl_node *next) >>>>>> +{ >>>>>> + struct hlist_bl_node **pprev = next->pprev; >>>>>> + >>>>>> + n->pprev = pprev; >>>>>> + n->next = next; >>>>>> + next->pprev = &n->next; >>>>>> + >>>>>> + /* pprev may be `first`, so be careful not to lose the lock bit */ >>>>>> + WRITE_ONCE(*pprev, >>>>>> + (struct hlist_bl_node *) >>>>>> + ((unsigned long)n | >>>>>> + ((unsigned long)*pprev & LIST_BL_LOCKMASK))); >>>> >>>> A nit, but use of uintptr_t shrinks things a bit: >>>> >>>> + (struct hlist_bl_node *) >>>> + ((uintptr_t)n | ((uintptr_t)*pprev & LIST_BL_LOCKMASK))); >>>> >>>> I am not too concerned about this, though. >>> >>> I'm fine with folding in your suggestion. >> >> Indeed, this looks better. >> >>>> The WRITE_ONCE() is to handle races with hlist_bl_empty() (which does contain >>>> the corresponding READ_ONCE()) correct? >>> >>> Correct. >> >> Yes that's correct. >> >>>>>> +} >>>>>> + >>>>>> +static inline void hlist_bl_add_behind(struct hlist_bl_node *n, >>>>>> + struct hlist_bl_node *prev) >>>>>> +{ >>>>>> + n->next = prev->next; >>>>>> + n->pprev = &prev->next; >>>>>> + WRITE_ONCE(prev->next, n); >>>> >>>> I don't see what this WRITE_ONCE() is interacting with. The traversals >>>> use plain C-language reads, and hlist_bl_empty() can't get here. All >>>> uses of hlist_bl_for_each_entry() invoke hlist_bl_lock() before starting >>>> the traversal, and hlist_bl_for_each_entry_safe() looks to be unused. >>>> (Perhaps it should be removed? Or is there some anticipated use?) >> >> I am using hlist_bl_for_each_entry_safe() in this proposed patch for >> dm-snapshot: https://patchwork.kernel.org/patch/10835709/ > > Probably should keep it, then. ;-) > >>>> >>>> I don't believe that the WRITE_ONCE() is needed. What am I missing? >>>> >>>> Other than that, looks good. >>>> >>>> Thanx, Paul >>>> >>> >>> I'd imagine it was just born out of symmetry with hlist_bl_add_before() >>> and/or caution. But let's see what Nikos has to say. >> >> I also don't believe that this WRITE_SAME() is needed. But, looking at >> hlist_add_behind() in include/linux/list.h, which, if I am not missing >> something, is used in the same way as hlist_bl_add_behind(), it also >> uses WRITE_ONCE() to update prev->next: >> >> static inline void hlist_add_behind(struct hlist_node *n, >> struct hlist_node *prev) >> { >> n->next = prev->next; >> WRITE_ONCE(prev->next, n); >> n->pprev = &prev->next; >> >> if (n->next) >> n->next->pprev = &n->next; >> } >> >> Could it be the case that the WRITE_ONCE() in hlist_add_behind() is also >> not needed? This WRITE_ONCE() was introduced by commit 1c97be677f72b3 >> ("list: Use WRITE_ONCE() when adding to lists and hlists"). > > Looks like I have no one to blame but myself! > > Would you like to remove that as part of your patch series? Yes, Of course. I will add an extra patch removing the WRITE_ONCE() from hlist_add_behind(). Thanks, Nikos > >> But, since I am not an expert in lockless programming, I opted to be on >> the safe side and followed the example of hlist_add_behind(). >> >> That said, I will follow up with a new version of the patch removing the >> WRITE_ONCE() and using uintptr_t instead of unsigned long. > > Sounds good! > > Thanx, Paul > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-03-20 20:25 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20181220180651.4879-1-ntsironis@arrikto.com>
[not found] ` <20181220180651.4879-2-ntsironis@arrikto.com>
2019-02-28 21:32 ` [PATCH 1/3] list_bl: Add hlist_bl_add_before/behind helpers Mike Snitzer
2019-02-28 21:34 ` Mike Snitzer
2019-03-11 18:16 ` Christoph Hellwig
2019-03-11 22:13 ` Paul E. McKenney
2019-03-11 22:43 ` Mike Snitzer
2019-03-14 0:25 ` Paul E. McKenney
2019-03-13 23:48 ` Paul E. McKenney
2019-03-14 0:30 ` Mike Snitzer
2019-03-14 13:28 ` [dm-devel] " Nikos Tsironis
2019-03-14 14:07 ` Paul E. McKenney
2019-03-14 15:03 ` Paul E. McKenney
2019-03-17 11:52 ` Nikos Tsironis
2019-03-18 17:16 ` Paul E. McKenney
2019-03-20 20:25 ` Nikos Tsironis
2019-03-14 17:01 ` Nikos Tsironis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).