8000 P4TC - Deparser: write only (possibly-)changed header fields by vbnogueira · Pull Request #4598 · p4lang/p4c · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

P4TC - Deparser: write only (possibly-)changed header fields #4598

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

P4TC - Deparser: write only (possibly-)changed header fields #4598

wants to merge 1 commit into from

Conversation

vbnogueira
Copy link
Contributor
@vbnogueira vbnogueira commented Apr 4, 2024 •
edited
Loading

Leverage compiler to write only (possibly-)changed header fields in deparser instead of always rewriting all of them

For example, in the simple program below:

control ingress(
    inout my_ingress_headers_t  hdr,
    inout my_ingress_metadata_t meta,
    in    pna_main_input_metadata_t  istd,
    inout pna_main_output_metadata_t ostd
)
{
   action send_nh(@tc_type("dev") PortId_t port, @tc_type("macaddr") bit<48> srcMac, @tc_type("macaddr") bit<48> dstMac) {
        hdr.ethernet.srcAddr = srcMac;
        hdr.ethernet.dstAddr = dstMac;
        send_to_port(port);
   }

   action drop() {
        drop_packet();
   }

   table nh_table {
        key = {
            hdr.ipv4.dstAddr : exact @tc_type("ipv4") @name("dstAddr");
        }
        actions = {
            send_nh;
            drop;
        }
        size = L3_TABLE_SIZE;
        const default_action = drop;
    }

    apply {
            nh_table.apply();
    }
   }
}

    /*********************  D E P A R S E R  ************************/

control Ingress_Deparser(
    packet_out pkt,
    inout    my_ingress_headers_t hdr,
    in    my_ingress_metadata_t meta,
    in    pna_main_output_metadata_t ostd)
{
    apply {
        pkt.emit(hdr.ethernet);
        pkt.emit(hdr.ipv4);
    }
}

The compiler should not rewrite every header in the deparser stage, just the ones that could be overwritten by the program itself, in the above example those would be source and destination mac addresses

This version has some debugging output, but we have a second patch that removes those

@vbnogueira vbnogueira changed the title Deparser: write only (possibly-)changed header fields P4TC - Deparser: write only (possibly-)changed header fields Apr 4, 2024
@vbnogueira vbnogueira force-pushed the deparser-optimisation branch from 4ac4911 to 7b8cf6c Compare April 4, 2024 15:54
@mouse-mt @vbnogueira
Deparser: write only (possibly-)changed header fields
6b38d4a 
Leverage compiler to write only (possibly-)changed header fields in
deparser instead of always rewriting all of them
@vbnogueira vbnogueira force-pushed the deparser-optimisation branch from 7b8cf6c to 6b38d4a Compare April 4, 2024 15:56
@fruffy fruffy added the p4tc Topics related to the P4-TC back end. On PRs, also triggers p4tc CI tests to run. label Apr 4, 2024
osinstom
osinstom reviewed Apr 24, 2024
View reviewed changes
Copy link
Contributor
@osinstom osinstom left a comment

Choose a reason for hiding this comment

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

I have some comments about the code itself, but I'd like to clarify the proposed feature first.

In general, I agree that we perform some redundant/useless write operations in the deparser when a field is not touched. The current approach is generalized for simplicity (at the cost of additional eBPF operations). The proposed solution can work perfectly fine when there is no encap/decap in a P4 program. Have you considered the encap/decap scenario? If the entire header changes its position in a packet, we must write all fields.

@thomascalvert-xlnx
Copy link
Member

I think this is a really cool improvement! Have you considered making it part of the core eBPF backend, so that it can benefit all of the dependent backends (tc, psa, ubpf, xdp, etc.) ?

@fruffy fruffy added the run-sanitizer Use this tag to run a Clang+Sanitzers CI run. label Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osinstom osinstom osinstom left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
p4tc Topics related to the P4-TC back end. On PRs, also triggers p4tc CI tests to run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vbnogueira @thomascalvert-xlnx @osinstom @fruffy @mouse-mt
0