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

Add MPLS push module #590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GalSagie
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 20, 2017

Codecov Report

Merging #590 into master will increase coverage by 0.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   67.29%   67.89%   +0.59%     
==========================================
  Files         202      205       +3     
  Lines       12522    12555      +33     
==========================================
+ Hits         8427     8524      +97     
+ Misses       4095     4031      -64
Impacted Files Coverage Δ
core/modules/mpls_push.cc 100% <100%> (ø)
core/modules/mpls_push.h 100% <100%> (ø)
core/utils/checksum_test.cc 98.8% <0%> (-0.4%) ⬇️
core/utils/mpls.h 100% <0%> (ø)
core/modules/flowgen.cc 74.07% <0%> (+0.37%) ⬆️
core/utils/histogram.h 95.89% <0%> (+5.47%) ⬆️
core/modules/drr.cc 81.6% <0%> (+7.54%) ⬆️
core/modules/measure.cc 67.69% <0%> (+63.07%) ⬆️
... and 1 more

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 c7fbee5...dc618ad. Read the comment docs.

@nrdmao
Copy link
Contributor

nrdmao commented Jul 20, 2017

So with this design you will have an instance of the MPLS Push module for every unique label to be pushed. Which, in some instances, might be 1000's of labels. Does this really scale well in BESS to have 1000's modules and therefore 1000's of connections between modules?

pkt->prepend(4);
eth = pkt->head_data<Ethernet *>();
eth->src_addr = src_addr;
eth->dst_addr = dst_addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

To the end of keeping the scope of this module narrow, might you just copy the original ethernet header here and change the ethertype? A sequence of SetMetadata and EtherEncap modules downstream of this module can take care of changing the addresses if needed.

Ethernet::Address src_addr = eth->src_addr;
Ethernet::Address dst_addr = eth->dst_addr;

pkt->prepend(4);
Copy link
Contributor

Choose a reason for hiding this comment

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

All code south of here should be guarded by a check that this call to Packet::prepend() didn't fail.

Ethernet *eth = pkt->head_data<Ethernet *>();

Ethernet::Address src_addr = eth->src_addr;
Ethernet::Address dst_addr = eth->dst_addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doh, sorry. Ignore my comment below. I didn't notice these lines.

@sangjinhan
Copy link
Member

@nrdmao That's a great point. At that scale probably we would want:

  1. All packets to go thorough a single pipeline, unless their processing flows diverge.
  2. Each packet carries its MPLS label as metadata, which is set by some upstream modules.
  3. Then MPLSPush module tags packets accordingly on a per-packet basis

(3) is what some existing modules do, such as IPEncap, EtherEncap, etc. In contrast, some other modules work on a per-"flow" basis, such as VLANPush or this module. Ideally, it should be configurable to choose which scheme to use.

At the moment, unfortunately, the per-packet metadata approach is not quite applicable in most cases. Existing classification modules (BPF, IPLookup, ExactMatch, etc.) support only flow split (packets are distributed across output gates, as a result of classification), but not metadata update.

@sangjinhan
Copy link
Member

sangjinhan commented Jul 26, 2017

Sorry, could you rebase the branch? Due to changes in #591 there are build errors in this PR...

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.

4 participants