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

Jump to bottom

Fixing bug in parserUnroll #3503

Merged
merged 4 commits into from
Sep 9, 2022
Merged

Fixing bug in parserUnroll #3503

merged 4 commits into from
Sep 9, 2022

Conversation

VolodymyrPeschanenkoLitSoft
Copy link
Contributor

These changes allow to unroll loops in the following examples: issue692-bmv2, issue2090, gauntlet_parser_test2, gautlet_parser_test4, spec-ex19.
To check, please, run the following command:
./backends/p4test/p4test -I "./p4include" --target bmv2 --std p4-16 --loopsUnroll --arch v1model "../testdata/p4_16_samples/example_name.p4"

mihaibudiu
mihaibudiu approved these changes Aug 31, 2022
View reviewed changes
Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

I am approving, but please make these small changes.

midend/parserUnroll.cpp Show resolved Hide resolved
midend/parserUnroll.cpp Show resolved Hide resolved
@mihaibudiu mihaibudiu merged commit c5b9873 into main Sep 9, 2022
@mihaibudiu mihaibudiu deleted the vp/puBug branch September 9, 2022 18:52
@kfcripps
Copy link
Contributor

@VolodymyrPeschanenkoLitSoft @mbudiu-vmw After this PR, the following parser IR:

...
state start {
        packet.extract<h1_t>(h1);
        packet.extract<h2_t>(h2);
        transition select(h2.etherType) {
            16w0x800: parse_ipv4;
            16w0x86dd: parse_ipv6;
            default: accept;
        }
    }
    state parse_ipv4 {
        packet.extract<h3_t>(h3);
        transition select(h3.fragOffset, h3.protocol) {
            (13w0, 8w6): parse_tcp;
            (13w0, 8w17): parse_udp;
            default: accept;
        }
    }
    state parse_ipv6 {
        packet.extract<h4_t>(h4);
        transition select(h4.nextHdr) {
            8w6: parse_tcp;
            8w17: parse_udp;
            default: accept;
        }
    }
    state parse_tcp {
        packet.extract<h5_t>(h5);
        transition accept;
    }
    state parse_udp {
        packet.extract<h6_t>(h6);
        transition select(h6.checksum) {
            16w0xcafe: parse_ipv4;
            16w0xffff: reject;
            default: accept;
        }
    }

is transformed into:

...
state stateOutOfBound {
        transition reject;
    }
    state start {
        packet.extract<h1_t>(h1);
        packet.extract<h2_t>(h2);
        transition select(h2.etherType) {
            16w0x800: parse_ipv4;
            16w0x86dd: parse_ipv6;
            default: accept;
        }
    }
    state parse_ipv4 {
        packet.extract<h3_t>(h3);
        transition select(h3.fragOffset, h3.protocol) {
            (13w0, 8w6): parse_tcp;
            (13w0, 8w17): parse_udp1;
            default: accept;
        }
    }
    state parse_ipv41 {
        packet.extract<h3_t>(h3);
        transition select(h3.fragOffset, h3.protocol) {
            (13w0, 8w6): parse_tcp;
            (13w0, 8w17): parse_udp2;
            default: accept;
        }
    }
    state parse_ipv6 {
        packet.extract<h4_t>(h4);
        transition select(h4.nextHdr) {
            8w6: parse_tcp;
            8w17: parse_udp;
            default: accept;
        }
    }
    state parse_tcp {
        packet.extract<h5_t>(h5);
        transition accept;
    }
    state parse_udp {
        packet.extract<h6_t>(h6);
        transition select(h6.checksum) {
            16w0xcafe: parse_ipv4;
            16w0xffff: reject;
            default: accept;
        }
    }
    state parse_udp1 {
        packet.extract<h6_t>(h6);
        transition select(h6.checksum) {
            16w0xcafe: parse_ipv41;
            16w0xffff: reject;
            default: accept;
        }
    }
    state parse_udp2 {
        transition stateOutOfBound;
    }

Is this expected, and if it is, could you please help me understand why this is desirable? Thanks.

@kfcripps
Copy link
Contributor

kfcripps commented Sep 26, 2022 •
edited
Loading

Also, could you please explain in a bit more detail what this PR is trying to achieve? I don't see any changes to expectations in the unit tests, so wasn't sure exactly what bug this PR is attempting to fix.

@mihaibudiu
Copy link
Contributor

Loop unrolling attempts to rewrite a parser with cycles in the state graph into a parser without cycles.
Some targets may not support cycles, e.g., older versions of ebpf (without tail calls).
This is not always possible, so the optimization is not always done, it is conditional on a command-line option.
The standard tests we run do not enable this option, that's why you don't see changes to unit tests.

@kfcripps
Copy link
Contributor

kfcripps commented Sep 26, 2022 •
edited
Loading

In this case though, isn't the transformed parser IR semantically different than the original IR? With the original parser, the following is a possible path through the states: start -> parse_ipv4 -> parse_udp -> parse_ipv4 -> parse_udp -> accept
However, we now end up in the reject instead of accept state when parsing an identical packet.

@mihaibudiu
Copy link
Contributor

It does look wrong, perhaps the code treats h6 as a 1-element stack.

@mihaibudiu
Copy link
Contributor

You should file an issue with a full P4 program that can be used to reproduce this issue.

@VolodymyrPeschanenkoLitSoft
Copy link
Contributor Author

Also, could you please explain in a bit more detail what this PR is trying to achieve? I don't see any changes to expectations in the unit tests, so wasn't sure exactly what bug this PR is attempting to fix.

@kfcripps, If such a path is possible then it looks like a bug. If you attach the full example to a new issue. I'll check it tomorrow morning in Ukrainian time.

@kfcripps
Copy link
Contributor

Will do, just wanted to make sure that I wasn't misunderstanding something before opening an issue. Thanks for checking!

@kfcripps kfcripps mentioned this pull request Sep 26, 2022
Closed
@kfcripps
Copy link
Contributor

opened #3537

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaibudiu mihaibudiu mihaibudiu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VolodymyrPeschanenkoLitSoft @kfcripps @mihaibudiu @VolodymyrPeschanenko