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

Simplify length computation for INT report encap headers #253

Merged
merged 7 commits into from
May 5, 2021

Conversation

ccascone
Copy link
Member

@ccascone ccascone commented May 3, 2021

In the current implementation, we compute the INT encap headers length (report_ipv4.total_len and report_udp.len) by using the frame length (intrinsic metadata), and by subtracting from it every possible header that we strip in the INT mirror parser, such as MPLS, VLAN, GTP-U, etc. As the list of stripped headers grows (adding more in #224 and #251) we try to simplify the logic.

In this PR we propose an alternative approach: instead of subtracting from the frame length, we add to the inner ipv4 length. Different from before, the addends are constant and do not depend on the parsed headers (i.e., no more stripped_* metadata). This is based o the assumption that we always strip all headers between Ethernet and IPv4 (inner one in case of GTP-U).

Moreover, we revisit the initialization of report headers to slightly reduce PHV utilization at the cost of T-PHV:

  • Set all constant fields in the INT mirror parser to allow T-PHV allocation;
  • Set variable fields in the pipe (to avoid redundant PHV initialization in the parser;
  • Avoid using hdr = {...} which sets validity bit to 1 and creates some unexpected behaviors in the pipe (likely a compiler bug).

Copy link
Member Author

@ccascone ccascone left a comment

Choose a reason for hiding this comment

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

test.FabricIntLocalReportTest fails with is_device_spine=True and send_report_to_spine=True. There are some issues with popping the MPLS label for the report packet after recirculation. fixed.

@ccascone ccascone requested a review from Yi-Tseng May 3, 2021 22:12
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #253 (a28a1ca) into main (5dcbfa6) will not change coverage.
The diff coverage is n/a.

❗ Current head a28a1ca differs from pull request most recent head 7d56850. Consider uploading reports for the commit 7d56850 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main     #253   +/-   ##
=========================================
  Coverage     74.11%   74.11%           
  Complexity      273      273           
=========================================
  Files            18       18           
  Lines          1866     1866           
  Branches        153      153           
=========================================
  Hits           1383     1383           
  Misses          399      399           
  Partials         84       84           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dcbfa6...7d56850. Read the comment docs.

@ccascone ccascone requested a review from osinstom May 4, 2021 13:19
@ccascone ccascone changed the title [WIP] Simplify length computation for INT report encap headers Simplify length computation for INT report encap headers May 4, 2021
Copy link
Collaborator

@Yi-Tseng Yi-Tseng left a comment

Choose a reason for hiding this comment

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

LGTM overall, only one comment

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 agree with @Yi-Tseng to consider adding @padding for the pad field. The other concern I have is that we'll have much code related to the IPv6 support commented out, and it may make code less "clean". I'd suggest adding FIXME to each occurrence of IPv6-related code and even add a keyword to correlate these comments (e.g. FIXME (ipv6-support)).

@ccascone
Copy link
Member Author

ccascone commented May 5, 2021

I removed the commented out ipv6 code, there's not much value in keeping it there. As for #227, support for generating INT reports for IPv6 packets was untested and without a clear use case. If we need it, we can add the parser logic again together with PTF tests.

@ccascone ccascone merged commit cafa6ed into main May 5, 2021
@ccascone ccascone deleted the simplify-int-len2 branch May 5, 2021 11:05
ccascone added a commit that referenced this pull request May 5, 2021
* Strip VLAN by the parser

* Simplify len computation for INT reports

* Fix mpls ttl initialized to zero

* Fix MPLS validity issue

* Mask out drop report pad field

* Remove commented out ipv6 code

Co-authored-by: Yi Tseng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants