Re: [PATCH v3] Fixes: 07928d9bfc81 ("padata: Remove broken queue flushing")

[Wang Jinchao]

On Fri, Oct 13, 2023 at 11:22:03AM -0400, Daniel Jordan wrote:
> Hi,
> 
> On Sat, Oct 07, 2023 at 11:52:33AM +0800, Wang Jinchao wrote:
> > Signed-off-by: Wang Jinchao <wangjinchao@xxxxxxxxxxx>
> 
> The Fixes: tag should be near your Signed-off-by:, and the subject of
> the patch should be something descriptive like
> 
>     padata: Fix refcnt handling in padata_free_shell()
> 
> Here's some documentation about this:
> 
>     https://docs.kernel.org/process/5.Posting.html#patch-formatting-and-changelogs
> 
Updated in patch v4.

> > Acked-by: Daniel Jordan <daniel.m.jordan@xxxxxxxxxx>
> > ---
> > V3: 
> >     Include Daniel's ack
> >
> > V2: https://lore.kernel.org/all/ZRTLHY5A+VqIKhA2@fedora/
> >     To satisfy Sparse, use rcu_dereference_protected.
> >     Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >     Closes: https://lore.kernel.org/oe-kbuild-all/202309270829.xHgTOMKw-lkp@xxxxxxxxx/
> 
> These two tags can also go near your SoB.
> 
>From intel kernel test robot:
    If you fix the issue in a separate patch/commit (i.e. not just a new version of
    the same patch/commit), kindly add following tags
    | Reported-by: kernel test robot <lkp@xxxxxxxxx>
    | Closes: https://lore.kernel.org/oe-kbuild-all/202309270829.xHgTOMKw-lkp@xxxxxxxxx/
So I updated the patch without these tags.

> > V1: https://lore.kernel.org/all/ZRE4XvOOhz4HSOgR@fedora/
> > 
> >  kernel/padata.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index 222d60195de6..79d04a97ded6 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -1102,12 +1102,16 @@ EXPORT_SYMBOL(padata_alloc_shell);
> >   */
> >  void padata_free_shell(struct padata_shell *ps)
> >  {
> > +	struct parallel_data *pd;
> > +
> >  	if (!ps)
> >  		return;
> >  
> >  	mutex_lock(&ps->pinst->lock);
> >  	list_del(&ps->list);
> > -	padata_free_pd(rcu_dereference_protected(ps->pd, 1));
> > +	pd = rcu_dereference_protected(ps->pd, 1);
> > +	if (refcount_dec_and_test(&pd->refcnt))
> > +		padata_free_pd(rcu_dereference_protected(ps->pd, 1));
> 
> As Herbert points out, this version changes the code by
> rcu-dereferencing pd twice.  Usually previous acks are dropped when
> introducing new changes, but you can have it back by only
> rcu-dereferencing once and addressing the other comments above.
Thanks for your and Herbert's acks, which were included in patch v4.

I know Herbert has acked my patch from your email; however, I have 
not received Herbert's acknowledgment email, and I do not know why.
> 
> >  	mutex_unlock(&ps->pinst->lock);
> >  
> >  	kfree(ps);
> > -- 
> > 2.40.0
> >