Session timer enhancement #257
Conversation
Due to a possible bottleneck scenario, the session timer needed to be sepparated from the other timers in order to have control on the execution time of such timer. The scenario is this: while the sessions are being removed, the package translation could be delayed by the execution of the sessions removal task. This is a massive trafic scenario that could happen when there's a huge amount of expired sessions to remove.
mod/stateful/session_timer.c
Outdated
| u64 max_session_rm; | ||
| }; | ||
|
|
||
| static struct clean_params sess_params; |
There was a problem hiding this comment.
Something that you might have understandably missed while reading the documentation is that a kernel can have multiple Jool instances, each of them will have a distinct BIB/session database, and each of these databases will have a separate spinlock.
If sess_params is a global variable, then all the instances will share the same "session parameters", and that means that, if one translator faces stress, it will affect the session removal limit of the other translators as well. This is not what we want, because a laggy spinlock only affects its own translator.
I believe that the session parameters should belong to the bib_table structure.
There was a problem hiding this comment.
Now the parameters and the timers belong to the bib_table structure.
mod/stateful/session_timer.c
Outdated
|
|
||
| static void timer_function(unsigned long arg) | ||
| { | ||
| xlator_foreach(clean_state, &sess_params); |
There was a problem hiding this comment.
Notice: This is the line that causes the timer (which is global) to be run on each translator instance.
There was a problem hiding this comment.
This has been removed.
mod/stateful/session_timer.c
Outdated
| timer.function = timer_function; | ||
| timer.expires = 0; | ||
| timer.data = 0; | ||
| sess_params.pend_rm = 0; |
There was a problem hiding this comment.
Not really a bug by any means, but notice that the kernel API declares boolean identifiers which Jool can access from anywhere, so you can make pend_rm more "boolean looking" by replacing 0 and 1 by more meaningful labels.
There was a problem hiding this comment.
Thanks, now the boolean labels are used.
Now the timer is related to each table, therefore it doesn't affect other instances. The code is still a work in progress, some parameters at some functions can be removed.
There are still some tests with warnings due to the "xlator" calls when the session timers are removed; I've taken this as a general warning: probably the BIB shouldn't be getting the "xlator". I'm analizing what to do, since this is probably not the best solution (to get the "xlator" from the BIB scope).
Under a load test scenario, the timer used for session expiration can affect the packet translation time. When the expired sessions are being removed, assuming that there are serveral expired sessions, the spin lock used can delay the packet translation the same time that the lock spends removing sessions.
The proposal (thanks @ydahhrk) is to "independize" the session removal timer, so that its behavior can be customized. The timer will do this:
Removing expired sessions isn't as important as translating packets, but it's still something that must be done (we don't want to keep expired sessions).