Re: [PATCH] netfilter: use per-CPU r**ursive lock {XV}

Linus Torvalds
2009-04-27T22:37:39Z

On Mon, 27 Apr 2009, Stephen Hemminger wrote:
>
> The part that concerns me is that the reader lock used in a nested manner on
> same cpu may still be broken on -rt.

I think that's a valid concern, and I don't actually object to not using a
rwlock, but using the "explicit counting + spinlock" that we had at one
point. It _might_ even be faster, since a spinlock can be faster than a
rwlock, and the (rare) case where you recurse you can then avoid the
atomic entirely.

But EVEN IF YOU DO THAT, it's still wrong to call it a "recursive lock".
Because it still wouldn't be one.

That's kind of my point, and always has been. It was why I objected to the
original patch description by Dumazet. It wasn't a recursive lock back
then _either_. For all the reasons I tried to explain to you, and you seem
to not care about.

Btw, if you do use the "explicit count" case, then please please please
make sure it's documented and bug-free. And dammit, correct documentation
in this case very much talks about how it is _not_ a "recursive lock", but
a spinlock that then has an explicit counter that avoids taking the lock
entirely in one very specific path (that happens to be recursive).

The thing is, using a rwlock kind of implicitly documents all of that,
because you can tell somebody who doesn't even read the code that it's a
"per-cpu rwlock", and they then know what the semantics are (given that
they know the kernel semantics for locking in the first place).

But once you start doing your own locking rules, you really have to
explain why it works, and what it does. And you do have to be very
careful, because it's so easy to get locking wrong.

> I don't care. I don't care. Don't you get the point yet.

If you don't care about the naming, then use the right one.

And if you don't care about the naming, why do you then say I'm deluding
myself, when I'm _correct_, and I _do_ happen to care about correct
naming.

Locking really is important. Code that gets locking wrong breaks in
really subtle and nasty ways. And it sadly tends to "work" in testing,
since the breakage cases require just the right timing. So locking
should be robust and as "obviously correct" as possible.

And naming really is important. Misnaming things makes people make
assumptions that aren't true. If you talk about recursive locks, it
should be reasonable that people who know how recursive locks work would
then make assumptions about them. If the code then doesn't actually
match those rules, that's bad.

It's like having a comment in front of a piece of code that says something
totally different than what the code actually does. It's _bad_. That's why
naming matters so much - because naming is commentary. If you mis-name
things on purpose, it's simply a bug.

Do _you_ get the point?

You _do_ care about bugs, don't you?

Linus