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@xxxxxxxxx wrote: > > > From: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx> > > > > > [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:
signature.asc
Description: Digital signature