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

Add support for parser value sets (PVS) to runtime_CLI #859

Merged
merged 1 commit into from Feb 3, 2020

Conversation

antoninbas
Copy link
Member

We now provide the following commands: pvs_add, pvs_remove, pvs_get,
pvs_clear. Implementing pvs_get and pvs_clear required adding the
equivalent RPCs to the Thrift API.

This commit also adds a bmv2 JSON sanity check for the "mask" attribute,
when it is used in a PVS parser transition.

The included JSON file, pvs_struct_2.json, was used to test the
implementation of the CLI commands. It was generated from a P4 program
written by @jafingerhut for the compiler testsuite.

Fixes #855

@codecov-io
Copy link

codecov-io commented Feb 2, 2020

Codecov Report

Merging #859 into master will decrease coverage by 0.19%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #859     +/-   ##
=========================================
- Coverage   74.55%   74.36%   -0.2%     
=========================================
  Files         115      115             
  Lines       10207    10235     +28     
=========================================
+ Hits         7610     7611      +1     
- Misses       2597     2624     +27
Impacted Files Coverage Δ
include/bm/bm_sim/parser.h 100% <ø> (ø) ⬆️
include/bm/bm_sim/context.h 66.66% <ø> (ø) ⬆️
include/bm/bm_sim/runtime_interface.h 0% <ø> (ø) ⬆️
src/bm_sim/parser.cpp 93.92% <0%> (-1.16%) ⬇️
src/bm_sim/P4Objects.cpp 79.71% <0%> (-0.35%) ⬇️
src/bm_sim/context.cpp 34.39% <0%> (-0.95%) ⬇️
include/bm/bm_sim/switch.h 1.29% <0%> (-0.04%) ⬇️
src/bm_sim/learning.cpp 82.06% <0%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 570cbeb...1b74a02. Read the comment docs.

@hesingh
Copy link
Contributor

hesingh commented Feb 2, 2020

The JSON generated by bmv2 has a unique id for each parse_vset. Why don't you use the id to search before add or delete?

Thanks for working on these changes so quick.

```
When adding or removing an entry, you must provide a single integer value, which
must fit within the value set's "compressed bitwidth", or the CLI will display
an error. When a value set's entries consitute of multiple individual fields, as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "consist of" instead of "consitute of" ? "consitute" is not a word, in any case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks for pointing this out

@jafingerhut
Copy link
Contributor

jafingerhut commented Feb 3, 2020

I can look into this in more detail to see if I can figure it out, but it is currently confusing me how this implementation can work, when the "compile time constant" entries of parser transitions of a select must have each field padded out to a whole number of bytes, but the parser value set entries must not be padded out to a whole number of bytes.

For example, if there are two fields in a select expression like select (a[5:0], b[2:0])`, every transition in the BMv2 JSON file that are not parser value sets will have two full bytes to specify the value and mask, with a[5:0] in the most significant byte, and 2 padding bits in that byte's most significant bits, and b[2:0] in the least significant byte, and 5 padding bits in that byte's most significant bits.

How does simple_switch know to take a 6+3=9-bit value from a pvs_add command, and where the boundary between the two fields is?

@jafingerhut
Copy link
Contributor

OK, looking more closely, I guess the value of the BMv2 JSON key "transition_key" is a list of fields that each have an exact bit width that need not be a multiple of 8, and matches what is in the P4 source code. That answers my question of how simple_switch can have enough information to interpret a compressed bit string correctly, and also the other constant select expressions in the BMv2 JSON file.

We now provide the following commands: pvs_add, pvs_remove, pvs_get,
pvs_clear. Implementing pvs_get and pvs_clear required adding the
equivalent RPCs to the Thrift API.

This commit also adds a bmv2 JSON sanity check for the "mask" attribute,
when it is used in a PVS parser transition.

The included JSON file, pvs_struct_2.json, was used to test the
implementation of the CLI commands. It was generated from a P4 program
written by @jafingerhut for the compiler testsuite.

Fixes #855
@antoninbas antoninbas force-pushed the antonin/add-support-for-pvs-to-runtime-CLI branch from 9329021 to 1b74a02 Compare February 3, 2020 18:21
@antoninbas
Copy link
Member Author

@jafingerhut yes that's correct. And I believe the rationale for the current implementation is that it was introduced for P4_14 parser value sets, which were initialized with a single bitwidth, and while you could combine multiple fields when performing a lookup on the set, the declaration of the pvs itself didn't impose any structure.

I think the PVS implementation in bmv2 could benefit from a major overhaul (maybe #857 should be a more general issue). The implementation really caters to P4_14 at the moment. It even accommodates for the case where a single PVS is used in 2 different parser states with different lookup keys.

@antoninbas
Copy link
Member Author

antoninbas commented Feb 3, 2020

To illustrate what I meant at the end of my previous comment. I believe the following program is a valid P4_14 program but cannot be translated easily in a P4_16 program and is rejected by p4c:

// Standard L2 Ethernet header
header_type ethernet_t {
    fields {
        dst_addr        : 48; // width in bits
        src_addr        : 48;
        ethertype       : 16;
    }
}

header ethernet_t ethernet;

header_type my_header_t {
    fields {
        f1 : 3;
        f2 : 13;
    }
}

header my_header_t my_header;

parser_value_set pv1;
parser_value_set pv2;

header_type my_meta_t {
    fields {
        pv_parsed : 8;
    }
}

metadata my_meta_t my_meta;

parser start {
    extract(ethernet);
    return select(ethernet.ethertype) {
        0x0800 : ingress;
        pv1 : parse_pv1;
        pv2 : parse_pv2;
        default : ingress;
    }
}

parser parse_pv1 {
    set_metadata(my_meta.pv_parsed, 1);
    extract(my_header);
    return select(my_header.f1, my_header.f2) {
        pv2 : parse_pv2;
        default : ingress;
    }
}

parser parse_pv2 {
    set_metadata(my_meta.pv_parsed, 2);
    return ingress;
}

action route_eth(egress_spec, src_addr) {
    modify_field(standard_metadata.egress_spec, egress_spec);
    modify_field(ethernet.src_addr, src_addr);
}
action noop() { }

table routing {
    reads {
	ethernet.dst_addr : lpm;
    }
    actions {
	route_eth;
	noop;
    }
}

control ingress {
    apply(routing);
}

P4_16 requires pvs2 to be mapped to 2 different P4_16 value_set instances, and because the compiler wants to give them the same name, compilation of the P4_16 translation fails.

@antoninbas antoninbas merged commit b2b8666 into master Feb 3, 2020
@antoninbas antoninbas deleted the antonin/add-support-for-pvs-to-runtime-CLI branch February 3, 2020 19:50
@mihaibudiu
Copy link

You should submit the p4-14 program as an issue to p4c so we can fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support parser value sets in runtime_CLI
5 participants