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

Should egress_thread call reset_exit method, too? #1275

Open
jafingerhut opened this issue Nov 3, 2024 · 1 comment
Open

Should egress_thread call reset_exit method, too? #1275

jafingerhut opened this issue Nov 3, 2024 · 1 comment

Comments

@jafingerhut
Copy link
Contributor

In method ingress_thread, after invoking the ingress pipeline, the method reset_exit() is called:

This is needed, otherwise if exit was executed for the packet during ingress processing, and it proceeds to do egress processing (or be resubmitted, or cloned), then it will abort the next egress or ingress pipeline processing too early.

However, in the method egress_thread, there is no call to reset_exit() after invoking the egress pipeline:

For normal packets that are dropped or go to an output port, that is not a problem.

However, if the packet is recirculated, or egress-to-egress cloned, it seems like it would be a problem.

I will try to create a simple test program to exhibit the problem -- shouldn't be too difficult to create one.

If it is a bug, the fix seems obvious: add a call packet->reset_exit(); just after the invocation of the egress_mau pipeline in egress_thread.

@antoninbas
Copy link
Member

For cloning and recirculation, we always create a new Packet instance using one of the "clone" functions. For example, for recirculate, we use clone_no_phv_ptr. I didn't take a deep look, but the clone functions do not copy marks such as the exit mark. So even if the packet was marked for exit in the egress pipeline, the recirculated packet (or rather the new Packet object instantiated to represent the recirculate packet) will not have that mark, and will go through packet processing normally.

Still probably a good idea to test this. It's possible to add a test to https://github.com/p4lang/behavioral-model/tree/main/targets/simple_switch/tests, but that may not be the easiest option.

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

No branches or pull requests

2 participants