A simple way to implement resubmit/recirculate/clone3 in P4-16 on v1model #1698

mihaibudiu · 2019-01-25T21:29:02Z

This implements a proposal that performs recirculate by using tags on the user metadata fields.
If one calls recirculate() (with no arguments!), all fields that are tagged with @recirculate are automatically recirculated at the end of the pipeline - similar with the P4-14 semantics.

Limitations on the current implementation:

all fields that are ever recirculated/resubmitted/cloned are always recirculated. If we want we can have 3 different annotations for these 3 use cases. This is mostly a performance optimization.
One cannot recirculate standard_metadata fields. This is a more serious limitation. I don't really understand what it means to recirculate standard_metadata, since it is supposed to reflect the state of the packet as received. One would think users should not be able to control what standard_metadata is received at the input of the pipeline. If we do desire to allow recirculation of standard_metadata (all of it, not field by field), I can imagine how we can easily support this in two ways
- add an extra boolean argument to recirculate/... that indicates standard_metadata should be recirculated
- require the user to add a @recirculate annotation on the corresponding metadata parameter when implementing the ingress control

This is meant to address #1669.

This has been a long-standing bug which only surfaced recently. We may want to discuss this proposal in the language design working group, although it is really architecture-specific.

mihaibudiu · 2019-01-25T21:29:36Z

backends/bmv2/common/extern.cpp

@@ -117,117 +117,6 @@ ExternConverter::convertExternFunction(ConversionContext* ctxt,
    return primitive;
 }

