[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: pipex(4): use reference counters for `ifnet'
From:       Martin Pieuchot <mpi () openbsd ! org>
Date:       2020-06-28 9:48:05
Message-ID: 20200628094805.GA5990 () oliva ! grenadille ! net
[Download RAW message or body]

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.

[prev in list] [next in list] [prev in thread] [next in thread]