Skip to content
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

switch to UPF specific timer wheel implementation #341

Merged
merged 4 commits into from
Jun 21, 2023
Merged

Conversation

RoadRunnr
Copy link
Member

@RoadRunnr RoadRunnr commented Jun 19, 2023

Decouple memory management for the timer handlers from the start/stop
logic.

Handles must now be allocated before they can be started and they are no
longer automatically freed when stopped. Reusing handles for restarting
or changing the timer is now possible.

This solves the problem that handles could accidentally reused.

time threshold behaves just like time quota, when activate its base
must be reset to the last report time.
Import the unmodified version from VPP as basis for future
modifications.
@RoadRunnr RoadRunnr force-pushed the feature/upf-timers branch 3 times, most recently from a32ac79 to 881f5ee Compare June 20, 2023 10:22
Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quite small change might be needed, also there's typo in one of the commit descriptions ("stoped"):

Handles must now be allocated before they can be started and they are not
longer automatically freed when stoped. Reusing handles for restarting
or changing timer is now possible.

should be

Handles must now be allocated before they can be started and are no
longer automatically freed when stopped. Reusing handles for restarting
or changing the timer is now possible.

@@ -1368,7 +1308,8 @@ static uword

/* run the timing wheel first, to that the internal base for new and updated timers
* is set to now */
upf_pfcp_server_expire_timers ();
psm->expired =
TW (tw_timer_expire_timers_vec) (&psm->timer, psm->now, psm->expired);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

psm->expired isn't used outside pfcp_process() anymore, so maybe the expired field could be removed from pfcp_server_main_t and a local var used instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@RoadRunnr
Copy link
Member Author

also fixed the commit message

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost approved, but there's a checkstyle error

Files upf/upf_pfcp_server.c and upf/upf_pfcp_server.c.out2 differ

Checkstyle failed for upf/upf_pfcp_server.c.
Run indent (twice!) as shown to fix the problem:
indent upf/upf_pfcp_server.c
indent upf/upf_pfcp_server.c
*******************************************************************
* VPP CHECKSTYLE FAILED
* CONSULT FAILURE LOG ABOVE
* NOTE: Running 'hack/checkstyle.sh --fix' *MAY* fix the issue
*******************************************************************
make: *** [Makefile:29: checkstyle] Error 1

Decouple memory management for the timer handlers from the start/stop
logic.

Handles must now be allocated before they can be started and they are no
longer automatically freed when stopped. Reusing handles for restarting
or changing the timer is now possible.

This solves the problem that handles could accidentally reused.
Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ivan4th ivan4th merged commit f182c69 into master Jun 21, 2023
@ivan4th ivan4th deleted the feature/upf-timers branch June 21, 2023 07:28
@korroot korroot added the fix label Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants