Fix simple_switch.md: mark_to_drop overrides multicast.

[smolkaj]

I empirically observed that mark_to_drop overrides multicast on BMv2.

@jafingerhut @antoninbas @verios-google

[jafingerhut]

@smolkaj Given the C++ code in ingress_thread, I am surprised by your statement. Do you have a sample v1model P4 program, set of table entries, and an input packet that demonstrates this?

[jafingerhut]

I just tried out a test case where for one test, for a packet in ingress I do these operations in this order:

        // order of operations #1
        stdmeta.mcast_grp = mcast_grp;
        mark_to_drop(stdmeta);

In this case, the packet is dropped at the end of ingress.

Then I tried this order of operations:

        // order of operations #2
        mark_to_drop(stdmeta);
        stdmeta.mcast_grp = mcast_grp;

In this case, the packet is not dropped at the end of ingress, but is multicast replicated according to the specified mcast_grp.

Fun, eh?

Why?

Because mark_to_drop(stdmeta) is effectively implemented as:

    stdmeta.egress_spec = DROP_PORT;
    stdmeta.mcast_grp = 0;     /// NOTE THIS LINE ESPECIALLY !!!

See https://github.com/p4lang/behavioral-model/blob/main/targets/simple_switch/primitives.cpp#L176-L203

So I think the pseudocode for what happens at the end of ingress is correct, but you need to be aware of the full behavior of mark_to_drop(stdmeta) in order to explain what is going on.

Should it be this way? I did not write the original BMv2 code, but the older drop() method also did this, and that goes back to 2016-Jan-18 when the source file primitives.cpp linked above was first added to the p4lang/behavioral-model repository.

[jafingerhut]

Based on my previous message, if we wanted to clarify the documentation, we should make explicit exactly what mark_to_drop is doing.

[smolkaj]

Thanks for figuring out what is going on here, @jafingerhut and @rst0git!

Based on my previous message, if we wanted to clarify the documentation, we should make explicit exactly what mark_to_drop is doing.

I went back to v1model.p4, and actually, this seems to be documented already: http://google3/third_party/p4lang_p4c/p4include/v1model.p4;l=412-413;rcl=425419938

So perhaps there is nothing to do here, except for me reading the documentation more carefully?

Should it be this way? I did not write the original BMv2 code, but the older drop() method also did this, and that goes back to 2016-Jan-18 when the source file primitives.cpp linked above was first added to the p4lang/behavioral-model repository.

It seems like reasonable behavior to me. I have yet to find out what our switches do, I am curious if they made the same choice there.

[jafingerhut]

It seems like reasonable behavior to me. I have yet to find out what our switches do, I am curious if they made the same choice there.

Good to know you realize it is a choice, one that can be made independently for every switch ASIC in existence. And some can make it baked into gates, others with some level of configurability, and others much more programmable. It is a continuum, but probably most switch ASICs are closer to the "some level of configurability, if you know who to ask or where in the documentation to look".

[antoninbas]

Are we closing this?

[smolkaj]

Sorry for dropping the ball on this. Taking another look at simple_switch.md to see if we should clarify this anywhere...

[smolkaj]

I'll send a PR shortly.

[smolkaj]

How is this?