p4lang / behavioral-model

The reference P4 software switch
Apache License 2.0
546 stars 329 forks source link

Should egress_thread call `reset_exit` method, too? #1275

Open jafingerhut opened 3 weeks ago

jafingerhut commented 3 weeks ago

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 commented 2 weeks ago

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.