Parameterizing actions over externs

[smolkaj]

Currently, p4c does not allow one to parameterize an action over an extern, such as a meter or a counter. For example:

acl_common_actions.p4(20): [--Werror=type-error] error: direct_counter: Action parameters cannot have extern types
action copy(direct_counter c) {
            ^^^^^^^^^^^^^^

This prevents reuse of actions that use such externs.

I'm wondering if the community has considered adding support for this kind of parameterization? Are there any fundamental reasons why allowing this would be difficult, or is it just something that no one has wanted thus far?

[smolkaj]

To give some context, we're working on a project where the precise definition of actions is rather subtle, and where we have many slight variations of the same underlying base program. Thus, we would like to provide a library of curated actions that can be reused between control blocks.

[mihaibudiu]

This is because we assume that action arguments without a direction come from the control-plane. This is perhaps an unfortunate decision to overload the meaning of directionless parameters: they are either compile-time constants, or control-plane-supplied. The right solution is to add a new direction to differentiate the two. Note that externs have to be resolved at compile-time, so even if we allowed this feature, the value used for c must be a compile-time known value for the compiler to synthesize the body of the action.

[vgurevich]

Another way might be to follow the same scheme that is already in place for controls and parsers, where compile-time-known parameters are passed via the second set of parentheses. Here is an example from the real program. Notice how the control calc_ipv4_hash has two sets of parameters:

control calc_ipv4_hash(
    in    my_ingress_headers_t   hdr,
    in    my_ingress_metadata_t  meta,
    out   bit<32>                hash)(
    CRCPolynomial<bit<32>>       poly)
{
    Hash<bit<32>>(HashAlgorithm_t.CUSTOM, poly) hash_algo;

    action do_hash() {
        hash = hash_algo.get({
                hdr.ipv4.src_addr,
                hdr.ipv4.dst_addr,
                hdr.ipv4.protocol,
                meta.l4_lookup.word_1,
                meta.l4_lookup.word_2
            });
    }

    apply {
        do_hash();
    }
}

control calc_ipv4_hashes(
    in    my_ingress_headers_t   hdr,
    in    my_ingress_metadata_t  meta,
    inout selector_hash_t        hash)
{
    calc_ipv4_hash(CRCPolynomial<bit<32>>(
            coeff=32w0x04C11DB7, reversed=true, msb=false, extended=false,
            init=32w0xFFFFFFFF, xor=32w0xFFFFFFFF))
    hash1;

    calc_ipv4_hash(CRCPolynomial<bit<32>>(
            coeff=32w0x1EDC6F41, reversed=true, msb=false, extended=false,
            init=32w0xFFFFFFFF, xor=32w0xFFFFFFFF))
    hash2;

    calc_ipv4_hash(CRCPolynomial<bit<32>>(
            coeff=32w0xA833982B, reversed=true, msb=false, extended=false,
            init=32w0xFFFFFFFF, xor=32w0xFFFFFFFF))
    hash3;

    apply {
        hash1.apply(hdr, meta, hash[31:0]);
        hash2.apply(hdr, meta, hash[63:32]);
        hash3.apply(hdr, meta, hash[95:64]);
    }
}

[vgurevich]

@ChrisDodd -- we should also consider this for virtual functions.

[mihaibudiu]

Yes, Vladimir is correct, actions can refer to externs declared in the surrounding environment. So perhaps you can use an extern used as a constructor argument for a control. If we had generic controls this could be even more powerful.

[vgurevich]

By "another way" I meant "another way to pass externs into actions should we decide to allow that" (might be to reuse the syntax with two sets of parentheses)

[smolkaj]

This is because we assume that action arguments without a direction come from the control-plane.

Not sure if I understood this comment correctly, but I tried adding a direction to the parameter. This is still rejected by the compiler:

./third_party/pins_infra/sai_p4/instantiations/google/acl_common_actions.p4(21): [--Werror=type-error] error: c: a parameter with type direct_counter cannot have a direction
action acl_copy(inout direct_counter c) {
                                     ^

Note that externs have to be resolved at compile-time, so even if we allowed this feature, the value used for c must be a compile-time known value for the compiler to synthesize the body of the action.

Yes, that makes sense and would be the case for the use case we have in mind.

[mihaibudiu]

The first comment implies that the language should have been designed differently. We should have an additional direction 'control' instead of overloading ''.

[mihaibudiu]

This is probably inherited from p4-14. Does Andy's suggestion work, defining separate controls?

[smolkaj]

Wrapping the action in a control block does not work, because control blocks cannot be used as (or from within) actions.

[QinshiWang]

This is perhaps an unfortunate decision to overload the meaning of directionless parameters: they are either compile-time constants, or control-plane-supplied. The right solution is to add a new direction to differentiate the two.

I support this suggestion.

[jnfoster]

At the risk of sounding like a politician, I was for this in 2016 and I'm for it now :-)

[jafingerhut]

@vgurevich wrote: By "another way" I meant "another way to pass externs into actions should we decide to allow that" (might be to reuse the syntax with two sets of parentheses).

If we added action calls that has one set of parens for the currently defined action parameters, and an optional second set of parens for passing extern object instances (for example), would you be able to have such actions in the actions list of a table?

If "yes", would you allow the control plane to pick the value for this parameter when it adds an entry to a table? If so, that seems to be a significant new thing in P4 runtime APIs (P4Runtime API, and other runtime APIs).

If "no", would you somehow require that the compile time known value of the extern object instance was supplied in the table's action list?

[smolkaj]

For our purpose, we would supply the extern object instance in the table's action list. This already works today for headers and structs, e.g. we have

action acl_drop(inout standard_metadata_t standard_metadata) {
  mark_to_drop(standard_metadata);
}

control foo(inout standard_metadata_t standard_metadata) {
  table bar {
    actions = {
      acl_drop(standard_metadata);
    }
  }
  ...
}

[jnfoster]

@smolkaj, it's super janky but this code passes p4test:

#include <core.p4>

control C(); 
package P(C c);

extern E { 
    E();
    void m(); 
}

control ActionMaker()(E e) {
    action a() { 
        e.m(); 
    } 
    apply { 
        a();
    }

}

control MyC()() {
    ActionMaker(E()) am;
    action a() {
        am.apply();
    }
    table t {
        keys = {};
        actions = { a; }
    }

    apply {
        t.apply();
    }
}

P(MyC()) main;

[smolkaj]

Thanks for giving this a shot, @jnfoster!

I tried something similar with the BMv2 backend and it didn't compile, seems to be a BMv2 restriction then.

acl_ingress.p4(79): [--Werror=type-error] error: am.apply: apply cannot be called from actions
  action a() { am.apply(); }
               ^^^^^^^^^^

[jnfoster]

Actually, this is a relatively recent restriction. I think my p4test I was running on this VM is just old!

https://github.com/p4lang/p4c/pull/2598

[mihaibudiu]

The ideal solution is to have something like interfaces, and to use an interface instead of an extern E. Then you implement multiple externs that implement the same interface and you instantiate the one you need.

[vgurevich]

Let's try to have a practical example.

We can start with a standard program fragment (written for TNA architecture, but v1model or PSA will be almost identical), which has two actions shared among four tables:

control Ingress( . . . ) {
    action send(PortId_t port) {
        ig_tm_md.ucast_egress_port = port;
    }

    action drop() {
        ig_dprsr_md.drop_ctl = 1;
    }

    /* IPv4 tables */
    table ipv4_host {
        key = { hdr.ipv4.dst_addr : exact; }
        actions = {
            send; drop;
        }
        size = IPV4_HOST_TABLE_SIZE;
    }

    table ipv4_lpm {
        key     = { hdr.ipv4.dst_addr : lpm; }
        actions = { send; drop; }

        default_action = send(CPU_PORT);
        size           = IPV4_LPM_TABLE_SIZE;
    }

    /* IPv6 Tables */
    table ipv6_host {
        key = { hdr.ipv6.dst_addr : exact; }
        actions = {
            send; drop;
        }
        size = IPV6_HOST_TABLE_SIZE;
    }

    table ipv6_lpm {
        key     = { hdr.ipv6.dst_addr : lpm; }
        actions = { send; drop; }

        default_action = send(CPU_PORT);
        size           = IPV6_LPM_TABLE_SIZE;
    }
    apply {
        if (hdr.ipv4.isValid()) {
            if (ipv4_host.apply().miss) {
                ipv4_lpm.apply();
            }
        } else if hdr.ipv6.isValid()) {
            if (ipv6_host.apply().miss) {
                ipv6_lpm.apply();
            }
        }
    }
}

Imagine now, that you decided to add direct counters to one or more tables , e.g.ipv4_host and ipv6_lpm. This is not difficult to do, but suddenly the actions cannot be shared anymore and one will have to use different actions in each table, since the counter objects attached to each table will have different names. So, we'll have something like that:

    DirectCounter<bit<64>>(CounterType_t.PACKETS_AND_BYTES) ipv4_host_stats;

    action ipv4_host_send(PortId_t port) {
        send(port);
        ipv4_host_stats.count();
    }

    action ipv4_host_drop() {
        drop();
        ipv4_host_stats.count();
    }

        table ipv4_host {
        key = { hdr.ipv4.dst_addr : exact; }
        actions = {
            send; drop;
        }
        size = IPV4_HOST_TABLE_SIZE;

        counters = ipv4_host_stats;
    }

(and potentially, the same for the other tables).

Besides having 8 actions instead of 2, this also changes the API, since each table will have its own actions.

Here is where passing an extern into an action would be helpful. As did already discuss here the syntax can vary, but from the table perspective, it might look like this:

    DirectCounter<bit<64>>(CounterType_t.PACKETS_AND_BYTES) ipv4_host_stats;

        table ipv4_host {
        key = { hdr.ipv4.dst_addr : exact; }
        actions = {
            /* Use the renaming operator we discussed before */
            send ::= generic_send(ipv4_host_stats);
            /* For the convenience sake, allow reference to the table attribute */
            drop ::= generic_drop(self.counters);
        }
        size = IPV4_HOST_TABLE_SIZE;

        counters = ipv4_host_stats;
    }

Note, that we are almost there with the actions that have directional parameters.

Therefore, one options that might make sense is to have a special "direction" for the externs. "No direction" would probably be a not a good idea, since the current semantic is "read from the table's action data" and this is the most widely used one, so I'd be careful changing it.

One of the additional challenges I see is how (or whether) to specify a type for these parameters. Currently, extern constructors essentially give us a type, so one can treat DirectCounter(CounterType_t.PACKETS_AND_BYTES) as a type of sorts. However using these constructors as types is probably not the best solution:

• First, these are not "real" types, e.g. they can't be typedef-ed and do not follow most other rules.
• Second, they are very long
• Third, they are probably too specific: for example do we really want to treat DirectCounter(CounterType_t.PACKETS_AND_BYTES) and DirectCounter(CounterType_t.PACKETS) as two different types? In the light of the example above -- probably not.

This is where @mbudiu-vmw suggestion to use interfaces might be useful as long as it is lightweight, because we want a solution that will use significantly fewer lines of code compared to writing these 8 actions directly and more readable compared to using a preprocessor. No pressure :)

