[PATCH 0/3] srcu: shrink the num of srcu_size_state

[PATCH 0/3] srcu: shrink the num of srcu_size_state

[Pingfan Liu]

This series fix a bug and improve the note of SRCU_SIZE_*

Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
To: rcu@vger.kernel.org

Pingfan Liu (3):
  srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL
  srcu: protect against concurrent srcu_gp_end()
  srcu: Decrease the srcu size state numbers

 include/linux/srcutree.h | 15 +++++++++------
 kernel/rcu/srcutree.c    | 19 ++++++++-----------
 2 files changed, 17 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH 1/3] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Pingfan Liu]

The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
And it is not needed. Because counter_wrap_check has guarantee that both
srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
srcu_gp_seq and no false alarm will be raised by the statement in
srcu_gp_start_if_needed()

  ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)

As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
used immediately, not need to wait for another srcu_gp_end() to update
the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
To: rcu@vger.kernel.org
---
 kernel/rcu/srcutree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..fe0759d89c2d 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1092,7 +1092,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	check_init_srcu_struct(ssp);
 	idx = srcu_read_lock(ssp);
 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
-	if (ss_state < SRCU_SIZE_WAIT_CALL)
+	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
 		sdp = per_cpu_ptr(ssp->sda, 0);
 	else
 		sdp = raw_cpu_ptr(ssp->sda);
-- 
2.31.1

[PATCH 2/3] srcu: protect against concurrent srcu_gp_end()

[Pingfan Liu]

At present, the code chunk "Transition to big if needed" is out of the
lock srcu_cb_mutex, which allows the following scene:

    srcu_gp_end() on cpu1                srcu_gp_end() on cpu2

    init_srcu_struct_nodes()
    ^^  scheduled out in the middle
                                         init_srcu_struct_nodes()
                                         ^^ overwrite the context set up by cpu1

Squashing out the race window by putting the code chunk under the
protection of srcu_cb_mutex.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
To: rcu@vger.kernel.org
---
 kernel/rcu/srcutree.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index fe0759d89c2d..281cd69fe804 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -811,6 +811,13 @@ static void srcu_gp_end(struct srcu_struct *ssp)
 			spin_unlock_irqrestore_rcu_node(sdp, flags);
 		}
 
+	/* Transition to big if needed. */
+	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
+		if (ss_state == SRCU_SIZE_ALLOC)
+			init_srcu_struct_nodes(ssp, GFP_KERNEL);
+		else
+			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
+	}
 	/* Callback initiation done, allow grace periods after next. */
 	mutex_unlock(&ssp->srcu_cb_mutex);
 
@@ -826,13 +833,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
 		spin_unlock_irq_rcu_node(ssp);
 	}
 
-	/* Transition to big if needed. */
-	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
-		if (ss_state == SRCU_SIZE_ALLOC)
-			init_srcu_struct_nodes(ssp, GFP_KERNEL);
-		else
-			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
-	}
 }
 
 /*
-- 
2.31.1

[PATCH 3/3] srcu: Decrease the srcu size state numbers

[Pingfan Liu]

With the previous patch, srcu_cb_mutex is held when setting
SRCU_SIZE_WAIT_BARRIER. It means that only two seq snap value can exist.
And these two snaps can not see the active snp tree if
srcu_gp_start_if_needed() fetches the srcu_size_state before it is
updated to SRCU_SIZE_WAIT_BARRIER.

After dispatching these two snap value's callbacks, the mask in
snp->srcu_data_have_cbs[idx] is assured to be right. Hence the size
state following SRCU_SIZE_WAIT_BARRIER can be shrink to three.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
To: rcu@vger.kernel.org
---
 include/linux/srcutree.h | 15 +++++++++------
 kernel/rcu/srcutree.c    |  3 ---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index e3014319d1ad..34cb9435c14d 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -94,13 +94,16 @@ struct srcu_struct {
 /* Values for size state variable (->srcu_size_state). */
 #define SRCU_SIZE_SMALL		0
 #define SRCU_SIZE_ALLOC		1
+/* If snp tree is initialized */
 #define SRCU_SIZE_WAIT_BARRIER	2
-#define SRCU_SIZE_WAIT_CALL	3
-#define SRCU_SIZE_WAIT_CBS1	4
-#define SRCU_SIZE_WAIT_CBS2	5
-#define SRCU_SIZE_WAIT_CBS3	6
-#define SRCU_SIZE_WAIT_CBS4	7
-#define SRCU_SIZE_BIG		8
+/*
+ * The following two states for the in-flight seq snap, who may not see
+ * the snp tree is active.
+ */
+#define SRCU_SIZE_WAIT_CBS1	3
+#define SRCU_SIZE_WAIT_CBS2	4
+/* All seq snap see the active tree */
+#define SRCU_SIZE_BIG		5
 
 /* Values for state variable (bottom bits of ->srcu_gp_seq). */
 #define SRCU_STATE_IDLE		0
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 281cd69fe804..bdf392e2ef63 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1652,11 +1652,8 @@ static const char * const srcu_size_state_name[] = {
 	"SRCU_SIZE_SMALL",
 	"SRCU_SIZE_ALLOC",
 	"SRCU_SIZE_WAIT_BARRIER",
-	"SRCU_SIZE_WAIT_CALL",
 	"SRCU_SIZE_WAIT_CBS1",
 	"SRCU_SIZE_WAIT_CBS2",
-	"SRCU_SIZE_WAIT_CBS3",
-	"SRCU_SIZE_WAIT_CBS4",
 	"SRCU_SIZE_BIG",
 	"SRCU_SIZE_???",
 };
