Clone3() Migration

P4 Dev
1

I am working on a program that uses the project p4-srv6 (GitHub - netgroup/p4-srv6) as a basis for multihoming. Still, there is a problem, this project uses an older version of P4c before the function clone3() was deprecated, being that function was only used once in the whole project.

So I have been trying to adapt it so that instead of using clone3() it uses “clone_preserving_field_list()” as it is advised. This is the OG code on the main.p4 file:

action clone_to_cpu() {
   clone3(CloneType.I2E, CPU_CLONE_SESSION_ID, standard_metadata); 
}

The EgressPipeline:

control EgressPipeImpl (inout parsed_headers_t hdr,
                        inout local_metadata_t local_metadata,
                        inout standard_metadata_t standard_metadata) {
    apply {
        if (standard_metadata.egress_port == CPU_PORT) {        //packets for the controller
		    hdr.packet_in.setValid();
		    hdr.packet_in.ingress_port = standard_metadata.ingress_port;		
        }

        if (local_metadata.is_multicast == true
             && standard_metadata.ingress_port == standard_metadata.egress_port) {
            mark_to_drop(standard_metadata);
        }
    }
}

As this is a project that is not mine I tried to change as little as possible, so my idea was to use the struct local_metadata_t (from another project that I am also using), to store all the metadata used by the v1model.p4 so the cloned packet keeps the access to it.

const bit<8> CLONE_FL_1  = 1;                   //used by INT
const bit<8> CLONE_FL_clone3  = 2; 
struct preserving_metadata_t {                 
    @field_list(CLONE_FL_1)
    @field_list(CLONE_FL_clone3)
    bit<9> ingress_port;
    @field_list(CLONE_FL_clone3)
    bit<9> egress_spec;
    @field_list(CLONE_FL_1)
    @field_list(CLONE_FL_clone3)
    bit<9> egress_port;
    @field_list(CLONE_FL_clone3)
    bit<32> clone_spec;
    @field_list(CLONE_FL_clone3)
    bit<32> instance_type;
    @field_list(CLONE_FL_clone3)
    bit<1> drop;
    @field_list(CLONE_FL_clone3)
    bit<16> recirculate_port;
    @field_list(CLONE_FL_clone3)
    bit<32> packet_length;
    @field_list(CLONE_FL_clone3)
    bit<32> enq_timestamp;
    @field_list(CLONE_FL_clone3)
    bit<19> enq_qdepth;
    @field_list(CLONE_FL_clone3)
    bit<32> deq_timedelta;
    @field_list(CLONE_FL_1)
    @field_list(CLONE_FL_clone3)
    bit<19> deq_qdepth;                 
    @field_list(CLONE_FL_1)
    @field_list(CLONE_FL_clone3)
    bit<48> ingress_global_timestamp;
    @field_list(CLONE_FL_clone3)
    bit<48> egress_global_timestamp;
    @field_list(CLONE_FL_clone3)
    bit<32> lf_field_list;
    @field_list(CLONE_FL_clone3)
    bit<16> mcast_grp;
    @field_list(CLONE_FL_clone3)
    bit<32> resubmit_flag;
    @field_list(CLONE_FL_clone3)
    bit<16> egress_rid;
    @field_list(CLONE_FL_clone3)
    bit<1> checksum_error;
    @field_list(CLONE_FL_clone3)
    bit<32> recirculate_flag;
}


typedef bit<32> switch_id_t;                
struct int_metadata_t {                     //used by INT                     
    switch_id_t switch_id;
    bit<16> new_bytes;
    bit<8>  new_words;
    bool  source;
    bool  sink;
    bool  transit;
    bit<8> intl4_shim_len;
    bit<16> int_shim_len;
}
//Custom metadata definition
struct local_metadata_t {                   //used by INT
    bool is_multicast;
    bool skip_l2;
    bool xconnect;
    ipv6_addr_t next_srv6_sid;
    ipv6_addr_t ua_next_hop;
    bit<8> ip_proto;
    bit<8> icmp_type;
    l4_port_t l4_src_port;
    l4_port_t l4_dst_port;
    bool ipv4_update;
    int_metadata_t int_meta;     
    preserving_metadata_t perserv_meta;     
}

right before the cloning is done, I copy all the information into the struct.

