Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditional primitives not supported in actions on simple_switch_grpc target #1515

Closed
vksysd opened this issue Sep 23, 2018 · 27 comments
Closed
Labels
duplicate This issue or pull request is a duplicate. question This is a topic requesting clarification.

Comments

@vksysd
Copy link

vksysd commented Sep 23, 2018

Hi all,
I have a P4 use case, in which I need to utilize the same table and inside the action part I need to take treat NORMAL and CLONED packets differently for which I need a conditional if primitive but unfortunately, the p4c backend throws this error.

error: MethodCallStatement: Conditional execution in actions is not supported on this target

Due to this, I found a possible workaround is to maintain two different tables with exactly the same entries but it causes unnecessary memory overhead and I need to call both tables separately.

Please add the conditional support for P4 actions in p4c compiler backend as bmv2 supports it through jmp and the jmp_ne primitives and I would like the bmv2 backend to generate the JSON accordingly.

My sample P4 program snippet is here

action populate_service_req_key_teid_map(bit<32> teid){
   // send cloned packets to CPU PORT
    if(IS_I2E_CLONE(standard_metadata)){
        hdr.new_service_req.setValid();
        hdr.new_service_req.key = hdr.service_req.key;
        hdr.new_service_req.sep1 = hdr.service_req.sep1;
        hdr.new_service_req.ksi_asme = hdr.service_req.ksi_asme;
        hdr.new_service_req.sep2 = hdr.service_req.sep2;
        hdr.new_service_req.ip = hdr.service_req.ip;
        hdr.new_service_req.sep3 = hdr.service_req.sep2;
        hdr.new_service_req.teid = teid;
        hdr.service_req.setInvalid();
        standard_metadata.egress_spec = CPU_PORT;
        hdr.packet_in.setValid();
        hdr.packet_in.ingress_port = standard_metadata.ingress_port;
    }
    else
    { // it is a NORMAL packet so send thre reply back to Source
            hdr.initial_ctxt_setup_req.setValid();
            hdr.initial_ctxt_setup_req.traffic_code = 17;
            hdr.initial_ctxt_setup_req.sep1 = hdr.service_req.sep1;
            hdr.initial_ctxt_setup_req.teid=teid;
            // set invalid the incoming headers as we are appending new one
            hdr.data.setInvalid();
            hdr.service_req.setInvalid();
    }
}
table service_req_key_teid_map{
    key={
        hdr.service_req.key :  exact;
    }
    actions={
        populate_service_req_key_teid_map;
        NoAction;
    }
    size = 1024;
    default_action = NoAction();
}

PS I am using simple_switch_grpc bmv2 target to run this code.

@hesingh
Copy link
Contributor

hesingh commented Sep 23, 2018

In the apply{} section of the P4 control, use if-else.

@vksysd
Copy link
Author

vksysd commented Sep 23, 2018

In the apply{} section of the P4 control, use if-else.

Using if-else in the apply{} section of the P4 control won't help here unless we maintain two copies of the same table, as we can apply a given P4 table only once in P4 code.

@hesingh
Copy link
Contributor

hesingh commented Sep 23, 2018

You reported two problems. I responded to one of them which was conditional statements are not allowed in action with a MethodCallStatement. The other problem you have is using two tables which is a data structure design issue. Consider adding standard_metadata to table key which also changes the args to the action.

table service_req_key_teid_map{
key={
hdr.service_req.key : exact;
standard_metadata : exact;
}
actions={
populate_service_req_key_teid_map;
action_normal;
}
size = 1024;
default_action = action_normal;
}

@mihaibudiu
Copy link
Contributor

Make one action for each of the two if branches and add the IS_CLONE bit to the table key.

@vksysd
Copy link
Author

vksysd commented Sep 23, 2018

Make one action for each of the two if branches and add the IS_CLONE bit to the table key.

This will work for this specific case but there might be similar use cases where conditional primitives might be required in actions. If possible, please add the conditional support for P4 actions in p4c compiler backend as well.

@mihaibudiu
Copy link
Contributor

The compiler can do nothing if the target you execute your program on does not support conditionals in actions. We can add this to the front end, but each backend will decide for its own.

@antoninbas
Copy link
Member

I think that's why @coolvikas brought up the "jump" and "conditional jump" primitives in his issue; these were added to bmv2 a while ago.

@antoninbas
Copy link
Member

See p4lang/behavioral-model#379

@jafingerhut
Copy link
Contributor

@coolvikas I just want to echo a comment from Mihai -- it is relatively straightforward to implement the capability of handling if statements inside of actions in a software switch like BMv2. If you want maximum portability for your P4 program to the highest speed targets, there may be some that will not handle this, no matter what the open source tools do with it.

Hence the suggestions from others on ways to write your program that do not require if statements inside the actions, but keep the conditional logic outside actions, e.g. in the table search key.