-- 
2.31.1

RE: [PATCH 1/3] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Zhang, Qiang1]

>The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
>And it is not needed. Because counter_wrap_check has guarantee that both
>srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
>srcu_gp_seq and no false alarm will be raised by the statement in
>srcu_gp_start_if_needed()
>
>  ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
>
>As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
>used immediately, not need to wait for another srcu_gp_end() to update
>the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
>
>Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
>Cc: Lai Jiangshan <jiangshanlai@gmail.com>
>Cc: "Paul E. McKenney" <paulmck@kernel.org>
>Cc: Frederic Weisbecker <frederic@kernel.org>
>Cc: Josh Triplett <josh@joshtriplett.org>
>Cc: Steven Rostedt <rostedt@goodmis.org>
>Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
>To: rcu@vger.kernel.org
>---
> kernel/rcu/srcutree.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>index 1c304fec89c0..fe0759d89c2d 100644
>--- a/kernel/rcu/srcutree.c
>+++ b/kernel/rcu/srcutree.c
>@@ -1092,7 +1092,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> 	check_init_srcu_struct(ssp);
> 	idx = srcu_read_lock(ssp);
> 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
>-	if (ss_state < SRCU_SIZE_WAIT_CALL)
>+	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> 		sdp = per_cpu_ptr(ssp->sda, 0);

Hi Pingfan

should it be used get_boot_cpu_id() ?

Thanks
Zqiang

> 	else
> 		sdp = raw_cpu_ptr(ssp->sda);
>-- 
>2.31.1
>

RE: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()

[Zhang, Qiang1]

>Subject: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()
>
>At present, the code chunk "Transition to big if needed" is out of the
>lock srcu_cb_mutex, which allows the following scene:
>
>    srcu_gp_end() on cpu1                srcu_gp_end() on cpu2
>
>    init_srcu_struct_nodes()
>    ^^  scheduled out in the middle
>                                         init_srcu_struct_nodes()
>                                         ^^ overwrite the context set up by cpu1

Hi Pingfan

This scenario shouldn't happen.
The srcu_gp_end() is invoked only in process_srcu() workfn.
for the same ssp->work, workqueue can guaranteed that the 
process_srcu() workfn is executed serially. (view __queue_work () details)


Thanks
Zqiang


