On Tue, 2018-02-20 at 07:45 +0200, Leon Romanovsky wrote:
> On Mon, Feb 19, 2018 at 03:11:49PM -0500, Doug Ledford wrote:
> > On Fri, 2018-02-16 at 17:51 +0200, Leon Romanovsky wrote:
> > > > > > +static unsigned long bnxt_re_lock_cqs(struct bnxt_re_qp *qp)
> > > > > > + __acquires(&qp->scq->cq_lock) __acquires(&qp->rcq->cq_lock)
> > > > > > +{
> > > > > > + unsigned long flags;
> > > > > > +
> > > > > > + spin_lock_irqsave(&qp->scq->cq_lock, flags);
> > > > > > + if (qp->rcq != qp->scq)
> > > > > > + spin_lock(&qp->rcq->cq_lock);
> > > > > > + else
> > > > > > + __acquire(&qp->rcq->cq_lock);
> > > > >
> > > > > Sorry for naive question, why do you need __acquire/__release? And why
> > > > > can't spin_is_locked(&qp->rcq->cq_lock) be used for unlock code?
> > > > >
> > > >
> > > > I used __acquire and __release for satisfying the sparse. Sparse used to
> > > > give warnings "context imbalance - wrong count at exit" since the locking
> > > > and unlocking happens from two different functions. seen similar implementation
> > > > in other drivers too.
> > >
> > > I see, actually I think that sparse complain was right and you went to
> > > far by wrapping standard calls. I see that it is very popular in bnxt_re
> > > code and the problem with that, that it makes very hard to refactor such
> > > code in case of some global change/improvement.
> >
> > Leon, are you actually objecting here, or just noting that you would
> > prefer a different style?
>
> Sometimes, it makes sense to provide wrappers e.g. different get/put,
> to_..(), from_..()) calls for annotate ambiguous ++/--. But locks
> are different, they need to be clear and can't be hided.
>
> So yes, I'm objecting here for this lock/unlock wrapper and would like
> to see more pushback from maintainers on extensive usage of ridiculous
> wrappers (including mlx code).
I've spent some time thinking about this, and I'm not sure I agree with
you Leon. Getting locking right is important, and in this case it
doesn't really hide any details, but it gets all of the annotations
right. I'm personally inclined to take the patch. I'll look around at
some other places in the kernel, but wrappers for getting locking right
aren't that rare I don't think.
And your comment about hard to refactor in case of some global
change/improvement make no sense to me. The locks are on their own
internal completion queues, and this locking function is nothing more
than "if you have one cq, you lock it, if you have two, you lock both
and in the right order". I don't see that impacting a refactor.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDDAttachment:
signature.asc
Description: This is a digitally signed message part