If you are not worried about maximizing portability, well even then there is the current limitation in p4c that may take some time to enhance, so learning about these alternatives is useful even then.

@jnfoster
Copy link
Contributor

jnfoster commented Sep 24, 2018 via email

@mihaibudiu
Copy link
Contributor

I will tag this as a question; the request itself is a duplicate of #644

@mihaibudiu mihaibudiu added the question This is a topic requesting clarification. label Sep 24, 2018
@jafingerhut
Copy link
Contributor

@coolvikas Looking at the code in your original issue again, I think there is a straightforward way to write it with only one table, shown below.

I think this kind of "move the action code after the table apply" transformation is pretty general, and works even if there were N actions other than NoAction.

bit<32> saved_teid;
action populate_service_req_key_teid_map(bit<32> teid){
    saved_teid = teid;
}
table service_req_key_teid_map{
    key = {
        hdr.service_req.key :  exact;
    }
    actions = {
        populate_service_req_key_teid_map;
        NoAction;
    }
    size = 1024;
    default_action = NoAction();
}
apply {
    // ... code before applying table service_req_key_teid_map here ...

    switch (service_req_key_teid_map.apply().action_run) {
        populate_service_req_key_teid_map: {
            // send cloned packets to CPU PORT
            if (IS_I2E_CLONE(standard_metadata)) {
                hdr.new_service_req.setValid();
                hdr.new_service_req.key = hdr.service_req.key;
                hdr.new_service_req.sep1 = hdr.service_req.sep1;
                hdr.new_service_req.ksi_asme = hdr.service_req.ksi_asme;
                hdr.new_service_req.sep2 = hdr.service_req.sep2;
                hdr.new_service_req.ip = hdr.service_req.ip;
                hdr.new_service_req.sep3 = hdr.service_req.sep2;
                hdr.new_service_req.teid = saved_teid;
                hdr.service_req.setInvalid();
                standard_metadata.egress_spec = CPU_PORT;
                hdr.packet_in.setValid();
                hdr.packet_in.ingress_port = standard_metadata.ingress_port;
            }
            else
            { // it is a NORMAL packet so send thre reply back to Source
                hdr.initial_ctxt_setup_req.setValid();
                hdr.initial_ctxt_setup_req.traffic_code = 17;
                hdr.initial_ctxt_setup_req.sep1 = hdr.service_req.sep1;
                hdr.initial_ctxt_setup_req.teid = saved_teid;
                // set invalid the incoming headers as we are appending new one
                hdr.data.setInvalid();
                hdr.service_req.setInvalid();
            }
        }
    }

    // ... code after applying table service_req_key_teid_map here ...
}

@mihaibudiu
Copy link
Contributor

The implementation cost may be quite different between these schemes, depending on the target.

@jafingerhut
Copy link
Contributor

Understood. That is one reason I stopped myself from suggesting that this was a transformation that a P4 compiler should always make for a developer on their behalf. As far as I can see, there are some targets where this transformation would cause an increase in the temporary storage for the packet to increase by the sum of the sizes of all action parameters for all actions of the affected table. It would only last until the switch statement completed, but that could be bad enough. That plus perhaps an extra stage or two in a pipeline.

At east for the BMv2 target, none of those resources are in scarce supply, though, so if it gets Vikas off the ground and running for now, it seemed worth mentioning.

@vksysd
Copy link
Author

vksysd commented Sep 26, 2018

@jafingerhut Thanks for the suggestion of rewriting the code. I tried it but it seems to be not working for me as I have put the table service_req_key_teid_map in a separate control block named as c_process_normal_clone_packet
and I want to access it in both ingress as well as egress control block because I am using cloneType I2E in which I am processing clones packets in egress and normal packets in ingress pipeline. Now while populating the tables its name changes to c_egress.c_process_normal_clone_packet.service_req_key_teid_map and c_ingress.c_process_normal_clone_packet.service_req_key_teid_map
as confirmed by using table_dump command in simple_switch_CLI
which is as good as maintaining two separate tables in ingress as well as egress control blocks.

Also, I am getting the warning
[--Wwarn=uninitialized_use] warning: saved_teid may be uninitialized hdr.new_service_req.teid = saved_teid;
while compiling the P4 code.

@mihaibudiu
Copy link
Contributor

Instantiating a control inside of another one makes a copy of all structures (inlines) the control.
There is no way to share a table between two controls. In many hardware targets each control may be mapped to a separate physical pipeline, with local memories.

Is your variable initialized on all paths that reach that assignment?

@vksysd
Copy link
Author

vksysd commented Sep 26, 2018

Yes, I have declared the variable at the start of the control block and initialized the variable inside the action body as suggested by Andy.

@jafingerhut
Copy link
Contributor