>
>Squashing out the race window by putting the code chunk under the
>protection of srcu_cb_mutex.
>
>Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
>Cc: Lai Jiangshan <jiangshanlai@gmail.com>
>Cc: "Paul E. McKenney" <paulmck@kernel.org>
>Cc: Frederic Weisbecker <frederic@kernel.org>
>Cc: Josh Triplett <josh@joshtriplett.org>
>Cc: Steven Rostedt <rostedt@goodmis.org>
>Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
>To: rcu@vger.kernel.org
>---
> kernel/rcu/srcutree.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>index fe0759d89c2d..281cd69fe804 100644
>--- a/kernel/rcu/srcutree.c
>+++ b/kernel/rcu/srcutree.c
>@@ -811,6 +811,13 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> 			spin_unlock_irqrestore_rcu_node(sdp, flags);
> 		}
> 
>+	/* Transition to big if needed. */
>+	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
>+		if (ss_state == SRCU_SIZE_ALLOC)
>+			init_srcu_struct_nodes(ssp, GFP_KERNEL);
>+		else
>+			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
>+	}
> 	/* Callback initiation done, allow grace periods after next. */
> 	mutex_unlock(&ssp->srcu_cb_mutex);
> 
>@@ -826,13 +833,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> 		spin_unlock_irq_rcu_node(ssp);
> 	}
> 
>-	/* Transition to big if needed. */
>-	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
>-		if (ss_state == SRCU_SIZE_ALLOC)
>-			init_srcu_struct_nodes(ssp, GFP_KERNEL);
>-		else
>-			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
>-	}
> }
> 
> /*
>-- 
>2.31.1
>

Re: [PATCH 1/3] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Pingfan Liu]

On Mon, Nov 28, 2022 at 08:40:22AM +0000, Zhang, Qiang1 wrote:
> >The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> >And it is not needed. Because counter_wrap_check has guarantee that both
> >srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> >srcu_gp_seq and no false alarm will be raised by the statement in
> >srcu_gp_start_if_needed()
> >
> >  ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> >
> >As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> >used immediately, not need to wait for another srcu_gp_end() to update
> >the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> >
> >Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> >Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> >Cc: "Paul E. McKenney" <paulmck@kernel.org>
> >Cc: Frederic Weisbecker <frederic@kernel.org>
> >Cc: Josh Triplett <josh@joshtriplett.org>
> >Cc: Steven Rostedt <rostedt@goodmis.org>
> >Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> >To: rcu@vger.kernel.org
> >---
> > kernel/rcu/srcutree.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> >index 1c304fec89c0..fe0759d89c2d 100644
> >--- a/kernel/rcu/srcutree.c
> >+++ b/kernel/rcu/srcutree.c
> >@@ -1092,7 +1092,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > 	check_init_srcu_struct(ssp);
> > 	idx = srcu_read_lock(ssp);
> > 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> >-	if (ss_state < SRCU_SIZE_WAIT_CALL)
> >+	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > 		sdp = per_cpu_ptr(ssp->sda, 0);
> 
> Hi Pingfan
> 
> should it be used get_boot_cpu_id() ?
> 

I am not sure which branch to work on.

Thanks,

	Pingfan

> Thanks
> Zqiang
> 
> > 	else
> > 		sdp = raw_cpu_ptr(ssp->sda);
> >-- 
> >2.31.1
> >

Re: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()

[Pingfan Liu]

On Mon, Nov 28, 2022 at 09:20:18AM +0000, Zhang, Qiang1 wrote:
> >Subject: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()
> >
> >At present, the code chunk "Transition to big if needed" is out of the
> >lock srcu_cb_mutex, which allows the following scene:
> >
> >    srcu_gp_end() on cpu1                srcu_gp_end() on cpu2
> >
> >    init_srcu_struct_nodes()
> >    ^^  scheduled out in the middle
> >                                         init_srcu_struct_nodes()
> >                                         ^^ overwrite the context set up by cpu1
> 
> Hi Pingfan
> 
> This scenario shouldn't happen.
> The srcu_gp_end() is invoked only in process_srcu() workfn.
> for the same ssp->work, workqueue can guaranteed that the 
> process_srcu() workfn is executed serially. (view __queue_work () details)
> 

How about this:

In process_one_work()

  set_work_pool_and_clear_pending(work, pool->id);
                                               -----> another ssp->work
					       can be queued, which can raise the
					       scenario mentioned above
					       
  worker->current_func(work);


Thanks,

	Pingfan
> 
> Thanks
> Zqiang
> 
> 
> >
> >Squashing out the race window by putting the code chunk under the
> >protection of srcu_cb_mutex.
> >
> >Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> >Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> >Cc: "Paul E. McKenney" <paulmck@kernel.org>
> >Cc: Frederic Weisbecker <frederic@kernel.org>
> >Cc: Josh Triplett <josh@joshtriplett.org>
> >Cc: Steven Rostedt <rostedt@goodmis.org>
> >Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> >To: rcu@vger.kernel.org
> >---
> > kernel/rcu/srcutree.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> >index fe0759d89c2d..281cd69fe804 100644
> >--- a/kernel/rcu/srcutree.c
> >+++ b/kernel/rcu/srcutree.c
> >@@ -811,6 +811,13 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > 			spin_unlock_irqrestore_rcu_node(sdp, flags);
> > 		}
> > 
> >+	/* Transition to big if needed. */
> >+	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
> >+		if (ss_state == SRCU_SIZE_ALLOC)
> >+			init_srcu_struct_nodes(ssp, GFP_KERNEL);
> >+		else
> >+			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
> >+	}
> > 	/* Callback initiation done, allow grace periods after next. */
> > 	mutex_unlock(&ssp->srcu_cb_mutex);
> > 
> >@@ -826,13 +833,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > 		spin_unlock_irq_rcu_node(ssp);
> > 	}
> > 
> >-	/* Transition to big if needed. */
> >-	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
> >-		if (ss_state == SRCU_SIZE_ALLOC)
> >-			init_srcu_struct_nodes(ssp, GFP_KERNEL);
> >-		else
> >-			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
> >-	}
> > }
> > 
> > /*
> >-- 
> >2.31.1
> >

RE: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()

[Zhang, Qiang1]

On Mon, Nov 28, 2022 at 09:20:18AM +0000, Zhang, Qiang1 wrote:
> >Subject: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()
> >
> >At present, the code chunk "Transition to big if needed" is out of the
> >lock srcu_cb_mutex, which allows the following scene:
> >
> >    srcu_gp_end() on cpu1                srcu_gp_end() on cpu2
> >
> >    init_srcu_struct_nodes()
> >    ^^  scheduled out in the middle
> >                                         init_srcu_struct_nodes()
> >                                         ^^ overwrite the context set up by cpu1
> 
> Hi Pingfan
> 
> This scenario shouldn't happen.
> The srcu_gp_end() is invoked only in process_srcu() workfn.
> for the same ssp->work, workqueue can guaranteed that the 
> process_srcu() workfn is executed serially. (view __queue_work () details)
> 

>
>How about this:
>
>In process_one_work()
>
>  set_work_pool_and_clear_pending(work, pool->id);
>                                               -----> another ssp->work
>					       can be queued, which can raise the
>					       scenario mentioned above
>					       
>  worker->current_func(work);
>


Hi Pingfan

The following two code snippets ensure that this scenario  does not happen

__queue_work(): 

last_pool = get_work_pool(work);
        if (last_pool && last_pool != pwq->pool) {
                struct worker *worker;

                raw_spin_lock(&last_pool->lock);

                worker = find_worker_executing_work(last_pool, work);

                if (worker && worker->current_pwq->wq == wq) {
                        pwq = worker->current_pwq;

and 
process_one_work():

collision = find_worker_executing_work(pool, work);
        if (unlikely(collision)) {
                move_linked_works(work, &collision->scheduled, NULL);
                return;
        }

Thanks
Zqiang

>
>Thanks,
>
	Pingfan
> 
> Thanks
> Zqiang
> 
> 
> >
> >Squashing out the race window by putting the code chunk under the
> >protection of srcu_cb_mutex.
> >
> >Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> >Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> >Cc: "Paul E. McKenney" <paulmck@kernel.org>
> >Cc: Frederic Weisbecker <frederic@kernel.org>
> >Cc: Josh Triplett <josh@joshtriplett.org>
> >Cc: Steven Rostedt <rostedt@goodmis.org>
> >Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> >To: rcu@vger.kernel.org
> >---
> > kernel/rcu/srcutree.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> >index fe0759d89c2d..281cd69fe804 100644
> >--- a/kernel/rcu/srcutree.c
> >+++ b/kernel/rcu/srcutree.c
> >@@ -811,6 +811,13 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > 			spin_unlock_irqrestore_rcu_node(sdp, flags);
> > 		}
> > 
> >+	/* Transition to big if needed. */
> >+	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
> >+		if (ss_state == SRCU_SIZE_ALLOC)
> >+			init_srcu_struct_nodes(ssp, GFP_KERNEL);
> >+		else
> >+			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
> >+	}
> > 	/* Callback initiation done, allow grace periods after next. */
> > 	mutex_unlock(&ssp->srcu_cb_mutex);
> > 
> >@@ -826,13 +833,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > 		spin_unlock_irq_rcu_node(ssp);
> > 	}
> > 
> >-	/* Transition to big if needed. */
> >-	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
> >-		if (ss_state == SRCU_SIZE_ALLOC)
> >-			init_srcu_struct_nodes(ssp, GFP_KERNEL);
> >-		else
> >-			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
> >-	}
> > }
> > 
> > /*
> >-- 
> >2.31.1
> >

Re: [PATCH 1/3] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Paul E. McKenney]

On Mon, Nov 28, 2022 at 10:01:08PM +0800, Pingfan Liu wrote:
> On Mon, Nov 28, 2022 at 08:40:22AM +0000, Zhang, Qiang1 wrote:
> > >The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > >And it is not needed. Because counter_wrap_check has guarantee that both
> > >srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > >srcu_gp_seq and no false alarm will be raised by the statement in
> > >srcu_gp_start_if_needed()
> > >
> > >  ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > >
> > >As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > >used immediately, not need to wait for another srcu_gp_end() to update
> > >the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> > >
> > >Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > >Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > >Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > >Cc: Frederic Weisbecker <frederic@kernel.org>
> > >Cc: Josh Triplett <josh@joshtriplett.org>
> > >Cc: Steven Rostedt <rostedt@goodmis.org>
> > >Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > >Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> > >To: rcu@vger.kernel.org
> > >---
> > > kernel/rcu/srcutree.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > >index 1c304fec89c0..fe0759d89c2d 100644
> > >--- a/kernel/rcu/srcutree.c
> > >+++ b/kernel/rcu/srcutree.c
> > >@@ -1092,7 +1092,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > 	check_init_srcu_struct(ssp);
> > > 	idx = srcu_read_lock(ssp);
> > > 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > >-	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > >+	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > > 		sdp = per_cpu_ptr(ssp->sda, 0);
> > 
> > Hi Pingfan
> > 
> > should it be used get_boot_cpu_id() ?
> 
> I am not sure which branch to work on.

For RCU patches, the "dev" branch of the -rcu tree.

For more information, please see:

https://mirrors.edge.kernel.org/pub/linux/kernel/people/paulmck/rcutodo.html

							Thanx, Paul

> Thanks,
> 
> 	Pingfan
> 
> > Thanks
> > Zqiang
> > 
> > > 	else
> > > 		sdp = raw_cpu_ptr(ssp->sda);
> > >-- 
> > >2.31.1
> > >

[PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Pingfan Liu]

From: Pingfan Liu <kernelfans@gmail.com>

The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
And it is not needed. Because counter_wrap_check has guarantee that both
srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
srcu_gp_seq and no false alarm will be raised by the statement in
srcu_gp_start_if_needed()

  ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)

As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
used immediately, not need to wait for another srcu_gp_end() to update
the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
To: rcu@vger.kernel.org
---
v1 -> v2:
 rebase onto dev branch
 kernel/rcu/srcutree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index b25f77df9e5e..b0278d1e9c1f 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	 */
 	idx = __srcu_read_lock_nmisafe(ssp);
 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
-	if (ss_state < SRCU_SIZE_WAIT_CALL)
+	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
 		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
 	else
 		sdp = raw_cpu_ptr(ssp->sda);
-- 
2.31.1

Re: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()

[Pingfan Liu]

On Mon, Nov 28, 2022 at 02:47:46PM +0000, Zhang, Qiang1 wrote:
> On Mon, Nov 28, 2022 at 09:20:18AM +0000, Zhang, Qiang1 wrote:
> > >Subject: [PATCH 2/3] srcu: protect against concurrent srcu_gp_end()
> > >
> > >At present, the code chunk "Transition to big if needed" is out of the
> > >lock srcu_cb_mutex, which allows the following scene:
> > >
> > >    srcu_gp_end() on cpu1                srcu_gp_end() on cpu2
> > >
> > >    init_srcu_struct_nodes()
> > >    ^^  scheduled out in the middle
> > >                                         init_srcu_struct_nodes()
> > >                                         ^^ overwrite the context set up by cpu1
> > 
> > Hi Pingfan
> > 
> > This scenario shouldn't happen.
> > The srcu_gp_end() is invoked only in process_srcu() workfn.
> > for the same ssp->work, workqueue can guaranteed that the 
> > process_srcu() workfn is executed serially. (view __queue_work () details)
> > 
> 
> >
> >How about this:
> >
> >In process_one_work()
> >
> >  set_work_pool_and_clear_pending(work, pool->id);
> >                                               -----> another ssp->work
> >					       can be queued, which can raise the
> >					       scenario mentioned above
> >					       
> >  worker->current_func(work);
> >
> 
> 
> Hi Pingfan
> 
> The following two code snippets ensure that this scenario  does not happen
> 
> __queue_work(): 
> 
> last_pool = get_work_pool(work);
>         if (last_pool && last_pool != pwq->pool) {
>                 struct worker *worker;
> 
>                 raw_spin_lock(&last_pool->lock);
> 
>                 worker = find_worker_executing_work(last_pool, work);
> 
>                 if (worker && worker->current_pwq->wq == wq) {
>                         pwq = worker->current_pwq;
> 
> and 
> process_one_work():
> 
> collision = find_worker_executing_work(pool, work);
>         if (unlikely(collision)) {
>                 move_linked_works(work, &collision->scheduled, NULL);
>                 return;
>         }
> 

Appreciate for pointing out this. It is helpful.

And I will send out a new patch about it.

Thanks,

	Pingfan

> Thanks
> Zqiang
> 
> >
> >Thanks,
> >
> 	Pingfan
> > 
> > Thanks
> > Zqiang
> > 
> > 
> > >
> > >Squashing out the race window by putting the code chunk under the
> > >protection of srcu_cb_mutex.
> > >
> > >Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > >Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > >Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > >Cc: Frederic Weisbecker <frederic@kernel.org>
> > >Cc: Josh Triplett <josh@joshtriplett.org>
> > >Cc: Steven Rostedt <rostedt@goodmis.org>
> > >Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > >Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> > >To: rcu@vger.kernel.org
> > >---
> > > kernel/rcu/srcutree.c | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > >index fe0759d89c2d..281cd69fe804 100644
> > >--- a/kernel/rcu/srcutree.c
> > >+++ b/kernel/rcu/srcutree.c
> > >@@ -811,6 +811,13 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > > 			spin_unlock_irqrestore_rcu_node(sdp, flags);
> > > 		}
> > > 
> > >+	/* Transition to big if needed. */
> > >+	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
> > >+		if (ss_state == SRCU_SIZE_ALLOC)
> > >+			init_srcu_struct_nodes(ssp, GFP_KERNEL);
> > >+		else
> > >+			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
> > >+	}
> > > 	/* Callback initiation done, allow grace periods after next. */
> > > 	mutex_unlock(&ssp->srcu_cb_mutex);
> > > 
> > >@@ -826,13 +833,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > > 		spin_unlock_irq_rcu_node(ssp);
> > > 	}
> > > 
> > >-	/* Transition to big if needed. */
> > >-	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
> > >-		if (ss_state == SRCU_SIZE_ALLOC)
> > >-			init_srcu_struct_nodes(ssp, GFP_KERNEL);
> > >-		else
> > >-			smp_store_release(&ssp->srcu_size_state, ss_state + 1);
> > >-	}
> > > }
> > > 
> > > /*
> > >-- 
> > >2.31.1
> > >

Re: [PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Paul E. McKenney]

On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> From: Pingfan Liu <kernelfans@gmail.com>
> 
> The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> And it is not needed. Because counter_wrap_check has guarantee that both
> srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> srcu_gp_seq and no false alarm will be raised by the statement in
> srcu_gp_start_if_needed()
> 
>   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> 
> As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> used immediately, not need to wait for another srcu_gp_end() to update
> the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.

During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
the boot CPU's callback list, but the srcu_barrier() function scans all
of the CPUs' callback lists.

So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
and rcu_segcblist_accelerate() a callback list that is guaranteed to be
empty?  Without this change, the might-not-be-empty boot CPU's callback
list is dealt with.

What am I missing here?  More specifically, what benefits does this
change provide?  I am not yet seeing any.

							Thanx, Paul

> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> To: rcu@vger.kernel.org
> ---
> v1 -> v2:
>  rebase onto dev branch
>  kernel/rcu/srcutree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index b25f77df9e5e..b0278d1e9c1f 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
>  	 */
>  	idx = __srcu_read_lock_nmisafe(ssp);
>  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
>  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
>  	else
>  		sdp = raw_cpu_ptr(ssp->sda);
> -- 
> 2.31.1
> 