-void
-ExternConverter::modelError(const char* format, const IR::Node* node) const {


I have moved these 3 methods to the Context class, where they make more sense.

mihaibudiu · 2019-01-25T21:30:33Z

backends/bmv2/common/helpers.cpp

@@ -82,6 +82,119 @@ unsigned nextId(cstring group) {
    return counters[group]++;
 }

-}  // namespace BMV2
+void
+ConversionContext::addToFieldList(const IR::Expression* expr, Util::JsonArray* fl) {


these methods have been moved from extern.cpp and slightly simplified.

mihaibudiu · 2019-01-25T21:31:50Z

backends/bmv2/simple_switch/simpleSwitch.cpp

@@ -109,22 +109,22 @@ ExternConverter_action_profile ExternConverter_action_profile::singleton;
 ExternConverter_action_selector ExternConverter_action_selector::singleton;

 Util::IJson* ExternConverter_clone::convertExternFunction(
-    UNUSED ConversionContext* ctxt, UNUSED const P4::ExternFunction* ef,
-    UNUSED const IR::MethodCallExpression* mc, UNUSED const IR::StatOrDecl* s,
+    ConversionContext* ctxt, UNUSED const P4::ExternFunction* ef,


many of these parameters are in fact used; the UNUSED annotation was wrong.

backends/bmv2/simple_switch/simpleSwitch.cpp

jafingerhut · 2019-01-26T01:22:43Z

I do not have enough experience with the compiler internals to make useful comments on the code changes proposed, so I will focus on questions on the approach of using annotations to specify what metadata should be recirculated/resubmitted/cloned.

If this annotation idea goes forward, it seems to have a significant limitation in how precise one can be in what you want to preserve, when there are nested structs.

For example, if you have user-defined metadata where the "top level" struct is user_meta_t, with sub-structs like this:

struct leaf_t {
    bit<4> a;
    bit<4> b;
}

struct mid_t {
    leaf_t x;
    leaf_t y;
}

struct user_meta_t {
    mid_t mid1;
    mid_t mid2;
}

Suppose I want to preserve mid1.x.b, but I do not need to preserve any of these:

mid1.y.b
mid2.x.b
mid2.y.b

If I put the @recirculate annotation on field b, does it preserve all 4 of those?

Unless I am missing a way to specify this via annotations, it seems like it restricts you to preserving a field everywhere that the containing struct type occurs, even if that is in many places.

jafingerhut · 2019-01-26T01:28:44Z

Mihai, regarding this comment of yours: "I don't really understand what it means to recirculate standard_metadata, since it is supposed to reflect the state of the packet as received." standard_metadata as defined in P4_14 and v1model is just as much about what fields are outputs of ingress, and outputs of egress, that control what happens to the packet next. They are not only inputs.

I do not know how common it is to do such a thing, but you could have a P4 program where the ingress control assigns a value to standard_metadata.egress_spec, which controls which output port the packet will go to after it finishes ingress (or whether it will be dropped, by storing a special invalid port number in that field). Maybe you want to do a resubmit to start ingress over again, and preserve the value of egress_spec calculated during the first pass.

Yes, I understand that it is certainly possible to copy that value into a user-defined metadata field and preserve that instead. The reason I raise the issue is not that there isn't another way, but wondering aloud how common it may be that an existing collection of P4_14 programs might already do that, and therefore it might be important for practical use cases to preserve what seems to be existing behavior there. Answering this question would best be done with feedback from others with access to larger collections of P4_14 code than I do.

hesingh · 2019-01-26T01:44:26Z

I do not have enough experience with the compiler internals to make useful comments on the code changes proposed, so I will focus on questions on the approach of using annotations to specify what metadata should be recirculated/resubmitted/cloned.

If this annotation idea goes forward, it seems to have a significant limitation in how precise one can be in what you want to preserve, when there are nested structs.

For example, if you have user-defined metadata where the "top level" struct is user_meta_t, with sub-structs like this:
struct leaf_t {
    bit<4> a;
    bit<4> b;
}

struct mid_t {
    leaf_t x;
    leaf_t y;
}

struct user_meta_t {
    mid_t mid1;
    mid_t mid2;
}
Suppose I want to preserve mid1.x.b, but I do not need to preserve any of these:

mid1.y.b

mid2.x.b

mid2.y.b

If I put the @recirculate annotation on field b, does it preserve all 4 of those?

Unless I am missing a way to specify this via annotations, it seems like it restricts you to preserving a field everywhere that the containing struct type occurs, even if that is in many places.

As far as these nested structs are used inside a P4 header, then the midend FlattenHeaders pass will flatten the struct and also preserve any annotation for any struct. The flattened fields can be annotated too but not so right now because the bmv2 backend/behavioral model loses its mind on receiving each flattened field with an annotation.

Likewise if the structs are use in instantiated controls, parsers, and packages, the struct will be flattened by the FlattenInterfaceStructs midend pass. You can use the p4test app and see that both midend passes will be active and flatten data.

I hope this helps.

mihaibudiu · 2019-01-26T02:11:58Z

You are right about preserving b everywhere. The only solution is to define two structs with the same layout one of which has annotations and one which doesn't.

What bmv2 does with the standard_metadata when recirculating is one thing - you can really do anything in a software switch, the question which I find more important is whether real routers allow this kind of recirculated information (where much of the intrinsic metadata is under user control), and whether it is indeed such an important feature that we have to make it easy to do.

mihaibudiu · 2019-01-26T02:26:50Z

Psa cannot recirculate standard metadata

jafingerhut · 2019-01-26T04:47:49Z

It isn't just two structs, necessarily. The structure of struct definitions can be an arbitrary DAG, and if you want only one field of a leaf, you would need to define copies of all structs from the root to the leaf.

It also seems like an odd, workaround kind of thing to suggest that a developer do.

mihaibudiu · 2019-01-28T17:01:14Z

@jafingerhut there are two questions to solve

if you want to recirculate standard metadata (directly, without copying), then the PSA design is broken as well, since it cannot do it
how to write P4-16 programs that use recirculate

The current PR is simple, but has some limitations. The one you point out is probably the most significant. The question is whether it is indeed significant in practice to cause a major inconvenience. In general the P4 programs I have seen have shallow structures. It may be also that the metadata that people want to recirculate is really application-specific, so it may not reside in structs that are reused in many places.

If you want full flexibility (including recirculating standard metadata) the only solution in P4-16 that I can imagine is a pair of functions send/receive.

Also, I consider v1model a legacy architecture (although it's the only one we really have at this point...), I think we should focus making PSA as nice as possible, even if this means some inconvenience for people who program against v1model. (But we should only allow minor inconveniences, we cannot afford to lose important features. It is my mistake that resubmit & co. do not work as expected, I should have realized that sooner. I hope that this patch is an example of an inconvenience.)

jafingerhut · 2019-01-29T21:08:20Z

Mihai, regarding this comment: "Psa cannot recirculate standard metadata", I guess the way I think of it is that PSA can recirculate any collection of bits you want to, and the source of these bits can be any state of the packet available, including standard metadata values, packet header field values, and/or user-defined metadata field values. You just copy the bits from wherever they are to the bits that are recirculated, then copy them from what was recirculated to wherever you want after recirculation.

After recirculation, P4_16 does not let you assign them to the values of in direction parameters of the ingress control block, and some parts of the PSA "standard metadata" are like that, but that is a language restriction that applies to all in direction parameters, even if they contain user-defined metdata or packet header field values.

mihaibudiu · 2019-01-29T21:15:01Z

Yes, if you are happy with this limitation for v1model then we're good.

jafingerhut · 2019-01-29T22:42:22Z

Before I mentioned a concern on the efficiency that a compiler can turn a P4 program using these annotations for which fields to preserve, or not, when there are multiple resubmit, recirculate, or clone operations in the same program.

Now I want to focus on the issue of whether translating a P4_14 program to a P4_16 program that uses a single set of preserved fields for all such operations could introduce a bug in the translated program.

The P4_14 version of switch.p4 in this directory, for example: https://github.com/p4lang/p4c/tree/master/testdata/p4_14_samples/switch_20160512
contains 6 different invocations of the primitive operation clone_ingress_pkt_to_egress, with a total of 5 different field lists (2 of the calls use the same field_list name).

The cloned packet, as it starts its egress processing, for each of these 5 different field lists, will in general have different values for the fields that are not preserved, vs. the ones that are preserved. The ones that are not preserved could be always 0 on some implementations, and perhaps the P4_14 language spec even mandates that, even though I suspect some implementations may let you weaken that to say that they have some unknown uninitialized values if they are not explicitly preserved.

At least for implementations that guarantee "if you do not preserve this metadata field value, it will be 0 at the beginning of egress", a P4 program author could have relied on a non-preserved field being 0. If an implementation took the union of all such field lists every time a clone_ingress_pkt_to_egress operation was invoked, then the program's behavior has been changed, perhaps introducing a bug.

I do not know if that switch.p4 program exhibits this property, because it is large enough that it is not very quick to check. It seems worth raising this issue for this kind of proposed implementation, at least, and also documenting it, if we choose to adopt this kind of translation.

hanw

P4-14 spec was written with certain architecture in mind. The field_list was used as a reference to a set of fields which are recirculated at the end of ingress/egress processing (here I use recirculate as a synonym for resubmit, recirculate and clone). The implementation of the recirculation mechanism is bandwidth-constrained. As a result, it is important to minimize the amount of the extra metadata to be recirculated. Hence, the idea of field list was created to allow programmer to specify the exact set of fields to be recirculated for a given packet.

With the current PR, it seems to me that the approach basically computes a union of recirculated fields among all recirculated packets. After the pass, compiler loses the field list information of each individual recirculate operation. It makes compiling v1model on certain target significantly more difficult.

If we do decide to go with the annotation approach (if that makes bmv2 implementation easier), I would suggest we have more information in the @recirculate annotation, for instance, @recirculate(1), @resubmit(1), @clone_i2e(1), @clone_e2e(1), where the number 1 can be used to differentiate the subset of fields used in different invocation of the primitives.

That said, if we have a way to make the original primitives work, that would be ideal.

mihaibudiu · 2019-03-05T01:41:57Z

Here is a presentation that describes the alternative possible implementations.
recirculate.pdf

jafingerhut · 2019-03-31T09:46:06Z

@mbudiu-vmw In your recirculate.pdf slides, on slide 3, you mention that this code

recirculate(a);

is equivalent to this code:

b = a;
recirculate(b);

I would agree with that, if the parameter of recirculate is direction in.

Would you agree that statement is not true in general if the parameter of recirculate is out or inout?

I believe some people may be reading that slide, and think you are implying those two code snippets are equivalent regardless of the direction of the parameter of the extern function.

mihaibudiu · 2019-04-08T15:31:04Z

Certainly, but in v1model recirculate did have an in argument.
For an inout argument the following are equivalent:

void f(inout bit<32> x);
f(a);

// same as

bit<32> b = a;
f(b);
a = b;

jafingerhut · 2019-04-10T16:27:06Z

Here are a P4_14 and some P4_16 example programs that do resubmit operations, with four different cases for the user-defined metadata that each of the four different resubmit operations perform.

They are just toy programs, but the first P4_14 one and the fanciful, imagined, last P4_16 version should process packets identically, given a proper implementation of the imagined new language constructs used in that last version, which admittedly is most similar to how PSA handles it, with extra parameters to the ingress parser and ingress control.

The P4_16 example could fairly easily be modified to use Mihai's idea of an extra extern to carry the preserved metadata, instead of additional parameters. That is a pretty minor variation.

https://github.com/jafingerhut/p4-guide/blob/master/v1model-special-ops/README-resubmit-examples.md

mihaibudiu · 2019-04-10T16:43:35Z

We can actually do this using annotations on the user metadata solely.

struct user_metadata_t {
    @resubmit(1)
    bit<128> f1;
    @resubmit(2)
    bit<160> f2;
    ...
}

This is the only thing needed, plus a call to the resubmit method (or setting a resubmit field in the standard_metadata) with a corresponding integer argument, which should probably be restricted to be a compile-time constant.

mihaibudiu · 2019-04-10T16:44:14Z

From this code we could generate entirely code equivalent with the fourth example.

jafingerhut · 2019-04-12T10:32:26Z

Here is an imagined P4_16 + v1model architecture program that uses the @resubmit(j) annotations on user-defined metadata fields that you want to be preserved for the corresponding resubmit(j) operation performed on a packet, where j is an integer, not a field list.

https://github.com/jafingerhut/p4-guide/blob/master/v1model-special-ops/v1model-multiple-resubmit-reasons-imagined-annotations.p4

The only down side I can think of now for this approach are if you have annotations on fields in nested structs within the user-defined metadata, as shown in the example on this earlier comment: #1698 (comment) I personally do not think that is a deal-breaker, but wanted to point it out to others who may not have thought of it before.

The other is that for recirculated packets and ingress-to-egress clones, this approach only works if the user-defined metadata is restricted to be the same type for ingress and egress parsers and controls. This is true for the v1model architecture, so no problem there.

It is currently not the case for the PSA architecture, but it defines a completely different way to preserve user-defined metadata for such packets. If people like this annotation approach even better than the method used in PSA for preserving user-defined metadata, and wanted to change PSA so that it also uses this technique, we could consider adding that restriction to PSA.

mihaibudiu · 2019-04-12T16:17:49Z

This is strictly for v1model.

mihaibudiu · 2019-04-12T16:19:21Z

I would add that a resubmit annotation can have multiple arguments, if the respective field can participate in multiple "field lists".

mihaibudiu · 2019-04-12T17:29:37Z

If there is no list the meaning could be that the field is always resubmitted...

mihaibudiu · 2019-04-23T22:50:05Z

@jafingerhut : I have re-implemented this along the lines you have suggested recently.
In particular, here is the translation of a p4-14 program:

enum bit<8> FieldLists {
    clone_e2e_FL = 8w0,
    recirculate_FL = 8w1,
    resubmit_FL = 8w2
}

struct mymeta_t {
    @field_list(FieldLists.clone_e2e_FL, FieldLists.recirculate_FL, FieldLists.resubmit_FL) 
    bit<8> resubmit_count;
    @field_list(FieldLists.clone_e2e_FL, FieldLists.recirculate_FL, FieldLists.resubmit_FL) 
    bit<8> recirculate_count;
    @field_list(FieldLists.clone_e2e_FL, FieldLists.recirculate_FL, FieldLists.resubmit_FL) 
    bit<8> clone_e2e_count;
    @field_list(FieldLists.clone_e2e_FL, FieldLists.recirculate_FL, FieldLists.resubmit_FL) 
    bit<8> last_ing_instance_type;
    @field_list(FieldLists.clone_e2e_FL, FieldLists.recirculate_FL, FieldLists.resubmit_FL) 
    bit<8> f1;
}

...
        clone3(CloneType.E2E, 32w1, (bit<8>)FieldLists.clone_e2e_FL);

With this change the tests using recirculate/clone/resubmit pass.

jafingerhut · 2019-04-25T16:54:44Z

@mbudiu-vmw Nice work!

I noticed that some of the test P4_14 programs like copy_to_cpu.p4 and packet_redirect.p4 have standard_metadata in their field lists for fields to preserve with recirculated/resubmitted/cloned packets.

If this approach goes forward, would attempting to compile such programs perhaps produce at least a warning, and perhaps an error, that they are not supported? In general, if any field was in such a field list that was not a user-defined metadata field?

jafingerhut · 2019-04-25T16:59:05Z

Also note that for digest calls, having a packet header field in them is a normal and expected thing, whereas they are not allowed in recirc/resub/clone field lists according to the P4_14 language spec. Example program issue1058.p4 creates a digest with a packet header field as one of the fields in its field lists, as well as all standard_metadata fields. I am not sure whether handling packet header fields presents any difficulties for this approach on digest function calls, or not.

jafingerhut · 2019-04-25T17:12:00Z

Also if this goes forward, we should review whether BMv2 simple_switch requires non-0 values for these field_list ids in the P4_16 source code, or whether it will cleanly handle 0 already.

… recirculated

mihaibudiu · 2019-04-29T16:20:15Z

The field list id in the p4-16 source code has no relationship with the one generated in the JSON.
For standard_metadata I can imagine several solutions. A simple one is to allow people to recirculate all of it or none. Another one is to require people to copy fields that need to be recirculated. A third one is to give a warning on such P4-14 programs.

mihaibudiu · 2021-09-17T01:03:01Z

This has been rebased and converted into #2902; this PR should be closed, but I will leave it for a while to preserve the discussion.

Added v1model operations of resubmit_preserving_field_list (and also recirculate and clone).

mihaibudiu commented Jan 25, 2019

View reviewed changes

backends/bmv2/simple_switch/simpleSwitch.cpp Show resolved Hide resolved

mihaibudiu requested review from hanw, ChrisDodd, jafingerhut and cc10512 January 25, 2019 21:42

mihaibudiu force-pushed the issue1669 branch from 14641e0 to 05d2b83 Compare January 28, 2019 16:53

mihaibudiu force-pushed the issue1669 branch from 05d2b83 to 1519514 Compare January 28, 2019 21:13

hanw reviewed Feb 4, 2019

View reviewed changes

mihaibudiu force-pushed the issue1669 branch from 1519514 to 4e8e2ca Compare April 23, 2019 22:45

mbudiu-vmw added 11 commits April 29, 2019 09:15

Preserve annotations when flattening structures

fe41417

Programs can have multiple declarations with the same name

f0b2671

Append method for annotations

b4eaffb

User metadata fields labeled with the annotation @recirculate will be…

f0f05bd

… recirculated

Updated v1model with simpler recirculate/resubmit/clone3 functions

43bd696

Tag metadata fields that need to be recirculated

ac36d25

Keep annotations when flattening

b59b6f9

Remove useless back-end pass

aeedec3

Delete code handling deprecated (and buggy) implementation

2bd73b6

Fixed broken test

e2f4d7d

Handle multiple resubmits/recirculate/clone calls

c084ebe

Rebased

24b36f1

mihaibudiu force-pushed the issue1669 branch from 4e8e2ca to 24b36f1 Compare April 29, 2019 18:36

mihaibudiu mentioned this pull request Jul 11, 2019

Warn on unsupported use of recirculate/resubmit/clone #2000

Merged

mihaibudiu marked this pull request as draft February 24, 2021 21:26

mihaibudiu mentioned this pull request Aug 17, 2021

Expanding structs into lists for BMv2 seems to have broken clone3 #2875

Closed

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this pull request Sep 17, 2021

Rebased version of p4lang#1698

db3b0da

mihaibudiu closed this Nov 2, 2021

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this pull request Nov 2, 2021

Rebased version of p4lang#1698

b790dac

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this pull request Nov 2, 2021

Rebased version of p4lang#1698

5218794

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this pull request Dec 1, 2021

Rebased version of p4lang#1698

c43a616

mihaibudiu pushed a commit that referenced this pull request Dec 7, 2021

Rebased version of #1698 (#2902)

2be448f

Added v1model operations of resubmit_preserving_field_list (and also recirculate and clone).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A simple way to implement resubmit/recirculate/clone3 in P4-16 on v1model #1698

A simple way to implement resubmit/recirculate/clone3 in P4-16 on v1model #1698

mihaibudiu commented Jan 25, 2019

mihaibudiu Jan 25, 2019

mihaibudiu Jan 25, 2019

mihaibudiu Jan 25, 2019

jafingerhut commented Jan 26, 2019 •

edited

Loading

jafingerhut commented Jan 26, 2019

hesingh commented Jan 26, 2019

mihaibudiu commented Jan 26, 2019

mihaibudiu commented Jan 26, 2019

jafingerhut commented Jan 26, 2019

mihaibudiu commented Jan 28, 2019

jafingerhut commented Jan 29, 2019

mihaibudiu commented Jan 29, 2019

jafingerhut commented Jan 29, 2019 •

edited

Loading

hanw left a comment •

edited

Loading

mihaibudiu commented Mar 5, 2019

jafingerhut commented Mar 31, 2019

mihaibudiu commented Apr 8, 2019

jafingerhut commented Apr 10, 2019

mihaibudiu commented Apr 10, 2019

mihaibudiu commented Apr 10, 2019

jafingerhut commented Apr 12, 2019

mihaibudiu commented Apr 12, 2019

mihaibudiu commented Apr 12, 2019

mihaibudiu commented Apr 12, 2019

mihaibudiu commented Apr 23, 2019

jafingerhut commented Apr 25, 2019

jafingerhut commented Apr 25, 2019

jafingerhut commented Apr 25, 2019

mihaibudiu commented Apr 29, 2019

mihaibudiu commented Sep 17, 2021

A simple way to implement resubmit/recirculate/clone3 in P4-16 on v1model #1698

A simple way to implement resubmit/recirculate/clone3 in P4-16 on v1model #1698

Conversation

mihaibudiu commented Jan 25, 2019

mihaibudiu Jan 25, 2019

Choose a reason for hiding this comment

mihaibudiu Jan 25, 2019

Choose a reason for hiding this comment

mihaibudiu Jan 25, 2019

Choose a reason for hiding this comment

jafingerhut commented Jan 26, 2019 • edited Loading

jafingerhut commented Jan 26, 2019

hesingh commented Jan 26, 2019

mihaibudiu commented Jan 26, 2019

mihaibudiu commented Jan 26, 2019

jafingerhut commented Jan 26, 2019

mihaibudiu commented Jan 28, 2019

jafingerhut commented Jan 29, 2019

mihaibudiu commented Jan 29, 2019

jafingerhut commented Jan 29, 2019 • edited Loading

hanw left a comment • edited Loading

Choose a reason for hiding this comment

mihaibudiu commented Mar 5, 2019

jafingerhut commented Mar 31, 2019

mihaibudiu commented Apr 8, 2019

jafingerhut commented Apr 10, 2019

mihaibudiu commented Apr 10, 2019

mihaibudiu commented Apr 10, 2019

jafingerhut commented Apr 12, 2019

mihaibudiu commented Apr 12, 2019

mihaibudiu commented Apr 12, 2019

mihaibudiu commented Apr 12, 2019

mihaibudiu commented Apr 23, 2019

jafingerhut commented Apr 25, 2019

jafingerhut commented Apr 25, 2019

jafingerhut commented Apr 25, 2019

mihaibudiu commented Apr 29, 2019

mihaibudiu commented Sep 17, 2021

jafingerhut commented Jan 26, 2019 •

edited

Loading

jafingerhut commented Jan 29, 2019 •

edited

Loading

hanw left a comment •

edited

Loading