action clone_to_cpu() {    
    local_metadata.perserv_meta.ingress_port = standard_metadata.ingress_port;
    local_metadata.perserv_meta.egress_spec = standard_metadata.egress_spec;
    local_metadata.perserv_meta.ingress_port = standard_metadata.ingress_port;
    local_metadata.perserv_meta.egress_spec = standard_metadata.egress_spec;
    local_metadata.perserv_meta.egress_port = standard_metadata.egress_port;
    local_metadata.perserv_meta.instance_type = standard_metadata.instance_type;
    local_metadata.perserv_meta.packet_length = standard_metadata.packet_length;
    local_metadata.perserv_meta.enq_timestamp = standard_metadata.enq_timestamp;
    local_metadata.perserv_meta.deq_timedelta = standard_metadata.deq_timedelta;
    local_metadata.perserv_meta.deq_qdepth = standard_metadata.deq_qdepth;
    local_metadata.perserv_meta.ingress_global_timestamp = standard_metadata.ingress_global_timestamp;
    local_metadata.perserv_meta.egress_global_timestamp = standard_metadata.egress_global_timestamp;
    local_metadata.perserv_meta.mcast_grp = standard_metadata.mcast_grp;
    local_metadata.perserv_meta.egress_rid = standard_metadata.egress_rid;
    local_metadata.perserv_meta.checksum_error = standard_metadata.checksum_error;

    clone_preserving_field_list(CloneType.I2E, CPU_CLONE_SESSION_ID, CLONE_FL_clone3);
}

I am testing using mininet, and when I compile and run the original project and try to ping between the hosts, those are detected by the topology and the pings work.
On my version, the hosts are not detected and the pings fail, when it comes to the logs from the ONOS controller, there is no difference between the warning logs of the 2 versions.

Some of the warnings are:

1. onos  | 17:06:30.114 WARN  [ModelCache] DeviceID device:r13 not found as a UiDevice
2. onos  | 17:06:48.365 WARN  [LinkDiscovery] LLDP Packet failed to validate!

So, does someone know how the conversion from clone3() to clone_preserving_field_list() should be done and why this one is failing?

2

If what you have shown is the entire contents of the egress code, then it should only be necessary to preserve copies of the standard_metadata fields that are actually used there during the clone action, not all of them.

But, in addition, you should use the preserved copies of those fields during egress processing, but only in the parts of the code that are relevant for cloned packets, which is probably only the first if statement, not the second if statement, i.e. in the first if statement, replace all occurrences of standard_metadata.some_field with local_metadata.preserv_meta.some_field.

3

so I am copying the 2 values to a new struct, just for the CPU case:

action clone_to_cpu() {
        local_metadata.perserv_CPU_meta.ingress_port = standard_metadata.ingress_port;
        local_metadata.perserv_CPU_meta.egress_port = standard_metadata.egress_port;
        clone_preserving_field_list(CloneType.I2E, CPU_CLONE_SESSION_ID, CLONE_FL_clone3);
}

control EgressPipeImpl (inout parsed_headers_t hdr,
                        inout local_metadata_t local_metadata,
                        inout standard_metadata_t standard_metadata) {
    apply {
        if (local_metadata.perserv_CPU_meta.egress_port == CPU_PORT) { 
		    hdr.packet_in.setValid();
		    hdr.packet_in.ingress_port = local_metadata.perserv_CPU_meta.ingress_port;		
        }

        if (local_metadata.is_multicast == true
             && standard_metadata.ingress_port == standard_metadata.egress_port) {
            mark_to_drop(standard_metadata);
        }
    }
}

the new struct

const bit<8> CLONE_FL_clone3  = 3; 
struct preserving_metadata_CPU_t {
    @field_list(CLONE_FL_clone3)
    bit<9> ingress_port;
    @field_list(CLONE_FL_clone3)
    bit<9> egress_port;
}

Now the switches seem to be setting up correctly in the ONOS GUI, but they are failing to establish the links between them, I imagine it’s because the 1º if() is getting triggered when it shouldn’t and is replacing the “hdr.packet_in.ingress_port” with the value that belonged to the previous packet cloned to the CPU, is this theory correct?

PS: I tried using a flag inside the struct to test it out and it did not solve the problem.
When the packet came to the igress, I seted the flag to 0, when copying the values to the struct seted the flag to 1 and at egress, added to the if condition that the value must also be 1, then I seted it back to 0.

const bit<8> CLONE_FL_clone3  = 3; 
struct preserving_metadata_CPU_t {
    @field_list(CLONE_FL_clone3)
    bit<9> ingress_port;
    @field_list(CLONE_FL_clone3)
    bit<9> egress_port;
    @field_list(CLONE_FL_clone3)
    bit<1> to_cpu;              //to_cpu = 1 means the packet is cloned to CPU
}
4

Sorry if my first suggestion may have been too quick and not well thought out.

In general, one possible goal is to change the program so that you use the new clone_preserving_field_list instead of clone3, and all possible packets that can reach egress processing will behave the same way they did with the original version of the program.

That might be a somewhat tricky case analysis. Here are the possible kinds of packets that reach egress processing:

(a) normal packets that during ingress processing were sent to a single output port by performing an assignment like standard_metadata.egress_spec = some_port. It is possible that such packets could be sent to the CPU_PORT, but whether your P4 program is ever doing so, can only be determined by carefully reading and thinking about the possible cases for ingress processing. Such packets should have all of the standard_metadata fields and user-defined metadata fields preserved for you, without you having to write any special code to do so. Such packets should begin egress processing with standard_metadata.instance_type == NORMAL (for constant definitions you cane use to compare instance_type against, see p4-guide/v1model-special-ops/v1model-special-ops.p4 at master · jafingerhut/p4-guide · GitHub )

