| Message ID | 20230321081346.192000-1-qiang1.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | srcu: Fix flush sup work warning in cleanup_srcu_struct() | expand |
On Tue, Mar 21, 2023 at 04:13:46PM +0800, Zqiang wrote: > insmod rcutorture.ko > rmmod rcutorture.ko > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 __flush_work+0x50a/0x540 > [ 209.437346] Modules linked in: rcutorture(-) torture [last unloaded: rcutorture] > [ 209.437382] CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 > ..... > [ 209.437758] flush_delayed_work+0x36/0x90 > [ 209.437776] cleanup_srcu_struct+0x68/0x2e0 > [ 209.437817] srcu_module_notify+0x71/0x140 > [ 209.437854] blocking_notifier_call_chain+0x9d/0xd0 > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > [ 209.438046] do_syscall_64+0x43/0x90 > [ 209.438062] entry_SYSCALL_64_after_hwframe+0x72/0xdc > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > when compiling and loading as modules, the srcu_module_coming() is invoked, > allocate memory for srcu structure's->sda and initialize sda structure, > due to not fully initialize srcu structure's->sup, so at this time the > sup structure's->delaywork.func is null, if not invoke init_srcu_struct_fields() > before unloading modules, in srcu_module_going() the __flush_work() find > work->func is empty, will raise the warning above. > > This commit add init_srcu_struct_fields() to initialize srcu structure's->sup, > in srcu_module_coming(). > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> Good catch, and thank you for testing the in-module case! One question below... Thanx, Paul > --- > kernel/rcu/srcutree.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 1fb078abbdc9..42d8720e016c 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > ssp->sda = alloc_percpu(struct srcu_data); > if (WARN_ON_ONCE(!ssp->sda)) > return -ENOMEM; > - init_srcu_struct_data(ssp); > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > + return -ENOMEM; Wouldn't it be better to simply delete the init_srcu_struct_data()? Then the first call to check_init_srcu_struct() would take care of the initialization, just as for the non-module case. Or am I missing something subtle? It should also be possible to eliminate duplicate code between the in-module and non-module statically allocated initialization cases, come to think of it. > } > return 0; > } > @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module *mod) > { > int i; > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > + struct srcu_struct *ssp; > > - for (i = 0; i < mod->num_srcu_structs; i++) > - cleanup_srcu_struct(*(sspp++)); > + for (i = 0; i < mod->num_srcu_structs; i++) { > + ssp = *(sspp++); > + cleanup_srcu_struct(ssp); > + free_percpu(ssp->sda); > + } And good catch on another memory leak with this one, looks proper to me. > } > > /* Handle one module, either coming or going. */ > -- > 2.25.1 >
On Tue, Mar 21, 2023 at 08:04:20AM -0700, Paul E. McKenney wrote: > On Tue, Mar 21, 2023 at 04:13:46PM +0800, Zqiang wrote: > > insmod rcutorture.ko > > rmmod rcutorture.ko > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 __flush_work+0x50a/0x540 > > [ 209.437346] Modules linked in: rcutorture(-) torture [last unloaded: rcutorture] > > [ 209.437382] CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 > > ..... > > [ 209.437758] flush_delayed_work+0x36/0x90 > > [ 209.437776] cleanup_srcu_struct+0x68/0x2e0 > > [ 209.437817] srcu_module_notify+0x71/0x140 > > [ 209.437854] blocking_notifier_call_chain+0x9d/0xd0 > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > [ 209.438046] do_syscall_64+0x43/0x90 > > [ 209.438062] entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > when compiling and loading as modules, the srcu_module_coming() is invoked, > > allocate memory for srcu structure's->sda and initialize sda structure, > > due to not fully initialize srcu structure's->sup, so at this time the > > sup structure's->delaywork.func is null, if not invoke init_srcu_struct_fields() > > before unloading modules, in srcu_module_going() the __flush_work() find > > work->func is empty, will raise the warning above. > > > > This commit add init_srcu_struct_fields() to initialize srcu structure's->sup, > > in srcu_module_coming(). > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > Good catch, and thank you for testing the in-module case! > > One question below... > > Thanx, Paul > > > --- > > kernel/rcu/srcutree.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 1fb078abbdc9..42d8720e016c 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > ssp->sda = alloc_percpu(struct srcu_data); > > if (WARN_ON_ONCE(!ssp->sda)) > > return -ENOMEM; > > - init_srcu_struct_data(ssp); > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > + return -ENOMEM; > > Wouldn't it be better to simply delete the init_srcu_struct_data()? > > Then the first call to check_init_srcu_struct() would take care of > the initialization, just as for the non-module case. Or am I missing > something subtle? > > It should also be possible to eliminate duplicate code between the > in-module and non-module statically allocated initialization cases, > come to think of it. But that would amount to only one line of duplicated code, so this last is probably not worth it. Thanx, Paul > > } > > return 0; > > } > > @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module *mod) > > { > > int i; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > + struct srcu_struct *ssp; > > > > - for (i = 0; i < mod->num_srcu_structs; i++) > > - cleanup_srcu_struct(*(sspp++)); > > + for (i = 0; i < mod->num_srcu_structs; i++) { > > + ssp = *(sspp++); > > + cleanup_srcu_struct(ssp); > > + free_percpu(ssp->sda); > > + } > > And good catch on another memory leak with this one, looks proper > to me. > > > } > > > > /* Handle one module, either coming or going. */ > > -- > > 2.25.1 > >
> insmod rcutorture.ko > rmmod rcutorture.ko > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 __flush_work+0x50a/0x540 > [ 209.437346] Modules linked in: rcutorture(-) torture [last unloaded: rcutorture] > [ 209.437382] CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 > ..... > [ 209.437758] flush_delayed_work+0x36/0x90 > [ 209.437776] cleanup_srcu_struct+0x68/0x2e0 > [ 209.437817] srcu_module_notify+0x71/0x140 > [ 209.437854] blocking_notifier_call_chain+0x9d/0xd0 > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > [ 209.438046] do_syscall_64+0x43/0x90 > [ 209.438062] entry_SYSCALL_64_after_hwframe+0x72/0xdc > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > when compiling and loading as modules, the srcu_module_coming() is invoked, > allocate memory for srcu structure's->sda and initialize sda structure, > due to not fully initialize srcu structure's->sup, so at this time the > sup structure's->delaywork.func is null, if not invoke init_srcu_struct_fields() > before unloading modules, in srcu_module_going() the __flush_work() find > work->func is empty, will raise the warning above. > > This commit add init_srcu_struct_fields() to initialize srcu structure's->sup, > in srcu_module_coming(). > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > >Good catch, and thank you for testing the in-module case! > >One question below... > > Thanx, Paul > > --- > kernel/rcu/srcutree.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 1fb078abbdc9..42d8720e016c 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > ssp->sda = alloc_percpu(struct srcu_data); > if (WARN_ON_ONCE(!ssp->sda)) > return -ENOMEM; > - init_srcu_struct_data(ssp); > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > + return -ENOMEM; > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > >Then the first call to check_init_srcu_struct() would take care of >the initialization, just as for the non-module case. Or am I missing >something subtle? Hi Paul Maybe the check_init_srcu_struct() is always not invoked, for example, In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), but we use torture_type=rcu to test, there will not be any interface calling check_init_srcu_struct() to initialize srcu_ctl and set structure's->delaywork.func is process_srcu(). when we unload the rcutorture module, invoke cleanup_srcu_struct() to flush sup structure's->delaywork.func, due to the func pointer is not initialize, it's null, will trigger warning. About kernel/workqueue.c:3167 __flush_work if (WARN_ON(!work->func)) <---------trigger waning return false; and in init_srcu_struct_fields(ssp, true), wil set srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed is zero, after that when we call check_init_srcu_struct() again, it not be initialized again. Thanks Zqiang > >It should also be possible to eliminate duplicate code between the >in-module and non-module statically allocated initialization cases, >come to think of it. > > } > return 0; > } > @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module *mod) > { > int i; > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > + struct srcu_struct *ssp; > > - for (i = 0; i < mod->num_srcu_structs; i++) > - cleanup_srcu_struct(*(sspp++)); > + for (i = 0; i < mod->num_srcu_structs; i++) { > + ssp = *(sspp++); > + cleanup_srcu_struct(ssp); > + free_percpu(ssp->sda); > + } > >And good catch on another memory leak with this one, looks proper >to me. > > } > > /* Handle one module, either coming or going. */ > -- > 2.25.1 >
On Wed, Mar 22, 2023 at 01:15:02AM +0000, Zhang, Qiang1 wrote: > > insmod rcutorture.ko > > rmmod rcutorture.ko > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 __flush_work+0x50a/0x540 > > [ 209.437346] Modules linked in: rcutorture(-) torture [last unloaded: rcutorture] > > [ 209.437382] CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 > > ..... > > [ 209.437758] flush_delayed_work+0x36/0x90 > > [ 209.437776] cleanup_srcu_struct+0x68/0x2e0 > > [ 209.437817] srcu_module_notify+0x71/0x140 > > [ 209.437854] blocking_notifier_call_chain+0x9d/0xd0 > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > [ 209.438046] do_syscall_64+0x43/0x90 > > [ 209.438062] entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > when compiling and loading as modules, the srcu_module_coming() is invoked, > > allocate memory for srcu structure's->sda and initialize sda structure, > > due to not fully initialize srcu structure's->sup, so at this time the > > sup structure's->delaywork.func is null, if not invoke init_srcu_struct_fields() > > before unloading modules, in srcu_module_going() the __flush_work() find > > work->func is empty, will raise the warning above. > > > > This commit add init_srcu_struct_fields() to initialize srcu structure's->sup, > > in srcu_module_coming(). > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > >Good catch, and thank you for testing the in-module case! > > > >One question below... > > > > Thanx, Paul > > > > --- > > kernel/rcu/srcutree.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 1fb078abbdc9..42d8720e016c 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > ssp->sda = alloc_percpu(struct srcu_data); > > if (WARN_ON_ONCE(!ssp->sda)) > > return -ENOMEM; > > - init_srcu_struct_data(ssp); > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > + return -ENOMEM; > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > >Then the first call to check_init_srcu_struct() would take care of > >the initialization, just as for the non-module case. Or am I missing > >something subtle? > > Hi Paul > > Maybe the check_init_srcu_struct() is always not invoked, for example, > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > but we use torture_type=rcu to test, there will not be any interface calling > check_init_srcu_struct() to initialize srcu_ctl and set structure's->delaywork.func > is process_srcu(). > when we unload the rcutorture module, invoke cleanup_srcu_struct() > to flush sup structure's->delaywork.func, due to the func pointer is not initialize, > it's null, will trigger warning. > > About kernel/workqueue.c:3167 > > __flush_work > if (WARN_ON(!work->func)) <---------trigger waning > return false; > > > and in init_srcu_struct_fields(ssp, true), wil set srcu_sup->sda_is_static is true > and set srcu_sup-> srcu_gp_seq_needed is zero, after that when we call > check_init_srcu_struct() again, it not be initialized again. Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the code in srcu_module_going() only did the the matching free_percpu() instead of the current cleanup_srcu_struct()? Thanx, Paul > Thanks > Zqiang > > > > >It should also be possible to eliminate duplicate code between the > >in-module and non-module statically allocated initialization cases, > >come to think of it. > > > > } > > return 0; > > } > > @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module *mod) > > { > > int i; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > + struct srcu_struct *ssp; > > > > - for (i = 0; i < mod->num_srcu_structs; i++) > > - cleanup_srcu_struct(*(sspp++)); > > + for (i = 0; i < mod->num_srcu_structs; i++) { > > + ssp = *(sspp++); > > + cleanup_srcu_struct(ssp); > > + free_percpu(ssp->sda); > > + } > > > >And good catch on another memory leak with this one, looks proper > >to me. > > > > } > > > > /* Handle one module, either coming or going. */ > > -- > > 2.25.1 > >
> > insmod rcutorture.ko > > rmmod rcutorture.ko > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > srcu_module_notify+0x71/0x140 [ 209.437854] > > blocking_notifier_call_chain+0x9d/0xd0 > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > when compiling and loading as modules, the srcu_module_coming() is > > invoked, allocate memory for srcu structure's->sda and initialize > > sda structure, due to not fully initialize srcu structure's->sup, so > > at this time the sup structure's->delaywork.func is null, if not > > invoke init_srcu_struct_fields() before unloading modules, in > > srcu_module_going() the __flush_work() find > > work->func is empty, will raise the warning above. > > > > This commit add init_srcu_struct_fields() to initialize srcu > > structure's->sup, in srcu_module_coming(). > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > >Good catch, and thank you for testing the in-module case! > > > >One question below... > > > > Thanx, Paul > > > > --- > > kernel/rcu/srcutree.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > 1fb078abbdc9..42d8720e016c 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > ssp->sda = alloc_percpu(struct srcu_data); > > if (WARN_ON_ONCE(!ssp->sda)) > > return -ENOMEM; > > - init_srcu_struct_data(ssp); > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > + return -ENOMEM; > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > >Then the first call to check_init_srcu_struct() would take care of > >the initialization, just as for the non-module case. Or am I missing > >something subtle? > > Hi Paul > > Maybe the check_init_srcu_struct() is always not invoked, for example, > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > but we use torture_type=rcu to test, there will not be any interface > calling > check_init_srcu_struct() to initialize srcu_ctl and set > structure's->delaywork.func is process_srcu(). > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > flush sup structure's->delaywork.func, due to the func pointer is not > initialize, it's null, will trigger warning. > > About kernel/workqueue.c:3167 > > __flush_work > if (WARN_ON(!work->func)) <---------trigger waning > return false; > > > and in init_srcu_struct_fields(ssp, true), wil set > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > is zero, after that when we call > check_init_srcu_struct() again, it not be initialized again. > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the >code in srcu_module_going() only did the the matching >free_percpu() instead of the current cleanup_srcu_struct()? But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module unload function. If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe can not free and also lost the opportunity to refresh the running work. Thanks Zqiang > > Thanx, Paul > > Thanks > Zqiang > > > > >It should also be possible to eliminate duplicate code between the > >in-module and non-module statically allocated initialization cases, > >come to think of it. > > > > } > > return 0; > > } > > @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module > > *mod) { > > int i; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > + struct srcu_struct *ssp; > > > > - for (i = 0; i < mod->num_srcu_structs; i++) > > - cleanup_srcu_struct(*(sspp++)); > > + for (i = 0; i < mod->num_srcu_structs; i++) { > > + ssp = *(sspp++); > > + cleanup_srcu_struct(ssp); > > + free_percpu(ssp->sda); > > + } > > > >And good catch on another memory leak with this one, looks proper to > >me. > > > > } > > > > /* Handle one module, either coming or going. */ > > -- > > 2.25.1 > >
On Wed, Mar 22, 2023 at 04:38:29AM +0000, Zhang, Qiang1 wrote: > > > insmod rcutorture.ko > > > rmmod rcutorture.ko > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > blocking_notifier_call_chain+0x9d/0xd0 > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > when compiling and loading as modules, the srcu_module_coming() is > > > invoked, allocate memory for srcu structure's->sda and initialize > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > at this time the sup structure's->delaywork.func is null, if not > > > invoke init_srcu_struct_fields() before unloading modules, in > > > srcu_module_going() the __flush_work() find > > > work->func is empty, will raise the warning above. > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > structure's->sup, in srcu_module_coming(). > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > >Good catch, and thank you for testing the in-module case! > > > > > >One question below... > > > > > > Thanx, Paul > > > > > > --- > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > 1fb078abbdc9..42d8720e016c 100644 > > > --- a/kernel/rcu/srcutree.c > > > +++ b/kernel/rcu/srcutree.c > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > ssp->sda = alloc_percpu(struct srcu_data); > > > if (WARN_ON_ONCE(!ssp->sda)) > > > return -ENOMEM; > > > - init_srcu_struct_data(ssp); > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > + return -ENOMEM; > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > >Then the first call to check_init_srcu_struct() would take care of > > >the initialization, just as for the non-module case. Or am I missing > > >something subtle? > > > > Hi Paul > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > but we use torture_type=rcu to test, there will not be any interface > > calling > > check_init_srcu_struct() to initialize srcu_ctl and set > > structure's->delaywork.func is process_srcu(). > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > flush sup structure's->delaywork.func, due to the func pointer is not > > initialize, it's null, will trigger warning. > > > > About kernel/workqueue.c:3167 > > > > __flush_work > > if (WARN_ON(!work->func)) <---------trigger waning > > return false; > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > is zero, after that when we call > > check_init_srcu_struct() again, it not be initialized again. > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > >code in srcu_module_going() only did the the matching > >free_percpu() instead of the current cleanup_srcu_struct()? > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > unload function. > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > can not free and also lost the opportunity to refresh the running work. But in the module case, isn't the srcu_sup->node also statically allocated via the "static struct srcu_usage" declaration? Thanx, Paul ------------------------------------------------------------------------ #ifdef MODULE # define __DEFINE_SRCU(name, is_static) \ static struct srcu_usage name##_srcu_usage = __SRCU_USAGE_INIT(name##_srcu_usage); \ is_static struct srcu_struct name = __SRCU_STRUCT_INIT_MODULE(name, name##_srcu_usage); \ extern struct srcu_struct * const __srcu_struct_##name; \ struct srcu_struct * const __srcu_struct_##name \ __section("___srcu_struct_ptrs") = &name #else # define __DEFINE_SRCU(name, is_static) \ static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data); \ static struct srcu_usage name##_srcu_usage = __SRCU_USAGE_INIT(name##_srcu_usage); \ is_static struct srcu_struct name = \ __SRCU_STRUCT_INIT(name, name##_srcu_usage, name##_srcu_data) #endif > Thanks > Zqiang > > > > > > Thanx, Paul > > > > Thanks > > Zqiang > > > > > > > >It should also be possible to eliminate duplicate code between the > > >in-module and non-module statically allocated initialization cases, > > >come to think of it. > > > > > > } > > > return 0; > > > } > > > @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module > > > *mod) { > > > int i; > > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > + struct srcu_struct *ssp; > > > > > > - for (i = 0; i < mod->num_srcu_structs; i++) > > > - cleanup_srcu_struct(*(sspp++)); > > > + for (i = 0; i < mod->num_srcu_structs; i++) { > > > + ssp = *(sspp++); > > > + cleanup_srcu_struct(ssp); > > > + free_percpu(ssp->sda); > > > + } > > > > > >And good catch on another memory leak with this one, looks proper to > > >me. > > > > > > } > > > > > > /* Handle one module, either coming or going. */ > > > -- > > > 2.25.1 > > >
> > > insmod rcutorture.ko > > > rmmod rcutorture.ko > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > blocking_notifier_call_chain+0x9d/0xd0 > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > when compiling and loading as modules, the srcu_module_coming() is > > > invoked, allocate memory for srcu structure's->sda and initialize > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > at this time the sup structure's->delaywork.func is null, if not > > > invoke init_srcu_struct_fields() before unloading modules, in > > > srcu_module_going() the __flush_work() find > > > work->func is empty, will raise the warning above. > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > structure's->sup, in srcu_module_coming(). > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > >Good catch, and thank you for testing the in-module case! > > > > > >One question below... > > > > > > Thanx, Paul > > > > > > --- > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > 1fb078abbdc9..42d8720e016c 100644 > > > --- a/kernel/rcu/srcutree.c > > > +++ b/kernel/rcu/srcutree.c > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > ssp->sda = alloc_percpu(struct srcu_data); > > > if (WARN_ON_ONCE(!ssp->sda)) > > > return -ENOMEM; > > > - init_srcu_struct_data(ssp); > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > + return -ENOMEM; > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > >Then the first call to check_init_srcu_struct() would take care of > > >the initialization, just as for the non-module case. Or am I missing > > >something subtle? > > > > Hi Paul > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > but we use torture_type=rcu to test, there will not be any interface > > calling > > check_init_srcu_struct() to initialize srcu_ctl and set > > structure's->delaywork.func is process_srcu(). > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > flush sup structure's->delaywork.func, due to the func pointer is not > > initialize, it's null, will trigger warning. > > > > About kernel/workqueue.c:3167 > > > > __flush_work > > if (WARN_ON(!work->func)) <---------trigger waning > > return false; > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > is zero, after that when we call > > check_init_srcu_struct() again, it not be initialized again. > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > >code in srcu_module_going() only did the the matching > >free_percpu() instead of the current cleanup_srcu_struct()? > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > unload function. > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > can not free and also lost the opportunity to refresh the running work. > > >But in the module case, isn't the srcu_sup->node also statically >allocated via the "static struct srcu_usage" declaration? static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) { sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); ... } Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. right? Thanks Zqiang > > Thanx, Paul > >------------------------------------------------------------------------ > >#ifdef MODULE ># define __DEFINE_SRCU(name, is_static) \ > static struct srcu_usage name##_srcu_usage = __SRCU_USAGE_INIT(name##_srcu_usage); \ > is_static struct srcu_struct name = __SRCU_STRUCT_INIT_MODULE(name, name##_srcu_usage); \ > extern struct srcu_struct * const __srcu_struct_##name; \ > struct srcu_struct * const __srcu_struct_##name \ > __section("___srcu_struct_ptrs") = &name >#else ># define __DEFINE_SRCU(name, is_static) \ > static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data); \ > static struct srcu_usage name##_srcu_usage = __SRCU_USAGE_INIT(name##_srcu_usage); \ > is_static struct srcu_struct name = \ > __SRCU_STRUCT_INIT(name, name##_srcu_usage, name##_srcu_data) >#endif > > Thanks > Zqiang > > > > > > Thanx, Paul > > > > Thanks > > Zqiang > > > > > > > >It should also be possible to eliminate duplicate code between the > > >in-module and non-module statically allocated initialization cases, > > >come to think of it. > > > > > > } > > > return 0; > > > } > > > @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module > > > *mod) { > > > int i; > > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > + struct srcu_struct *ssp; > > > > > > - for (i = 0; i < mod->num_srcu_structs; i++) > > > - cleanup_srcu_struct(*(sspp++)); > > > + for (i = 0; i < mod->num_srcu_structs; i++) { > > > + ssp = *(sspp++); > > > + cleanup_srcu_struct(ssp); > > > + free_percpu(ssp->sda); > > > + } > > > > > >And good catch on another memory leak with this one, looks proper to > > >me. > > > > > > } > > > > > > /* Handle one module, either coming or going. */ > > > -- > > > 2.25.1 > > >
On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > insmod rcutorture.ko > > > > rmmod rcutorture.ko > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > at this time the sup structure's->delaywork.func is null, if not > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > srcu_module_going() the __flush_work() find > > > > work->func is empty, will raise the warning above. > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > >One question below... > > > > > > > > Thanx, Paul > > > > > > > > --- > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > --- a/kernel/rcu/srcutree.c > > > > +++ b/kernel/rcu/srcutree.c > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > return -ENOMEM; > > > > - init_srcu_struct_data(ssp); > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > + return -ENOMEM; > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > >the initialization, just as for the non-module case. Or am I missing > > > >something subtle? > > > > > > Hi Paul > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > but we use torture_type=rcu to test, there will not be any interface > > > calling > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > structure's->delaywork.func is process_srcu(). > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > initialize, it's null, will trigger warning. > > > > > > About kernel/workqueue.c:3167 > > > > > > __flush_work > > > if (WARN_ON(!work->func)) <---------trigger waning > > > return false; > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > is zero, after that when we call > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > >code in srcu_module_going() only did the the matching > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > unload function. > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > can not free and also lost the opportunity to refresh the running work. > > > > > >But in the module case, isn't the srcu_sup->node also statically > >allocated via the "static struct srcu_usage" declaration? > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > { > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > ... > } > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > right? You are absolutely right, thank you! There are a couple of ways to resolve this. One is to simply add a check_init_srcu_struct() before the call to cleanup_srcu_struct() from srcu_module_going(), as shown below. This seems a bit silly, potentially initializing fields for no good reason. Another way is to make cleanup_srcu_struct() do the same check that check_init_srcu_struct() does: rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) If the value is non-zero, then cleanup_srcu_struct() should skip consistency checks that complain about things that cannot happen if there never was an RCU grace period. Maybe something as shown after the second line of dashes. Thoughts? Thanx, Paul ------------------------------------------------------------------------ /* Initialize any global-scope srcu_struct structures used by this module. */ static int srcu_module_coming(struct module *mod) { int i; struct srcu_struct **sspp = mod->srcu_struct_ptrs; struct srcu_struct *ssp; for (i = 0; i < mod->num_srcu_structs; i++) { ssp = *(sspp++); ssp->sda = alloc_percpu(struct srcu_data); if (WARN_ON_ONCE(!ssp->sda)) return -ENOMEM; init_srcu_struct_data(ssp); } return 0; } /* Clean up any global-scope srcu_struct structures used by this module. */ static void srcu_module_going(struct module *mod) { int i; struct srcu_struct *ssp; struct srcu_struct **sspp = mod->srcu_struct_ptrs; for (i = 0; i < mod->num_srcu_structs; i++) { ssp = *(sspp++); check_init_srcu_struct(ssp); cleanup_srcu_struct(ssp); } } ------------------------------------------------------------------------ void cleanup_srcu_struct(struct srcu_struct *ssp) { int cpu; struct srcu_usage *sup = ssp->srcu_sup; bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); if (WARN_ON(wasused && !srcu_get_delay(ssp))) return; /* Just leak it! */ if (WARN_ON(srcu_readers_active(ssp))) return; /* Just leak it! */ flush_delayed_work(&sup->work); if (wasused) { for_each_possible_cpu(cpu) { struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); del_timer_sync(&sdp->delay_work); flush_work(&sdp->work); if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) return; /* Forgot srcu_barrier(), so just leak it! */ } } if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || WARN_ON(srcu_readers_active(ssp))) { pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); return; /* Caller forgot to stop doing call_srcu()? */ } kfree(sup->node); sup->node = NULL; sup->srcu_size_state = SRCU_SIZE_SMALL; if (!sup->sda_is_static) { free_percpu(ssp->sda); ssp->sda = NULL; kfree(sup); ssp->srcu_sup = NULL; } }
>On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > insmod rcutorture.ko > > > > rmmod rcutorture.ko > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > at this time the sup structure's->delaywork.func is null, if not > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > srcu_module_going() the __flush_work() find > > > > work->func is empty, will raise the warning above. > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > >One question below... > > > > > > > > Thanx, Paul > > > > > > > > --- > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > --- a/kernel/rcu/srcutree.c > > > > +++ b/kernel/rcu/srcutree.c > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > return -ENOMEM; > > > > - init_srcu_struct_data(ssp); > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > + return -ENOMEM; > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > >the initialization, just as for the non-module case. Or am I missing > > > >something subtle? > > > > > > Hi Paul > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > but we use torture_type=rcu to test, there will not be any interface > > > calling > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > structure's->delaywork.func is process_srcu(). > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > initialize, it's null, will trigger warning. > > > > > > About kernel/workqueue.c:3167 > > > > > > __flush_work > > > if (WARN_ON(!work->func)) <---------trigger waning > > > return false; > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > is zero, after that when we call > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > >code in srcu_module_going() only did the the matching > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > unload function. > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > can not free and also lost the opportunity to refresh the running work. > > > > > >But in the module case, isn't the srcu_sup->node also statically > >allocated via the "static struct srcu_usage" declaration? > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > { > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > ... > } > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > right? > >You are absolutely right, thank you! > >There are a couple of ways to resolve this. One is to simply add >a check_init_srcu_struct() before the call to cleanup_srcu_struct() >from srcu_module_going(), as shown below. This seems a bit silly, >potentially initializing fields for no good reason. > >Another way is to make cleanup_srcu_struct() do the same check >that check_init_srcu_struct() does: > > rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) > >If the value is non-zero, then cleanup_srcu_struct() should skip >consistency checks that complain about things that cannot happen if >there never was an RCU grace period. Maybe something as shown after >the second line of dashes. > >Thoughts? > > Thanx, Paul > >------------------------------------------------------------------------ > > >/* Initialize any global-scope srcu_struct structures used by this module. */ >static int srcu_module_coming(struct module *mod) >{ > int i; > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > struct srcu_struct *ssp; > > for (i = 0; i < mod->num_srcu_structs; i++) { > ssp = *(sspp++); > ssp->sda = alloc_percpu(struct srcu_data); > if (WARN_ON_ONCE(!ssp->sda)) > return -ENOMEM; > init_srcu_struct_data(ssp); > } > return 0; >} > >/* Clean up any global-scope srcu_struct structures used by this module. */ >static void srcu_module_going(struct module *mod) >{ > int i; > struct srcu_struct *ssp; > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > for (i = 0; i < mod->num_srcu_structs; i++) { > ssp = *(sspp++); > check_init_srcu_struct(ssp); > cleanup_srcu_struct(ssp); > } >} > >------------------------------------------------------------------------ > >void cleanup_srcu_struct(struct srcu_struct *ssp) >{ > int cpu; > struct srcu_usage *sup = ssp->srcu_sup; > bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > > if (WARN_ON(wasused && !srcu_get_delay(ssp))) > return; /* Just leak it! */ > if (WARN_ON(srcu_readers_active(ssp))) > return; /* Just leak it! */ > flush_delayed_work(&sup->work); > if (wasused) { > for_each_possible_cpu(cpu) { > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > > del_timer_sync(&sdp->delay_work); > flush_work(&sdp->work); > if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) > return; /* Forgot srcu_barrier(), so just leak it! */ > } > } > if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || > WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > WARN_ON(srcu_readers_active(ssp))) { > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), > rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); > return; /* Caller forgot to stop doing call_srcu()? */ > } > kfree(sup->node); > sup->node = NULL; > sup->srcu_size_state = SRCU_SIZE_SMALL; > if (!sup->sda_is_static) { > free_percpu(ssp->sda); > ssp->sda = NULL; > kfree(sup); > ssp->srcu_sup = NULL; > } >} If we have not invoke check_init_srcu_struct() , that means call_srcu(), synchronize_srcu(), srcu_barrier(), start_poll_synchronize_srcu() are also not invoke, so Is there no need to check srcu_readers_active()? Thanks Zqiang
>On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > insmod rcutorture.ko > > > > rmmod rcutorture.ko > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > at this time the sup structure's->delaywork.func is null, if not > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > srcu_module_going() the __flush_work() find > > > > work->func is empty, will raise the warning above. > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > >One question below... > > > > > > > > Thanx, Paul > > > > > > > > --- > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > --- a/kernel/rcu/srcutree.c > > > > +++ b/kernel/rcu/srcutree.c > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > return -ENOMEM; > > > > - init_srcu_struct_data(ssp); > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > + return -ENOMEM; > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > >the initialization, just as for the non-module case. Or am I missing > > > >something subtle? > > > > > > Hi Paul > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > but we use torture_type=rcu to test, there will not be any interface > > > calling > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > structure's->delaywork.func is process_srcu(). > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > initialize, it's null, will trigger warning. > > > > > > About kernel/workqueue.c:3167 > > > > > > __flush_work > > > if (WARN_ON(!work->func)) <---------trigger waning > > > return false; > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > is zero, after that when we call > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > >code in srcu_module_going() only did the the matching > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > unload function. > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > can not free and also lost the opportunity to refresh the running work. > > > > > >But in the module case, isn't the srcu_sup->node also statically > >allocated via the "static struct srcu_usage" declaration? > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > { > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > ... > } > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > right? > >You are absolutely right, thank you! > >There are a couple of ways to resolve this. One is to simply add >a check_init_srcu_struct() before the call to cleanup_srcu_struct() >from srcu_module_going(), as shown below. This seems a bit silly, >potentially initializing fields for no good reason. > >Another way is to make cleanup_srcu_struct() do the same check >that check_init_srcu_struct() does: > > rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) > >If the value is non-zero, then cleanup_srcu_struct() should skip >consistency checks that complain about things that cannot happen if >there never was an RCU grace period. Maybe something as shown after >the second line of dashes. > >Thoughts? > > Thanx, Paul > >------------------------------------------------------------------------ > > >/* Initialize any global-scope srcu_struct structures used by this module. */ >static int srcu_module_coming(struct module *mod) >{ > int i; > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > struct srcu_struct *ssp; > > for (i = 0; i < mod->num_srcu_structs; i++) { > ssp = *(sspp++); > ssp->sda = alloc_percpu(struct srcu_data); > if (WARN_ON_ONCE(!ssp->sda)) > return -ENOMEM; > init_srcu_struct_data(ssp); > } > return 0; >} > >/* Clean up any global-scope srcu_struct structures used by this module. */ >static void srcu_module_going(struct module *mod) >{ > int i; > struct srcu_struct *ssp; > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > for (i = 0; i < mod->num_srcu_structs; i++) { > ssp = *(sspp++); > check_init_srcu_struct(ssp); > cleanup_srcu_struct(ssp); > } >} > >------------------------------------------------------------------------ > >void cleanup_srcu_struct(struct srcu_struct *ssp) >{ > int cpu; > struct srcu_usage *sup = ssp->srcu_sup; > bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > > if (WARN_ON(wasused && !srcu_get_delay(ssp))) > return; /* Just leak it! */ > if (WARN_ON(srcu_readers_active(ssp))) > return; /* Just leak it! */ > flush_delayed_work(&sup->work); > if (wasused) { If wasused=false It not need to invoke flush_delayed_work(&sup->work); this trigger WARN_ON(!work->func)) . > for_each_possible_cpu(cpu) { > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > > del_timer_sync(&sdp->delay_work); > flush_work(&sdp->work); > if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) > return; /* Forgot srcu_barrier(), so just leak it! */ > } > } > if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || > WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > WARN_ON(srcu_readers_active(ssp))) { > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), > rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); > return; /* Caller forgot to stop doing call_srcu()? */ > } > kfree(sup->node); > sup->node = NULL; > sup->srcu_size_state = SRCU_SIZE_SMALL; > if (!sup->sda_is_static) { > free_percpu(ssp->sda); > ssp->sda = NULL; > kfree(sup); > ssp->srcu_sup = NULL; > } >} If we have not invoke check_init_srcu_struct() , that means call_srcu(), synchronize_srcu(), srcu_barrier(), start_poll_synchronize_srcu() are also not invoke, so Is there no need to check srcu_readers_active()? Thanks Zqiang
On Wed, Mar 22, 2023 at 11:52:13PM +0000, Zhang, Qiang1 wrote: > >On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > > insmod rcutorture.ko > > > > > rmmod rcutorture.ko > > > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > > at this time the sup structure's->delaywork.func is null, if not > > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > > srcu_module_going() the __flush_work() find > > > > > work->func is empty, will raise the warning above. > > > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > > > >One question below... > > > > > > > > > > Thanx, Paul > > > > > > > > > > --- > > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > > --- a/kernel/rcu/srcutree.c > > > > > +++ b/kernel/rcu/srcutree.c > > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > > return -ENOMEM; > > > > > - init_srcu_struct_data(ssp); > > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > > + return -ENOMEM; > > > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > > >the initialization, just as for the non-module case. Or am I missing > > > > >something subtle? > > > > > > > > Hi Paul > > > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > > but we use torture_type=rcu to test, there will not be any interface > > > > calling > > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > > structure's->delaywork.func is process_srcu(). > > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > > initialize, it's null, will trigger warning. > > > > > > > > About kernel/workqueue.c:3167 > > > > > > > > __flush_work > > > > if (WARN_ON(!work->func)) <---------trigger waning > > > > return false; > > > > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > > is zero, after that when we call > > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > > >code in srcu_module_going() only did the the matching > > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > > unload function. > > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > > can not free and also lost the opportunity to refresh the running work. > > > > > > > > >But in the module case, isn't the srcu_sup->node also statically > > >allocated via the "static struct srcu_usage" declaration? > > > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > > { > > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > > ... > > } > > > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > > right? > > > >You are absolutely right, thank you! > > > >There are a couple of ways to resolve this. One is to simply add > >a check_init_srcu_struct() before the call to cleanup_srcu_struct() > >from srcu_module_going(), as shown below. This seems a bit silly, > >potentially initializing fields for no good reason. > > > >Another way is to make cleanup_srcu_struct() do the same check > >that check_init_srcu_struct() does: > > > > rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) > > > >If the value is non-zero, then cleanup_srcu_struct() should skip > >consistency checks that complain about things that cannot happen if > >there never was an RCU grace period. Maybe something as shown after > >the second line of dashes. > > > >Thoughts? > > > > Thanx, Paul > > > >------------------------------------------------------------------------ > > > > > >/* Initialize any global-scope srcu_struct structures used by this module. */ > >static int srcu_module_coming(struct module *mod) > >{ > > int i; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > struct srcu_struct *ssp; > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > ssp = *(sspp++); > > ssp->sda = alloc_percpu(struct srcu_data); > > if (WARN_ON_ONCE(!ssp->sda)) > > return -ENOMEM; > > init_srcu_struct_data(ssp); > > } > > return 0; > >} > > > >/* Clean up any global-scope srcu_struct structures used by this module. */ > >static void srcu_module_going(struct module *mod) > >{ > > int i; > > struct srcu_struct *ssp; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > ssp = *(sspp++); > > check_init_srcu_struct(ssp); > > cleanup_srcu_struct(ssp); > > } > >} > > > >------------------------------------------------------------------------ > > > >void cleanup_srcu_struct(struct srcu_struct *ssp) > >{ > > int cpu; > > struct srcu_usage *sup = ssp->srcu_sup; > > bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > > > > if (WARN_ON(wasused && !srcu_get_delay(ssp))) > > return; /* Just leak it! */ > > if (WARN_ON(srcu_readers_active(ssp))) > > return; /* Just leak it! */ > > flush_delayed_work(&sup->work); > > if (wasused) { > > If wasused=false It not need to invoke flush_delayed_work(&sup->work); > this trigger WARN_ON(!work->func)) . Again, good catch! I will pull that flush_delayed_work() into this "if" statement. > > for_each_possible_cpu(cpu) { > > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > > > > del_timer_sync(&sdp->delay_work); > > flush_work(&sdp->work); > > if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) > > return; /* Forgot srcu_barrier(), so just leak it! */ > > } > > } > > if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || > > WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > > WARN_ON(srcu_readers_active(ssp))) { > > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), > > rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); > > return; /* Caller forgot to stop doing call_srcu()? */ > > } > > kfree(sup->node); > > sup->node = NULL; > > sup->srcu_size_state = SRCU_SIZE_SMALL; > > if (!sup->sda_is_static) { > > free_percpu(ssp->sda); > > ssp->sda = NULL; > > kfree(sup); > > ssp->srcu_sup = NULL; > > } > >} > > > If we have not invoke check_init_srcu_struct() , that means call_srcu(), synchronize_srcu(), srcu_barrier(), start_poll_synchronize_srcu() are also not invoke, so Is there no need to check > srcu_readers_active()? The module might have had lots of SRCU readers, but no updaters, and a bug in that module might mean that that readers are still active. For example, the module might have passed the srcu_struct structure to some function in the main kernel, and then forgotten to tell that function to stop doing srcu_read_lock() and srcu_read_unlock() on it. Or the module might have created a kthread that did SRCU readers, and then have forgotten to stop that kthread. Please see below for an untested patch. And yet again, thoughts? ;-) Thanx, Paul ------------------------------------------------------------------------ diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 1fb078abbdc9..fe04214ce84c 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -645,22 +645,25 @@ void cleanup_srcu_struct(struct srcu_struct *ssp) { int cpu; struct srcu_usage *sup = ssp->srcu_sup; + bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); - if (WARN_ON(!srcu_get_delay(ssp))) + if (WARN_ON(wasused && !srcu_get_delay(ssp))) return; /* Just leak it! */ if (WARN_ON(srcu_readers_active(ssp))) return; /* Just leak it! */ - flush_delayed_work(&sup->work); - for_each_possible_cpu(cpu) { - struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); + if (wasused) { + flush_delayed_work(&sup->work); + for_each_possible_cpu(cpu) { + struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); - del_timer_sync(&sdp->delay_work); - flush_work(&sdp->work); - if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) - return; /* Forgot srcu_barrier(), so just leak it! */ + del_timer_sync(&sdp->delay_work); + flush_work(&sdp->work); + if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) + return; /* Forgot srcu_barrier(), so just leak it! */ + } } - if (WARN_ON(rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || - WARN_ON(rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || + if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || + WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || WARN_ON(srcu_readers_active(ssp))) { pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)),
> >On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > > insmod rcutorture.ko > > > > > rmmod rcutorture.ko > > > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > > at this time the sup structure's->delaywork.func is null, if not > > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > > srcu_module_going() the __flush_work() find > > > > > work->func is empty, will raise the warning above. > > > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > > > >One question below... > > > > > > > > > > Thanx, Paul > > > > > > > > > > --- > > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > > --- a/kernel/rcu/srcutree.c > > > > > +++ b/kernel/rcu/srcutree.c > > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > > return -ENOMEM; > > > > > - init_srcu_struct_data(ssp); > > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > > + return -ENOMEM; > > > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > > >the initialization, just as for the non-module case. Or am I missing > > > > >something subtle? > > > > > > > > Hi Paul > > > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > > but we use torture_type=rcu to test, there will not be any interface > > > > calling > > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > > structure's->delaywork.func is process_srcu(). > > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > > initialize, it's null, will trigger warning. > > > > > > > > About kernel/workqueue.c:3167 > > > > > > > > __flush_work > > > > if (WARN_ON(!work->func)) <---------trigger waning > > > > return false; > > > > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > > is zero, after that when we call > > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > > >code in srcu_module_going() only did the the matching > > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > > unload function. > > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > > can not free and also lost the opportunity to refresh the running work. > > > > > > > > >But in the module case, isn't the srcu_sup->node also statically > > >allocated via the "static struct srcu_usage" declaration? > > > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > > { > > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > > ... > > } > > > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > > right? > > > >You are absolutely right, thank you! > > > >There are a couple of ways to resolve this. One is to simply add > >a check_init_srcu_struct() before the call to cleanup_srcu_struct() > >from srcu_module_going(), as shown below. This seems a bit silly, > >potentially initializing fields for no good reason. > > > >Another way is to make cleanup_srcu_struct() do the same check > >that check_init_srcu_struct() does: > > > > rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) > > > >If the value is non-zero, then cleanup_srcu_struct() should skip > >consistency checks that complain about things that cannot happen if > >there never was an RCU grace period. Maybe something as shown after > >the second line of dashes. > > > >Thoughts? > > > > Thanx, Paul > > > >------------------------------------------------------------------------ > > > > > >/* Initialize any global-scope srcu_struct structures used by this module. */ > >static int srcu_module_coming(struct module *mod) > >{ > > int i; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > struct srcu_struct *ssp; > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > ssp = *(sspp++); > > ssp->sda = alloc_percpu(struct srcu_data); > > if (WARN_ON_ONCE(!ssp->sda)) > > return -ENOMEM; > > init_srcu_struct_data(ssp); > > } > > return 0; > >} > > > >/* Clean up any global-scope srcu_struct structures used by this module. */ > >static void srcu_module_going(struct module *mod) > >{ > > int i; > > struct srcu_struct *ssp; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > ssp = *(sspp++); > > check_init_srcu_struct(ssp); > > cleanup_srcu_struct(ssp); > > } > >} > > > >------------------------------------------------------------------------ > > > >void cleanup_srcu_struct(struct srcu_struct *ssp) > >{ > > int cpu; > > struct srcu_usage *sup = ssp->srcu_sup; > > bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > > > > if (WARN_ON(wasused && !srcu_get_delay(ssp))) > > return; /* Just leak it! */ > > if (WARN_ON(srcu_readers_active(ssp))) > > return; /* Just leak it! */ > > flush_delayed_work(&sup->work); > > if (wasused) { > > If wasused=false It not need to invoke flush_delayed_work(&sup->work); > this trigger WARN_ON(!work->func)) . > >Again, good catch! I will pull that flush_delayed_work() into this >"if" statement. > > > for_each_possible_cpu(cpu) { > > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > > > > del_timer_sync(&sdp->delay_work); > > flush_work(&sdp->work); > > if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) > > return; /* Forgot srcu_barrier(), so just leak it! */ > > } > > } > > if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || > > WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > > WARN_ON(srcu_readers_active(ssp))) { > > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), > > rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); > > return; /* Caller forgot to stop doing call_srcu()? */ > > } > > kfree(sup->node); > > sup->node = NULL; > > sup->srcu_size_state = SRCU_SIZE_SMALL; > > if (!sup->sda_is_static) { > > free_percpu(ssp->sda); > > ssp->sda = NULL; > > kfree(sup); > > ssp->srcu_sup = NULL; > > } > >} > > > If we have not invoke check_init_srcu_struct() , that means call_srcu(), synchronize_srcu(), srcu_barrier(), start_poll_synchronize_srcu() are also not invoke, so Is there no need to check > srcu_readers_active()? > >The module might have had lots of SRCU readers, but no updaters, and >a bug in that module might mean that that readers are still active. >For example, the module might have passed the srcu_struct structure >to some function in the main kernel, and then forgotten to tell that >function to stop doing srcu_read_lock() and srcu_read_unlock() on it. > >Or the module might have created a kthread that did SRCU readers, and >then have forgotten to stop that kthread. > fully understand. > >Please see below for an untested patch. I will test the following modification. Thanks Zqiang > >And yet again, thoughts? ;-) > > Thanx, Paul > >------------------------------------------------------------------------ > >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c >index 1fb078abbdc9..fe04214ce84c 100644 >--- a/kernel/rcu/srcutree.c >+++ b/kernel/rcu/srcutree.c >@@ -645,22 +645,25 @@ void cleanup_srcu_struct(struct srcu_struct *ssp) > { > int cpu; > struct srcu_usage *sup = ssp->srcu_sup; >+ bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > >- if (WARN_ON(!srcu_get_delay(ssp))) >+ if (WARN_ON(wasused && !srcu_get_delay(ssp))) > return; /* Just leak it! */ > if (WARN_ON(srcu_readers_active(ssp))) > return; /* Just leak it! */ >- flush_delayed_work(&sup->work); >- for_each_possible_cpu(cpu) { >- struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); >+ if (wasused) { >+ flush_delayed_work(&sup->work); >+ for_each_possible_cpu(cpu) { >+ struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > >- del_timer_sync(&sdp->delay_work); >- flush_work(&sdp->work); >- if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) >- return; /* Forgot srcu_barrier(), so just leak it! */ >+ del_timer_sync(&sdp->delay_work); >+ flush_work(&sdp->work); >+ if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) >+ return; /* Forgot srcu_barrier(), so just leak it! */ >+ } > } >- if (WARN_ON(rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || >- WARN_ON(rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || >+ if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || >+ WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > WARN_ON(srcu_readers_active(ssp))) { > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)),
> >On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > > insmod rcutorture.ko > > > > > rmmod rcutorture.ko > > > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > > at this time the sup structure's->delaywork.func is null, if not > > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > > srcu_module_going() the __flush_work() find > > > > > work->func is empty, will raise the warning above. > > > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > > > >One question below... > > > > > > > > > > Thanx, Paul > > > > > > > > > > --- > > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > > --- a/kernel/rcu/srcutree.c > > > > > +++ b/kernel/rcu/srcutree.c > > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > > return -ENOMEM; > > > > > - init_srcu_struct_data(ssp); > > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > > + return -ENOMEM; > > > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > > >the initialization, just as for the non-module case. Or am I missing > > > > >something subtle? > > > > > > > > Hi Paul > > > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > > but we use torture_type=rcu to test, there will not be any interface > > > > calling > > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > > structure's->delaywork.func is process_srcu(). > > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > > initialize, it's null, will trigger warning. > > > > > > > > About kernel/workqueue.c:3167 > > > > > > > > __flush_work > > > > if (WARN_ON(!work->func)) <---------trigger waning > > > > return false; > > > > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > > is zero, after that when we call > > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > > >code in srcu_module_going() only did the the matching > > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > > unload function. > > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > > can not free and also lost the opportunity to refresh the running work. > > > > > > > > >But in the module case, isn't the srcu_sup->node also statically > > >allocated via the "static struct srcu_usage" declaration? > > > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > > { > > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > > ... > > } > > > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > > right? > > > >You are absolutely right, thank you! > > > >There are a couple of ways to resolve this. One is to simply add > >a check_init_srcu_struct() before the call to cleanup_srcu_struct() > >from srcu_module_going(), as shown below. This seems a bit silly, > >potentially initializing fields for no good reason. > > > >Another way is to make cleanup_srcu_struct() do the same check > >that check_init_srcu_struct() does: > > > > rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) > > > >If the value is non-zero, then cleanup_srcu_struct() should skip > >consistency checks that complain about things that cannot happen if > >there never was an RCU grace period. Maybe something as shown after > >the second line of dashes. > > > >Thoughts? > > > > Thanx, Paul > > > >------------------------------------------------------------------------ > > > > > >/* Initialize any global-scope srcu_struct structures used by this module. */ > >static int srcu_module_coming(struct module *mod) > >{ > > int i; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > struct srcu_struct *ssp; > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > ssp = *(sspp++); > > ssp->sda = alloc_percpu(struct srcu_data); > > if (WARN_ON_ONCE(!ssp->sda)) > > return -ENOMEM; > > init_srcu_struct_data(ssp); > > } > > return 0; > >} > > > >/* Clean up any global-scope srcu_struct structures used by this module. */ > >static void srcu_module_going(struct module *mod) > >{ > > int i; > > struct srcu_struct *ssp; > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > ssp = *(sspp++); > > check_init_srcu_struct(ssp); > > cleanup_srcu_struct(ssp); > > } > >} > > > >------------------------------------------------------------------------ > > > >void cleanup_srcu_struct(struct srcu_struct *ssp) > >{ > > int cpu; > > struct srcu_usage *sup = ssp->srcu_sup; > > bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > > > > if (WARN_ON(wasused && !srcu_get_delay(ssp))) > > return; /* Just leak it! */ > > if (WARN_ON(srcu_readers_active(ssp))) > > return; /* Just leak it! */ > > flush_delayed_work(&sup->work); > > if (wasused) { > > If wasused=false It not need to invoke flush_delayed_work(&sup->work); > this trigger WARN_ON(!work->func)) . > >Again, good catch! I will pull that flush_delayed_work() into this >"if" statement. > > > for_each_possible_cpu(cpu) { > > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > > > > del_timer_sync(&sdp->delay_work); > > flush_work(&sdp->work); > > if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) > > return; /* Forgot srcu_barrier(), so just leak it! */ > > } > > } > > if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || > > WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > > WARN_ON(srcu_readers_active(ssp))) { > > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), > > rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); > > return; /* Caller forgot to stop doing call_srcu()? */ > > } > > kfree(sup->node); > > sup->node = NULL; > > sup->srcu_size_state = SRCU_SIZE_SMALL; > > if (!sup->sda_is_static) { > > free_percpu(ssp->sda); > > ssp->sda = NULL; > > kfree(sup); > > ssp->srcu_sup = NULL; > > } > >} > > > If we have not invoke check_init_srcu_struct() , that means call_srcu(), synchronize_srcu(), srcu_barrier(), start_poll_synchronize_srcu() are also not invoke, so Is there no need to check > srcu_readers_active()? > >The module might have had lots of SRCU readers, but no updaters, and >a bug in that module might mean that that readers are still active. >For example, the module might have passed the srcu_struct structure >to some function in the main kernel, and then forgotten to tell that >function to stop doing srcu_read_lock() and srcu_read_unlock() on it. > >Or the module might have created a kthread that did SRCU readers, and >then have forgotten to stop that kthread. > > >fully understand. > > >Please see below for an untested patch. > >I will test the following modification. > >Thanks >Zqiang > > >And yet again, thoughts? ;-) Maybe add the following modification, otherwise, in cleanup_srcu_struct() Kfree(sup) will release ssp->srcu_sup which is statically allocated. @@ -1926,6 +1926,7 @@ static int srcu_module_coming(struct module *mod) if (WARN_ON_ONCE(!ssp->sda)) return -ENOMEM; init_srcu_struct_data(ssp); + ssp->srcu_sup->sda_is_static = true; } Thanks Zqiang > > Thanx, Paul > >------------------------------------------------------------------------ > >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c >index 1fb078abbdc9..fe04214ce84c 100644 >--- a/kernel/rcu/srcutree.c >+++ b/kernel/rcu/srcutree.c >@@ -645,22 +645,25 @@ void cleanup_srcu_struct(struct srcu_struct *ssp) > { > int cpu; > struct srcu_usage *sup = ssp->srcu_sup; >+ bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > >- if (WARN_ON(!srcu_get_delay(ssp))) >+ if (WARN_ON(wasused && !srcu_get_delay(ssp))) > return; /* Just leak it! */ > if (WARN_ON(srcu_readers_active(ssp))) > return; /* Just leak it! */ >- flush_delayed_work(&sup->work); >- for_each_possible_cpu(cpu) { >- struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); >+ if (wasused) { >+ flush_delayed_work(&sup->work); >+ for_each_possible_cpu(cpu) { >+ struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > >- del_timer_sync(&sdp->delay_work); >- flush_work(&sdp->work); >- if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) >- return; /* Forgot srcu_barrier(), so just leak it! */ >+ del_timer_sync(&sdp->delay_work); >+ flush_work(&sdp->work); >+ if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) >+ return; /* Forgot srcu_barrier(), so just leak it! */ >+ } > } >- if (WARN_ON(rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || >- WARN_ON(rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || >+ if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || >+ WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > WARN_ON(srcu_readers_active(ssp))) { > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)),
On Thu, Mar 23, 2023 at 03:39:12AM +0000, Zhang, Qiang1 wrote: > > >On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > > > insmod rcutorture.ko > > > > > > rmmod rcutorture.ko > > > > > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > > > at this time the sup structure's->delaywork.func is null, if not > > > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > > > srcu_module_going() the __flush_work() find > > > > > > work->func is empty, will raise the warning above. > > > > > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > > > > > >One question below... > > > > > > > > > > > > Thanx, Paul > > > > > > > > > > > > --- > > > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > > > --- a/kernel/rcu/srcutree.c > > > > > > +++ b/kernel/rcu/srcutree.c > > > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > > > return -ENOMEM; > > > > > > - init_srcu_struct_data(ssp); > > > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > > > + return -ENOMEM; > > > > > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > > > >the initialization, just as for the non-module case. Or am I missing > > > > > >something subtle? > > > > > > > > > > Hi Paul > > > > > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > > > but we use torture_type=rcu to test, there will not be any interface > > > > > calling > > > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > > > structure's->delaywork.func is process_srcu(). > > > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > > > initialize, it's null, will trigger warning. > > > > > > > > > > About kernel/workqueue.c:3167 > > > > > > > > > > __flush_work > > > > > if (WARN_ON(!work->func)) <---------trigger waning > > > > > return false; > > > > > > > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > > > is zero, after that when we call > > > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > > > >code in srcu_module_going() only did the the matching > > > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > > > unload function. > > > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > > > can not free and also lost the opportunity to refresh the running work. > > > > > > > > > > > >But in the module case, isn't the srcu_sup->node also statically > > > >allocated via the "static struct srcu_usage" declaration? > > > > > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > > > { > > > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > > > ... > > > } > > > > > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > > > right? > > > > > >You are absolutely right, thank you! > > > > > >There are a couple of ways to resolve this. One is to simply add > > >a check_init_srcu_struct() before the call to cleanup_srcu_struct() > > >from srcu_module_going(), as shown below. This seems a bit silly, > > >potentially initializing fields for no good reason. > > > > > >Another way is to make cleanup_srcu_struct() do the same check > > >that check_init_srcu_struct() does: > > > > > > rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) > > > > > >If the value is non-zero, then cleanup_srcu_struct() should skip > > >consistency checks that complain about things that cannot happen if > > >there never was an RCU grace period. Maybe something as shown after > > >the second line of dashes. > > > > > >Thoughts? > > > > > > Thanx, Paul > > > > > >------------------------------------------------------------------------ > > > > > > > > >/* Initialize any global-scope srcu_struct structures used by this module. */ > > >static int srcu_module_coming(struct module *mod) > > >{ > > > int i; > > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > struct srcu_struct *ssp; > > > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > > ssp = *(sspp++); > > > ssp->sda = alloc_percpu(struct srcu_data); > > > if (WARN_ON_ONCE(!ssp->sda)) > > > return -ENOMEM; > > > init_srcu_struct_data(ssp); > > > } > > > return 0; > > >} > > > > > >/* Clean up any global-scope srcu_struct structures used by this module. */ > > >static void srcu_module_going(struct module *mod) > > >{ > > > int i; > > > struct srcu_struct *ssp; > > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > > ssp = *(sspp++); > > > check_init_srcu_struct(ssp); > > > cleanup_srcu_struct(ssp); > > > } > > >} > > > > > >------------------------------------------------------------------------ > > > > > >void cleanup_srcu_struct(struct srcu_struct *ssp) > > >{ > > > int cpu; > > > struct srcu_usage *sup = ssp->srcu_sup; > > > bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > > > > > > if (WARN_ON(wasused && !srcu_get_delay(ssp))) > > > return; /* Just leak it! */ > > > if (WARN_ON(srcu_readers_active(ssp))) > > > return; /* Just leak it! */ > > > flush_delayed_work(&sup->work); > > > if (wasused) { > > > > If wasused=false It not need to invoke flush_delayed_work(&sup->work); > > this trigger WARN_ON(!work->func)) . > > > >Again, good catch! I will pull that flush_delayed_work() into this > >"if" statement. > > > > > for_each_possible_cpu(cpu) { > > > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > > > > > > del_timer_sync(&sdp->delay_work); > > > flush_work(&sdp->work); > > > if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) > > > return; /* Forgot srcu_barrier(), so just leak it! */ > > > } > > > } > > > if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || > > > WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > > > WARN_ON(srcu_readers_active(ssp))) { > > > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > > > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), > > > rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); > > > return; /* Caller forgot to stop doing call_srcu()? */ > > > } > > > kfree(sup->node); > > > sup->node = NULL; > > > sup->srcu_size_state = SRCU_SIZE_SMALL; > > > if (!sup->sda_is_static) { > > > free_percpu(ssp->sda); > > > ssp->sda = NULL; > > > kfree(sup); > > > ssp->srcu_sup = NULL; > > > } > > >} > > > > > > If we have not invoke check_init_srcu_struct() , that means call_srcu(), synchronize_srcu(), srcu_barrier(), start_poll_synchronize_srcu() are also not invoke, so Is there no need to check > > srcu_readers_active()? > > > >The module might have had lots of SRCU readers, but no updaters, and > >a bug in that module might mean that that readers are still active. > >For example, the module might have passed the srcu_struct structure > >to some function in the main kernel, and then forgotten to tell that > >function to stop doing srcu_read_lock() and srcu_read_unlock() on it. > > > >Or the module might have created a kthread that did SRCU readers, and > >then have forgotten to stop that kthread. > > > > > >fully understand. > > > > > >Please see below for an untested patch. > > > >I will test the following modification. > > > >Thanks > >Zqiang > > > > > >And yet again, thoughts? ;-) > > Maybe add the following modification, otherwise, in cleanup_srcu_struct() > Kfree(sup) will release ssp->srcu_sup which is statically allocated. > > @@ -1926,6 +1926,7 @@ static int srcu_module_coming(struct module *mod) > if (WARN_ON_ONCE(!ssp->sda)) > return -ENOMEM; > init_srcu_struct_data(ssp); > + ssp->srcu_sup->sda_is_static = true; > } Good catch and good point! But the underlying problem is that I am still making things too complex. How about the following? The idea is to skip the cleanup_srcu_struct() unless there was a call to check_init_srcu_struct(), and to free the per-CPU data either way. Thanx, Paul ------------------------------------------------------------------------ diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 1fb078abbdc9..06f8ed1ce272 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1921,7 +1921,6 @@ static int srcu_module_coming(struct module *mod) ssp->sda = alloc_percpu(struct srcu_data); if (WARN_ON_ONCE(!ssp->sda)) return -ENOMEM; - init_srcu_struct_data(ssp); } return 0; } @@ -1930,10 +1929,17 @@ static int srcu_module_coming(struct module *mod) static void srcu_module_going(struct module *mod) { int i; + struct srcu_data __percpu *sda; + struct srcu_struct *ssp; struct srcu_struct **sspp = mod->srcu_struct_ptrs; - for (i = 0; i < mod->num_srcu_structs; i++) - cleanup_srcu_struct(*(sspp++)); + for (i = 0; i < mod->num_srcu_structs; i++) { + ssp = *(sspp++); + sda = ssp->sda; + if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed))) + cleanup_srcu_struct(ssp); + free_percpu(sda); + } } /* Handle one module, either coming or going. */
> > >On Wed, Mar 22, 2023 at 10:08:54PM +0000, Zhang, Qiang1 wrote: > > > > > > insmod rcutorture.ko > > > > > > rmmod rcutorture.ko > > > > > > > > > > > > [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 > > > > > > __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: > > > > > > rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] > > > > > > CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ > > > > > > [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... > > > > > > [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] > > > > > > cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] > > > > > > srcu_module_notify+0x71/0x140 [ 209.437854] > > > > > > blocking_notifier_call_chain+0x9d/0xd0 > > > > > > [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 > > > > > > [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] > > > > > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > > > > > > > > > > > For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > > > when compiling and loading as modules, the srcu_module_coming() is > > > > > > invoked, allocate memory for srcu structure's->sda and initialize > > > > > > sda structure, due to not fully initialize srcu structure's->sup, so > > > > > > at this time the sup structure's->delaywork.func is null, if not > > > > > > invoke init_srcu_struct_fields() before unloading modules, in > > > > > > srcu_module_going() the __flush_work() find > > > > > > work->func is empty, will raise the warning above. > > > > > > > > > > > > This commit add init_srcu_struct_fields() to initialize srcu > > > > > > structure's->sup, in srcu_module_coming(). > > > > > > > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > > > > > > > > > >Good catch, and thank you for testing the in-module case! > > > > > > > > > > > >One question below... > > > > > > > > > > > > Thanx, Paul > > > > > > > > > > > > --- > > > > > > kernel/rcu/srcutree.c | 11 ++++++++--- > > > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index > > > > > > 1fb078abbdc9..42d8720e016c 100644 > > > > > > --- a/kernel/rcu/srcutree.c > > > > > > +++ b/kernel/rcu/srcutree.c > > > > > > @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) > > > > > > ssp->sda = alloc_percpu(struct srcu_data); > > > > > > if (WARN_ON_ONCE(!ssp->sda)) > > > > > > return -ENOMEM; > > > > > > - init_srcu_struct_data(ssp); > > > > > > + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) > > > > > > + return -ENOMEM; > > > > > > > > > > > >Wouldn't it be better to simply delete the init_srcu_struct_data()? > > > > > > > > > > > >Then the first call to check_init_srcu_struct() would take care of > > > > > >the initialization, just as for the non-module case. Or am I missing > > > > > >something subtle? > > > > > > > > > > Hi Paul > > > > > > > > > > Maybe the check_init_srcu_struct() is always not invoked, for example, > > > > > In rcutorture.c, here is such a definition DEFINE_STATIC_SRCU(srcu_ctl), > > > > > but we use torture_type=rcu to test, there will not be any interface > > > > > calling > > > > > check_init_srcu_struct() to initialize srcu_ctl and set > > > > > structure's->delaywork.func is process_srcu(). > > > > > when we unload the rcutorture module, invoke cleanup_srcu_struct() to > > > > > flush sup structure's->delaywork.func, due to the func pointer is not > > > > > initialize, it's null, will trigger warning. > > > > > > > > > > About kernel/workqueue.c:3167 > > > > > > > > > > __flush_work > > > > > if (WARN_ON(!work->func)) <---------trigger waning > > > > > return false; > > > > > > > > > > > > > > > and in init_srcu_struct_fields(ssp, true), wil set > > > > > srcu_sup->sda_is_static is true and set srcu_sup-> srcu_gp_seq_needed > > > > > is zero, after that when we call > > > > > check_init_srcu_struct() again, it not be initialized again. > > > > > > > > > > > > > > >Good point! In the non-module statically allocated case there is never a call to cleanup_srcu_struct(). > > > > > > > > > >So suppose the code in srcu_module_coming() only did the alloc_percpu(), and then the > > > > >code in srcu_module_going() only did the the matching > > > > >free_percpu() instead of the current cleanup_srcu_struct()? > > > > > > > > But in modules, for srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), > > > > when a module is unloaded, we usually don't call cleanup_srcu_struct() in the module > > > > unload function. > > > > If in srcu_module_going() only do free_percpu(), the srcu_sup->node memory maybe > > > > can not free and also lost the opportunity to refresh the running work. > > > > > > > > > > > >But in the module case, isn't the srcu_sup->node also statically > > > >allocated via the "static struct srcu_usage" declaration? > > > > > > static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags) > > > { > > > sp->srcu_sup->node = kcalloc(rcu_num_nodes, sizeof(*ssp->srcu_sup->node), gfp_flags); > > > ... > > > } > > > > > > Regardless of whether the srcu object is declared in the module or not, sup->node is dynamically allocated. > > > right? > > > > > >You are absolutely right, thank you! > > > > > >There are a couple of ways to resolve this. One is to simply add > > >a check_init_srcu_struct() before the call to cleanup_srcu_struct() > > >from srcu_module_going(), as shown below. This seems a bit silly, > > >potentially initializing fields for no good reason. > > > > > >Another way is to make cleanup_srcu_struct() do the same check > > >that check_init_srcu_struct() does: > > > > > > rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) > > > > > >If the value is non-zero, then cleanup_srcu_struct() should skip > > >consistency checks that complain about things that cannot happen if > > >there never was an RCU grace period. Maybe something as shown after > > >the second line of dashes. > > > > > >Thoughts? > > > > > > Thanx, Paul > > > > > >------------------------------------------------------------------------ > > > > > > > > >/* Initialize any global-scope srcu_struct structures used by this module. */ > > >static int srcu_module_coming(struct module *mod) > > >{ > > > int i; > > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > struct srcu_struct *ssp; > > > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > > ssp = *(sspp++); > > > ssp->sda = alloc_percpu(struct srcu_data); > > > if (WARN_ON_ONCE(!ssp->sda)) > > > return -ENOMEM; > > > init_srcu_struct_data(ssp); > > > } > > > return 0; > > >} > > > > > >/* Clean up any global-scope srcu_struct structures used by this module. */ > > >static void srcu_module_going(struct module *mod) > > >{ > > > int i; > > > struct srcu_struct *ssp; > > > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > > > > > > for (i = 0; i < mod->num_srcu_structs; i++) { > > > ssp = *(sspp++); > > > check_init_srcu_struct(ssp); > > > cleanup_srcu_struct(ssp); > > > } > > >} > > > > > >------------------------------------------------------------------------ > > > > > >void cleanup_srcu_struct(struct srcu_struct *ssp) > > >{ > > > int cpu; > > > struct srcu_usage *sup = ssp->srcu_sup; > > > bool wasused = !rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)); > > > > > > if (WARN_ON(wasused && !srcu_get_delay(ssp))) > > > return; /* Just leak it! */ > > > if (WARN_ON(srcu_readers_active(ssp))) > > > return; /* Just leak it! */ > > > flush_delayed_work(&sup->work); > > > if (wasused) { > > > > If wasused=false It not need to invoke flush_delayed_work(&sup->work); > > this trigger WARN_ON(!work->func)) . > > > >Again, good catch! I will pull that flush_delayed_work() into this > >"if" statement. > > > > > for_each_possible_cpu(cpu) { > > > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu); > > > > > > del_timer_sync(&sdp->delay_work); > > > flush_work(&sdp->work); > > > if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist))) > > > return; /* Forgot srcu_barrier(), so just leak it! */ > > > } > > > } > > > if (WARN_ON(wasused && rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)) != SRCU_STATE_IDLE) || > > > WARN_ON(wasused && rcu_seq_current(&sup->srcu_gp_seq) != sup->srcu_gp_seq_needed) || > > > WARN_ON(srcu_readers_active(ssp))) { > > > pr_info("%s: Active srcu_struct %p read state: %d gp state: %lu/%lu\n", > > > __func__, ssp, rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)), > > > rcu_seq_current(&sup->srcu_gp_seq), sup->srcu_gp_seq_needed); > > > return; /* Caller forgot to stop doing call_srcu()? */ > > > } > > > kfree(sup->node); > > > sup->node = NULL; > > > sup->srcu_size_state = SRCU_SIZE_SMALL; > > > if (!sup->sda_is_static) { > > > free_percpu(ssp->sda); > > > ssp->sda = NULL; > > > kfree(sup); > > > ssp->srcu_sup = NULL; > > > } > > >} > > > > > > If we have not invoke check_init_srcu_struct() , that means call_srcu(), synchronize_srcu(), srcu_barrier(), start_poll_synchronize_srcu() are also not invoke, so Is there no need to check > > srcu_readers_active()? > > > >The module might have had lots of SRCU readers, but no updaters, and > >a bug in that module might mean that that readers are still active. > >For example, the module might have passed the srcu_struct structure > >to some function in the main kernel, and then forgotten to tell that > >function to stop doing srcu_read_lock() and srcu_read_unlock() on it. > > > >Or the module might have created a kthread that did SRCU readers, and > >then have forgotten to stop that kthread. > > > > > >fully understand. > > > > > >Please see below for an untested patch. > > > >I will test the following modification. > > > >Thanks > >Zqiang > > > > > >And yet again, thoughts? ;-) > > Maybe add the following modification, otherwise, in cleanup_srcu_struct() > Kfree(sup) will release ssp->srcu_sup which is statically allocated. > > @@ -1926,6 +1926,7 @@ static int srcu_module_coming(struct module *mod) > if (WARN_ON_ONCE(!ssp->sda)) > return -ENOMEM; > init_srcu_struct_data(ssp); > + ssp->srcu_sup->sda_is_static = true; > } > >Good catch and good point! > >But the underlying problem is that I am still making things too complex. > >How about the following? The idea is to skip the cleanup_srcu_struct() >unless there was a call to check_init_srcu_struct(), and to free the >per-CPU data either way. I prefer this modification, it looks simpler and clearer, I will test and resend. Thanks Zqiang > > Thanx, Paul > >------------------------------------------------------------------------ > >diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c >index 1fb078abbdc9..06f8ed1ce272 100644 >--- a/kernel/rcu/srcutree.c >+++ b/kernel/rcu/srcutree.c >@@ -1921,7 +1921,6 @@ static int srcu_module_coming(struct module *mod) > ssp->sda = alloc_percpu(struct srcu_data); > if (WARN_ON_ONCE(!ssp->sda)) > return -ENOMEM; >- init_srcu_struct_data(ssp); > } > return 0; > } >@@ -1930,10 +1929,17 @@ static int srcu_module_coming(struct module *mod) > static void srcu_module_going(struct module *mod) > { > int i; >+ struct srcu_data __percpu *sda; >+ struct srcu_struct *ssp; > struct srcu_struct **sspp = mod->srcu_struct_ptrs; > >- for (i = 0; i < mod->num_srcu_structs; i++) >- cleanup_srcu_struct(*(sspp++)); >+ for (i = 0; i < mod->num_srcu_structs; i++) { >+ ssp = *(sspp++); >+ sda = ssp->sda; >+ if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed))) >+ cleanup_srcu_struct(ssp); >+ free_percpu(sda); >+ } > } > > /* Handle one module, either coming or going. */
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 1fb078abbdc9..42d8720e016c 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod) ssp->sda = alloc_percpu(struct srcu_data); if (WARN_ON_ONCE(!ssp->sda)) return -ENOMEM; - init_srcu_struct_data(ssp); + if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true))) + return -ENOMEM; } return 0; } @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module *mod) { int i; struct srcu_struct **sspp = mod->srcu_struct_ptrs; + struct srcu_struct *ssp; - for (i = 0; i < mod->num_srcu_structs; i++) - cleanup_srcu_struct(*(sspp++)); + for (i = 0; i < mod->num_srcu_structs; i++) { + ssp = *(sspp++); + cleanup_srcu_struct(ssp); + free_percpu(ssp->sda); + } } /* Handle one module, either coming or going. */
insmod rcutorture.ko rmmod rcutorture.ko [ 209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 __flush_work+0x50a/0x540 [ 209.437346] Modules linked in: rcutorture(-) torture [last unloaded: rcutorture] [ 209.437382] CPU: 0 PID: 508 Comm: rmmod Tainted: G W 6.3.0-rc1-yocto-standard+ [ 209.437406] RIP: 0010:__flush_work+0x50a/0x540 ..... [ 209.437758] flush_delayed_work+0x36/0x90 [ 209.437776] cleanup_srcu_struct+0x68/0x2e0 [ 209.437817] srcu_module_notify+0x71/0x140 [ 209.437854] blocking_notifier_call_chain+0x9d/0xd0 [ 209.437880] __x64_sys_delete_module+0x223/0x2e0 [ 209.438046] do_syscall_64+0x43/0x90 [ 209.438062] entry_SYSCALL_64_after_hwframe+0x72/0xdc For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(), when compiling and loading as modules, the srcu_module_coming() is invoked, allocate memory for srcu structure's->sda and initialize sda structure, due to not fully initialize srcu structure's->sup, so at this time the sup structure's->delaywork.func is null, if not invoke init_srcu_struct_fields() before unloading modules, in srcu_module_going() the __flush_work() find work->func is empty, will raise the warning above. This commit add init_srcu_struct_fields() to initialize srcu structure's->sup, in srcu_module_coming(). Signed-off-by: Zqiang <qiang1.zhang@intel.com> --- kernel/rcu/srcutree.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)