Re: [PATCH] IB/hfi1,IB/qib: Fix qp_stats sleep with rcu read lock held

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2203 bytes --]

On Wed, Aug 10, 2016 at 01:51:52AM -0400, ira.weiny wrote:
> On Tue, Aug 09, 2016 at 09:11:46PM +0300, Leon Romanovsky wrote:
> > On Tue, Aug 09, 2016 at 11:16:26AM -0400, ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> > > From: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > 
> 
> [snip]
> 
> > > diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
> > > index dbab9d9cc288..c35bef8dd5aa 100644
> > > --- a/drivers/infiniband/hw/hfi1/debugfs.c
> > > +++ b/drivers/infiniband/hw/hfi1/debugfs.c
> > > @@ -223,28 +223,35 @@ DEBUGFS_SEQ_FILE_OPEN(ctx_stats)
> > >  DEBUGFS_FILE_OPS(ctx_stats);
> > >  
> > >  static void *_qp_stats_seq_start(struct seq_file *s, loff_t *pos)
> > > -__acquires(RCU)
> > > +	__acquires(RCU)
> > >  {
> > >  	struct qp_iter *iter;
> > >  	loff_t n = *pos;
> > >  
> > > -	rcu_read_lock();
> > >  	iter = qp_iter_init(s->private);
> > > +
> > > +	/* stop calls rcu_read_unlock */
> > > +	rcu_read_lock();
> > 
> > IMHO, it should be placed after your if(!iter) check below.
> 
> I know this seems weird but this makes the rcu locking "balanced".  Returning
> NULL here still calls "stop" which is going to call rcu_read_unlock.  If we
> move rcu_read_lock we need to maintain state to determine if we called lock or
> not.  This is easier.  To me it does seem odd that the sequence operation does
> not stop on its own when NULL is returned but that is the way it works.

Thanks for the explanation.

> 
> > 
> > > +
> > >  	if (!iter)
> > >  		return NULL;
> > >  
> > > -	while (n--) {
> > > +	if (qp_iter_next(iter)) {
> > > +		kfree(iter);
> > > +		return NULL;
> > > +	}
> > > +	while (n--)
> > >  		if (qp_iter_next(iter)) {
> > >  			kfree(iter);
> > >  			return NULL;
> > >  		}
> > 
> > It looks like you forgot to remove the lines above.
> 
> Nope this replaces a call to qp_iter_next which was in qp_iter_init.
> 
> As the commit messages says we move the implict next's back into the respective
> start functions.

Did you consider as an option to use do{...}while(...) construction
instead of this duplicated code?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]