Hello!
This series has miscellaneous fixes:
1. Update synchronize_rcu_mult() comment for call_rcu_hurry().
2. Clarify rcu_is_watching() kernel-doc comment.
3. srcu,notifier: Remove #ifdefs in favor of SRCU Tiny srcu_usage.
4. Mark __rcu_irq_enter_check_tick() ->rcu_urgent_qs load.
5. Make the rcu_nocb_poll boot parameter usable via boot config.
6. Use WRITE_ONCE() for assignments to ->next for rculist_nulls,
courtesy of Alan Huang.
Thanx, Paul
------------------------------------------------------------------------
b/include/linux/notifier.h | 11 -----------
b/include/linux/rculist_nulls.h | 4 ++--
b/include/linux/rcupdate_wait.h | 5 +++++
b/include/linux/srcutiny.h | 7 +++++++
b/kernel/rcu/tree.c | 12 ++++++++----
b/kernel/rcu/tree_nocb.h | 4 ++--
kernel/rcu/tree.c | 2 +-
7 files changed, 25 insertions(+), 20 deletions(-)
This commit removes two #ifdef directives from include/linux/notifier.h
by causing SRCU Tiny to provide a dummy srcu_usage structure and a dummy
__SRCU_USAGE_INIT() macro.
Suggested-by: Linus Torvalds <[email protected]>
Cc: atthias Brugger <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: "Michał Mirosław" <[email protected]>
Cc: Dmitry Osipenko <[email protected]>
Cc: Sachin Sant <[email protected]>
Cc: Joel Fernandes (Google) <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
include/linux/notifier.h | 11 -----------
include/linux/srcutiny.h | 7 +++++++
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 86544707236a..45702bdcbceb 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -73,9 +73,7 @@ struct raw_notifier_head {
struct srcu_notifier_head {
struct mutex mutex;
-#ifdef CONFIG_TREE_SRCU
struct srcu_usage srcuu;
-#endif
struct srcu_struct srcu;
struct notifier_block __rcu *head;
};
@@ -106,7 +104,6 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh);
#define RAW_NOTIFIER_INIT(name) { \
.head = NULL }
-#ifdef CONFIG_TREE_SRCU
#define SRCU_NOTIFIER_INIT(name, pcpu) \
{ \
.mutex = __MUTEX_INITIALIZER(name.mutex), \
@@ -114,14 +111,6 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh);
.srcuu = __SRCU_USAGE_INIT(name.srcuu), \
.srcu = __SRCU_STRUCT_INIT(name.srcu, name.srcuu, pcpu), \
}
-#else
-#define SRCU_NOTIFIER_INIT(name, pcpu) \
- { \
- .mutex = __MUTEX_INITIALIZER(name.mutex), \
- .head = NULL, \
- .srcu = __SRCU_STRUCT_INIT(name.srcu, name.srcuu, pcpu), \
- }
-#endif
#define ATOMIC_NOTIFIER_HEAD(name) \
struct atomic_notifier_head name = \
diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index ebd72491af99..06ce65518974 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -48,6 +48,13 @@ void srcu_drive_gp(struct work_struct *wp);
#define DEFINE_STATIC_SRCU(name) \
static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name)
+// Dummy structure for srcu_notifier_head.
+struct srcu_usage {
+ char srcuu_dummy;
+};
+
+#define __SRCU_USAGE_INIT(name) { .srcuu_dummy = 0, }
+
void synchronize_srcu(struct srcu_struct *ssp);
/*
--
2.40.1
On Mon, Jul 17, 2023 at 11:09:56AM -0700, Linus Torvalds wrote:
> On Mon, 17 Jul 2023 at 11:03, Paul E. McKenney <[email protected]> wrote:
> >
> > +// Dummy structure for srcu_notifier_head.
> > +struct srcu_usage {
> > + char srcuu_dummy;
> > +};
> > +
> > +#define __SRCU_USAGE_INIT(name) { .srcuu_dummy = 0, }
>
> You really should be able to just do
>
> struct srcu_usage { };
> #define __SRCU_USAGE_INIT(name) { }
>
> which is something we've done for ages for spinlocks in
>
> include/linux/spinlock_types_up.h
>
> because while we had a gcc bug wrt empty structures, that was ages ago
> (ie "gcc-2.x").
>
> See commit a1365647022e ("[PATCH] remove gcc-2 checks") from 2006.
>
> So we've already had these kinds of empty dummy structs for literally
> over a decade in active use. Exactly so that you can avoid having to
> use #ifdef's etc, and can just always assume you have a spinlock, even
> if it doesn't generate any code or any data overhead.
Showing my age again, I guess. ;-)
Thank you for the hint! I will rework this as you suggest.
Thanx, Paul
The rcu_request_urgent_qs_task() function does a cross-CPU store
to ->rcu_urgent_qs, so this commit therefore marks the load in
__rcu_irq_enter_check_tick() with READ_ONCE().
Signed-off-by: Paul E. McKenney <[email protected]>
---
kernel/rcu/tree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index fae9b4e29c93..aec07f2ec638 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -632,7 +632,7 @@ void __rcu_irq_enter_check_tick(void)
// prevents self-deadlock. So we can safely recheck under the lock.
// Note that the nohz_full state currently cannot change.
raw_spin_lock_rcu_node(rdp->mynode);
- if (rdp->rcu_urgent_qs && !rdp->rcu_forced_tick) {
+ if (READ_ONCE(rdp->rcu_urgent_qs) && !rdp->rcu_forced_tick) {
// A nohz_full CPU is in the kernel and RCU needs a
// quiescent state. Turn on the tick!
WRITE_ONCE(rdp->rcu_forced_tick, true);
--
2.40.1
From: Alan Huang <[email protected]>
When the objects managed by rculist_nulls are allocated with
SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
even though it is just now being added, which means the modification of
->next is visible to readers. This patch therefore uses WRITE_ONCE()
for assignments to ->next.
Signed-off-by: Alan Huang <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
include/linux/rculist_nulls.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index ba4c00dd8005..89186c499dd4 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
{
struct hlist_nulls_node *first = h->first;
- n->next = first;
+ WRITE_ONCE(n->next, first);
WRITE_ONCE(n->pprev, &h->first);
rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
if (!is_a_nulls(first))
@@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
last = i;
if (last) {
- n->next = last->next;
+ WRITE_ONCE(n->next, last->next);
n->pprev = &last->next;
rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
} else {
--
2.40.1
On Mon, 17 Jul 2023 at 11:03, Paul E. McKenney <[email protected]> wrote:
>
> +// Dummy structure for srcu_notifier_head.
> +struct srcu_usage {
> + char srcuu_dummy;
> +};
> +
> +#define __SRCU_USAGE_INIT(name) { .srcuu_dummy = 0, }
You really should be able to just do
struct srcu_usage { };
#define __SRCU_USAGE_INIT(name) { }
which is something we've done for ages for spinlocks in
include/linux/spinlock_types_up.h
because while we had a gcc bug wrt empty structures, that was ages ago
(ie "gcc-2.x").
See commit a1365647022e ("[PATCH] remove gcc-2 checks") from 2006.
So we've already had these kinds of empty dummy structs for literally
over a decade in active use. Exactly so that you can avoid having to
use #ifdef's etc, and can just always assume you have a spinlock, even
if it doesn't generate any code or any data overhead.
Linus
On 7/17/23 14:03, Paul E. McKenney wrote:
> The rcu_request_urgent_qs_task() function does a cross-CPU store
> to ->rcu_urgent_qs, so this commit therefore marks the load in
> __rcu_irq_enter_check_tick() with READ_ONCE().
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> ---
> kernel/rcu/tree.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index fae9b4e29c93..aec07f2ec638 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -632,7 +632,7 @@ void __rcu_irq_enter_check_tick(void)
> // prevents self-deadlock. So we can safely recheck under the lock.
> // Note that the nohz_full state currently cannot change.
> raw_spin_lock_rcu_node(rdp->mynode);
> - if (rdp->rcu_urgent_qs && !rdp->rcu_forced_tick) {
> + if (READ_ONCE(rdp->rcu_urgent_qs) && !rdp->rcu_forced_tick) {
> // A nohz_full CPU is in the kernel and RCU needs a
> // quiescent state. Turn on the tick!
> WRITE_ONCE(rdp->rcu_forced_tick,
Reviewed-by: Joel Fernandes (Google) <[email protected]>
thanks,
- Joel
On 7/17/23 14:03, Paul E. McKenney wrote:
> From: Alan Huang <[email protected]>
>
> When the objects managed by rculist_nulls are allocated with
> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> even though it is just now being added, which means the modification of
> ->next is visible to readers. This patch therefore uses WRITE_ONCE()
> for assignments to ->next.
>
> Signed-off-by: Alan Huang <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
But incremental progress and all, so this LGTM:
Reviewed-by: Joel Fernandes (Google) <[email protected]>
thanks,
- Joel
> ---
> include/linux/rculist_nulls.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> index ba4c00dd8005..89186c499dd4 100644
> --- a/include/linux/rculist_nulls.h
> +++ b/include/linux/rculist_nulls.h
> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
> {
> struct hlist_nulls_node *first = h->first;
>
> - n->next = first;
> + WRITE_ONCE(n->next, first);
> WRITE_ONCE(n->pprev, &h->first);
> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
> if (!is_a_nulls(first))
> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
> last = i;
>
> if (last) {
> - n->next = last->next;
> + WRITE_ONCE(n->next, last->next);
> n->pprev = &last->next;
> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> } else {
> 2023年7月18日 21:49，Joel Fernandes <[email protected]> 写道：
>
> On 7/17/23 14:03, Paul E. McKenney wrote:
>> From: Alan Huang <[email protected]>
>> When the objects managed by rculist_nulls are allocated with
>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
>> even though it is just now being added, which means the modification of
>> ->next is visible to readers. This patch therefore uses WRITE_ONCE()
>> for assignments to ->next.
>> Signed-off-by: Alan Huang <[email protected]>
>> Signed-off-by: Paul E. McKenney <[email protected]>
>
> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
>
> But incremental progress and all, so this LGTM:
> Reviewed-by: Joel Fernandes (Google) <[email protected]>
>
> thanks,
>
> - Joel
>
>
>> ---
>> include/linux/rculist_nulls.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
>> index ba4c00dd8005..89186c499dd4 100644
>> --- a/include/linux/rculist_nulls.h
>> +++ b/include/linux/rculist_nulls.h
>> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
>> {
>> struct hlist_nulls_node *first = h->first;
>> - n->next = first;
>> + WRITE_ONCE(n->next, first);
>> WRITE_ONCE(n->pprev, &h->first);
>> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
>> if (!is_a_nulls(first))
>> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
>> last = i;
>> if (last) {
>> - n->next = last->next;
>> + WRITE_ONCE(n->next, last->next);
>> n->pprev = &last->next;
>> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
>> } else {
>
On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
>
> > 2023年7月18日 21:49，Joel Fernandes <[email protected]> 写道：
> >
> > On 7/17/23 14:03, Paul E. McKenney wrote:
> >> From: Alan Huang <[email protected]>
> >> When the objects managed by rculist_nulls are allocated with
> >> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> >> even though it is just now being added, which means the modification of
> >> ->next is visible to readers. This patch therefore uses WRITE_ONCE()
> >> for assignments to ->next.
> >> Signed-off-by: Alan Huang <[email protected]>
> >> Signed-off-by: Paul E. McKenney <[email protected]>
> >
> > Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
>
> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
It turns out that different traversal synchronization designs want
different pointers using WRITE_ONCE().
> > But incremental progress and all, so this LGTM:
> > Reviewed-by: Joel Fernandes (Google) <[email protected]>
I will apply all four on my next rebase, thank you!
Thanx, Paul
> > thanks,
> >
> > - Joel
> >
> >
> >> ---
> >> include/linux/rculist_nulls.h | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> >> index ba4c00dd8005..89186c499dd4 100644
> >> --- a/include/linux/rculist_nulls.h
> >> +++ b/include/linux/rculist_nulls.h
> >> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
> >> {
> >> struct hlist_nulls_node *first = h->first;
> >> - n->next = first;
> >> + WRITE_ONCE(n->next, first);
> >> WRITE_ONCE(n->pprev, &h->first);
> >> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
> >> if (!is_a_nulls(first))
> >> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
> >> last = i;
> >> if (last) {
> >> - n->next = last->next;
> >> + WRITE_ONCE(n->next, last->next);
> >> n->pprev = &last->next;
> >> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> >> } else {
> >
>
On 7/18/23 14:32, Paul E. McKenney wrote:
> On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
>>
>>> 2023年7月18日 21:49，Joel Fernandes <[email protected]> 写道：
>>>
>>> On 7/17/23 14:03, Paul E. McKenney wrote:
>>>> From: Alan Huang <[email protected]>
>>>> When the objects managed by rculist_nulls are allocated with
>>>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
>>>> even though it is just now being added, which means the modification of
>>>> ->next is visible to readers. This patch therefore uses WRITE_ONCE()
>>>> for assignments to ->next.
>>>> Signed-off-by: Alan Huang <[email protected]>
>>>> Signed-off-by: Paul E. McKenney <[email protected]>
>>>
>>> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
>>
>> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
>
> It turns out that different traversal synchronization designs want
> different pointers using WRITE_ONCE().
Thank you Alan and Paul,
Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it
can be removed?
- Joel
On Tue, Jul 18, 2023 at 09:48:59PM -0400, Joel Fernandes wrote:
>
>
> On 7/18/23 14:32, Paul E. McKenney wrote:
> > On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
> > >
> > > > 2023年7月18日 21:49，Joel Fernandes <[email protected]> 写道：
> > > >
> > > > On 7/17/23 14:03, Paul E. McKenney wrote:
> > > > > From: Alan Huang <[email protected]>
> > > > > When the objects managed by rculist_nulls are allocated with
> > > > > SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> > > > > even though it is just now being added, which means the modification of
> > > > > ->next is visible to readers. This patch therefore uses WRITE_ONCE()
> > > > > for assignments to ->next.
> > > > > Signed-off-by: Alan Huang <[email protected]>
> > > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > >
> > > > Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
> > >
> > > Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
> >
> > It turns out that different traversal synchronization designs want
> > different pointers using WRITE_ONCE().
>
> Thank you Alan and Paul,
>
> Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it can
> be removed?
Either that or the people who removed uses injected bugs...
But if this one really does go away, do we need ->pprev to be
protected by _ONCE()?
Thanx, Paul
> 2023年7月20日 02:20，Paul E. McKenney <[email protected]> 写道：
>
> On Tue, Jul 18, 2023 at 09:48:59PM -0400, Joel Fernandes wrote:
>>
>>
>> On 7/18/23 14:32, Paul E. McKenney wrote:
>>> On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
>>>>
>>>>> 2023年7月18日 21:49，Joel Fernandes <[email protected]> 写道：
>>>>>
>>>>> On 7/17/23 14:03, Paul E. McKenney wrote:
>>>>>> From: Alan Huang <[email protected]>
>>>>>> When the objects managed by rculist_nulls are allocated with
>>>>>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
>>>>>> even though it is just now being added, which means the modification of
>>>>>> ->next is visible to readers. This patch therefore uses WRITE_ONCE()
>>>>>> for assignments to ->next.
>>>>>> Signed-off-by: Alan Huang <[email protected]>
>>>>>> Signed-off-by: Paul E. McKenney <[email protected]>
>>>>>
>>>>> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
>>>>
>>>> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
>>>
>>> It turns out that different traversal synchronization designs want
>>> different pointers using WRITE_ONCE().
>>
>> Thank you Alan and Paul,
>>
>> Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it can
>> be removed?
>
> Either that or the people who removed uses injected bugs...
It has never been used.
That said, the data race has been there almost for four years.
And the network people use sk_unhashed() for both hlist_node and hlist_nulls_node.
So, I plan to use hlist_unhashed_lockless() in sk_unhashed(), that will be one of my future patches.
>
> But if this one really does go away, do we need ->pprev to be
> protected by _ONCE()?
The ->pprev thing is what I’m currently working on. :)
>
> Thanx, Paul
On Thu, Jul 20, 2023 at 03:17:58AM +0800, Alan Huang wrote:
>
> > 2023年7月20日 02:20，Paul E. McKenney <[email protected]> 写道：
> >
> > On Tue, Jul 18, 2023 at 09:48:59PM -0400, Joel Fernandes wrote:
> >>
> >>
> >> On 7/18/23 14:32, Paul E. McKenney wrote:
> >>> On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
> >>>>
> >>>>> 2023年7月18日 21:49，Joel Fernandes <[email protected]> 写道：
> >>>>>
> >>>>> On 7/17/23 14:03, Paul E. McKenney wrote:
> >>>>>> From: Alan Huang <[email protected]>
> >>>>>> When the objects managed by rculist_nulls are allocated with
> >>>>>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> >>>>>> even though it is just now being added, which means the modification of
> >>>>>> ->next is visible to readers. This patch therefore uses WRITE_ONCE()
> >>>>>> for assignments to ->next.
> >>>>>> Signed-off-by: Alan Huang <[email protected]>
> >>>>>> Signed-off-by: Paul E. McKenney <[email protected]>
> >>>>>
> >>>>> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
> >>>>
> >>>> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
> >>>
> >>> It turns out that different traversal synchronization designs want
> >>> different pointers using WRITE_ONCE().
> >>
> >> Thank you Alan and Paul,
> >>
> >> Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it can
> >> be removed?
> >
> > Either that or the people who removed uses injected bugs...
>
> It has never been used.
>
> That said, the data race has been there almost for four years.
>
> And the network people use sk_unhashed() for both hlist_node and hlist_nulls_node.
> So, I plan to use hlist_unhashed_lockless() in sk_unhashed(), that will be one of my future patches.
>
> >
> > But if this one really does go away, do we need ->pprev to be
> > protected by _ONCE()?
>
> The ->pprev thing is what I’m currently working on. :)
Very good, looking forward to seeing what you come up with!
Thanx, Paul