Conditional execution in actions is not supported

[hanw]

    action drop() {
        mark_to_drop();
        exit;
    }

    action forward_ipv4() {
        hdr.ipv4.ttl         = hdr.ipv4.ttl - 1;

        if (hdr.ipv4.ttl == 0) {
            drop();
        }
    }

The above code snippet from P4D2 ARP exercise will trigger a compiler error in the frontend. Both p4test and p4c-bm2-ss report the same error.

error: MethodCallStatement: Conditional execution in actions is not supported on this target
        mark_to_drop();
        ^^^^^^^^^^^^^^
Compile failed.

[jnfoster]

@hanw and I just discussed this:

• It's fine for the bmv2-ss backend to reject the program since architectures are allowed to restrict which statements may appear in actions.

• However, then v1model.p4 should be updated to mention this restriction.

• And the check for conditionals in actions is implemented in the wrong place: p4test should accept the program since it's valid P4_16.

[jafingerhut]

I think it would be good, long term, if bmv2 became the least restrictive P4 target in existence, and could handle pretty much anything the language spec allows, plus have a documented method for adding new externs.

There is no fundamental reason why bmv2 should prevent one from running a syntactically and semantically legal P4-16 program. It has no hardware restrictions to limit it.

I agree in some cases it might take significant development effort to remove such restrictions (e.g. enahncement issue #457 could require significant changes to bmv2 and p4c back end code), but why do we want the 'flag ship' software P4 implementation to reject legal programs?

[mihaibudiu]

Bmv2 is a platform on which you can build any number of architectures. V1model is fixed, we can't change it. You want another architecture. Note that there is a significant cost to adding and supporting a new architecture and the associated back-end.

[jafingerhut]

I am not in any way trying to claim that such an architecture is a quick day's worth of work. I fully expect it could be months before such a thing exists, if ever. I still think it is a worthwhile goal.

[mihaibudiu]

As discussed before, p4test is an arbitrary architecture, so it is not clear that it should accept all programs.
In particular, to test some features used in restricted architecture, it needs to reject some programs.

The test is done in a mid-end pass, so I don't see why the test is in the wrong place. If the test was in the front-end you could argue that it is in the wrong place.

[jnfoster]

As discussed before, p4test is an arbitrary architecture, so it is not clear that it should accept all programs.

I would say that it should accept any semantically valid program, assuming no architecture-specific restrictions.

In particular, to test some features used in restricted architecture, it needs to reject some programs.

What's an example of this?

The test is done in a mid-end pass, so I don't see why the test is in the wrong place. If the test was in the front-end you could argue that it is in the wrong place.

If front vs. mid-end is the correct distinction, perhaps p4test should not use any mid-end passes then.

[jnfoster]

I think we should consider removing the wontfix label.

[mihaibudiu]

We already have issue #530 for this. The solution is described there: we should have an option for p4test to stop after the front-end. But for testing we use p4test to run lots of passes, including mid-end passes. Changing that would disrupt all our tests, which save the output after the mid-end.

This change should take a few lines of code in p4test.

[jafingerhut]

@mbudiu-vmw The issue you mention is a desirable improvement, I agree, and is probably relatively quick and easy compared to the potential goal of making bmv2 into something that can emulate all legal P4-16 programs.

If that potential goal is shared by p4.org, and there are people willing to help implement it at some point in the future, I think it would be worth having an enhancement issue for that (one that would probably be broken into multiple other smaller issues over time).

[mihaibudiu]

You cannot really build an architecture that emulates all P4-16 targets: the number of parsers and control blocks is statically fixed in a P4-16 architecture. The way these blocks are chained is also highly architecture-dependent. The intrinsic metadata and externs supported are architecture dependent.

[jafingerhut]

@mbudiu-vmw bmv2 could become a program with well-documented steps for adding new architectures and externs to it, with externs implemented in C++ or whatever you can link to it. Yes?

[mihaibudiu]

I think that BMv2 is such a program already. But we still cannot have a universal compiler for all P4-16 programs, which is what you are suggesting. You will need a custom back-end for each architecture, if that architecture does something useful.

[ChrisDodd]

I disagree -- we can and should have such a universal compiler for bmv2 targets. Its not easy to do, but that does not make it not a worthwhile goal. It means having a way to specify what all of the architecture-specific externs and metadata do; whether that is in P4 (additions to the language) or some other mechanism is TBD.

…

On Tue, May 23, 2017 at 10:41 AM, Mihai Budiu ***@***.***> wrote: I think that BMv2 is such a program already. But we still cannot have a universal compiler for all P4-16 programs, which is what you are suggesting. You will need a custom back-end for each architecture, if that architecture does something useful. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4c8d7-JiC8ZUedPAgC0W3NCn_UQFh-ks5r8xo9gaJpZM4Ngxlw> .

[mihaibudiu]

Look at the current simple_switch BMv2 back-end: it treats several control blocks specially, the checksum blocks for example, and the deparser block. These cannot have arbitrary code inside, each of them has completely different restrictions. I don't see how you could build a compiler for such a target in a completely generic way.

[jafingerhut]

@mbudiu-vmw Aren't those restrictions derived from Barefoot Tofino-like hardware constraints? bmv2 could be written to do arbitrary table lookups in the deparser, and would have no trouble implementing them, even if a particular hardware target might.

[mihaibudiu]

This has nothing to do with Tofino. This is just how this particular architecture is, and it is a software switch. The deparser cannot do any table accesses on this architecture - that's how BMv2 works. You cannot choose the architecture, it is given by the vendor; the compiler has to match the architecture. We chose an architecture where the deparser can only specify a list of headers.

[ChrisDodd]

So what is needed is a way to tell the backend what the restrictions for each control block are. The control blocks themselves are already defined in the package declaration of the architecture. If there are only a few categories of controls of interest, we could use annotations on those package args. If that's not adequate, there could be some kind of framework description in a separate file.

The ultimate goal should be to make it easy for someone to add support for a new architecture -- just define a new arch.p4 file and perhaps a config file of some kind for the compiler and a .so file or some such with the externs and metadata (plus whatever other config needed) for bmv2.

[mihaibudiu]

It can be made easier probably, but in practice for a real target I expect it won't be easy.

In a real architecture there will be lots of fixed-function blocks which are not exposed to P4, for these you will have to write some custom code. Queueing, packet generation, scheduling, all control-plane APIs which have nothing to do with P4, etc.

@gbrebner and @rhalstea know something about emulating a very flexible architecture with lots of custom blocks; we should ask for their advice.

[jafingerhut]

So the proposal here for bmv2 is not that it can implement arbitrary restrictions of any architecture with no work required, but that it can be the least restrictive implementation, i.e. parsers and control blocks that allow anything the P4-16 language spec allows, which includes, e.g. if statements within actions that call extern methods in its then/else branch, nested arbitrarily deeply. There are currently several restrictions imposed by bmv2 that should be relatively small work to generalize, i.e. allow the deparser control block to do everything that an ingress control block can do. That should be as simple a matter as making the part of bmv2 that implements the deparser reuse the same code that ingress/egress already use today. The restrictions that prevent this today were human-chosen, and I will bet you $10 those restrictions were chosen for reasons that have nothing to do with a pure software forwarding engine.

If a different target has restrictions on what it can do there, it is up to the back end for that target to implement them, and that might be significant work. No argument there.

[mihaibudiu]

I think that using the term bmv2 is misleading; bmv2 is a framework for building any number of simulators. We should always talk about bmv2 + a certain architecture. The compiler we have is called p4c-bm2-ss for a good reason.

You should join the PSA working group and make sure that the bmv2 PSA simulator satisfies your requests, and that p4c-bm2-psa as well.

But in general, I think that the main point of BMv2 is to allow a large number of simulators to be implemented, each of which should reflect constraints of some specific target. So in general BMv2 simulators should not be arbitrarily powerful.

[mihaibudiu]

BTW: we considered in a previous design having a deparser block, which is much like a control but cannot have tables. We settled for reusing control and allowing calls to emit. But it is very reasonable to assume that on some (most? all?) architectures a deparser is very restricted in power compared to a control.

[jafingerhut]

@mbudiu-vmw A very reasonable suggestion on the PSA arch group and simulator.

@cc10512 Is the PSA arch group open for joining yet?

[antoninbas]

You guys have started to digress a little bit... Regarding the original issue (arbitrary condition support in actions), I have opened a pull request in bmv2 (p4lang/behavioral-model#379), which introduces 2 new primitive operations: _jump (unconditional jump) and _jump_if_zero (conditional jump). The commit message has more details but I believe this will enable the translation of condition in P4_16 actions to bmv2 JSON. The bmv2 backend pass should not be too hard to write.

[hanw]

Thanks @antoninbas

[cc10512]

@jafingerhut I was told it would take a day ... last week. The mailing list is still not up. Will keep bugging the vendors.

[pwkuan]

Hello @antoninbas ,
Has this bug been fixed?

[mihaibudiu]

This bug is not fixed in the compiler.

[hesingh]

I debugged an if-statement inside the apply block of a P4 control as follows:

$ ./p4c-bm2-ss --std p4-16 ../testdata/p4_16_samples/basic_routing-bmv2.p4 -o tmp.json

The if-statement exists at this line of code:

https://github.com/p4lang/p4c/blob/master/testdata/p4_16_samples/basic_routing-bmv2.p4#L146

In the emitted JSON, I think, the if-statement is emitted as a conditional. A snippet of JSON is included below.

"conditionals" : [
        {
          "name" : "node_2",
          "id" : 0,
          "source_info" : {
            "filename" : "../testdata/p4_16_samples/basic_routing-bmv2.p4",
            "line" : 146,
            "column" : 12,
            "source_fragment" : "hdr.ipv4.isValid()"
          },
          "expression" : {
            "type" : "expression",
            "value" : {
              "op" : "d2b",
              "left" : null,
              "right" : {
                "type" : "field",
                "value" : ["ipv4", "$valid$"]
              }
            }
          },
          "false_next" : null,
          "true_next" : "ingress.port_mapping"
        }
      ]

Now, my question. If the if-statement is in apply block of a P4 control, the if-statement is emitted in JSON one way (using conditional). But, when we propose adding an if-statement to a P4 action, we are proposing this JSON:

[op0(_jump_if_zero, [hdr1.f8, 3]), op1(assign, [hdr1.f32, 0xaa]), op2(_jump, [4]), op3(assign, [hdr1.f32, 0xbb])]

Why don't we use the same JSON for if-statement, no matter where the statement exists in the P4 program?

[hesingh]

There is another vagary with p4c for if-statement. Using the same P4 program in my previous comment, I see the checksum related code (perhaps this code exists in a P4 library?) using an "if_cond". See relevant JSON output below.

{
      "name" : "cksum",
      "id" : 0,
      "source_info" : {
        "filename" : "../testdata/p4_16_samples/basic_routing-bmv2.p4",
        "line" : 179,
        "column" : 8,
        "source_fragment" : "update_checksum( ..."
      },
      "target" : ["ipv4", "hdrChecksum"],
      "type" : "generic",
      "calculation" : "calc",
      "verify" : false,
      "update" : true,
      "if_cond" : {
        "type" : "bool",
        "value" : true
      }
    }

[mihaibudiu]

This question is more suitable to the bmv2 repository.
However, we cannot use the same representation because in controls the program is expressed as a sequence of table invocations, and there are no table invocations in actions.

[hesingh]

I see. Thanks, @mbudiu-vmw

What about the "if_cond" used in checksum JSON?

[mihaibudiu]

I am not sure I understand what the question is.
The specification for the json format for bmv2 is here:
https://github.com/p4lang/behavioral-model/blob/master/docs/JSON_format.md; look for the checksums section.

As you see, a checksum has a condition field.

[hesingh]

Got it about if_cond in JSON for checksum.

My question is, why can't we use if_cond, and new else_cond, else_if_cond for supporting if-statement JSON?

[jafingerhut]

I do not know the full story and rationale for the design of the BMv2 JSON file format, but in my look at a substantial fraction of the format and how the p4c back end uses it, it seems highly likely that much of its design was influenced by the P4_14 language and architecture. Not 100% determined and it could not have been any other way, mind you, but definitely influenced.

There are many other ways the BMv2 JSON format could have been defined, of course. In most cases, it probably wasn't made more general because it didn't need to be.

[hesingh]

The nuance with changing p4c to support the behavioral-model's __jump and __jump_if_zero is because p4c changes if-else statements inside an action to a C ternary operation as discussed in restrictions [here](https://github.com/p4lang/behavioral-model/blob/master/docs/simple_switch.md#restrictions-on-code-within-actions

So, the bmv2 backend emits JSON with op : "?" . I suppose simple_switch supports this JSON and also the '_jump` one. Hopefully, now p4c can decide what changes to make to support if-statements inside an action if the if-statement does not call any other function/extern. Thereafter, we can see what to do with this issue which may not be easy to fix.

[mihaibudiu]

I will probably not work on new features until we close most of the outstanding bugs.
But, as you may see, we don't even have enough people to review the contributions.

[hesingh]

Understood, @mbudiu-vmw.

This issue has a workaround. One could set a drop_flag in metadata inside the action. Once the table.apply() has been invoked and the action run, the apply block in control can check the flag and call drop(). Sorry, even I am buried right now to work on p4c changes.