| Message ID | 20190601222738.6856-2-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Harden list_for_each_entry_rcu() and family | expand |
On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote: > +#define list_for_each_entry_rcu(pos, head, member, cond...) \ > + if (COUNT_VARGS(cond) != 0) { \ > + __list_check_rcu_cond(0, ## cond); \ > + } else { \ > + __list_check_rcu(); \ > + } \ > + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > + &pos->member != (head); \ > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) > > /** > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, > * the _rcu list-mutation primitives such as hlist_add_head_rcu() > * as long as the traversal is guarded by rcu_read_lock(). > */ > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \ > + if (COUNT_VARGS(cond) != 0) { \ > + __list_check_rcu_cond(0, ## cond); \ > + } else { \ > + __list_check_rcu(); \ > + } \ > for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ > typeof(*(pos)), member); \ > pos; \ This breaks code like: if (...) list_for_each_entry_rcu(...); as they are no longer a single statement. You'll have to frob it into the initializer part of the for statement.
On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote: > On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote: > > +#define list_for_each_entry_rcu(pos, head, member, cond...) \ > > + if (COUNT_VARGS(cond) != 0) { \ > > + __list_check_rcu_cond(0, ## cond); \ > > + } else { \ > > + __list_check_rcu(); \ > > + } \ > > + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > > + &pos->member != (head); \ > > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) > > > > /** > > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, > > * the _rcu list-mutation primitives such as hlist_add_head_rcu() > > * as long as the traversal is guarded by rcu_read_lock(). > > */ > > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \ > > + if (COUNT_VARGS(cond) != 0) { \ > > + __list_check_rcu_cond(0, ## cond); \ > > + } else { \ > > + __list_check_rcu(); \ > > + } \ > > for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ > > typeof(*(pos)), member); \ > > pos; \ > > > This breaks code like: > > if (...) > list_for_each_entry_rcu(...); > > as they are no longer a single statement. You'll have to frob it into > the initializer part of the for statement. Thanks a lot for that. I fixed it as below (diff is on top of the patch): If not for that '##' , I could have abstracted the whole if/else expression into its own macro and called it from list_for_each_entry_rcu() to keep it more clean. ---8<----------------------- diff --git a/include/linux/rculist.h b/include/linux/rculist.h index b641fdd9f1a2..cc742d294bb0 100644 --- a/include/linux/rculist.h +++ b/include/linux/rculist.h @@ -371,12 +372,15 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, * as long as the traversal is guarded by rcu_read_lock(). */ #define list_for_each_entry_rcu(pos, head, member, cond...) \ - if (COUNT_VARGS(cond) != 0) { \ - __list_check_rcu_cond(0, ## cond); \ - } else { \ - __list_check_rcu(); \ - } \ - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ + for ( \ + ({ \ + if (COUNT_VARGS(cond) != 0) { \ + __list_check_rcu_cond(0, ## cond); \ + } else { \ + __list_check_rcu_nocond(); \ + } \ + }), \ + pos = list_entry_rcu((head)->next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) @@ -649,12 +653,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, * as long as the traversal is guarded by rcu_read_lock(). */ #define hlist_for_each_entry_rcu(pos, head, member, cond...) \ - if (COUNT_VARGS(cond) != 0) { \ - __list_check_rcu_cond(0, ## cond); \ - } else { \ - __list_check_rcu(); \ - } \ - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ + for ( \ + ({ \ + if (COUNT_VARGS(cond) != 0) { \ + __list_check_rcu_cond(0, ## cond); \ + } else { \ + __list_check_rcu_nocond(); \ + } \ + }), \ + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ typeof(*(pos)), member); \ pos; \ pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
On Mon, Jun 03, 2019 at 10:18:47AM -0400, Joel Fernandes wrote: > On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote: > > On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote: > > > +#define list_for_each_entry_rcu(pos, head, member, cond...) \ > > > + if (COUNT_VARGS(cond) != 0) { \ > > > + __list_check_rcu_cond(0, ## cond); \ > > > + } else { \ > > > + __list_check_rcu(); \ > > > + } \ > > > + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > > > + &pos->member != (head); \ > > > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) > > > > > > /** > > > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, > > > * the _rcu list-mutation primitives such as hlist_add_head_rcu() > > > * as long as the traversal is guarded by rcu_read_lock(). > > > */ > > > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \ > > > + if (COUNT_VARGS(cond) != 0) { \ > > > + __list_check_rcu_cond(0, ## cond); \ > > > + } else { \ > > > + __list_check_rcu(); \ > > > + } \ > > > for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ > > > typeof(*(pos)), member); \ > > > pos; \ > > > > > > This breaks code like: > > > > if (...) > > list_for_each_entry_rcu(...); > > > > as they are no longer a single statement. You'll have to frob it into > > the initializer part of the for statement. > > Thanks a lot for that. I fixed it as below (diff is on top of the patch): > > If not for that '##' , I could have abstracted the whole if/else > expression into its own macro and called it from list_for_each_entry_rcu() to > keep it more clean. Actually was able to roll the if/else into its own macro as well, thus keeping it clean. thanks! ---8<----------------------- diff --git a/include/linux/rculist.h b/include/linux/rculist.h index b641fdd9f1a2..cc9c382b080c 100644 --- a/include/linux/rculist.h +++ b/include/linux/rculist.h @@ -43,7 +43,11 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list) /* * Check during list traversal that we are within an RCU reader */ -#define __list_check_rcu() \ + +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) + +#define __list_check_rcu_nocond() \ RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), \ "RCU-list traversed in non-reader section!") @@ -59,6 +63,16 @@ static inline void __list_check_rcu_cond(int dummy, ...) RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), "RCU-list traversed in non-reader section!"); } + +#define __list_check_rcu(cond...) \ + ({ \ + if (COUNT_VARGS(cond) != 0) { \ + __list_check_rcu_cond(0, ## cond); \ + } else { \ + __list_check_rcu_nocond(); \ + } \ + }) + /* * Insert a new entry between two known consecutive entries. * @@ -357,9 +371,6 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, member) : NULL; \ }) -#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 -#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) - /** * list_for_each_entry_rcu - iterate over rcu list of given type * @pos: the type * to use as a loop cursor. @@ -371,12 +382,8 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, * as long as the traversal is guarded by rcu_read_lock(). */ #define list_for_each_entry_rcu(pos, head, member, cond...) \ - if (COUNT_VARGS(cond) != 0) { \ - __list_check_rcu_cond(0, ## cond); \ - } else { \ - __list_check_rcu(); \ - } \ - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ + for (__list_check_rcu(cond), \ + pos = list_entry_rcu((head)->next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) @@ -649,12 +656,8 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, * as long as the traversal is guarded by rcu_read_lock(). */ #define hlist_for_each_entry_rcu(pos, head, member, cond...) \ - if (COUNT_VARGS(cond) != 0) { \ - __list_check_rcu_cond(0, ## cond); \ - } else { \ - __list_check_rcu(); \ - } \ - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ + for (__list_check_rcu(cond), \ + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ typeof(*(pos)), member); \ pos; \ pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
On Mon, 3 Jun 2019 10:18:47 -0400 Joel Fernandes <joel@joelfernandes.org> wrote: > On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote: > > On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote: > > > +#define list_for_each_entry_rcu(pos, head, member, cond...) \ > > > + if (COUNT_VARGS(cond) != 0) { \ > > > + __list_check_rcu_cond(0, ## cond); \ > > > + } else { \ > > > + __list_check_rcu(); \ > > > + } \ > > > + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > > > + &pos->member != (head); \ > > > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) > > > > > > /** > > > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, > > > * the _rcu list-mutation primitives such as hlist_add_head_rcu() > > > * as long as the traversal is guarded by rcu_read_lock(). > > > */ > > > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \ > > > + if (COUNT_VARGS(cond) != 0) { \ > > > + __list_check_rcu_cond(0, ## cond); \ > > > + } else { \ > > > + __list_check_rcu(); \ > > > + } \ > > > for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ > > > typeof(*(pos)), member); \ > > > pos; \ > > > > > > This breaks code like: > > > > if (...) > > list_for_each_entry_rcu(...); > > > > as they are no longer a single statement. You'll have to frob it into > > the initializer part of the for statement. > > Thanks a lot for that. I fixed it as below (diff is on top of the patch): > > If not for that '##' , I could have abstracted the whole if/else > expression into its own macro and called it from list_for_each_entry_rcu() to > keep it more clean. > > ---8<----------------------- > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > index b641fdd9f1a2..cc742d294bb0 100644 > --- a/include/linux/rculist.h > +++ b/include/linux/rculist.h > @@ -371,12 +372,15 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, > * as long as the traversal is guarded by rcu_read_lock(). > */ > #define list_for_each_entry_rcu(pos, head, member, cond...) \ > - if (COUNT_VARGS(cond) != 0) { \ > - __list_check_rcu_cond(0, ## cond); \ > - } else { \ > - __list_check_rcu(); \ > - } \ > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > + for ( \ > + ({ \ > + if (COUNT_VARGS(cond) != 0) { \ > + __list_check_rcu_cond(0, ## cond); \ > + } else { \ > + __list_check_rcu_nocond(); \ > + } \ > + }), \ For easier to read I would do something like this: #define check_rcu_list(cond) \ ({ \ if (COUNT_VARGS(cond) != 0) \ __list_check_rcu_cond(0, ## cond); \ else \ __list_check_rcu_nocond(); \ }) #define list_for_each_entry_rcu(pos, head, member, cond...) \ for (check_rcu_list(cond), \ -- Steve > + pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > &pos->member != (head); \ > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) > > @@ -649,12 +653,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, > * as long as the traversal is guarded by rcu_read_lock(). > */ > #define hlist_for_each_entry_rcu(pos, head, member, cond...) \ > - if (COUNT_VARGS(cond) != 0) { \ > - __list_check_rcu_cond(0, ## cond); \ > - } else { \ > - __list_check_rcu(); \ > - } \ > - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ > + for ( \ > + ({ \ > + if (COUNT_VARGS(cond) != 0) { \ > + __list_check_rcu_cond(0, ## cond); \ > + } else { \ > + __list_check_rcu_nocond(); \ > + } \ > + }), \ > + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ > typeof(*(pos)), member); \ > pos; \ > pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
On 02/06/2019 00.27, Joel Fernandes (Google) wrote: > This patch adds support for checking RCU reader sections in list > traversal macros. Optionally, if the list macro is called under SRCU or > other lock/mutex protection, then appropriate lockdep expressions can be > passed to make the checks pass. > > Existing list_for_each_entry_rcu() invocations don't need to pass the > optional fourth argument (cond) unless they are under some non-RCU > protection and needs to make lockdep check pass. > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > include/linux/rculist.h | 40 ++++++++++++++++++++++++++++++++++++---- > include/linux/rcupdate.h | 7 +++++++ > kernel/rcu/update.c | 26 ++++++++++++++++++++++++++ > 3 files changed, 69 insertions(+), 4 deletions(-) > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > index e91ec9ddcd30..b641fdd9f1a2 100644 > --- a/include/linux/rculist.h > +++ b/include/linux/rculist.h > @@ -40,6 +40,25 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list) > */ > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next))) > > +/* > + * Check during list traversal that we are within an RCU reader > + */ > +#define __list_check_rcu() \ > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), \ > + "RCU-list traversed in non-reader section!") > + > +static inline void __list_check_rcu_cond(int dummy, ...) > +{ > + va_list ap; > + int cond; > + > + va_start(ap, dummy); > + cond = va_arg(ap, int); > + va_end(ap); > + > + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), > + "RCU-list traversed in non-reader section!"); > +} > /* > * Insert a new entry between two known consecutive entries. > * > @@ -338,6 +357,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, > member) : NULL; \ > }) > > +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) > +> /** > * list_for_each_entry_rcu - iterate over rcu list of given type > * @pos: the type * to use as a loop cursor. > @@ -348,9 +370,14 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, > * the _rcu list-mutation primitives such as list_add_rcu() > * as long as the traversal is guarded by rcu_read_lock(). > */ > -#define list_for_each_entry_rcu(pos, head, member) \ > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > - &pos->member != (head); \ > +#define list_for_each_entry_rcu(pos, head, member, cond...) \ > + if (COUNT_VARGS(cond) != 0) { \ > + __list_check_rcu_cond(0, ## cond); \ > + } else { \ > + __list_check_rcu(); \ > + } \ > + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > + &pos->member != (head); \ > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) Wouldn't something as simple as #define __list_check_rcu(dummy, cond, ...) \ RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ "RCU-list traversed in non-reader section!"); for ( ({ __list_check_rcu(junk, ##cond, 0); }), pos = ... ) work just as well (i.e., no need for two list_check_rcu and list_check_rcu_cond variants)? If there's an optional cond, we use that, if not, we pick the trailing 0, so !cond disappears and it reduces to your __list_check_rcu(). Moreover, this ensures the RCU_LOCKDEP_WARN expansion actually picks up the __LINE__ and __FILE__ where the for loop is used, and not the __FILE__ and __LINE__ of the static inline function from the header file. It also makes it a bit more type safe/type generic (if the cond expression happened to have type long or u64 something rather odd could happen with the inline vararg function). Rasmus
On Tue, Jun 04, 2019 at 06:53:58AM -0400, Steven Rostedt wrote: > On Mon, 3 Jun 2019 10:18:47 -0400 > Joel Fernandes <joel@joelfernandes.org> wrote: > > > On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote: > > > On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote: > > > > +#define list_for_each_entry_rcu(pos, head, member, cond...) \ > > > > + if (COUNT_VARGS(cond) != 0) { \ > > > > + __list_check_rcu_cond(0, ## cond); \ > > > > + } else { \ > > > > + __list_check_rcu(); \ > > > > + } \ > > > > + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > > > > + &pos->member != (head); \ > > > > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) > > > > > > > > /** > > > > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, > > > > * the _rcu list-mutation primitives such as hlist_add_head_rcu() > > > > * as long as the traversal is guarded by rcu_read_lock(). > > > > */ > > > > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \ > > > > + if (COUNT_VARGS(cond) != 0) { \ > > > > + __list_check_rcu_cond(0, ## cond); \ > > > > + } else { \ > > > > + __list_check_rcu(); \ > > > > + } \ > > > > for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ > > > > typeof(*(pos)), member); \ > > > > pos; \ > > > > > > > > > This breaks code like: > > > > > > if (...) > > > list_for_each_entry_rcu(...); > > > > > > as they are no longer a single statement. You'll have to frob it into > > > the initializer part of the for statement. > > > > Thanks a lot for that. I fixed it as below (diff is on top of the patch): > > > > If not for that '##' , I could have abstracted the whole if/else > > expression into its own macro and called it from list_for_each_entry_rcu() to > > keep it more clean. > > > > ---8<----------------------- > > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > > index b641fdd9f1a2..cc742d294bb0 100644 > > --- a/include/linux/rculist.h > > +++ b/include/linux/rculist.h > > @@ -371,12 +372,15 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, > > * as long as the traversal is guarded by rcu_read_lock(). > > */ > > #define list_for_each_entry_rcu(pos, head, member, cond...) \ > > - if (COUNT_VARGS(cond) != 0) { \ > > - __list_check_rcu_cond(0, ## cond); \ > > - } else { \ > > - __list_check_rcu(); \ > > - } \ > > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > > + for ( \ > > + ({ \ > > + if (COUNT_VARGS(cond) != 0) { \ > > + __list_check_rcu_cond(0, ## cond); \ > > + } else { \ > > + __list_check_rcu_nocond(); \ > > + } \ > > + }), \ > > For easier to read I would do something like this: > > #define check_rcu_list(cond) \ > ({ \ > if (COUNT_VARGS(cond) != 0) \ > __list_check_rcu_cond(0, ## cond); \ > else \ > __list_check_rcu_nocond(); \ > }) > > #define list_for_each_entry_rcu(pos, head, member, cond...) \ > for (check_rcu_list(cond), \ Yes, already doing it this way as I replied to Peter here: https://lore.kernel.org/patchwork/patch/1082846/#1278489 Thanks! - Joel
On Tue, Jun 04, 2019 at 04:01:00PM +0200, Rasmus Villemoes wrote: > On 02/06/2019 00.27, Joel Fernandes (Google) wrote: > > This patch adds support for checking RCU reader sections in list > > traversal macros. Optionally, if the list macro is called under SRCU or > > other lock/mutex protection, then appropriate lockdep expressions can be > > passed to make the checks pass. > > > > Existing list_for_each_entry_rcu() invocations don't need to pass the > > optional fourth argument (cond) unless they are under some non-RCU > > protection and needs to make lockdep check pass. > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > include/linux/rculist.h | 40 ++++++++++++++++++++++++++++++++++++---- > > include/linux/rcupdate.h | 7 +++++++ > > kernel/rcu/update.c | 26 ++++++++++++++++++++++++++ > > 3 files changed, 69 insertions(+), 4 deletions(-) > > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > > index e91ec9ddcd30..b641fdd9f1a2 100644 > > --- a/include/linux/rculist.h > > +++ b/include/linux/rculist.h > > @@ -40,6 +40,25 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list) > > */ > > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next))) > > > > +/* > > + * Check during list traversal that we are within an RCU reader > > + */ > > +#define __list_check_rcu() \ > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), \ > > + "RCU-list traversed in non-reader section!") > > + > > +static inline void __list_check_rcu_cond(int dummy, ...) > > +{ > > + va_list ap; > > + int cond; > > + > > + va_start(ap, dummy); > > + cond = va_arg(ap, int); > > + va_end(ap); > > + > > + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), > > + "RCU-list traversed in non-reader section!"); > > +} > > /* > > * Insert a new entry between two known consecutive entries. > > * > > @@ -338,6 +357,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, > > member) : NULL; \ > > }) > > > > +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 > > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) > > +> /** > > * list_for_each_entry_rcu - iterate over rcu list of given type > > * @pos: the type * to use as a loop cursor. > > @@ -348,9 +370,14 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, > > * the _rcu list-mutation primitives such as list_add_rcu() > > * as long as the traversal is guarded by rcu_read_lock(). > > */ > > -#define list_for_each_entry_rcu(pos, head, member) \ > > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > > - &pos->member != (head); \ > > +#define list_for_each_entry_rcu(pos, head, member, cond...) \ > > + if (COUNT_VARGS(cond) != 0) { \ > > + __list_check_rcu_cond(0, ## cond); \ > > + } else { \ > > + __list_check_rcu(); \ > > + } \ > > + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ > > + &pos->member != (head); \ > > pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) > > Wouldn't something as simple as > > #define __list_check_rcu(dummy, cond, ...) \ > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > "RCU-list traversed in non-reader section!"); > > for ( ({ __list_check_rcu(junk, ##cond, 0); }), pos = ... ) > > work just as well (i.e., no need for two list_check_rcu and > list_check_rcu_cond variants)? If there's an optional cond, we use that, > if not, we pick the trailing 0, so !cond disappears and it reduces to > your __list_check_rcu(). Moreover, this ensures the RCU_LOCKDEP_WARN > expansion actually picks up the __LINE__ and __FILE__ where the for loop > is used, and not the __FILE__ and __LINE__ of the static inline function > from the header file. It also makes it a bit more type safe/type generic > (if the cond expression happened to have type long or u64 something > rather odd could happen with the inline vararg function). This is much better. I will do it this way. Thank you! - Joel
diff --git a/include/linux/rculist.h b/include/linux/rculist.h index e91ec9ddcd30..b641fdd9f1a2 100644 --- a/include/linux/rculist.h +++ b/include/linux/rculist.h @@ -40,6 +40,25 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list) */ #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next))) +/* + * Check during list traversal that we are within an RCU reader + */ +#define __list_check_rcu() \ + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), \ + "RCU-list traversed in non-reader section!") + +static inline void __list_check_rcu_cond(int dummy, ...) +{ + va_list ap; + int cond; + + va_start(ap, dummy); + cond = va_arg(ap, int); + va_end(ap); + + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), + "RCU-list traversed in non-reader section!"); +} /* * Insert a new entry between two known consecutive entries. * @@ -338,6 +357,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, member) : NULL; \ }) +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) + /** * list_for_each_entry_rcu - iterate over rcu list of given type * @pos: the type * to use as a loop cursor. @@ -348,9 +370,14 @@ static inline void list_splice_tail_init_rcu(struct list_head *list, * the _rcu list-mutation primitives such as list_add_rcu() * as long as the traversal is guarded by rcu_read_lock(). */ -#define list_for_each_entry_rcu(pos, head, member) \ - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ - &pos->member != (head); \ +#define list_for_each_entry_rcu(pos, head, member, cond...) \ + if (COUNT_VARGS(cond) != 0) { \ + __list_check_rcu_cond(0, ## cond); \ + } else { \ + __list_check_rcu(); \ + } \ + for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \ + &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member)) /** @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n, * the _rcu list-mutation primitives such as hlist_add_head_rcu() * as long as the traversal is guarded by rcu_read_lock(). */ -#define hlist_for_each_entry_rcu(pos, head, member) \ +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \ + if (COUNT_VARGS(cond) != 0) { \ + __list_check_rcu_cond(0, ## cond); \ + } else { \ + __list_check_rcu(); \ + } \ for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\ typeof(*(pos)), member); \ pos; \ diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index 922bb6848813..712b464ab960 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -223,6 +223,7 @@ int debug_lockdep_rcu_enabled(void); int rcu_read_lock_held(void); int rcu_read_lock_bh_held(void); int rcu_read_lock_sched_held(void); +int rcu_read_lock_any_held(void); #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ @@ -243,6 +244,12 @@ static inline int rcu_read_lock_sched_held(void) { return !preemptible(); } + +static inline int rcu_read_lock_any_held(void) +{ + return !preemptible(); +} + #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ #ifdef CONFIG_PROVE_RCU diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c index c3bf44ba42e5..9cb30006a5e1 100644 --- a/kernel/rcu/update.c +++ b/kernel/rcu/update.c @@ -298,6 +298,32 @@ int rcu_read_lock_bh_held(void) } EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); +int rcu_read_lock_any_held(void) +{ + int lockdep_opinion = 0; + + if (!debug_lockdep_rcu_enabled()) + return 1; + if (!rcu_is_watching()) + return 0; + if (!rcu_lockdep_current_cpu_online()) + return 0; + + /* Preemptible RCU flavor */ + if (lock_is_held(&rcu_lock_map)) + return 1; + + /* BH flavor */ + if (in_softirq() || irqs_disabled()) + return 1; + + /* Sched flavor */ + if (debug_locks) + lockdep_opinion = lock_is_held(&rcu_sched_lock_map); + return lockdep_opinion || !preemptible(); +} +EXPORT_SYMBOL_GPL(rcu_read_lock_any_held); + #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ /**
This patch adds support for checking RCU reader sections in list traversal macros. Optionally, if the list macro is called under SRCU or other lock/mutex protection, then appropriate lockdep expressions can be passed to make the checks pass. Existing list_for_each_entry_rcu() invocations don't need to pass the optional fourth argument (cond) unless they are under some non-RCU protection and needs to make lockdep check pass. Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- include/linux/rculist.h | 40 ++++++++++++++++++++++++++++++++++++---- include/linux/rcupdate.h | 7 +++++++ kernel/rcu/update.c | 26 ++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-)