Introduce an one-shot timer which runs more often after init() call #340
Conversation
guvenc
force-pushed
the
feature/faster_arp_after_start
branch
from
0f2218e to
e0b0ac5
Compare
guvenc
force-pushed
the
feature/faster_arp_after_start
branch
2 times, most recently
from
851de08 to
c4557ea
Compare
src/dp_timers.c
Outdated
| } | ||
|
|
||
| uint64_t dp_timers_get_manage_interval_cycles(void) | ||
| { | ||
| return dp_timer_manage_interval_cycles; | ||
| } | ||
|
|
||
| static inline int dp_timers_add(struct rte_timer *timer, int period, rte_timer_cb_t callback) | ||
| static inline int dp_timers_add(struct rte_timer *timer, int period, enum rte_timer_type type, rte_timer_cb_t callback) |
There was a problem hiding this comment.
Since you are changing the signature, you can change the name to dp_add_timer() as your naming scheme is actually better than mine. I just did not want to make a bug PR out of renaming stuff.
src/grpc/dp_grpc_service.cpp
Outdated
| @@ -43,6 +44,7 @@ char* GRPCService::GetUUID() | |||
|
|
|||
| void GRPCService::SetInitStatus(bool status) | |||
| { | |||
| dp_timers_kick_one_shot_timer(); | |||
There was a problem hiding this comment.
I am having trouble guessing what the name means, kick is not obvious to me and also one shot timer for what exactly? How about dp_start_oneshot_maintenance_timer or something (or "suick amintenance timer" or similar?
And why is it called here instead in dp_timers_init(), you want to explicitely wait for grpc init? This way we have the 30s and in parallel another one that runs from grpc init 4x5s. I thought you'd simply create a 5s timer, run it 6 times and then switch over to 30s timer instead. If the possibility of running two timers at once (if for example grpc takes 20s before init), then I guess it's not a problem.
src/dp_timers.c
Outdated
| if (dp_conf_is_ipv6_overlay_enabled()) { | ||
| trigger_nd_ra(); | ||
| trigger_nd_unsol_adv(); | ||
| } | ||
| trigger_garp(); | ||
|
|
||
| if ((timer->period == SINGLE) && (counter < 4)) { |
There was a problem hiding this comment.
I think this either needs a comment or a define for the 4 (or both). Ideally a define that divides the maintenance timer intervals, like 30s/5s means you run this one 6 times before the proper one starts. But I guess the four is there because gRPC takes some time?
Also maybe separate the callbacks (and call a common inline or something) so we do not have to ask the period to be SINGLE as that seems odd at first sight.
Or set this timer to 5s PERIODIC and after N iterations (4) remove it and re-create it with 30s?
|
Thanks for the comments @PlagueCZ Makes sense. I changed the approach completely now and using the one you suggested "Or set this timer to 5s PERIODIC and after N iterations (4) remove it and re-create it with 30s?" I wouldnt change the function names in this PR but the other points should be addressed now with the new approach. |
guvenc
force-pushed
the
feature/faster_arp_after_start
branch
from
c4557ea to
63e087a
Compare
src/dp_timers.c
Outdated
| @@ -17,10 +17,13 @@ | |||
|
|
|||
| // how often to perform network maintenance tasks (ND, GARP, ...) | |||
| #define TIMER_DP_MAINTENANCE_INTERVAL 30 | |||
| #define TIMER_DP_MAINTENANCE_STARTUP_INTERVAL 5 | |||
| #define TIMER_DP_MAINTANANCE_STARTUP_PERIOD 5 | |||
There was a problem hiding this comment.
typo MAINTANANCE/MAINTENANCE
And maybe change _PERIOD to _CYCLES, since period is by definition a time span between cycles, so I would automatically ready it as period 5s
| @@ -63,6 +80,11 @@ static inline int dp_timers_add(struct rte_timer *timer, int period, rte_timer_c | |||
| return rte_timer_reset(timer, cycles, PERIODICAL, rte_lcore_id(), callback, NULL); | |||
| } | |||
|
|
|||
| void dp_timers_signal_initialization(void) | |||
| { | |||
| dp_maintenance_interval = TIMER_DP_MAINTENANCE_INTERVAL; | |||
There was a problem hiding this comment.
This is now quite elegant actually, you made it asynchronous.
| cycles = dp_maintenance_interval * rte_get_timer_hz(); | ||
| rte_timer_reset(timer, cycles, PERIODICAL, rte_lcore_id(), dp_maintenance_timer_cb, NULL); | ||
| } | ||
| } |
There was a problem hiding this comment.
Now due to the asynchronous implementation though, I think this needs the two-callback imeplementation, otherwise the whole condition will get evaluated for eternity, since interval will be always big and then counter will always be small and there are two unneeded comparisons.
So I would create a startup callbak that does what is above, but then sets the callback to the old function.
Start the maintenance timer with a small interval and after the first GRPC init call, gracefully increase the interval. Signed-off-by: Guvenc Gulce <[email protected]>
guvenc
force-pushed
the
feature/faster_arp_after_start
branch
from
63e087a to
937d428
Compare
|
Here comes the new version. @PlagueCZ |
One shot timer gets kicked after the init() grpc call and sends the ARP requests more often for the first 30 seconds. After that the normal interval is used. (30 seconds)