Re: [PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Pingfan Liu]

Sorry to reply late. I was interrupted by something else.

On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > From: Pingfan Liu <kernelfans@gmail.com>
> > 
> > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > And it is not needed. Because counter_wrap_check has guarantee that both
> > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > srcu_gp_seq and no false alarm will be raised by the statement in
> > srcu_gp_start_if_needed()
> > 
> >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > 
> > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > used immediately, not need to wait for another srcu_gp_end() to update
> > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> 
> During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> the boot CPU's callback list, but the srcu_barrier() function scans all
> of the CPUs' callback lists.
> 

So at present, your concern is around how it affects srcu_barrier(), right?

> So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> empty?  Without this change, the might-not-be-empty boot CPU's callback
> list is dealt with.
> 

Is it a problem that srcu_gp_start_if_needed() asks
rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
srcu_barrier() will take care of all cpus' callback list.

> What am I missing here?  More specifically, what benefits does this
> change provide?  I am not yet seeing any.
> 

I am trying to improve the note around SRCU_SIZE_xx, and this is a
trivial improve from 8 SRCU_SIZE state to 5.


Thanks,

	Pingfan

> 							Thanx, Paul
> 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> > To: rcu@vger.kernel.org
> > ---
> > v1 -> v2:
> >  rebase onto dev branch
> >  kernel/rcu/srcutree.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index b25f77df9e5e..b0278d1e9c1f 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> >  	 */
> >  	idx = __srcu_read_lock_nmisafe(ssp);
> >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> >  	else
> >  		sdp = raw_cpu_ptr(ssp->sda);
> > -- 
> > 2.31.1
> > 

