On Sat, Jun 27, 2020 at 12:41:29PM +0200, Martin Pieuchot wrote:
> On 27/06/20(Sat) 01:02, Vitaliy Makkoveev wrote:
> > On Fri, Jun 26, 2020 at 09:15:38PM +0200, Martin Pieuchot wrote:
> > > On 26/06/20(Fri) 17:53, Vitaliy Makkoveev wrote:
> > > > On Fri, Jun 26, 2020 at 02:29:03PM +0200, Martin Pieuchot wrote:
> > > > > On 26/06/20(Fri) 12:35, Vitaliy Makkoveev wrote:
> > > > > > On Fri, Jun 26, 2020 at 10:23:42AM +0200, Martin Pieuchot wrote:
> > > > > > > On 25/06/20(Thu) 19:56, Vitaliy Makkoveev wrote:
> > > > > > > > Updated diff.
> > > > > > > >
> > > > > > > > OpenBSD uses 16 bit counter for allocate interface indexes. So
> > > > > > > > we can't
> > > > > > > > store index in session and be sure if_get(9) returned `ifnet'
> > > > > > > > is our
> > > > > > > > original `ifnet'.
> > > > > > >
> > > > > > > Why not? The point of if_get(9) is to be sure. If that doesn't
> > > > > > > work
> > > > > > > for whatever reason then the if_get(9) interface has to be fixed.
> > > > > > > Which
> > > > > > > case doesn't work for you? Do you have a reproducer?
> > > > > > >
> > > > > > > How does sessions stay around if their corresponding interface is
> > > > > > > destroyed?
> > > > > >
> > > > > > We have `pipexinq' and `pipexoutq' which can store pointers to
> > > > > > session.
> > > > > > pipexintr() process these queues. pipexintr() and
> > > > > > pipex_destroy_session() are *always* different context. This mean we
> > > > > > *can't* free pipex(4) session without be sure there is no reference
> > > > > > to
> > > > > > this session in `pipexinq' or `pipexoutq'. Elsewhere this will
> > > > > > cause use
> > > > > > afret free issue. Look please at net/pipex.c:846. The way pppx(4)
> > > > > > destroy sessions is wrong. While pppac(4) destroys sessions by
> > > > > > pipex_iface_fini() it's also wrong. Because we don't check
> > > > > > `pipexinq'
> > > > > > and `pipexoutq' state. I'am said it again and again.
> > > > >
> > > > > I understand. Why is it a problem? Using reference counting the way
> > > > > you're suggesting is *one* possible solution to a problem we don't
> > > > > fully
> > > > > understand. What are we trying to achieve? Which problem are we
> > > > > trying
> > > > > to solve?
> > > >
> > > > Sorry, may be I misunderstand something.
> > > >
> > > > `pipexoutq' case:
> > > >
> > > > 1. pppac_start() calls pipex_output()
> > > > 2. pipex_output() calls pipex_ip_output()
> > > > 3. pipex_ip_output() calls pipex_ppp_enqueue()
> > > > 4. pipex_ppp_enqueue() calls schednetisr() which is task_add()
> > > >
> > > > `pipexinq' cases:
> > > >
> > > > 1.1. ether_input() calls pipex_pppoe_input()
> > > > 1.2. gre_input() calls gre_input_1()
> > > > gre_input_1() calls pipex_pptp_input()
> > > > 1.3. udp_input() calls pipex_l2tp_input()
> > > >
> > > > 2. pipex_{pppoe,pptp,l2tp}_input() calls pipex_common_input()
> > > > 3. pipex_common_input() calls schednetisr() which is task_add()
> > > >
> > > > task_add(9) just schedules the execution of the work specified by `tq'.
> > > > So we can do pipex_destroy_session() * between * schednetisr() and
> > > > pipexintr(). And we can do this right * now *, with our current locking.
> > > > And this is the problem I'am trying to solve.
> > > >
> > > > My apologies if I'am wrong above. Please point me where I'am wrong.
> > > >
> > > > Also before pipex_{pppoe,pptp,l2tp}_input() we call corresponding
> > > > pipex_{pptp,l2tp}_lookup_session() to obtain pointer to pipex(4)
> > > > session. We should be shure `session' is still walid between
> > > > pipex_*_lookup() and pipex_*_input(). It's not required now, but will be
> > > > required in future.
> > >
> > > Why not iterate over the queues and garbage collect the sessions that
> > > are about to be removed? That's what the network stack was doing with
> > > mbuf queues prior to if_get(9) when interfaces where destroyed.
> > >
> >
> > Do you mean net/if.c:1185 and below? This is the queues associated with
> > this `ifp'. But for pipex(4) we should go through all mbufs associated
> > with pipex(4). This can be heavy if we have hundreds of sessions. Also
> > this would work until session destruction and `pr_input' are serialized.
> >
> > Point me please the line in source to see if I'am wrong about `ifnet's
> > mbuf queues claninig.
>
> 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?
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? The
cases you suggested are serious performance impact in high load.
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? 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? 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 :) ?
You said you like to make kernel more MP capable. It's great but before
releasing the locks we need to make straigt logic to be sure the object
we obtained is the object we requested. And this is the problem not only
for pipex(4) but for net/if.c too [1]. The problems are fully idntical.
Also while I was student we had a little internet provider. It was in
2005-2008. And freebsd with mpd was the easiest way to start. So I had a
lot of problems and receive a lot of experience in this area.
1. https://marc.info/?t=159289590100001&r=1&w=2
