On 27/08/15(Thu) 11:10, Alexandr Nedvedicky wrote: > On Wed, Aug 26, 2015 at 06:12:10PM +0200, Mark Kettenis wrote: > > > Date: Wed, 26 Aug 2015 17:30:14 +0200 > > > From: Alexandr Nedvedicky <[email protected]> > > > > > > Hello, > > > > > > I'm not sure I got everything right in Calgary. So this patch should > > > roughly illustrates how I think we should start moving forward to > > > make PF MULTIPROCESSOR friendly. It's quite likely my proposal/way > > > is completely off, I'll be happy if you put me back to ground. > > > > > > The brief summary of what patch is trying to achieve is as follows: > > > > > > patch trades all splsoftnet() for KERNEL_LOCK() when it gets compiled > > > with MULTIPROCESSOR option on. > > > > > > if MULTIPROCESSOR option is off, the compiler produces PF, which uses > > > splsoftnet. > > > > > > To achieve this the patch introduces macros PF_LOCK()/PF_UNLOCK(), > > > which expand to KERNEL_LOCK()/KERNEL_UNLOCK(), when MULTIPROCESSOR is > > > on. > > > On the other hand if MULTIPROCESSOR is off the PF_*LOCK() macros become > > > splsoftnet()/splx() > > > > I don't think this will work. Even on MULTIPROCESSOR kernels you'll > > need to raise the spl to prevent soft interrupts from running on the > > same CPU. KERNEL_LOCK() will not prevent this from happening as it is > > a recursive lock. This is why OpenBSD's mutexes (spinning locks) > > raise the spl. > > > > So I think you'll have to define PF_LOCK()/PF_UNLOCK() to do the spl > > stuff even for MULTIPROCESSOR kernels. > > Thanks for putting my feet to ground. I'm trying to make some sense from that. > So if I understand splsotfnet() right in MULTIPROCESSOR kernel, it prevents > CPU from handling another network interrupt until the first one is > handled. So that splsoftnet should be raised by network driver, before > packet reaches PF (pf_test() function).
pf_test() is not always called in interrupt context. When it is called from a process context you want to make sure the CPU wont try to execute a softnet interrupt that might corrupt the states it is currently messing with. > the pf_test() must protect consistency of firewall data, which are shared > between CPUs (?can we say kernel threads?). So for the first phase, we opt for > KERNEL_LOCK() being grabbed by pf_test() as soon as it gets called. It makes > PF > to still process single packet at time. All other CPUs will have to wait on > KERNEL_LOCK(), holding their splsoftnets(). Do I still follow the concept, or > am I wrong again? You're correct. We cannot say kernel threads right now because incoming packets are *still* processed in (soft-)interrupt context. > Assuming the locking in MULTIPROCESSOR goes like: > interrupt grabs splsoftnet -> ip_input -> PF grabs KERNEL_LOCK() > We need to take care of ioctl() call path and purge thread. Those need to > get synchronize with packets using KERNEL_LOCK(). They should not to mess with > splsoftnet() any more in MULTIPROCESSOR version. As long as we are using (soft-)interrupts to process incoming packets they should. If you don't raise the SPL level to softnet while CPU0 is executing some code in ioctl() path and it takes an interrupt at this point it might end up executing ip_input() *before* returning to the ioctl() (process) context. This scenario might corrupt the states you're trying to protect. > Does that idea sound right? If not what piece I'm still missing in puzzle? I think you're assuming that the "softnet" code path is running in a thread which is not (yet) the case. Martin