Re: [PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Paul E. McKenney]

On Tue, Dec 13, 2022 at 08:51:29PM +0800, Pingfan Liu wrote:
> Sorry to reply late. I was interrupted by something else.
> 
> On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > > From: Pingfan Liu <kernelfans@gmail.com>
> > > 
> > > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > > And it is not needed. Because counter_wrap_check has guarantee that both
> > > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > > srcu_gp_seq and no false alarm will be raised by the statement in
> > > srcu_gp_start_if_needed()
> > > 
> > >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > > 
> > > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > > used immediately, not need to wait for another srcu_gp_end() to update
> > > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> > 
> > During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> > the boot CPU's callback list, but the srcu_barrier() function scans all
> > of the CPUs' callback lists.
> 
> So at present, your concern is around how it affects srcu_barrier(), right?

At present, as always, I am concerned about all potential added bugs
and potential added complexity, both in terms of lines of code and in
terms of size and irregularity of state space.

And yes, part of the concern about both correctness and complexity is
the interaction between call_srcu() and srcu_barrier().  Right now, the
design very straightforwardly ensures that rcu_barrier() and the rest of
SRCU is looking at all CPU's callback lists before call_srcu() places any
callbacks on non-boot CPUs.  This is because all the srcu_barrier() calls
that think that the system is still in SRCU_SIZE_SMALL or SRCU_SIZE_ALLOC
mode must have completed before any call_srcu() invocation uses a non-boot
CPU's callback list.

Your change causes rcu_barrier() and call_srcu() to start using non-boot
CPUs' callback lists at the same time.  This change therefore introduces
significant state-space complexity.  And maybe even bugs, if not today
then some later time when someone makes a change that looks obviously
correct on the surface, but which subtly weakens the ordering.

> > So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> > and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> > empty?  Without this change, the might-not-be-empty boot CPU's callback
> > list is dealt with.
> 
> Is it a problem that srcu_gp_start_if_needed() asks
> rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
> not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
> srcu_barrier() will take care of all cpus' callback list.

The other way around, please.  My concern is instead that it *fails*
to rcu_segcblist_advance() and rcu_segcblist_accelerate() the
might-not-be-empty callback lists.

> > What am I missing here?  More specifically, what benefits does this
> > change provide?  I am not yet seeing any.
> 
> I am trying to improve the note around SRCU_SIZE_xx, and this is a
> trivial improve from 8 SRCU_SIZE state to 5.

I do understand that you would like to remove SRCU_SIZE_* states.
What I need from you is why it is important to remove these states.
What is being gained by removing them?  Why is that gain so important
that we should (at the very least) make the state space more complex?
And does this change really avoid introducing bugs?

							Thanx, Paul

> Thanks,
> 
> 	Pingfan
> 
> > 							Thanx, Paul
> > 
> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> > > To: rcu@vger.kernel.org
> > > ---
> > > v1 -> v2:
> > >  rebase onto dev branch
> > >  kernel/rcu/srcutree.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index b25f77df9e5e..b0278d1e9c1f 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > >  	 */
> > >  	idx = __srcu_read_lock_nmisafe(ssp);
> > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> > >  	else
> > >  		sdp = raw_cpu_ptr(ssp->sda);
> > > -- 
> > > 2.31.1
> > > 

Re: [PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Pingfan Liu]

On Tue, Dec 13, 2022 at 12:02:07PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 13, 2022 at 08:51:29PM +0800, Pingfan Liu wrote:
> > Sorry to reply late. I was interrupted by something else.
> > 
> > On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > > > From: Pingfan Liu <kernelfans@gmail.com>
> > > > 
> > > > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > > > And it is not needed. Because counter_wrap_check has guarantee that both
> > > > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > > > srcu_gp_seq and no false alarm will be raised by the statement in
> > > > srcu_gp_start_if_needed()
> > > > 
> > > >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > > > 
> > > > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > > > used immediately, not need to wait for another srcu_gp_end() to update
> > > > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> > > 
> > > During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> > > the boot CPU's callback list, but the srcu_barrier() function scans all
> > > of the CPUs' callback lists.
> > 
> > So at present, your concern is around how it affects srcu_barrier(), right?
> 
> At present, as always, I am concerned about all potential added bugs
> and potential added complexity, both in terms of lines of code and in
> terms of size and irregularity of state space.
> 
> And yes, part of the concern about both correctness and complexity is
> the interaction between call_srcu() and srcu_barrier().  Right now, the
> design very straightforwardly ensures that rcu_barrier() and the rest of
> SRCU is looking at all CPU's callback lists before call_srcu() places any
> callbacks on non-boot CPUs.  This is because all the srcu_barrier() calls
> that think that the system is still in SRCU_SIZE_SMALL or SRCU_SIZE_ALLOC
> mode must have completed before any call_srcu() invocation uses a non-boot
> CPU's callback list.
> 

Catch your point. Thanks for patient explanation. And indeed this does
not rely on anything like memory order to build a total order, and more
reliable.

> Your change causes rcu_barrier() and call_srcu() to start using non-boot
> CPUs' callback lists at the same time.  This change therefore introduces
> significant state-space complexity.  And maybe even bugs, if not today
> then some later time when someone makes a change that looks obviously
> correct on the surface, but which subtly weakens the ordering.
> 

Yes, that is true.

> > > So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> > > and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> > > empty?  Without this change, the might-not-be-empty boot CPU's callback
> > > list is dealt with.
> > 
> > Is it a problem that srcu_gp_start_if_needed() asks
> > rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
> > not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
> > srcu_barrier() will take care of all cpus' callback list.
> 
> The other way around, please.  My concern is instead that it *fails*
> to rcu_segcblist_advance() and rcu_segcblist_accelerate() the
> might-not-be-empty callback lists.
> 
> > > What am I missing here?  More specifically, what benefits does this
> > > change provide?  I am not yet seeing any.
> > 
> > I am trying to improve the note around SRCU_SIZE_xx, and this is a
> > trivial improve from 8 SRCU_SIZE state to 5.
> 
> I do understand that you would like to remove SRCU_SIZE_* states.
> What I need from you is why it is important to remove these states.
> What is being gained by removing them?  Why is that gain so important
> that we should (at the very least) make the state space more complex?
> And does this change really avoid introducing bugs?
> 

I am driven by the curiosity about the size state machine, and try to
document it as Frederic asked [1].

And yes as you demand, it should not introduce any complexity without
gains.

Again, appreciate for your detailed explanation. I have a better
understainding of the design. Later I will try to document it with a
patch.

[1]: https://lore.kernel.org/rcu/20221117142025.GE839309@lothringen/

> 							Thanx, Paul
> 
> > Thanks,
> > 
> > 	Pingfan
> > 
> > > 							Thanx, Paul
> > > 
> > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> > > > To: rcu@vger.kernel.org
> > > > ---
> > > > v1 -> v2:
> > > >  rebase onto dev branch
> > > >  kernel/rcu/srcutree.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > index b25f77df9e5e..b0278d1e9c1f 100644
> > > > --- a/kernel/rcu/srcutree.c
> > > > +++ b/kernel/rcu/srcutree.c
> > > > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > >  	 */
> > > >  	idx = __srcu_read_lock_nmisafe(ssp);
> > > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > > >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> > > >  	else
> > > >  		sdp = raw_cpu_ptr(ssp->sda);
> > > > -- 
> > > > 2.31.1
> > > > 