(b) packets sent to a multicast group by performing an assignment like standard_metadata.mcast_grp = some_expression. Like case (a), these packets should begin egress processing with all header changes and metadata changes preserved, and instance_type == REPLICATION.

(c) Case (a) or (b), but also the packet is ingress-to-egress cloned during ingress processing. Such cloned packets should begin egress processing with none of the header changes preserved, and only the user-defined metadata fields preserved that are annotated with the corresponding @field_list annotation. They will have instance_type == INGRESS_CLONE.

So it is not immediately obvious to me the best way to ensure that your new egress processing code behaves the same as the original in all of these cases, but one way might be to structure it like this:

if (standard_metadata.instance_type == INGRESS_CLONE) {
    // write code here that works the same was as the original, except it uses
    // only metadata field values that you have explicitly preserved.
} else if ((standard_metadata.instance_type == NORMAL) || (standard_metadata.instance_type == REPLICATION))  {
    // Put a copy of the original egress code here, which seems to have been
    // written assuming that all standard_metadata fields were preserved, which
    // should be the case for NORMAL packets.
} else {
    // Not clear to me whether you need any further branches to handle other
    // cases of the value of instance_type, but if. you want to be cautious
    // I would put a log_msg() extern call here that prints a special message
    // you can easily 'grep' for in the log files to see if this ever happens.
}
5

Should I just copy the constants that are _typy_something? these ones is that enough?

//transition from clone3()
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_NORMAL        = 0;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_INGRESS_CLONE = 1;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_EGRESS_CLONE  = 2;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_COALESCED     = 3;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_RECIRC        = 4;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_REPLICATION   = 5;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_RESUBMIT      = 6;

The conversion became like this, the links are still not being established between the switches, I did not try the “log_msg()” but I will still do it once I can, you are right there is still something missing, can you just check if this implementation is what you suggested in the previous reply?

if (standard_metadata.instance_type == BMV2_V1MODEL_INSTANCE_TYPE_INGRESS_CLONE) {
    // write code here that works the same was as the original, except it uses
    // only metadata field values that you have explicitly preserved.
    if (local_metadata.perserv_CPU_meta.egress_port == CPU_PORT) { //packets for the controller, being a cloned packet we look at the struct
        hdr.packet_in.setValid();
        hdr.packet_in.ingress_port = local_metadata.perserv_CPU_meta.ingress_port;
    }
    if (local_metadata.is_multicast == true
        && standard_metadata.ingress_port == standard_metadata.egress_port) {
        mark_to_drop(standard_metadata);
    }
} else if ((standard_metadata.instance_type == BMV2_V1MODEL_INSTANCE_TYPE_NORMAL) || (standard_metadata.instance_type == BMV2_V1MODEL_INSTANCE_TYPE_REPLICATION))  {
    // Put a copy of the original egress code here, which seems to have been
    // written assuming that all standard_metadata fields were preserved, which
    // should be the case for NORMAL packets.
    if (standard_metadata.egress_port == CPU_PORT) {
        hdr.packet_in.setValid();
        hdr.packet_in.ingress_port = standard_metadata.ingress_port;		
    }
    if (local_metadata.is_multicast == true
        && standard_metadata.ingress_port == standard_metadata.egress_port) {
        mark_to_drop(standard_metadata);
    }
} else {
    // Not clear to me whether you need any further branches to handle other
    // cases of the value of instance_type, but if. you want to be cautious
    // I would put a log_msg() extern call here that prints a special message
    // you can easily 'grep' for in the log files to see if this ever happens.
}
6

Almost. You have one occurrence in an if condition of standard_metadata.ingress_port == standard_metadata.egress_port in your first branch for handling ingress-cloned packets. I would recommend replacing that with local_metadata.perserv_CPU_meta.ingress_port == local_metadata.perserv_CPU_meta.egress_port, because I do not know whether standard_metadata fields are correctly preserved for ingress-cloned packets, or not. Both of those fields should be marked with a @field_list(some_number) annotation in your definition of the type of struct that perserv_CPU_meta is defined as, and that some_number should be the number given as the field list argument to clone_preserving_field_list call.

For cloned packets, you should probably do local_metadata.perserv_CPU_meta.egress_port = CPU_PORT in your ingress code, wherever you call clone_preserving_field_list, because standard_metadata.egress_port does not yet have the value of the packet’s output port during ingress processing. It is not updated for a packet until just before egress processing begins.

7

That was it the problem was solved, the links are now established between the switches and the hosts are detected by the switches and are capable of communicating between themselves, thank you very much for your help. : )

8

Excellent.

Are you a contributor to the p4-srv6 project you linked to in your original question?

Or if not, would you be willing to create a public fork of it with your changes that you believe improve on the original code?

Either of those approaches would help others in the future who want to use that project.

9

I am not a contributor, so I will just create a fork, this version can use newer P4c versions because it no longer relies on clone3().