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.
>
> >
> > > +
> > > 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 to use do{..}while(..) construction instead of
duplicating code?
Attachment:
signature.asc
Description: Digital signature