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