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

[Pingfan Liu <kernelfans@gmail.com>]

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