Prev: [PATCH 00/10] __rcu annotations, first draft
Next: rcu: annotated list rcu code
From: Alexey Dobriyan on 24 Feb 2010 15:20
On Wed, Feb 24, 2010 at 09:04:03PM +0100, Arnd Bergmann wrote:
> @@ -360,10 +360,12 @@ struct module *find_module(const char *name)
> {
> struct module *mod;
>
> - list_for_each_entry(mod, &modules, list) {
> + rcu_read_lock();
> + list_for_each_entry_rcu(mod, &modules, list) {
> if (strcmp(mod->name, name) == 0)
> return mod;
> }
> + rcu_read_unlock();
> return NULL;
> }
> EXPORT_SYMBOL_GPL(find_module);

modules list is under module_mutex, nothing should be done here.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 24 Feb 2010 15:40
On Wednesday 24 February 2010, Alexey Dobriyan wrote:
> On Wed, Feb 24, 2010 at 09:04:03PM +0100, Arnd Bergmann wrote:
> > @@ -360,10 +360,12 @@ struct module *find_module(const char *name)
> > {
> > struct module *mod;
> >
> > - list_for_each_entry(mod, &modules, list) {
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(mod, &modules, list) {
> > if (strcmp(mod->name, name) == 0)
> > return mod;
> > }
> > + rcu_read_unlock();
> > return NULL;
> > }
> > EXPORT_SYMBOL_GPL(find_module);
>
> modules list is under module_mutex, nothing should be done here.

Ok, this is a significant limitation of the list rcu annotation
then, it's not possible to pass the same list into list_for_each_entry
and list_for_each_entry_rcu with the way I changed the rcu list
definition. I would be possible to do a __list_for_each_entry_rcu
macro that takes an rcu_list_head but does not actually use
rcu_dereference, but I'm not sure if that's good enough.

Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 24 Feb 2010 17:20
On Wed, Feb 24, 2010 at 09:26:09PM +0100, Arnd Bergmann wrote:
> On Wednesday 24 February 2010, Alexey Dobriyan wrote:
> > On Wed, Feb 24, 2010 at 09:04:03PM +0100, Arnd Bergmann wrote:
> > > @@ -360,10 +360,12 @@ struct module *find_module(const char *name)
> > > {
> > > struct module *mod;
> > >
> > > - list_for_each_entry(mod, &modules, list) {
> > > + rcu_read_lock();
> > > + list_for_each_entry_rcu(mod, &modules, list) {
> > > if (strcmp(mod->name, name) == 0)
> > > return mod;
> > > }
> > > + rcu_read_unlock();
> > > return NULL;
> > > }
> > > EXPORT_SYMBOL_GPL(find_module);
> >
> > modules list is under module_mutex, nothing should be done here.
>
> Ok, this is a significant limitation of the list rcu annotation
> then, it's not possible to pass the same list into list_for_each_entry
> and list_for_each_entry_rcu with the way I changed the rcu list
> definition. I would be possible to do a __list_for_each_entry_rcu
> macro that takes an rcu_list_head but does not actually use
> rcu_dereference, but I'm not sure if that's good enough.

Hmmm... If the __rcu annotation was visible at runtime, it would be
easy provide an annotated version of list_for_each_entry_rcu() that
checks for module_mutex being held under lockdep.

Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 24 Feb 2010 18:10
On Wednesday 24 February 2010 23:17:49 Paul E. McKenney wrote:
> > Ok, this is a significant limitation of the list rcu annotation
> > then, it's not possible to pass the same list into list_for_each_entry
> > and list_for_each_entry_rcu with the way I changed the rcu list
> > definition. I would be possible to do a __list_for_each_entry_rcu
> > macro that takes an rcu_list_head but does not actually use
> > rcu_dereference, but I'm not sure if that's good enough.
>
> Hmmm... If the __rcu annotation was visible at runtime, it would be
> easy provide an annotated version of list_for_each_entry_rcu() that
> checks for module_mutex being held under lockdep.

Well, if we keep the struct rcu_list_head and make it mandatory for
rcu protected lists, it could be defined as

struct rcu_list_head {
struct list_head head;
#ifdef CONFIG_PROVE_RCU
bool (*check)(void);
#endif
};

#ifdef CONFIG_PROVE_RCU
#define RCU_LIST_HEAD_INIT_CHECK(__head, __check) \
{ .head = LIST_HEAD_INIT((__head).head), .check = (__check) }
#else
#define RCU_LIST_HEAD_INIT_CHECK(__list,__check) {.head = LIST_HEAD_INIT((__head).head) }
#endif

#define RCU_LIST_HEAD_INIT(head) RCU_LIST_HEAD_INIT_CHECK(head,&rcu_read_lock_held)
#define RCU_LIST_HEAD_INIT_BH(head) RCU_LIST_HEAD_INIT_CHECK(head,&rcu_read_lock_bh_held)

#define list_entry_rcu_check(ptr, type, member, check) \
container_of(rcu_dereference_check((void __rcu __force *)(ptr), check), type, member)

#define list_for_each_entry_rcu(pos, __head, member) \
for (pos = list_entry_rcu_check((__head)->head.next, typeof(*pos), \
member, (__head)->check); \
prefetch(pos->member.next), &pos->member != (head); \
pos = list_entry_rcu_check(pos->member.next, typeof(*pos), \
member, (__head)->check)))

That would let us check all the heads for correct usage, and at the same
time avoid having to annotate all the list entries.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 24 Feb 2010 19:10
On Thu, Feb 25, 2010 at 12:07:47AM +0100, Arnd Bergmann wrote:
> On Wednesday 24 February 2010 23:17:49 Paul E. McKenney wrote:
> > > Ok, this is a significant limitation of the list rcu annotation
> > > then, it's not possible to pass the same list into list_for_each_entry
> > > and list_for_each_entry_rcu with the way I changed the rcu list
> > > definition. I would be possible to do a __list_for_each_entry_rcu
> > > macro that takes an rcu_list_head but does not actually use
> > > rcu_dereference, but I'm not sure if that's good enough.
> >
> > Hmmm... If the __rcu annotation was visible at runtime, it would be
> > easy provide an annotated version of list_for_each_entry_rcu() that
> > checks for module_mutex being held under lockdep.
>
> Well, if we keep the struct rcu_list_head and make it mandatory for
> rcu protected lists, it could be defined as
>
> struct rcu_list_head {
> struct list_head head;
> #ifdef CONFIG_PROVE_RCU
> bool (*check)(void);
> #endif
> };
>
> #ifdef CONFIG_PROVE_RCU
> #define RCU_LIST_HEAD_INIT_CHECK(__head, __check) \
> { .head = LIST_HEAD_INIT((__head).head), .check = (__check) }
> #else
> #define RCU_LIST_HEAD_INIT_CHECK(__list,__check) {.head = LIST_HEAD_INIT((__head).head) }
> #endif
>
> #define RCU_LIST_HEAD_INIT(head) RCU_LIST_HEAD_INIT_CHECK(head,&rcu_read_lock_held)
> #define RCU_LIST_HEAD_INIT_BH(head) RCU_LIST_HEAD_INIT_CHECK(head,&rcu_read_lock_bh_held)
>
> #define list_entry_rcu_check(ptr, type, member, check) \
> container_of(rcu_dereference_check((void __rcu __force *)(ptr), check), type, member)
>
> #define list_for_each_entry_rcu(pos, __head, member) \
> for (pos = list_entry_rcu_check((__head)->head.next, typeof(*pos), \
> member, (__head)->check); \
> prefetch(pos->member.next), &pos->member != (head); \
> pos = list_entry_rcu_check(pos->member.next, typeof(*pos), \
> member, (__head)->check)))
>
> That would let us check all the heads for correct usage, and at the same
> time avoid having to annotate all the list entries.

Cool!!!

The nice thing about this is that we don't end up with the API explosion
for the RCU list primitives. However, it does require that a given
rcu_list_head have a single synchronization-design rule for all uses.
Of course, if there were multiple rules, one could construct a check
that was simply the union of all the rules, but that would miss some
types of errors.

Of course, if this became a problem, there could be an argument to the
->check function that the normal list_for_each_entry_rcu() defaults to
"no effect".

Or is there a better way to handle this?

Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH 00/10] __rcu annotations, first draft
Next: rcu: annotated list rcu code