Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Mar 2023 16:10:24 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Zhang, Qiang1" <qiang1.zhang@intel.com>
Cc:     "frederic@kernel.org" <frederic@kernel.org>,
        "joel@joelfernandes.org" <joel@joelfernandes.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "qiang.zhang1211@gmail.com" <qiang.zhang1211@gmail.com>
Subject: Re: [PATCH] srcu: Fix flush sup work warning in cleanup_srcu_struct()
Message-ID: <4a9525f8-30a3-4ca9-87ec-355cde7f6ed6@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20230321081346.192000-1-qiang1.zhang@intel.com>
 <e7951c31-4a24-4493-be4b-603fec0730ff@paulmck-laptop>
 <PH0PR11MB588087645FE02C9F2C113CC8DA869@PH0PR11MB5880.namprd11.prod.outlook.com>
 <f13e878f-65c3-40cb-aa3f-eb87ae6c9d6b@paulmck-laptop>
 <PH0PR11MB5880CAE4D4A58BD71AE984CFDA869@PH0PR11MB5880.namprd11.prod.outlook.com>
 <dc8b5a44-02ca-4e5a-bd4a-644af3e7c11e@paulmck-laptop>
 <PH0PR11MB58804435C08AEADF23B095D1DA869@PH0PR11MB5880.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PH0PR11MB58804435C08AEADF23B095D1DA869@PH0PR11MB5880.namprd11.prod.outlook.com>
Precedence: bulk

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