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

[SDFAB-1054] Use p4 auto generated constants #501

Merged
merged 96 commits into from Mar 11, 2022

Conversation

EmanueleGallone
Copy link
Contributor

@EmanueleGallone EmanueleGallone commented Feb 17, 2022

@EmanueleGallone EmanueleGallone marked this pull request as draft February 17, 2022 16:04
@EmanueleGallone EmanueleGallone marked this pull request as ready for review February 18, 2022 21:49
pfcpiface/p4rt_translator.go Outdated Show resolved Hide resolved
pfcpiface/up4.go Outdated Show resolved Hide resolved
pudelkoM
pudelkoM previously approved these changes Mar 4, 2022
@EmanueleGallone EmanueleGallone changed the title Use p4 auto generated constants [SDFAB-1054] Use p4 auto generated constants Mar 7, 2022
Tomasz Osiński and others added 3 commits March 7, 2022 19:38
Co-authored-by: Maximilian Pudelko <pudelkoM@users.noreply.github.com>
pfcpiface/up4.go Outdated Show resolved Hide resolved
pfcpiface/up4.go Outdated Show resolved Hide resolved
pfcpiface/up4.go Outdated Show resolved Hide resolved
osinstom
osinstom previously approved these changes Mar 11, 2022
Copy link
Contributor

@osinstom osinstom left a comment

Choose a reason for hiding this comment

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

LGTM, but I left minor comments that we should address before merging.

pfcpiface/up4.go Outdated

switch counterID {
case p4constants.CounterPreQosPipePreQosCounter:
//FIXME to fully exploit p4constants, counters should be a map instead of a slice
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean by fully exploit p4constants ? We get counterName from p4constants and counter size is retrieved from UP4's P4Info. What can we do more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch counterID {
		case p4constants.CounterPreQosPipePreQosCounter:
			//FIXME to fully exploit p4constants, counters should be a map instead of a slice
			up4.initCounter(preQosCounterID, counterName, uint64(counterSize))
		case p4constants.CounterPostQosPipePostQosCounter:
			up4.initCounter(postQosCounterID, counterName, uint64(counterSize))
		}

all of the above code could be replaced by up4.initCounter(counterID, counterName, counterSize) .
This means that we could also get rid of

preQosCounterID = iota
postQosCounterID

That FIXME is pointing to this improvement, basically. It is not implemented because counters is a slice and it's allocating counters sequentially, in setUpfInfo().

The above simplification can be achieved, however, with a refactor on how we handle counters IMO.
Anyway, I removed that FIXME so that we can merge this and address the improvements on a later PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I see. Getting rid of preQosCounterID and postQosCounterID requires refactoring of the entire counter allocation mechanism. We can address it later.

pfcpiface/up4.go Outdated Show resolved Hide resolved
@osinstom osinstom merged commit eefa932 into omec-project:master Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants