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@xxxxxxxxx>
> > >Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> > >Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> > >Cc: Frederic Weisbecker <frederic@xxxxxxxxxx>
> > >Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> > >Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > >Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> > >Cc: "Zhang, Qiang1" <qiang1.zhang@xxxxxxxxx>
> > >To: rcu@xxxxxxxxxxxxxxx
> > >---
> > > 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
> > >
