| Message ID | 152403402192.16895.9740762152906281009.stgit2@noble |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Assorted rhashtable improvements. RESEND | expand |
On Wed, Apr 18, 2018 at 04:47:01PM +1000, NeilBrown wrote: > Neither rhashtable_walk_enter() or rhltable_walk_enter() sleep, so > remove the comments which suggest that they do. > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > include/linux/rhashtable.h | 3 --- > lib/rhashtable.c | 3 --- > 2 files changed, 6 deletions(-) > > diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h > index 87d443a5b11d..b01d88e196c2 100644 > --- a/include/linux/rhashtable.h > +++ b/include/linux/rhashtable.h > @@ -1268,9 +1268,6 @@ static inline int rhashtable_walk_init(struct rhashtable *ht, > * For a completely stable walk you should construct your own data > * structure outside the hash table. > * > - * This function may sleep so you must not call it from interrupt > - * context or with spin locks held. It does a naked spin lock so even though we removed the memory allocation you still mustn't call it from interrupt context. Why do you need to do that anyway? Cheers,
On Wed, Apr 18 2018, Herbert Xu wrote: > On Wed, Apr 18, 2018 at 04:47:01PM +1000, NeilBrown wrote: >> Neither rhashtable_walk_enter() or rhltable_walk_enter() sleep, so >> remove the comments which suggest that they do. >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> --- >> include/linux/rhashtable.h | 3 --- >> lib/rhashtable.c | 3 --- >> 2 files changed, 6 deletions(-) >> >> diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h >> index 87d443a5b11d..b01d88e196c2 100644 >> --- a/include/linux/rhashtable.h >> +++ b/include/linux/rhashtable.h >> @@ -1268,9 +1268,6 @@ static inline int rhashtable_walk_init(struct rhashtable *ht, >> * For a completely stable walk you should construct your own data >> * structure outside the hash table. >> * >> - * This function may sleep so you must not call it from interrupt >> - * context or with spin locks held. > > It does a naked spin lock so even though we removed the memory > allocation you still mustn't call it from interrupt context. > > Why do you need to do that anyway? I don't want to do that - I just want the documentation to be correct (or at least, not be blatantly incorrect). The function does not sleep, and is safe to call with spin locks held. Do we need to spell out when it can be called? If so, maybe: This function may be called from any process context, including non-preemptable context, but cannot be called from interrupts. ?? Thanks, NeilBrown
On Thu, Apr 19, 2018 at 08:56:28AM +1000, NeilBrown wrote: > > I don't want to do that - I just want the documentation to be correct > (or at least, not be blatantly incorrect). The function does not sleep, > and is safe to call with spin locks held. > Do we need to spell out when it can be called? If so, maybe: > > This function may be called from any process context, including > non-preemptable context, but cannot be called from interrupts. Just to make it perfectly clear, how about "cannot be called from softirq or hardirq context"? Previously the not able to sleep part completely ruled out any ambiguity but the new wording could confuse people into thinking that this can be called from softirq context where it would be unsafe if mixed with process context usage. Thanks,
On Thu, Apr 19 2018, Herbert Xu wrote: > On Thu, Apr 19, 2018 at 08:56:28AM +1000, NeilBrown wrote: >> >> I don't want to do that - I just want the documentation to be correct >> (or at least, not be blatantly incorrect). The function does not sleep, >> and is safe to call with spin locks held. >> Do we need to spell out when it can be called? If so, maybe: >> >> This function may be called from any process context, including >> non-preemptable context, but cannot be called from interrupts. > > Just to make it perfectly clear, how about "cannot be called from > softirq or hardirq context"? Previously the not able to sleep part > completely ruled out any ambiguity but the new wording could confuse > people into thinking that this can be called from softirq context > where it would be unsafe if mixed with process context usage. > Sound fair. Could you Ack the following? Then I'll resend all the patches that have an ack. I've realised that the "further improve stability of rhashtable_walk" patch isn't actually complete, so I'll withdraw that for now. Thanks, NeilBrown From e16c037398b6c057787437f3a0aaa7fd44c3bee3 Mon Sep 17 00:00:00 2001 From: NeilBrown <neilb@suse.com> Date: Mon, 16 Apr 2018 11:05:39 +1000 Subject: [PATCH] rhashtable: Revise incorrect comment on r{hl,hash}table_walk_enter() Neither rhashtable_walk_enter() or rhltable_walk_enter() sleep, though they do take a spinlock without irq protection. So revise the comments to accurately state the contexts in which these functions can be called. Signed-off-by: NeilBrown <neilb@suse.com> --- include/linux/rhashtable.h | 5 +++-- lib/rhashtable.c | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h index 87d443a5b11d..4e1f535c2034 100644 --- a/include/linux/rhashtable.h +++ b/include/linux/rhashtable.h @@ -1268,8 +1268,9 @@ static inline int rhashtable_walk_init(struct rhashtable *ht, * For a completely stable walk you should construct your own data * structure outside the hash table. * - * This function may sleep so you must not call it from interrupt - * context or with spin locks held. + * This function may be called from any process context, including + * non-preemptable context, but cannot be called from softirq or + * hardirq context. * * You must call rhashtable_walk_exit after this function returns. */ diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 2b2b79974b61..6d490f51174e 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -668,8 +668,9 @@ EXPORT_SYMBOL_GPL(rhashtable_insert_slow); * For a completely stable walk you should construct your own data * structure outside the hash table. * - * This function may sleep so you must not call it from interrupt - * context or with spin locks held. + * This function may be called from any process context, including + * non-preemptable context, but cannot be called from softirq or + * hardirq context. * * You must call rhashtable_walk_exit after this function returns. */
On Mon, Apr 23, 2018 at 11:39:17AM +1000, NeilBrown wrote: > > Sound fair. Could you Ack the following? Then I'll resend all the > patches that have an ack. > I've realised that the "further improve stability of rhashtable_walk" > patch isn't actually complete, so I'll withdraw that for now. Looks good to me. > From e16c037398b6c057787437f3a0aaa7fd44c3bee3 Mon Sep 17 00:00:00 2001 > From: NeilBrown <neilb@suse.com> > Date: Mon, 16 Apr 2018 11:05:39 +1000 > Subject: [PATCH] rhashtable: Revise incorrect comment on > r{hl,hash}table_walk_enter() > > Neither rhashtable_walk_enter() or rhltable_walk_enter() sleep, though > they do take a spinlock without irq protection. > So revise the comments to accurately state the contexts in which > these functions can be called. > > Signed-off-by: NeilBrown <neilb@suse.com> Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Thanks,
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h index 87d443a5b11d..b01d88e196c2 100644 --- a/include/linux/rhashtable.h +++ b/include/linux/rhashtable.h @@ -1268,9 +1268,6 @@ static inline int rhashtable_walk_init(struct rhashtable *ht, * For a completely stable walk you should construct your own data * structure outside the hash table. * - * This function may sleep so you must not call it from interrupt - * context or with spin locks held. - * * You must call rhashtable_walk_exit after this function returns. */ static inline void rhltable_walk_enter(struct rhltable *hlt, diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 2b2b79974b61..19db8e563c40 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -668,9 +668,6 @@ EXPORT_SYMBOL_GPL(rhashtable_insert_slow); * For a completely stable walk you should construct your own data * structure outside the hash table. * - * This function may sleep so you must not call it from interrupt - * context or with spin locks held. - * * You must call rhashtable_walk_exit after this function returns. */ void rhashtable_walk_enter(struct rhashtable *ht, struct rhashtable_iter *iter)
Neither rhashtable_walk_enter() or rhltable_walk_enter() sleep, so remove the comments which suggest that they do. Signed-off-by: NeilBrown <neilb@suse.com> --- include/linux/rhashtable.h | 3 --- lib/rhashtable.c | 3 --- 2 files changed, 6 deletions(-)