You probably want to initialize the variable before calling .apply() on the table (which could be done where the variable is declared, with an initializer, too). Except in unusual cases, apply'ing a table does not guarantee that a particular action will be called, so there are execution paths that do not invoke that action, as far as the compiler can tell, hence the warning.

@jafingerhut
Copy link
Contributor

This behavior of not being able to call the same instance of a control from more than one other control in a P4 program is similar to the issue of the current p4c bmv2 back end not allowing a table to be applied more than once, I think.

One could imagine a P4 tools chain and target that do allow a single instantiation of a control to be called, even if it contained tables or externs, from more than one other control in your program, and I can imagine that some targets could implement that behavior in a fairly straightfoward way, e.g. software targets on general purpose CPUs, and FPGAs, and some NPUs.

Like applying a table multiple times for the same packet, there is an issue that the highest speed targets, e.g. billions of packets per second in a single device, will likely not allow you to do that. If you aren't targeting those, I understand that the current restrictions may seem arbitrary.

@jafingerhut
Copy link
Contributor

More discussion here on the 'apply a table more than once per packet' issue: #457

@mihaibudiu
Copy link
Contributor

This behavior is part of the language semantics. If you want a shared control it must be part of the architecture.

@jafingerhut
Copy link
Contributor

I agree that it would be a change to the P4_16 language spec to allow an instantiation of a control at the top level, and then calling it from multiple other controls, but that is not a big change syntactically or semantically, I think. I know it was considered by the language working group and decided not to be included, but it was a design choice, not some kind of logical contradiction in the spec, that prevented it from being included, wasn't it?

@mihaibudiu
Copy link
Contributor

Indeed, instantiation = allocation. If you instantiate once, you get a single copy.

@jnfoster
Copy link
Contributor

jnfoster commented Sep 27, 2018

Another reason for this restriction is that it limits the flow of information to the in/out parameters in the data plane signature.

Suppose that you were allowed to write top-level instantiations:

C() c;

control D(...) {
  apply {
    c.apply(...);
}

control E(...) {
  apply {
    c.apply(...);
}

Switch(..., D(), E(), ...) main;

Now D and E can pass information to each other using the implicit channel via C.

This may not be possible to implement -- e.g., if the data plane signatures of the D and E components are fixed in advance by the architecture.

@jafingerhut
Copy link
Contributor

@jnfoster Is limiting the flow of information to the in/out parameters of a data plane signature a design goal of P4_16? I can see it being a desirable property -- I just didn't recall it being mentioned before.

Please take a look at the code snippet below. It is similar to the one you gave above, but with no top level instantiations. Is this an example of allowing information flow between D and E that uses C as an implicit channel? If so, it appears to be completely legal P4_16 to me.

I have also attached a complete P4_16 + v1model arch program that I've run through the current p4c and behavioral-model code, and it appears to pass the data through a common instantiation of C named c_inst just fine.

control X(in bit<4> idx,
          in bit<8> new_val_to_write,
          out bit<8> orig_val_before_write);

control C(in bit<4> idx,
          in bit<8> new_val_to_write,
          out bit<8> orig_val_before_write)
{
    register<bit<8>>(16) r1;
    bit<8> read_val;
    apply {
        r1.read(read_val, (bit<32>) idx);
        orig_val_before_write = read_val;
        r1.write((bit<32>) idx, new_val_to_write);
    }
}

control D(in bit<8> rabbit_into_hat)(X c) {
    bit<4> idx;
    bit<8> new_val_to_write;
    bit<8> orig_val_before_write;
    apply {
        idx = 7;
        new_val_to_write = rabbit_into_hat;
        c.apply(idx, new_val_to_write, orig_val_before_write);
    }
}

control E(out bit<8> rabbit_from_hat)(X c) {
    bit<4> idx;
    bit<8> new_val_to_write;
    bit<8> orig_val_before_write;
    apply {
        idx = 7;
        new_val_to_write = 87;
        c.apply(idx, new_val_to_write, orig_val_before_write);
        rabbit_from_hat = orig_val_before_write;
    }
}

control cIngress(inout Parsed_packet hdr,
                 inout metadata_t meta,
                 inout standard_metadata_t stdmeta) {
    C() c_inst;
    D(c_inst) d_inst;
    E(c_inst) e_inst;
    bit<8> rabbit_from_hat;
    table debug_table {
        key = { rabbit_from_hat : exact; }
        actions = { NoAction; }
        const default_action = NoAction;
    }
    apply {
        d_inst.apply(17);
        e_inst.apply(rabbit_from_hat);
        debug_table.apply();
    }
}

pass-info-outside-of-params.p4.txt

@jnfoster
Copy link
Contributor

jnfoster commented Oct 29, 2018 via email

@mihaibudiu
Copy link
Contributor

I will close this question, but keep the original one open.
If interesting targets can support conditionals in actions we should try to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request is a duplicate. question This is a topic requesting clarification.
Projects
None yet
Development

No branches or pull requests

6 participants