| Message ID | 20230815173031.168344-2-ja@ssi.bg (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipvs: per-net tables and optimizations | expand |
On Tue, Aug 15, 2023 at 08:30:18PM +0300, Julian Anastasov wrote: > Add hlist_bl_for_each_entry_continue_rcu and hlist_bl_next_rcu > > Signed-off-by: Julian Anastasov <ja@ssi.bg> > --- > include/linux/rculist_bl.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/include/linux/rculist_bl.h b/include/linux/rculist_bl.h > index 0b952d06eb0b..93a757793d83 100644 > --- a/include/linux/rculist_bl.h > +++ b/include/linux/rculist_bl.h > @@ -24,6 +24,10 @@ static inline struct hlist_bl_node *hlist_bl_first_rcu(struct hlist_bl_head *h) > ((unsigned long)rcu_dereference_check(h->first, hlist_bl_is_locked(h)) & ~LIST_BL_LOCKMASK); > } > > +/* return the next element in an RCU protected list */ > +#define hlist_bl_next_rcu(node) \ > + (*((struct hlist_bl_node __rcu **)(&(node)->next))) > + > /** > * hlist_bl_del_rcu - deletes entry from hash list without re-initialization > * @n: the element to delete from the hash list. > @@ -98,4 +102,17 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, > ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ > pos = rcu_dereference_raw(pos->next)) > > +/** > + * hlist_bl_for_each_entry_continue_rcu - iterate over a list continuing after > + * current point Please add a comment to the effect that the element continued from must have been either: (1) Iterated to within the same RCU read-side critical section or (2) Nailed down using some lock, reference count, or whatever suffices to keep the continued-from element from being freed in the meantime. Thanx, Paul > + * @tpos: the type * to use as a loop cursor. > + * @pos: the &struct hlist_bl_node to use as a loop cursor. > + * @member: the name of the hlist_bl_node within the struct. > + */ > +#define hlist_bl_for_each_entry_continue_rcu(tpos, pos, member) \ > + for (pos = rcu_dereference_raw(hlist_bl_next_rcu(&(tpos)->member)); \ > + pos && \ > + ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ > + pos = rcu_dereference_raw(hlist_bl_next_rcu(pos))) > + > #endif > -- > 2.41.0 > >
Hello, On Tue, 15 Aug 2023, Paul E. McKenney wrote: > On Tue, Aug 15, 2023 at 08:30:18PM +0300, Julian Anastasov wrote: > > Add hlist_bl_for_each_entry_continue_rcu and hlist_bl_next_rcu > > > > Signed-off-by: Julian Anastasov <ja@ssi.bg> > > --- > > include/linux/rculist_bl.h | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/include/linux/rculist_bl.h b/include/linux/rculist_bl.h > > index 0b952d06eb0b..93a757793d83 100644 > > --- a/include/linux/rculist_bl.h > > +++ b/include/linux/rculist_bl.h > > @@ -24,6 +24,10 @@ static inline struct hlist_bl_node *hlist_bl_first_rcu(struct hlist_bl_head *h) > > ((unsigned long)rcu_dereference_check(h->first, hlist_bl_is_locked(h)) & ~LIST_BL_LOCKMASK); > > } > > > > +/* return the next element in an RCU protected list */ > > +#define hlist_bl_next_rcu(node) \ > > + (*((struct hlist_bl_node __rcu **)(&(node)->next))) > > + > > /** > > * hlist_bl_del_rcu - deletes entry from hash list without re-initialization > > * @n: the element to delete from the hash list. > > @@ -98,4 +102,17 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, > > ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ > > pos = rcu_dereference_raw(pos->next)) > > > > +/** > > + * hlist_bl_for_each_entry_continue_rcu - iterate over a list continuing after > > + * current point > > Please add a comment to the effect that the element continued from > must have been either: (1) Iterated to within the same RCU read-side > critical section or (2) Nailed down using some lock, reference count, > or whatever suffices to keep the continued-from element from being freed > in the meantime. I created 2nd version which has more changes. I'm not sure what are the desired steps for this patch, should I keep it as part of my patchset if accepted? Or should I post it separately? Here it is v2 for comments. [PATCHv2 RFC] rculist_bl: add hlist_bl_for_each_entry_continue_rcu Change the old hlist_bl_first_rcu to hlist_bl_first_rcu_dereference to indicate that it is a RCU dereference. Add hlist_bl_next_rcu and hlist_bl_first_rcu to use RCU pointers and use them to fix sparse warnings. Add hlist_bl_for_each_entry_continue_rcu. Signed-off-by: Julian Anastasov <ja@ssi.bg> --- include/linux/rculist_bl.h | 49 +++++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/include/linux/rculist_bl.h b/include/linux/rculist_bl.h index 0b952d06eb0b..36363b876e53 100644 --- a/include/linux/rculist_bl.h +++ b/include/linux/rculist_bl.h @@ -8,21 +8,31 @@ #include <linux/list_bl.h> #include <linux/rcupdate.h> +/* return the first ptr or next element in an RCU protected list */ +#define hlist_bl_first_rcu(head) \ + (*((struct hlist_bl_node __rcu **)(&(head)->first))) +#define hlist_bl_next_rcu(node) \ + (*((struct hlist_bl_node __rcu **)(&(node)->next))) + static inline void hlist_bl_set_first_rcu(struct hlist_bl_head *h, struct hlist_bl_node *n) { LIST_BL_BUG_ON((unsigned long)n & LIST_BL_LOCKMASK); LIST_BL_BUG_ON(((unsigned long)h->first & LIST_BL_LOCKMASK) != LIST_BL_LOCKMASK); - rcu_assign_pointer(h->first, + rcu_assign_pointer(hlist_bl_first_rcu(h), (struct hlist_bl_node *)((unsigned long)n | LIST_BL_LOCKMASK)); } -static inline struct hlist_bl_node *hlist_bl_first_rcu(struct hlist_bl_head *h) -{ - return (struct hlist_bl_node *) - ((unsigned long)rcu_dereference_check(h->first, hlist_bl_is_locked(h)) & ~LIST_BL_LOCKMASK); -} +#define hlist_bl_first_rcu_dereference(head) \ +({ \ + struct hlist_bl_head *__head = (head); \ + \ + (struct hlist_bl_node *) \ + ((unsigned long)rcu_dereference_check(hlist_bl_first_rcu(__head), \ + hlist_bl_is_locked(__head)) & \ + ~LIST_BL_LOCKMASK); \ +}) /** * hlist_bl_del_rcu - deletes entry from hash list without re-initialization @@ -73,7 +83,7 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, { struct hlist_bl_node *first; - /* don't need hlist_bl_first_rcu because we're under lock */ + /* don't need hlist_bl_first_rcu* because we're under lock */ first = hlist_bl_first(h); n->next = first; @@ -93,9 +103,30 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, * */ #define hlist_bl_for_each_entry_rcu(tpos, pos, head, member) \ - for (pos = hlist_bl_first_rcu(head); \ + for (pos = hlist_bl_first_rcu_dereference(head); \ pos && \ ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ - pos = rcu_dereference_raw(pos->next)) + pos = rcu_dereference_raw(hlist_bl_next_rcu(pos))) + +/** + * hlist_bl_for_each_entry_continue_rcu - continue iteration over list of given + * type + * @tpos: the type * to use as a loop cursor. + * @pos: the &struct hlist_bl_node to use as a loop cursor. + * @member: the name of the hlist_bl_node within the struct. + * + * Continue to iterate over list of given type, continuing after + * the current position which must have been in the list when the RCU read + * lock was taken. + * This would typically require either that you obtained the node from a + * previous walk of the list in the same RCU read-side critical section, or + * that you held some sort of non-RCU reference (such as a reference count) + * to keep the node alive *and* in the list. + */ +#define hlist_bl_for_each_entry_continue_rcu(tpos, pos, member) \ + for (pos = rcu_dereference_raw(hlist_bl_next_rcu(&(tpos)->member)); \ + pos && \ + ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ + pos = rcu_dereference_raw(hlist_bl_next_rcu(pos))) #endif
On Wed, Aug 16, 2023 at 07:02:39PM +0300, Julian Anastasov wrote: > > Hello, > > On Tue, 15 Aug 2023, Paul E. McKenney wrote: > > > On Tue, Aug 15, 2023 at 08:30:18PM +0300, Julian Anastasov wrote: > > > Add hlist_bl_for_each_entry_continue_rcu and hlist_bl_next_rcu > > > > > > Signed-off-by: Julian Anastasov <ja@ssi.bg> > > > --- > > > include/linux/rculist_bl.h | 17 +++++++++++++++++ > > > 1 file changed, 17 insertions(+) > > > > > > diff --git a/include/linux/rculist_bl.h b/include/linux/rculist_bl.h > > > index 0b952d06eb0b..93a757793d83 100644 > > > --- a/include/linux/rculist_bl.h > > > +++ b/include/linux/rculist_bl.h > > > @@ -24,6 +24,10 @@ static inline struct hlist_bl_node *hlist_bl_first_rcu(struct hlist_bl_head *h) > > > ((unsigned long)rcu_dereference_check(h->first, hlist_bl_is_locked(h)) & ~LIST_BL_LOCKMASK); > > > } > > > > > > +/* return the next element in an RCU protected list */ > > > +#define hlist_bl_next_rcu(node) \ > > > + (*((struct hlist_bl_node __rcu **)(&(node)->next))) > > > + > > > /** > > > * hlist_bl_del_rcu - deletes entry from hash list without re-initialization > > > * @n: the element to delete from the hash list. > > > @@ -98,4 +102,17 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, > > > ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ > > > pos = rcu_dereference_raw(pos->next)) > > > > > > +/** > > > + * hlist_bl_for_each_entry_continue_rcu - iterate over a list continuing after > > > + * current point > > > > Please add a comment to the effect that the element continued from > > must have been either: (1) Iterated to within the same RCU read-side > > critical section or (2) Nailed down using some lock, reference count, > > or whatever suffices to keep the continued-from element from being freed > > in the meantime. > > I created 2nd version which has more changes. I'm not sure > what are the desired steps for this patch, should I keep it as part > of my patchset if accepted? Or should I post it separately? Here it > is v2 for comments. This looks plausible to me, but of course needs more eyes and more testing. Thanx, Paul > [PATCHv2 RFC] rculist_bl: add hlist_bl_for_each_entry_continue_rcu > > Change the old hlist_bl_first_rcu to hlist_bl_first_rcu_dereference > to indicate that it is a RCU dereference. > > Add hlist_bl_next_rcu and hlist_bl_first_rcu to use RCU pointers > and use them to fix sparse warnings. > > Add hlist_bl_for_each_entry_continue_rcu. > > Signed-off-by: Julian Anastasov <ja@ssi.bg> > --- > include/linux/rculist_bl.h | 49 +++++++++++++++++++++++++++++++------- > 1 file changed, 40 insertions(+), 9 deletions(-) > > diff --git a/include/linux/rculist_bl.h b/include/linux/rculist_bl.h > index 0b952d06eb0b..36363b876e53 100644 > --- a/include/linux/rculist_bl.h > +++ b/include/linux/rculist_bl.h > @@ -8,21 +8,31 @@ > #include <linux/list_bl.h> > #include <linux/rcupdate.h> > > +/* return the first ptr or next element in an RCU protected list */ > +#define hlist_bl_first_rcu(head) \ > + (*((struct hlist_bl_node __rcu **)(&(head)->first))) > +#define hlist_bl_next_rcu(node) \ > + (*((struct hlist_bl_node __rcu **)(&(node)->next))) > + > static inline void hlist_bl_set_first_rcu(struct hlist_bl_head *h, > struct hlist_bl_node *n) > { > LIST_BL_BUG_ON((unsigned long)n & LIST_BL_LOCKMASK); > LIST_BL_BUG_ON(((unsigned long)h->first & LIST_BL_LOCKMASK) != > LIST_BL_LOCKMASK); > - rcu_assign_pointer(h->first, > + rcu_assign_pointer(hlist_bl_first_rcu(h), > (struct hlist_bl_node *)((unsigned long)n | LIST_BL_LOCKMASK)); > } > > -static inline struct hlist_bl_node *hlist_bl_first_rcu(struct hlist_bl_head *h) > -{ > - return (struct hlist_bl_node *) > - ((unsigned long)rcu_dereference_check(h->first, hlist_bl_is_locked(h)) & ~LIST_BL_LOCKMASK); > -} > +#define hlist_bl_first_rcu_dereference(head) \ > +({ \ > + struct hlist_bl_head *__head = (head); \ > + \ > + (struct hlist_bl_node *) \ > + ((unsigned long)rcu_dereference_check(hlist_bl_first_rcu(__head), \ > + hlist_bl_is_locked(__head)) & \ > + ~LIST_BL_LOCKMASK); \ > +}) > > /** > * hlist_bl_del_rcu - deletes entry from hash list without re-initialization > @@ -73,7 +83,7 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, > { > struct hlist_bl_node *first; > > - /* don't need hlist_bl_first_rcu because we're under lock */ > + /* don't need hlist_bl_first_rcu* because we're under lock */ > first = hlist_bl_first(h); > > n->next = first; > @@ -93,9 +103,30 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, > * > */ > #define hlist_bl_for_each_entry_rcu(tpos, pos, head, member) \ > - for (pos = hlist_bl_first_rcu(head); \ > + for (pos = hlist_bl_first_rcu_dereference(head); \ > pos && \ > ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ > - pos = rcu_dereference_raw(pos->next)) > + pos = rcu_dereference_raw(hlist_bl_next_rcu(pos))) > + > +/** > + * hlist_bl_for_each_entry_continue_rcu - continue iteration over list of given > + * type > + * @tpos: the type * to use as a loop cursor. > + * @pos: the &struct hlist_bl_node to use as a loop cursor. > + * @member: the name of the hlist_bl_node within the struct. > + * > + * Continue to iterate over list of given type, continuing after > + * the current position which must have been in the list when the RCU read > + * lock was taken. > + * This would typically require either that you obtained the node from a > + * previous walk of the list in the same RCU read-side critical section, or > + * that you held some sort of non-RCU reference (such as a reference count) > + * to keep the node alive *and* in the list. > + */ > +#define hlist_bl_for_each_entry_continue_rcu(tpos, pos, member) \ > + for (pos = rcu_dereference_raw(hlist_bl_next_rcu(&(tpos)->member)); \ > + pos && \ > + ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ > + pos = rcu_dereference_raw(hlist_bl_next_rcu(pos))) > > #endif > -- > 2.41.0 > > > Regards > > -- > Julian Anastasov <ja@ssi.bg> >
diff --git a/include/linux/rculist_bl.h b/include/linux/rculist_bl.h index 0b952d06eb0b..93a757793d83 100644 --- a/include/linux/rculist_bl.h +++ b/include/linux/rculist_bl.h @@ -24,6 +24,10 @@ static inline struct hlist_bl_node *hlist_bl_first_rcu(struct hlist_bl_head *h) ((unsigned long)rcu_dereference_check(h->first, hlist_bl_is_locked(h)) & ~LIST_BL_LOCKMASK); } +/* return the next element in an RCU protected list */ +#define hlist_bl_next_rcu(node) \ + (*((struct hlist_bl_node __rcu **)(&(node)->next))) + /** * hlist_bl_del_rcu - deletes entry from hash list without re-initialization * @n: the element to delete from the hash list. @@ -98,4 +102,17 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n, ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ pos = rcu_dereference_raw(pos->next)) +/** + * hlist_bl_for_each_entry_continue_rcu - iterate over a list continuing after + * current point + * @tpos: the type * to use as a loop cursor. + * @pos: the &struct hlist_bl_node to use as a loop cursor. + * @member: the name of the hlist_bl_node within the struct. + */ +#define hlist_bl_for_each_entry_continue_rcu(tpos, pos, member) \ + for (pos = rcu_dereference_raw(hlist_bl_next_rcu(&(tpos)->member)); \ + pos && \ + ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \ + pos = rcu_dereference_raw(hlist_bl_next_rcu(pos))) + #endif
Add hlist_bl_for_each_entry_continue_rcu and hlist_bl_next_rcu Signed-off-by: Julian Anastasov <ja@ssi.bg> --- include/linux/rculist_bl.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)