Re: [PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

[Paul E. McKenney]

On Thu, Dec 15, 2022 at 11:37:41AM +0800, Pingfan Liu wrote:
> On Tue, Dec 13, 2022 at 12:02:07PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 13, 2022 at 08:51:29PM +0800, Pingfan Liu wrote:
> > > Sorry to reply late. I was interrupted by something else.
> > > 
> > > On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> > > > On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > > > > From: Pingfan Liu <kernelfans@gmail.com>
> > > > > 
> > > > > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > > > > And it is not needed. Because counter_wrap_check has guarantee that both
> > > > > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > > > > srcu_gp_seq and no false alarm will be raised by the statement in
> > > > > srcu_gp_start_if_needed()
> > > > > 
> > > > >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > > > > 
> > > > > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > > > > used immediately, not need to wait for another srcu_gp_end() to update
> > > > > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> > > > 
> > > > During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> > > > the boot CPU's callback list, but the srcu_barrier() function scans all
> > > > of the CPUs' callback lists.
> > > 
> > > So at present, your concern is around how it affects srcu_barrier(), right?
> > 
> > At present, as always, I am concerned about all potential added bugs
> > and potential added complexity, both in terms of lines of code and in
> > terms of size and irregularity of state space.
> > 
> > And yes, part of the concern about both correctness and complexity is
> > the interaction between call_srcu() and srcu_barrier().  Right now, the
> > design very straightforwardly ensures that rcu_barrier() and the rest of
> > SRCU is looking at all CPU's callback lists before call_srcu() places any
> > callbacks on non-boot CPUs.  This is because all the srcu_barrier() calls
> > that think that the system is still in SRCU_SIZE_SMALL or SRCU_SIZE_ALLOC
> > mode must have completed before any call_srcu() invocation uses a non-boot
> > CPU's callback list.
> > 
> 
> Catch your point. Thanks for patient explanation. And indeed this does
> not rely on anything like memory order to build a total order, and more
> reliable.
> 
> > Your change causes rcu_barrier() and call_srcu() to start using non-boot
> > CPUs' callback lists at the same time.  This change therefore introduces
> > significant state-space complexity.  And maybe even bugs, if not today
> > then some later time when someone makes a change that looks obviously
> > correct on the surface, but which subtly weakens the ordering.
> > 
> 
> Yes, that is true.
> 
> > > > So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> > > > and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> > > > empty?  Without this change, the might-not-be-empty boot CPU's callback
> > > > list is dealt with.
> > > 
> > > Is it a problem that srcu_gp_start_if_needed() asks
> > > rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
> > > not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
> > > srcu_barrier() will take care of all cpus' callback list.
> > 
> > The other way around, please.  My concern is instead that it *fails*
> > to rcu_segcblist_advance() and rcu_segcblist_accelerate() the
> > might-not-be-empty callback lists.
> > 
> > > > What am I missing here?  More specifically, what benefits does this
> > > > change provide?  I am not yet seeing any.
> > > 
> > > I am trying to improve the note around SRCU_SIZE_xx, and this is a
> > > trivial improve from 8 SRCU_SIZE state to 5.
> > 
> > I do understand that you would like to remove SRCU_SIZE_* states.
> > What I need from you is why it is important to remove these states.
> > What is being gained by removing them?  Why is that gain so important
> > that we should (at the very least) make the state space more complex?
> > And does this change really avoid introducing bugs?
> > 
> 
> I am driven by the curiosity about the size state machine, and try to
> document it as Frederic asked [1].
> 
> And yes as you demand, it should not introduce any complexity without
> gains.
> 
> Again, appreciate for your detailed explanation. I have a better
> understainding of the design. Later I will try to document it with a
> patch.

That sounds good, and I look forward to seeing your state-explanation
patch!

							Thanx, Paul

> [1]: https://lore.kernel.org/rcu/20221117142025.GE839309@lothringen/
> 
> > 							Thanx, Paul
> > 
> > > Thanks,
> > > 
> > > 	Pingfan
> > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> > > > > To: rcu@vger.kernel.org
> > > > > ---
> > > > > v1 -> v2:
> > > > >  rebase onto dev branch
> > > > >  kernel/rcu/srcutree.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > index b25f77df9e5e..b0278d1e9c1f 100644
> > > > > --- a/kernel/rcu/srcutree.c
> > > > > +++ b/kernel/rcu/srcutree.c
> > > > > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > > >  	 */
> > > > >  	idx = __srcu_read_lock_nmisafe(ssp);
> > > > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > > > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > > > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > > > >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> > > > >  	else
> > > > >  		sdp = raw_cpu_ptr(ssp->sda);
> > > > > -- 
> > > > > 2.31.1
> > > > >