On 27/06/20(Sat) 17:58, Vitaliy Makkoveev wrote: > > [...] > > Look at r1.329 of net/if.c. Prior to this change if_detach_queues() was > > used to free all mbufs when an interface was removed. Now lazy freeing > > is used everytime if_get(9) rerturns NULL. > > > > This is possible because we store an index and not a pointer directly in > > the mbuf. > > > > The advantage of storing a session pointer in `ph_cookie' is that no > > lookup is required in pipexintr(), right? Maybe we could save a ID > > instead and do a lookup. How big can be the `pipex_session_list'? > > > > It's unlimited. In pppac(4) case you create the only one interface and > you can share it between the count of sessions you wish. In my practice > I had machines with 800+ active ppp interfaces in 2005. We can have > dosens of cores and hundreds gigs of ram now. How big can be real > count of active ppp interfaces on VPN provider's NAS?
With that number of items a linear list might not be the best fit if we decide to stop using pointers. So if we want to use a "lookup" a different data structure might be more appropriate. > I looked at r1.328 of net/if.c. > Imagine we have a lot of connected clients with high traffic. One on them > starts connect/disconnect games. It's not malicious hacker, just mobile > phone in area with low signal. You need: > > 1. block pipexintr() (and netisr too). > 2. block insertion to queues from concurrent threads. > 3. Walk through very loaded queues and compare. And most or may be all > packets are foreign. > 4. Repeat it every time you lost connection. > > Yes, now it's all serealized. And pipexintr() is already blocked while > we do session destruction(). But what is the reason to make your future > life harder? pipex(4) session already has pointer to it's relared > `ifnet'. `ifnet' already has reference counters. Why don't use them? We decided as a team to not use reference counting because they are hard to debug. Using if_get(9)/if_put(9) allows us to check that our changes where correct with static analysis tools. If we start using reference counting for ifps in pipex(4) they we now have an exception in the network stack. That means the techniques applied are not coherent and it makes it harder to work across a huge amount of code. But I don't care, if there's a consensus that it is the way to go, then go for it. > In the way I suggest to use refcounters for pipex(4) session you don't > need to block your packet processing. You don't need to do searchs. You > just need to be sure the `ifnet' you has is still alive. You already has > mechanics for that. Why don't use this? The same can be said with the actual machinery. Using a if_get(9)-like approach doesn't block packet processing, you don't need to to search, there's no reference counting bug that can lead to deadlock, it is consistent with the existing network stack and there are already example of implementation like: if_get() and rtable_get(). > You don't like if_ref() to be > used outside net/if.c? Ok, what's wrong with referenced pointers > obtained by if_get(9)? While session is alive it *uses* it's `ifnet'. It > uses `ifnet' all lifetime *not* only while output. And we should be > shure we didn't destroy `ifnet' while someone uses it. What's wrong with > referenced pointers? If there's a bug, if the reference counting is messed up, it is hard to find where that happened. It is like a use-after-free, where the crash happens is not where the bug is. If you see a leak you don't know where the reference drop is missing. Sure they are many ways to deal with that, but since we did not embrace them. I'm not sure that it makes sense to change direction or introduce a difference now. > The way I wish to go used in file(9). May be it is > totally wrong and we need global in-kernel descriptor table where `fp' > will be referenced by index too :) ? That's not what I'm saying. if_get(9) already exists and is already used in a certain way, I don't see why not embrace that and keep the network stack coherent. But once again, I don't want to block anyone in its development, if you and others agree that's the way to go, then let's go this way.