-
Notifications
You must be signed in to change notification settings - Fork 50
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
[CENNSO-1762] fix: reliable out of volume quota handling #388
[CENNSO-1762] fix: reliable out of volume quota handling #388
Conversation
The meat of this PR seems to be the recording of OVER_QUOTA and THRESHOLD_REACHED state in the URR. Tat seems resonable. But almost everything else looks to be to be just noise that is not doing anything useful.
I'm not convicend that this actually an improvement. The static function accessing the URRs will most likes be inlined (or can be force to be inline) in which case the compiler is high likely to generate almost the same or at least very similar code for the old and the new version.
Seems reasonable, but for the sake of reviewing this, it would be much easier to have a separate commit within the PR for the rename only. |
upf/upf.h
Outdated
struct | ||
{ | ||
u64 ul; | ||
u64 dl; | ||
u64 total; | ||
}; | ||
u64 as_array[URR_VOLUME_N_COUNTERS]; | ||
} urr_volume_counter_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you haev seen my comment, so let me repeat it as inline comment on the relevant lines.
I'm not convicend that this actually an improvement. The static function accessing the URRs will most likes be inlined (or can be force to be inline) in which case the compiler is highly likely to generate almost the same or at least very similar code for the old and the new version.
Changing the datastructur from an plain struct to an union where an array aliases the structure member makes this more error prone without adding any benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not improvement for compiler, but for reader. Mostly to get rid of urr_incr_and_check
macro which does strange access to fields and make it clear which fields are accessed in urr_increment_and_check_counter. Also to get rid of offsetof in format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it exactly the other way arround, the change makes it IMHO harder for the reader to understand the code.
Besides the style issue which could be down personal prefences, I still maintain that aliasing the field members through the union with the array is a bad idea. Constructs like that should only be used if there is absolutely not other god way to achive the same result.
If you insist on using an array, then use only the array and get rid of the struct.
The use of offsetof is valid and well known C syntax. I have no idea what would be a problem with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, okay. @demo-exe has similar position. So I will cut this commit to minimal change and prepare separate one with more types and use of arrays everywhere.
d563ea7
to
f9d2148
Compare
Instead of trying to catch moment when traffic counter is stepped over quota/threshold, track quota/threshold statuses to more reliably catch case when we out of quota/threshold after URR update.