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

Fix some issues for transparent cube when packet forwarded to slow path #218

Merged
merged 4 commits into from
Oct 12, 2019

Conversation

goldenrye
Copy link
Contributor

The modification fixed the following issues:

  • core dump when transparent cube sends packet to the slow path
  • stack topology (0xffff) not recognized by controller bpf program
  • calling send_packet_out() in transprent cube slow path may cause dead loop

Issue #217

  - core dump when transparent cube sends packet to the slow path
  - stack topology (0xffff) not recognized by controller bpf program
  - calling send_packet_out() in transprent cube slow path may cause dead loop

Signed-off-by: Jianwen Pi <jianwpi@gmail.com>
@mauriciovasquezbernal
Copy link
Contributor

According to my understanding this PR is mainly focused in the case a transparent cube is attached to a NIC, and the support when the transparent cube is attached to another cube's port is not changed with this PR, right?

Overall looks good to me, maybe clarifying the previous point will help to understand it better.

@goldenrye
Copy link
Contributor Author

According to my understanding this PR is mainly focused in the case a transparent cube is attached to a NIC, and the support when the transparent cube is attached to another cube's port is not changed with this PR, right?

Overall looks good to me, maybe clarifying the previous point will help to understand it better.

@mauriciovasquezbernal Yes this PR is only focusing on the transparent cube, it shouldn't change any behavior for regular cube.

@mauriciovasquezbernal
Copy link
Contributor

It could be a good idea to extend the description of the PR making it clear that it adds support for the case a transparent cube is attached to a NIC.

For the rest, it LGMT.

Copy link
Contributor Author

@goldenrye goldenrye left a comment

Choose a reason for hiding this comment

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

It could be a good idea to extend the description of the PR making it clear that it adds support for the case a transparent cube is attached to a NIC.

For the rest, it LGMT.

Sounds good to me.

@goldenrye goldenrye changed the title Adding support for transparent cube to send packet to slow path Fix some issues for transparent cube when packet forwarded to slow path Oct 7, 2019
Copy link
Contributor

@frisso frisso left a comment

Choose a reason for hiding this comment

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

Thanks, Jianwen!

@mauriciovasquezbernal
Copy link
Contributor

Is XDP support missing from this PR?, will be done in the future or it's not needed?

@goldenrye
Copy link
Contributor Author

Is XDP support missing from this PR?, will be done in the future or it's not needed?

You are right, this PR doesn't cover XDP case. Will evaluate whether XDP is impacted or not in the future.

forwarded to slow path from ingress context (BPF program executed
in ingress TC hook), the original packet's dest mac needs to be
adjusted to the controller interface's mac - otherwise packet
will be dropped by linux stack, since linux doesn't recoginize
the packet destined to a mac other than the ingress interface
even the mac belongs to one of its interface. Unlike the packet
forwarded from egress context, the packet has to be re-injected
and walk throught the full kernel stack before it was sent out
eventually.

On the other hand, for packet forwarded to slow path from egress
context, we don't have to adjust the mac since the packet has
been processed by linux stack before the slow path, so just send
it out as is.

Signed-off-by: Jianwen Pi<jianwpi@gmail.com>
Copy link
Contributor

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

LGTM.

@frisso frisso merged commit c5e8bce into master Oct 12, 2019
@frisso frisso deleted the issue_217 branch October 12, 2019 06:57
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