On Wed, May 11, 2022 at 12:38:56AM +0200, Alexandr Nedvedicky wrote: > Hello, > > > > > Yes. It is similar. > > > > I have read the whole mail thread and the final fix got commited. > > But it looks incomplete, pf is still sleeping. > > > > Hrvoje, can you run the tests again that triggered the panics a > > year ago? > > > > Sasha, I still think the way to go is mutex for pf locks. I don't > > see a performance impact. > > you mean performance impact against 7.1? If it is the case, > then I agree. > > > a single CPU anyway. And with sleeping locks you have to schedule > > in the hot packet path. Our schedueler was never build for that. > > I don't think it is a problem of scheduler. I rather see it as problem of > lock primitives, which can't keep consistency across a switch from CPU > (a.k.a. sleep). > > > > At genua we started with mutex, made it fine grained, and converted > > to rcu later. > > > > I think the only reason why PF_LOCK() is a writer lock is overload tables. > packet, which is about to update an overload table requires an exclusive > PF_LOCK(). As soon as there will be a way to update overload table without > an exclusive lock, then we can turn packets to PF_LOCK() readers. > > Also I somewhat disagree with idea 'network stack must never sleep'. The > rw-lock adds places to network subsystem, where scheduler has a chance to > give CPU to someone else. > > on the other hand let's stay real. if we want to keep at least part of the > network stack running in parallel these days, then pf locks need to be turn to > mutexes unless we want to hunt down all those SMR_READ sections and > fix/workaround them to deal with sleep in underlying call-stack. > > I think either way (putting mutexes in vs. hunting down sleeping SMR section) > is fine for me. My current plan is to move all those memory allocations > in ioctl(2) path outside of PF_LOCK() scope.
I do not fully agree with this. SMR sections should be short and not span deep callstacks. This is not a good design and finding and fixing these errors is important. -- :wq Claudio
