[PATCH rcu 1/6] rcu: Update synchronize_rcu_mult() comment for call_rcu_hurry()

[PATCH rcu 1/6] rcu: Update synchronize_rcu_mult() comment for call_rcu_hurry()

[Paul E. McKenney]

Those who have worked with RCU for some time will naturally think in
terms of the long-standing call_rcu() API rather than the much newer
call_rcu_hurry() API.  But it is call_rcu_hurry() that you should normally
pass to synchronize_rcu_mult().  This commit therefore updates the header
comment to point this out.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 include/linux/rcupdate_wait.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
index 699b938358bf..5e0f74f2f8ca 100644
--- a/include/linux/rcupdate_wait.h
+++ b/include/linux/rcupdate_wait.h
@@ -42,6 +42,11 @@ do {									\
  * call_srcu() function, with this wrapper supplying the pointer to the
  * corresponding srcu_struct.
  *
+ * Note that call_rcu_hurry() should be used instead of call_rcu()
+ * because in kernels built with CONFIG_RCU_LAZY=y the delay between the
+ * invocation of call_rcu() and that of the corresponding RCU callback
+ * can be multiple seconds.
+ *
  * The first argument tells Tiny RCU's _wait_rcu_gp() not to
  * bother waiting for RCU.  The reason for this is because anywhere
  * synchronize_rcu_mult() can be called is automatically already a full
-- 
2.40.1

[PATCH rcu 0/6] Miscellaneous fixes for v6.6

[Paul E. McKenney]

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(-)

[PATCH rcu 2/6] rcu: Clarify rcu_is_watching() kernel-doc comment

[Paul E. McKenney]

Make it clear that this function always returns either true or false
without other planned failure modes.

Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1449cb69a0e0..fae9b4e29c93 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp)
 /**
  * rcu_is_watching - see if RCU thinks that the current CPU is not idle
  *
- * Return true if RCU is watching the running CPU, which means that this
- * CPU can safely enter RCU read-side critical sections.  In other words,
- * if the current CPU is not in its idle loop or is in an interrupt or
- * NMI handler, return true.
+ * Return @true if RCU is watching the running CPU and @false otherwise.
+ * An @true return means that this CPU can safely enter RCU read-side
+ * critical sections.
+ *
+ * More specifically, if the current CPU is not deep within its idle
+ * loop, return @true.  Note that rcu_is_watching() will return @true if
+ * invoked from an interrupt or NMI handler, even if that interrupt or
+ * NMI interrupted the CPU while it was deep within its idle loop.
  *
  * Make notrace because it can be called by the internal functions of
  * ftrace, and making this notrace removes unnecessary recursion calls.
-- 
2.40.1

[PATCH rcu 3/6] srcu,notifier: Remove #ifdefs in favor of SRCU Tiny srcu_usage

[Paul E. McKenney]

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 <torvalds@linux-foundation.org>
Cc: atthias Brugger <matthias.bgg@gmail.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Sachin Sant <sachinp@linux.ibm.com>
Cc: Joel Fernandes (Google) <joel@joelfernandes.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 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

[PATCH rcu 4/6] rcu: Mark __rcu_irq_enter_check_tick() ->rcu_urgent_qs load

[Paul E. McKenney]

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 <paulmck@kernel.org>
---
 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

[PATCH rcu 5/6] rcu: Make the rcu_nocb_poll boot parameter usable via boot config

[Paul E. McKenney]

The rcu_nocb_poll kernel boot parameter is defined via early_param(),
whose parsing functions are invoked from parse_early_param() which
is in turn invoked by setup_arch(), which is very early indeed.  It
is invoked so early that the console output timestamps read 0.000000,
in other words, before time begins.

This use of early_param() means that the rcu_nocb_poll kernel boot
parameter cannot usefully be embedded into the kernel image.  Yes, you
can embed it, but setup_boot_config() is invoked from start_kernel()
too late for it to be parsed.

But it makes no sense to parse this parameter so early.  After all,
it cannot do anything until the rcuog kthreads are created, which is
long after rcu_init() time, let alone setup_boot_config() time.

This commit therefore switches the rcu_nocb_poll kernel boot parameter
from early_param() to __setup(), which allows boot-config parsing of
this parameter, in turn allowing it to be embedded into the kernel image.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree_nocb.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
index 43229d2b0c44..5598212d1f27 100644
--- a/kernel/rcu/tree_nocb.h
+++ b/kernel/rcu/tree_nocb.h
@@ -77,9 +77,9 @@ __setup("rcu_nocbs", rcu_nocb_setup);
 static int __init parse_rcu_nocb_poll(char *arg)
 {
 	rcu_nocb_poll = true;
-	return 0;
+	return 1;
 }
-early_param("rcu_nocb_poll", parse_rcu_nocb_poll);
+__setup("rcu_nocb_poll", parse_rcu_nocb_poll);
 
 /*
  * Don't bother bypassing ->cblist if the call_rcu() rate is low.
-- 
2.40.1

[PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Paul E. McKenney]

From: Alan Huang <mmpgouride@gmail.com>

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 <mmpgouride@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 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

Re: [PATCH rcu 3/6] srcu,notifier: Remove #ifdefs in favor of SRCU Tiny srcu_usage

[Linus Torvalds]

On Mon, 17 Jul 2023 at 11:03, Paul E. McKenney <paulmck@kernel.org> 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

Re: [PATCH rcu 3/6] srcu,notifier: Remove #ifdefs in favor of SRCU Tiny srcu_usage

[Paul E. McKenney]

On Mon, Jul 17, 2023 at 11:09:56AM -0700, Linus Torvalds wrote:
> On Mon, 17 Jul 2023 at 11:03, Paul E. McKenney <paulmck@kernel.org> 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

Re: [PATCH rcu 2/6] rcu: Clarify rcu_is_watching() kernel-doc comment

[Joel Fernandes]

Hi Paul,

On 7/17/23 14:03, Paul E. McKenney wrote:
> Make it clear that this function always returns either true or false
> without other planned failure modes.
> 
> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>   kernel/rcu/tree.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1449cb69a0e0..fae9b4e29c93 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp)
>   /**
>    * rcu_is_watching - see if RCU thinks that the current CPU is not idle

Would it be better to modify the 'not idle' to 'not idle from an RCU viewpoint'? 
This matches the comments in ct_nmi_enter() as well.

>    *
> - * Return true if RCU is watching the running CPU, which means that this
> - * CPU can safely enter RCU read-side critical sections.  In other words,
> - * if the current CPU is not in its idle loop or is in an interrupt or
> - * NMI handler, return true.
> + * Return @true if RCU is watching the running CPU and @false otherwise.
> + * An @true return means that this CPU can safely enter RCU read-side
> + * critical sections.
> + *
> + * More specifically, if the current CPU is not deep within its idle
> + * loop, return @true.  Note that rcu_is_watching() will return @true if
> + * invoked from an interrupt or NMI handler, even if that interrupt or
> + * NMI interrupted the CPU while it was deep within its idle loop.

But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task 
running could be idle from RCU's viewpoint? Could that be clarified more?

thanks,

  - Joel


>    *
>    * Make notrace because it can be called by the internal functions of
>    * ftrace, and making this notrace removes unnecessary recursion calls.

Re: [PATCH rcu 4/6] rcu: Mark __rcu_irq_enter_check_tick() ->rcu_urgent_qs load

[Joel Fernandes]

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 <paulmck@kernel.org>
> ---
>   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) <joel@joelfernandes.org>


thanks,

- Joel

Re: [PATCH rcu 1/6] rcu: Update synchronize_rcu_mult() comment for call_rcu_hurry()

[Joel Fernandes]

On 7/17/23 14:03, Paul E. McKenney wrote:
> Those who have worked with RCU for some time will naturally think in
> terms of the long-standing call_rcu() API rather than the much newer
> call_rcu_hurry() API.  But it is call_rcu_hurry() that you should normally
> pass to synchronize_rcu_mult().  This commit therefore updates the header
> comment to point this out.
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>   include/linux/rcupdate_wait.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
> index 699b938358bf..5e0f74f2f8ca 100644
> --- a/include/linux/rcupdate_wait.h
> +++ b/include/linux/rcupdate_wait.h
> @@ -42,6 +42,11 @@ do {									\
>    * call_srcu() function, with this wrapper supplying the pointer to the
>    * corresponding srcu_struct.
>    *
> + * Note that call_rcu_hurry() should be used instead of call_rcu()
> + * because in kernels built with CONFIG_RCU_LAZY=y the delay between the
> + * invocation of call_rcu() and that of the corresponding RCU callback
> + * can be multiple seconds.
> + *
>    * The first argument tells Tiny RCU's _wait_rcu_gp() not to
>    * bother waiting for RCU.  The reason for this is because anywhere
>    * synchronize_rcu_mult() can be called is automatically already a full

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

  - Joel

Re: [PATCH rcu 5/6] rcu: Make the rcu_nocb_poll boot parameter usable via boot config

[Joel Fernandes]

On 7/17/23 14:03, Paul E. McKenney wrote:
> The rcu_nocb_poll kernel boot parameter is defined via early_param(),
> whose parsing functions are invoked from parse_early_param() which
> is in turn invoked by setup_arch(), which is very early indeed.  It
> is invoked so early that the console output timestamps read 0.000000,
> in other words, before time begins.
> 
> This use of early_param() means that the rcu_nocb_poll kernel boot
> parameter cannot usefully be embedded into the kernel image.  Yes, you
> can embed it, but setup_boot_config() is invoked from start_kernel()
> too late for it to be parsed.
> 
> But it makes no sense to parse this parameter so early.  After all,
> it cannot do anything until the rcuog kthreads are created, which is
> long after rcu_init() time, let alone setup_boot_config() time. >
> This commit therefore switches the rcu_nocb_poll kernel boot parameter
> from early_param() to __setup(), which allows boot-config parsing of
> this parameter, in turn allowing it to be embedded into the kernel image.
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>   kernel/rcu/tree_nocb.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> index 43229d2b0c44..5598212d1f27 100644
> --- a/kernel/rcu/tree_nocb.h
> +++ b/kernel/rcu/tree_nocb.h
> @@ -77,9 +77,9 @@ __setup("rcu_nocbs", rcu_nocb_setup);
>   static int __init parse_rcu_nocb_poll(char *arg)
>   {
>   	rcu_nocb_poll = true;
> -	return 0;
> +	return 1;
>   }
> -early_param("rcu_nocb_poll", parse_rcu_nocb_poll);
> +__setup("rcu_nocb_poll", parse_rcu_nocb_poll);

I was trying to see if core_param() could be used. But I am not sure if the RCU 
offload threads are spawned too early for that.

I am Ok with it:
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

  -Joel


>   /*
>    * Don't bother bypassing ->cblist if the call_rcu() rate is low.

Re: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Joel Fernandes]

On 7/17/23 14:03, Paul E. McKenney wrote:
> From: Alan Huang <mmpgouride@gmail.com>
> 
> 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 <mmpgouride@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

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) <joel@joelfernandes.org>

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 {

Re: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Alan Huang]

> 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
> 
> On 7/17/23 14:03, Paul E. McKenney wrote:
>> From: Alan Huang <mmpgouride@gmail.com>
>> 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 <mmpgouride@gmail.com>
>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> 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) <joel@joelfernandes.org>
> 
> 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 {
> 

Re: [PATCH rcu 2/6] rcu: Clarify rcu_is_watching() kernel-doc comment

[Paul E. McKenney]

On Tue, Jul 18, 2023 at 08:52:30AM -0400, Joel Fernandes wrote:
> Hi Paul,
> 
> On 7/17/23 14:03, Paul E. McKenney wrote:
> > Make it clear that this function always returns either true or false
> > without other planned failure modes.
> > 
> > Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >   kernel/rcu/tree.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 1449cb69a0e0..fae9b4e29c93 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp)
> >   /**
> >    * rcu_is_watching - see if RCU thinks that the current CPU is not idle
> 
> Would it be better to modify the 'not idle' to 'not idle from an RCU
> viewpoint'? This matches the comments in ct_nmi_enter() as well.

We have the "if RCU thinks that" earlier.

But maybe something like this?

 * rcu_is_watching - RCU read-side critical sections permitted on current CPU?

> >    *
> > - * Return true if RCU is watching the running CPU, which means that this
> > - * CPU can safely enter RCU read-side critical sections.  In other words,
> > - * if the current CPU is not in its idle loop or is in an interrupt or
> > - * NMI handler, return true.
> > + * Return @true if RCU is watching the running CPU and @false otherwise.
> > + * An @true return means that this CPU can safely enter RCU read-side
> > + * critical sections.
> > + *
> > + * More specifically, if the current CPU is not deep within its idle
> > + * loop, return @true.  Note that rcu_is_watching() will return @true if
> > + * invoked from an interrupt or NMI handler, even if that interrupt or
> > + * NMI interrupted the CPU while it was deep within its idle loop.
> 
> But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task
> running could be idle from RCU's viewpoint? Could that be clarified more?

Perhaps something like this?

 * Although calls to rcu_is_watching() from most parts of the kernel
 * will return @true, there are important exceptions.  For example, if the
 * current CPU is deep within its idle loop, in kernel entry/exit code,
 * or offline, rcu_is_watching() will return @false.

(Where nohz_full CPUs are covered by kernel entry/exit code.)

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> 
> >    *
> >    * Make notrace because it can be called by the internal functions of
> >    * ftrace, and making this notrace removes unnecessary recursion calls.
> 

Re: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Paul E. McKenney]

On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
> 
> > 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
> > 
> > On 7/17/23 14:03, Paul E. McKenney wrote:
> >> From: Alan Huang <mmpgouride@gmail.com>
> >> 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 <mmpgouride@gmail.com>
> >> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > 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) <joel@joelfernandes.org>

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 {
> > 
> 

Re: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Joel Fernandes]

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 <joel@joelfernandes.org> 写道：
>>>
>>> On 7/17/23 14:03, Paul E. McKenney wrote:
>>>> From: Alan Huang <mmpgouride@gmail.com>
>>>> 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 <mmpgouride@gmail.com>
>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>>
>>> 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

Re: [PATCH rcu 2/6] rcu: Clarify rcu_is_watching() kernel-doc comment

[Joel Fernandes]

Hi Paul,

On 7/18/23 14:12, Paul E. McKenney wrote:
> On Tue, Jul 18, 2023 at 08:52:30AM -0400, Joel Fernandes wrote:
>> Hi Paul,
>>
>> On 7/17/23 14:03, Paul E. McKenney wrote:
>>> Make it clear that this function always returns either true or false
>>> without other planned failure modes.
>>>
>>> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> ---
>>>    kernel/rcu/tree.c | 12 ++++++++----
>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>>> index 1449cb69a0e0..fae9b4e29c93 100644
>>> --- a/kernel/rcu/tree.c
>>> +++ b/kernel/rcu/tree.c
>>> @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp)
>>>    /**
>>>     * rcu_is_watching - see if RCU thinks that the current CPU is not idle
>>
>> Would it be better to modify the 'not idle' to 'not idle from an RCU
>> viewpoint'? This matches the comments in ct_nmi_enter() as well.
> 
> We have the "if RCU thinks that" earlier.
> 
> But maybe something like this?
> 
>   * rcu_is_watching - RCU read-side critical sections permitted on current CPU?
> 

Yes, that's better.

>>>     *
>>> - * Return true if RCU is watching the running CPU, which means that this
>>> - * CPU can safely enter RCU read-side critical sections.  In other words,
>>> - * if the current CPU is not in its idle loop or is in an interrupt or
>>> - * NMI handler, return true.
>>> + * Return @true if RCU is watching the running CPU and @false otherwise.
>>> + * An @true return means that this CPU can safely enter RCU read-side
>>> + * critical sections.
>>> + *
>>> + * More specifically, if the current CPU is not deep within its idle
>>> + * loop, return @true.  Note that rcu_is_watching() will return @true if
>>> + * invoked from an interrupt or NMI handler, even if that interrupt or
>>> + * NMI interrupted the CPU while it was deep within its idle loop.
>>
>> But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task
>> running could be idle from RCU's viewpoint? Could that be clarified more?
> 
> Perhaps something like this?
> 
>   * Although calls to rcu_is_watching() from most parts of the kernel
>   * will return @true, there are important exceptions.  For example, if the
>   * current CPU is deep within its idle loop, in kernel entry/exit code,
>   * or offline, rcu_is_watching() will return @false.
> 
> (Where nohz_full CPUs are covered by kernel entry/exit code.)

To me, "kernel exit" does not immediately make the nohz_full CPU case 
obvious. But yes, your suggestion is an improvement so we can go with 
that. :)

Also because we agree on the changes, for next revision:
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

  - Joel

Re: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Paul E. McKenney]

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 <joel@joelfernandes.org> 写道：
> > > > 
> > > > On 7/17/23 14:03, Paul E. McKenney wrote:
> > > > > From: Alan Huang <mmpgouride@gmail.com>
> > > > > 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 <mmpgouride@gmail.com>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > 
> > > > 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

Re: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Alan Huang]

> 2023年7月20日 02:20，Paul E. McKenney <paulmck@kernel.org> 写道：
> 
> 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 <joel@joelfernandes.org> 写道：
>>>>> 
>>>>> On 7/17/23 14:03, Paul E. McKenney wrote:
>>>>>> From: Alan Huang <mmpgouride@gmail.com>
>>>>>> 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 <mmpgouride@gmail.com>
>>>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>>>> 
>>>>> 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

Re: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

[Paul E. McKenney]

On Thu, Jul 20, 2023 at 03:17:58AM +0800, Alan Huang wrote:
> 
> > 2023年7月20日 02:20，Paul E. McKenney <paulmck@kernel.org> 写道：
> > 
> > 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 <joel@joelfernandes.org> 写道：
> >>>>> 
> >>>>> On 7/17/23 14:03, Paul E. McKenney wrote:
> >>>>>> From: Alan Huang <mmpgouride@gmail.com>
> >>>>>> 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 <mmpgouride@gmail.com>
> >>>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> >>>>> 
> >>>>> 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

Re: [PATCH rcu 2/6] rcu: Clarify rcu_is_watching() kernel-doc comment

[Paul E. McKenney]

On Tue, Jul 18, 2023 at 09:56:38PM -0400, Joel Fernandes wrote:
> Hi Paul,
> 
> On 7/18/23 14:12, Paul E. McKenney wrote:
> > On Tue, Jul 18, 2023 at 08:52:30AM -0400, Joel Fernandes wrote:
> > > Hi Paul,
> > > 
> > > On 7/17/23 14:03, Paul E. McKenney wrote:
> > > > Make it clear that this function always returns either true or false
> > > > without other planned failure modes.
> > > > 
> > > > Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >    kernel/rcu/tree.c | 12 ++++++++----
> > > >    1 file changed, 8 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 1449cb69a0e0..fae9b4e29c93 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp)
> > > >    /**
> > > >     * rcu_is_watching - see if RCU thinks that the current CPU is not idle
> > > 
> > > Would it be better to modify the 'not idle' to 'not idle from an RCU
> > > viewpoint'? This matches the comments in ct_nmi_enter() as well.
> > 
> > We have the "if RCU thinks that" earlier.
> > 
> > But maybe something like this?
> > 
> >   * rcu_is_watching - RCU read-side critical sections permitted on current CPU?
> > 
> 
> Yes, that's better.
> 
> > > >     *
> > > > - * Return true if RCU is watching the running CPU, which means that this
> > > > - * CPU can safely enter RCU read-side critical sections.  In other words,
> > > > - * if the current CPU is not in its idle loop or is in an interrupt or
> > > > - * NMI handler, return true.
> > > > + * Return @true if RCU is watching the running CPU and @false otherwise.
> > > > + * An @true return means that this CPU can safely enter RCU read-side
> > > > + * critical sections.
> > > > + *
> > > > + * More specifically, if the current CPU is not deep within its idle
> > > > + * loop, return @true.  Note that rcu_is_watching() will return @true if
> > > > + * invoked from an interrupt or NMI handler, even if that interrupt or
> > > > + * NMI interrupted the CPU while it was deep within its idle loop.
> > > 
> > > But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task
> > > running could be idle from RCU's viewpoint? Could that be clarified more?
> > 
> > Perhaps something like this?
> > 
> >   * Although calls to rcu_is_watching() from most parts of the kernel
> >   * will return @true, there are important exceptions.  For example, if the
> >   * current CPU is deep within its idle loop, in kernel entry/exit code,
> >   * or offline, rcu_is_watching() will return @false.
> > 
> > (Where nohz_full CPUs are covered by kernel entry/exit code.)
> 
> To me, "kernel exit" does not immediately make the nohz_full CPU case
> obvious. But yes, your suggestion is an improvement so we can go with that.
> :)
> 
> Also because we agree on the changes, for next revision:
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Very good, thank you!  The update should appear shortly.  For some
definition of "shortly".  ;-)

							Thanx, Paul