| Message ID | 20221128082816.28518-3-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | srcu: shrink the num of srcu_size_state | expand |
>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 >
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 > >
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 > >
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 > > >
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); - } } /*
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(-)