-
-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamic Interface list update #10
Comments
@dfskoll I've been thinking about this one a bit. Would appreciate input on the below. Sorry for the essay, just want to make sure all bases are covered. It seems pointers to Interfaces are kept in sessions. If interfaces could go away during this change, it may be a good option to ref-count interfaces (or somehow determine , but it also means we cannot re-allocate the array - are you in any way objected to me turning the Interface* list into a single-linked list? It means "removals" from the list are going to be a potentially slow operation, but I'm OK with that (I don't see this list being longer than a few hundred max. We're already contemplating rather grouping interfaces for different interfaces groups (eg, by FNO). That way we can somewhat trivially shut related interface groups. For the time being with session numbers being indexes into an array I'm actually quite happy with that for the sake of performance, but in theory if we can allocate session numbers on a per interface basis (?) then we can exceed the 64k session limit, and we can then start looping session numbers on a per interface basis, there may be another benefit to turning sessions into linked lists too, in that we have a per interface list (also allows cleaning up interfaces once all sessions have terminated, or trivially killing all sessions for a relevant interface - it does slow down finding a specific session, but since we only ever expect PADTs for this - and can go directly to the correct Interface, or a child process dying (?) I don't see that as a major concern). Currently my understanding is that the session id is per pppoe-server instance, I'm quite OK with keeping it that way for the time being (for the sake of smaller incremental changes). I believe it's possible to make this per interface (or even per remote client). This is way outside the scope of my current planning, but can definitely be done if needed. As I understand, a session is uniquely identified by two mac addresses and the session identifier? In other words, in theory we can have an unlimited number of sessions per interface, assuming that we calculate the remote side's MAC as part of the unique identifier for a ClientSession? In terms of "administrative" interface, I am suggesting to do the following:
Internally things gets a bit more complicated, the current structure whilst extremely simple, does have one major draw-back: Due to a pointer being kept in the ClientSession I note there are a number of L2TP bits and pieces, which I'm not familiar with (but might need some time soon, testing to be actioned in the next month of two), and this may all influence things somehow. So please confirm the following strategy:
If you are happy with this strategy I'm quite happy to start with the implementation. We are in fair need of this, but we also don't want to start down a road that's not sustainable, and you definitely understand this stuff better than me. |
Moving the interface list to a dynamic structure makes sense. Whether it should be a linked list or a hash table is something that has to be decided based on the use-case. It could be that an O(N) lookup for an interface is too slow. As for implementation, I would have the PPPoE server take a command-line option that makes it open a UNIX-domain (or TCP) socket that listens for control connections. We could then define a command protocol for changing the config. For example, to add an interface, maybe we could send the command This is what I implemented for ServPOET. Clearly, we can't reuse that source, but I think the idea is sound. Unfortunately, I have a full-time day job and also an after hours comedy career of sorts, so I have pretty much no time to work on this. I'd be happy to discuss implementation ideas with you, however, and review/comment on PRs. |
Ah, and by "This is what I implemented for ServPOET", I meant the control socket, not a method to dynamically add or remove interfaces. |
Thank you. I think I've got enough to get started. Unless you have objections to any of the below I'm going to push forward with this during the week. I like that idea of a control socket, but it's considerably more work. Signal is I believe less work. Also, it enforces that you don't make a live change and then forgot to also update the config file (which is something that's been known to happen). I would implement two signals for now though (and sure we can add a control socket at some point), USR1 - reload reloadable (eg, Interface* if there is a file, and that file has been updated since the last reload), and USR2, a form of state dump which basically just outputs the current server state to a file, say /tmp/pppoe-server-${pid}-${seconds_since_epoch}.txt (this can assist with debugging probably). Since I don't believe the Interface* array is iterated frequently I believe a linked list should be adequate. I believe the event handlers already have an Interface* at hand based on the file descriptor, so other than for a very short period when the reload happens I don't believe there should be performance impact, and compared to restarting the daemon ... whatever I end up doing is definitely less disruptive, as long as it doesn't take several seconds, but I believe even stalling the daemon up to two seconds or so should be just fine, and I believe this should be well sub 1s regardless. A hash table for per-Interface ClientSession's might make more sense than for Interface*, but again, how frequently do we really need to search those (we use kernel mode and our sessions are generally long - except when Eskom does load shedding we're typically looking at several days/session)? And this can be changed if the performance isn't sufficient using a linked list, but I do like the idea of a hashtable here just to pre-empt this scenario. Do we keep session numbers on a per interface basis, or do I make it per remote MAC? Ie, what's the key into the hash table? (packet->session) or (packet->ethHdr.h_source, packet->session)? Do you have a preference in terms of hash function to use? Since session id's are 16-bit integers, if we only use then a simple % is probably fine, else if we include the remote MAC then that's 8 bytes, we can probably thread that as a sequence of 8 bytes with a loop, something like (R=small prime, eg 11/17/31, M=table size - also prime, say 97 or smaller, by default, but we can scale this up as the required data increases automatically say up to a maximum size of 997, every time we reach say an "average bucket size" of 16 or so entries?):
|
On Mon, 23 May 2022 12:42:07 -0700 Jaco Kroon ***@***.***> wrote:
Thank you. I think I've got enough to get started. Unless you have
objections to any of the below I'm going to push forward with this
during the week.
OK.
I like that idea of a control socket, but it's considerably more
work. Signal is I believe less work.
It is, but it's also tricky. If you want to use signals, then I recommend
doing as little as possible in the signal handler itself, but just setting
a flag for the main loop to indicate that there was a signal. I usually
use the technique of creating a pipe and having the signal handler write a byte
to the write-end of the pipe, which then is part of the poll array so it wakes
up the poll statement in the main loop. There's an example of this
in another of my projects:
https://github.com/dfskoll/mailmunge/blob/f34be5b0869757fab82dab6e52d4f722169009f6/c/mailmunge-multiplexor.c#L1421
Also, it enforces that you don't make a live change and then forgot
to also update the config file (which is something that's been known
to happen).
That's true, but in order to avoid dropping existing sessions, we'd
have to read the control file and then compare it to the current
state, dropping interfaces that were removed and adding new ones.
That seems tricky. If we just exec a new copy of pppoe-server, we'll
lose all existing session information.
Since I don't believe the Interface* array is iterated frequently I
believe a linked list should be adequate.
OK.
[...]
A hash table for per-Interface ClientSession's might make more sense
than for Interface*, but again, how frequently do we really need to
search those (we use kernel mode and our sessions are generally long
- except when Eskom does load shedding we're typically looking at
several days/session)? And this can be changed if the performance
isn't sufficient using a linked list, but I do like the idea of a
hashtable here just to pre-empt this scenario.
We search the session list when there's a PADT, I guess. That doesn't
happen that often and modern hardware is fast, so you may be
right... a linked list of sessions might be OK.
Do we keep session numbers on a per interface basis, or do I make it
per remote MAC? Ie, what's the key into the hash table?
(packet->session) or (packet->ethHdr.h_source, packet->session)?
If we have one hash table per interface, then the hash key only needs to be
the session number. If we have one big hash table, then it'll have
to be some interface identifier combined with the session number.
Do you have a preference in terms of hash function to use? Since
session id's are 16-bit integers, if we only use then a simple % is
probably fine,
If we use one hash table per interface, then yes... % is fine.
[...]
something like (R=small prime, eg 11/17/31, M=table size - also
prime, say 97 or smaller, by default, but we can scale this up as the
required data increases automatically say up to a maximum size of
997, every time we reach say an "average bucket size" of 16 or so
entries?):
Yep, resizable hash tables are not too hard to do. There's a
non-resizable implementation included with rp-pppoe in
libevent/hash.c. It's used by the L2TP code (which is not included in
the GPL'd version of rp-pppoe) but feel free to leverage it. If you
need a hand understanding the API, I can help out. It could probably be
made resizable without too much work.
Regards,
Dianne.
|
Another issue is what happens if the underlying Interface gets shut down (ip li del of a VLAN for example) whilst rp-pppoe has the socket open. Not even sure how to detect that. Busy working on another (semi related) project where this came up. |
We should test if it's even possible to delete an interface while a socket is open. I suspect one of the three scenarios will happen:
Only scenario (3) is actually a problem for rp-pppoe, so we should test to see if it's what happens. |
Currently the list of PPPoE interfaces is specified on the CLI with -I.
I suggest implementing an option whereby a file with a list of interfaces can be given, one per line (ideally with an option for comments, or a space separated comment (We currently have around 70 interfaces).
Currently one could run an instance per interface, but this requires an IP Pool per instance (without #8 - which would still allow a combined pool).
The downside of instance per interface (or group of interfaces) even with #8 is higher memory use, although, I suspect this is overall fairly negligible.
The text was updated successfully, but these errors were encountered: