On Tue, Feb 20, 2018 at 11:56:29AM -0500, Doug Ledford wrote: > On Tue, 2018-02-20 at 11:14 -0500, Doug Ledford wrote: > > On Tue, 2018-02-20 at 07:45 +0200, Leon Romanovsky wrote: > > > > > > 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. > > I used the following command to do a quick scan of whether or not > wrappers for locking are common: > > grep -r __acquires -B 2 --include=*.[hc] > > After reviewing the output, I'm fine with this patch. It shows that > some authors do simple locking directly in the functions that need to > take the lock, but by in large, when either the locking complexity goes > up or even just the annotation complexity goes up, locking helpers are > in fact very common. As an example, kernel/sched/sched.h is full of > helpers. > > I'm also going to disagree with you on the "more pushback". It is going > to depend on the complexity of the locking and annotations as to whether > I might push back. No problem, different opinion is always good. I don't think that sched (kernel core) subsystem code/requirements are the same as some random driver code, but whatever. Thanks > > > -- > Doug Ledford <dledford@xxxxxxxxxx> > GPG KeyID: B826A3330E572FDD > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: PGP signature
