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.

Sign up for GitHub

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

Remove old intrinsic_metadata fields #1852

Merged
merged 1 commit into from
Apr 16, 2019
Merged

Remove old intrinsic_metadata fields #1852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion testdata/p4_14_samples/TLV_parsing.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
}
}
Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/copy_to_cpu.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
}
}
Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/counter.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
}
}
Expand Down
2 changes: 1 addition & 1 deletion testdata/p4_14_samples/issue1058.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ header_type meta_t {
// declare the special timestamp metadata
header_type intrinsic_metadata_t {
fields {
ingress_global_tstamp : 48;
ingress_global_timestamp : 48;
}
}

Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/meter.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
}
}
Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/meter1.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
}
}
Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/packet_redirect.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
ingress_global_timestamp : 64;
resubmit_flag : 16;
Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/register.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
}
}
Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/resubmit.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list : 32;
resubmit_flag : 16;
}
Expand Down
5 changes: 1 addition & 4 deletions testdata/p4_14_samples/sai_p4.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ header_type ingress_intrinsic_metadata_t {
fields {
ingress_port : 9; // ingress physical port id.
lf_field_list : 32; // hack for learn filter.
ucast_egress_port : 9; // outgoing port
}
}
metadata ingress_intrinsic_metadata_t intrinsic_metadata;
Expand Down Expand Up @@ -382,7 +381,6 @@ table learn_notify {

action fdb_set(type_, port_id) {
modify_field(ingress_metadata.mac_type, type_);
modify_field(intrinsic_metadata.ucast_egress_port, port_id);
modify_field(standard_metadata.egress_spec, port_id);
modify_field(ingress_metadata.routed, 0);
}
Expand Down Expand Up @@ -556,7 +554,7 @@ table cos_map {
action set_router_interface(virtual_router_id, type_, port_id, vlan_id, src_mac_address, admin_v4_state, admin_v6_state, mtu) {
modify_field(ingress_metadata.vrf, virtual_router_id);
modify_field(ingress_metadata.interface_type, type_);
modify_field(intrinsic_metadata.ucast_egress_port, port_id);
modify_field(standard_metadata.egress_spec, port_id);
modify_field(ingress_metadata.vlan_id, vlan_id);
modify_field(ingress_metadata.def_smac, src_mac_address);
modify_field(ingress_metadata.v4_enable, admin_v4_state);
Expand Down Expand Up @@ -598,7 +596,6 @@ table virtual_router {
action set_dmac(dst_mac_address, port_id) {
modify_field(eth.dstAddr, dst_mac_address);
modify_field(eth.srcAddr, ingress_metadata.def_smac);
modify_field(intrinsic_metadata.ucast_egress_port, port_id);
modify_field(standard_metadata.egress_spec, port_id);
}

Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples/simple_nat.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ header_type intrinsic_metadata_t {
fields {
mcast_grp : 4;
egress_rid : 4;
mcast_hash : 16;
lf_field_list: 32;
}
}
Expand Down
2 changes: 2 additions & 0 deletions testdata/p4_14_samples/switch_20160226/acl.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,9 @@ action egress_copy_to_cpu(reason_code) {
table egress_acl {
reads {
standard_metadata.egress_port : ternary;
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
intrinsic_metadata.deflection_flag : ternary;
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
l3_metadata.l3_mtu_check : ternary;
}
actions {
Expand Down
4 changes: 4 additions & 0 deletions testdata/p4_14_samples/switch_20160226/hashes.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,13 +163,17 @@ table compute_non_ip_hashes {
}

action computed_two_hashes() {
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(intrinsic_metadata.mcast_hash, hash_metadata.hash1);
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(hash_metadata.entropy_hash, hash_metadata.hash2);
}

action computed_one_hash() {
modify_field(hash_metadata.hash1, hash_metadata.hash2);
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(intrinsic_metadata.mcast_hash, hash_metadata.hash2);
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(hash_metadata.entropy_hash, hash_metadata.hash2);
}

Expand Down
38 changes: 20 additions & 18 deletions testdata/p4_14_samples/switch_20160226/includes/intrinsic.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,44 +19,46 @@ header_type ingress_intrinsic_metadata_t {
resubmit_flag : 1; // flag distinguishing original packets
// from resubmitted packets.

ingress_global_tstamp : 48; // global timestamp (ns) taken upon
ingress_global_timestamp : 48; // global timestamp (ns) taken upon
// arrival at ingress.

mcast_grp : 16; // multicast group id (key for the
// mcast replication table)

#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove that macro and the #ifdef and corresponding #endif, then three of the old intrinsic_metadata fields remain in the post-preprocessed program, and these changes are attempting to prevent that from happening, because simple_switch has no implementation of these fields.

I could delete that code entirely, but the reason I introduced a new macro was because I was thinking that perhaps someone somewhere might want to see the source code that accessed these old fields, and re-enable it.

If folks think that is not going to happen, I could simply delete the few dozen lines of code that are currently between such #ifdef / #endif pairs in the current proposed changes.

deflection_flag : 1; // flag indicating whether a packet is
// deflected due to deflect_on_drop.
deflect_on_drop : 1; // flag indicating whether a packet can
// be deflected by TM on congestion drop

enq_qdepth : 19; // queue depth at the packet enqueue
// time.
enq_tstamp : 32; // time snapshot taken when the packet
// is enqueued (in nsec).
enq_congest_stat : 2; // queue congestion status at the packet
// enqueue time.

deq_qdepth : 19; // queue depth at the packet dequeue
// time.
deq_congest_stat : 2; // queue congestion status at the packet
// dequeue time.
deq_timedelta : 32; // time delta between the packet's
// enqueue and dequeue time.

mcast_hash : 13; // multicast hashing
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
egress_rid : 16; // Replication ID for multicast
lf_field_list : 32; // Learn filter field list
priority : 3; // set packet priority
}
}
metadata ingress_intrinsic_metadata_t intrinsic_metadata;

#define _ingress_global_tstamp_ intrinsic_metadata.ingress_global_tstamp
#define _ingress_global_tstamp_ intrinsic_metadata.ingress_global_timestamp

action deflect_on_drop() {
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(intrinsic_metadata.deflect_on_drop, 1);
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
}

header_type queueing_metadata_t {
fields {
enq_timestamp : 48; // time snapshot taken when the packet
// is enqueued (in microsec).
enq_qdepth : 16; // queue depth at the packet enqueue
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if they are here or in intrinsic_metadata it's rather the same thing; they won't be handled correctly by the compiler if they are expected to have values when the pipeline starts. We still need to move them to stdandard_metadata in v1model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but at least this is the name of the metadata "header" that simple_switch documentation has said that these fields must be defined in a P4_14 program for several years now, here: https://github.com/p4lang/behavioral-model/blob/master/docs/simple_switch.md

Copy link
Contributor Author

@jafingerhut jafingerhut Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also, these fields are initialized by simple_switch today.

EDIT: I mean, they are if they have the expected correct names in the BMv2 JSON file, which they do not today because of the related PR #1704 or something similarly effective goes through, and a similar change is made for queueing_metadata, then simple_switch initializes these fields correctly today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but the way the P4-14 to P4-16 translates this struct will not enable it to use the simple_switch values. We essentially need to put these fields in standard_metadata and make the translator refer to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The simple_switch documentation about queueing_metadata is the reason I asked about it in this comment earlier: #1704 (comment)

You replied you had no example programs that used queueing_metadata: #1704 (comment)

I just did some grep'ing through the code and found this test program that uses queueing_metadata, that was committed to the code in 2016 and hasn't been modified since then: https://github.com/p4lang/p4c/blob/master/testdata/p4_14_samples/queueing.p4

Please let me know if there is something you would like changed in this PR, though. I am fine with whatever on this P4_14 intrinsic_metadata and queueing_metadata stuff. It has been documented for years as working this way, then broken some time around Nov 2018. Just looking for a path to getting back to working here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, if part of the path to getting things working is to change documentation that has been the same for a couple of years, I have no problem with that, either. Just nice if the documentation and implementation are consistent with each other, is all.

// time.
deq_timedelta : 32; // time delta between the packet's
// enqueue and dequeue time.
deq_qdepth : 16; // queue depth at the packet dequeue
// time.
}
}
metadata queueing_metadata_t queueing_metadata;

#define PKT_INSTANCE_TYPE_NORMAL 0
#define PKT_INSTANCE_TYPE_INGRESS_CLONE 1
Expand Down
4 changes: 2 additions & 2 deletions testdata/p4_14_samples/switch_20160226/int_transit.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,13 @@ action int_set_header_1() { /* ingress_port_id */
action int_set_header_2() { /* hop_latency */
add_header(int_hop_latency_header);
modify_field(int_hop_latency_header.hop_latency,
intrinsic_metadata.deq_timedelta);
queueing_metadata.deq_timedelta);
}
/* Instr Bit 3 */
action int_set_header_3() { /* q_occupancy */
add_header(int_q_occupancy_header);
modify_field(int_q_occupancy_header.q_occupancy,
intrinsic_metadata.enq_qdepth);
queueing_metadata.enq_qdepth);
}
/* Instr Bit 4 */
action int_set_header_4() { /* ingress_tstamp */
Expand Down
5 changes: 4 additions & 1 deletion testdata/p4_14_samples/switch_20160226/switch.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,10 @@ control egress {
} else {
#endif /* OPENFLOW_ENABLE */
/* check for -ve mirrored pkt */
if ((intrinsic_metadata.deflection_flag == FALSE) and
if (
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
(intrinsic_metadata.deflection_flag == FALSE) and
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
(egress_metadata.bypass == FALSE)) {

/* check if pkt is mirrored */
Expand Down
2 changes: 2 additions & 0 deletions testdata/p4_14_samples/switch_20160512/acl.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,9 @@ action egress_copy_to_cpu(reason_code) {
table egress_acl {
reads {
standard_metadata.egress_port : ternary;
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
intrinsic_metadata.deflection_flag : ternary;
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
l3_metadata.l3_mtu_check : ternary;
}
actions {
Expand Down
4 changes: 4 additions & 0 deletions testdata/p4_14_samples/switch_20160512/hashes.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,17 @@ table compute_non_ip_hashes {
}

action computed_two_hashes() {
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(intrinsic_metadata.mcast_hash, hash_metadata.hash1);
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(hash_metadata.entropy_hash, hash_metadata.hash2);
}

action computed_one_hash() {
modify_field(hash_metadata.hash1, hash_metadata.hash2);
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(intrinsic_metadata.mcast_hash, hash_metadata.hash2);
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(hash_metadata.entropy_hash, hash_metadata.hash2);
}

Expand Down
38 changes: 20 additions & 18 deletions testdata/p4_14_samples/switch_20160512/includes/intrinsic.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,45 +19,47 @@ header_type ingress_intrinsic_metadata_t {
resubmit_flag : 1; // flag distinguishing original packets
// from resubmitted packets.

ingress_global_tstamp : 48; // global timestamp (ns) taken upon
ingress_global_timestamp : 48; // global timestamp (ns) taken upon
// arrival at ingress.

mcast_grp : 16; // multicast group id (key for the
// mcast replication table)

#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
deflection_flag : 1; // flag indicating whether a packet is
// deflected due to deflect_on_drop.
deflect_on_drop : 1; // flag indicating whether a packet can
// be deflected by TM on congestion drop

enq_qdepth : 19; // queue depth at the packet enqueue
// time.
enq_tstamp : 32; // time snapshot taken when the packet
// is enqueued (in nsec).
enq_congest_stat : 2; // queue congestion status at the packet
// enqueue time.

deq_qdepth : 19; // queue depth at the packet dequeue
// time.
deq_congest_stat : 2; // queue congestion status at the packet
// dequeue time.
deq_timedelta : 32; // time delta between the packet's
// enqueue and dequeue time.

mcast_hash : 13; // multicast hashing
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
egress_rid : 16; // Replication ID for multicast
lf_field_list : 32; // Learn filter field list
priority : 3; // set packet priority
}
}
metadata ingress_intrinsic_metadata_t intrinsic_metadata;

#define _ingress_global_tstamp_ intrinsic_metadata.ingress_global_tstamp
#define _ingress_global_tstamp_ intrinsic_metadata.ingress_global_timestamp
#define modify_field_from_rng(_d, _w) modify_field_rng_uniform(_d, 0, (1<<(_w))-1)

action deflect_on_drop(enable_dod) {
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
modify_field(intrinsic_metadata.deflect_on_drop, enable_dod);
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
}

header_type queueing_metadata_t {
fields {
enq_timestamp : 48; // time snapshot taken when the packet
// is enqueued (in microsec).
enq_qdepth : 16; // queue depth at the packet enqueue
// time.
deq_timedelta : 32; // time delta between the packet's
// enqueue and dequeue time.
deq_qdepth : 16; // queue depth at the packet dequeue
// time.
}
}
metadata queueing_metadata_t queueing_metadata;

#define PKT_INSTANCE_TYPE_NORMAL 0
#define PKT_INSTANCE_TYPE_INGRESS_CLONE 1
Expand Down
4 changes: 2 additions & 2 deletions testdata/p4_14_samples/switch_20160512/int_transit.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ action int_set_header_1() { //ingress_port_id
action int_set_header_2() { //hop_latency
add_header(int_hop_latency_header);
modify_field(int_hop_latency_header.hop_latency,
intrinsic_metadata.deq_timedelta);
queueing_metadata.deq_timedelta);
}
/* Instr Bit 3 */
action int_set_header_3() { //q_occupancy
add_header(int_q_occupancy_header);
modify_field(int_q_occupancy_header.q_occupancy1, 0);
modify_field(int_q_occupancy_header.q_occupancy0,
intrinsic_metadata.enq_qdepth);
queueing_metadata.enq_qdepth);
}
/* Instr Bit 4 */
action int_set_header_4() { //ingress_tstamp
Expand Down
5 changes: 4 additions & 1 deletion testdata/p4_14_samples/switch_20160512/switch.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,10 @@ control egress {
} else {
#endif /* OPENFLOW_ENABLE */
/* check for -ve mirrored pkt */
if ((intrinsic_metadata.deflection_flag == FALSE) and
if (
#ifdef INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
(intrinsic_metadata.deflection_flag == FALSE) and
#endif // INCLUDE_OLD_INTRINSIC_METADATA_FIELDS
(egress_metadata.bypass == FALSE)) {

/* check if pkt is mirrored */
Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples_outputs/TLV_parsing-first.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ header ipv4_option_timestamp_t_1 {
struct intrinsic_metadata_t {
bit<4> mcast_grp;
bit<4> egress_rid;
bit<16> mcast_hash;
bit<32> lf_field_list;
}

Expand Down
1 change: 0 additions & 1 deletion testdata/p4_14_samples_outputs/TLV_parsing-frontend.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ header ipv4_option_timestamp_t_1 {
struct intrinsic_metadata_t {
bit<4> mcast_grp;
bit<4> egress_rid;
bit<16> mcast_hash;
bit<32> lf_field_list;
}

Expand Down
Loading