Messages in this thread | | | Date | Wed, 22 Apr 2009 10:18:50 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [PATCH] netfilter: use per-cpu recursive lock (v11) |
| |
On Wed, 22 Apr 2009, Eric Dumazet wrote: > > I actually sent *one* buggy patch, and you already gave your feedback > and NACK.
Actually, the thing is, I don't even think your original patch was even buggy. The bug crept in later. I NAK'ed it not because it was buggy, but because of the ad-hoc'ness and the naming. Really.
And I actually even said so in my original rant:
'The fact that code "happens to work by mistake" (and I'm not saying that your does - but it might just because of the per-cpu'ness of it [..]'
because your original patch still had the
rcu_read_lock_bh();
in place before the whole
rl = &__get_cpu_var(arp_tables_lock); if (likely(rl->count++ == 0)) spin_lock(&rl->lock);
and that should have protected against both BH callers and preemption.
So I actually believe that your original patch probably worked fine (but as I said in my reaction to it, I thought it was almost by mistake and I wasn't going to review it)
So the actual _bug_ crept in later, when the RCU lock was removed, and the lock was cleaned up and separated into a function of its own.
And in fact, that is kind of my point: "uncommented locking with ad-hoc semantics is very fragile". Even _correct_ code ends up not being correct in the long run, because people don't realize all the subtle issues.
> I even relayed this to Stephen suggesting him not calling this a recursive lock. > (Note how I use 'suggesting' here) > > So, what do you want from me ? Should I copy 100 times :
So I consider this thread ended from a technical standpoint.
[ That said, I will not be at all shocked to hear if people decide later that the RCU method was better after all, and that even the per-cpu rwlock or spinlock is just too expensive. ]
My problem today (apart from the relatively minor issue of also wanting to get the commit log fixed up) is just that I see emails from you finding my reaction shocking and from Jarek Poplawski that seem to still think that I'm a troll.
Just because I pointed out real technical problems? Is that shocking or trolling?
Really - please go back to my _original_ email. No, it was not polite. But here's another quote from it:
"Because even if it works today, it's just a bug waiting to happen."
and dammit, I sent that out _before_ the very next version of the patch that actually _did_ introduce that exact bug.
So dammit - what part of my email was "shocking" or "trolling"? The part where I was right? Or what?
Linus
| |