[smolkaj]

Re types, why not use the object name as the type? E.g., for extern counter { ... }, the type would be counter, and for extern<T> meter { ... }, the type would be meter<bit<2>> or similar.

[vgurevich]

@smolkaj -- it might work, but we need to consider, whether the types, like: DirectCounter<bit<64>>(CounterType_t.BYTES) and DirectCounter<bit<64>>(CounterType_t.PACKETS) or DirectCounter<bit<32>>(CounterType_t.BYTES) or DirectCounter<bit<64>>(CounterType_t.PACKETS_AND_BYTES) are equivalent (enough to be used in the same place). And there are many cases like this.

I am not saying "no" -- we just need to make sure we do not open too wide of a hole.

[smolkaj]

@vgurevich my proposal would be to use DirectCounter<bit<64>> as the type, not DirectCounter<bit<64>>(CounterType_t.BYTES).

[vgurevich]

Well, this is what we need to research.

For counters, actually, it might make sense to ignore the type in the angle brackets since it doesn't affect anything, except resource allocations (one might argue that it should've been an instantiation parameter instead), but for the other types we probably should respect it (e.g. for the meters or the hashes it determines the type for the output value).

At the same time, I am not sure if it is 100% true that the instantiation parameters do not have any effect on the interface, exposed by the extern.

Granted, some of these "quirks" might be just the properties of some specific architectures...

[jnfoster]

Let me just point out that there now exist a formalization of P4's type system, at least for a large core fragment.

Rather than following our noses and winging it, we could ask these questions there. One can even prove the required mathematical theorems to settle questions like whether it matters (I'm with Steffen on this -- the extern is the type) clearly and unambiguously.

[jnfoster]

In the interest of tidying up the set of active issues on the P4 specification repository, I'm marking this as "stalled" and closing it. Of course, we can always re-open it in the future if there is interest in resurrecting it.