Fix for default_action #1

Sosutha · 2024-02-09T08:37:22Z

tc template code change for default miss action
NoAction case generation in c code for tc backend
default case deletion in c code for tc backend
p4tc testcases output updated
https://github.com/p4tc-dev/p4tc-e2e/issues/40

syogender

Looks Good

usha1830 · 2024-02-22T06:50:54Z

backends/tc/backend.cpp

+                bool directionParamPresent = false;
+                auto paramList = actionCall->action->getParameters();
+                for (auto param : paramList->parameters) {
+                    if (param->direction != IR::Direction::None) directionParamPresent = true;


All parameters should be directionless?

For now, default action with directionless parameter is handled
For direction parameter, they are still discussing, hence we need not set any default param value until then

usha1830 · 2024-02-22T06:53:00Z

backends/tc/backend.cpp

+                            defaultParam->setDefaultValue(
+                                Util::toString(constVal->value, 0, sign, constVal->base));
+                        } else
+                            defaultParam->setDefaultValue(defaultArg->expression->toString());


What are the possible expressions? Can we avoid using toString()?

BoolLiteral or String Literal are the other possible expressions. For those, we can add else if cases if needed

Updated the code

usha1830 · 2024-02-22T06:54:24Z

backends/tc/backend.cpp

+                        auto defaultParam = new IR::TCDefaultActionParam();
+                        defaultParam->setParamName(param->name.originalName);
+                        auto defaultArg = methodexp->arguments->at(i++);
+                        if (defaultArg->expression->is<IR::Constant>()) {


Why not use if (auto constVal = defaultArg->expression->toIR::Constant())

Updated the code

usha1830 · 2024-02-22T07:00:20Z

backends/tc/tc.def

@@ -278,19 +292,6 @@ class TCTable {
                }
            }
        }
-
-        if (preaction != nullptr) {


Is this related to default action or some stale code being removed now?

Stale code removed now

usha1830 · 2024-02-22T07:01:51Z

testdata/p4tc_samples_outputs/calculator_control_blocks.c

                        }
                    } else {
-/* drop_packet() */


else part removal is expected?

Yes. else is expected to be empty to cater the case when value->action is NULL.

usha1830

Please check comments.
Is frontend emitting error when parameter with direction is used as default action? If not, we should handle it in backend and add some tests for the same.

Sosutha · 2024-02-22T07:21:36Z

Please check comments. Is frontend emitting error when parameter with direction is used as default action? If not, we should handle it in backend and add some tests for the same.

default_action with parameter is allowed by the backend and no template code change is needed for now. Once design is confirmed for those, we need to generate code accordingly

jhsmt · 2024-02-22T16:58:29Z

@Sosutha a couple of comments/observations which i dont believe P4 covers but we would need it for practical reasons. I dont think we elaborated these well before but since you are fixing this we might as well make sure. I will add a second comment after this so the text reads better. For the first one, consider:

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123);
}

In your change, the template will generate the value 123 in the template as such:

$TC p4template update table/... default_miss_action action b param y 123

And the eBPF side will be able to retrieve the value from the P4TC domain. IOW, the eBPF code will not have any "hardcoding".
This is good.

There is a usecase where we may want to override the value 123 for operational reasons. In order to override the value of "y" we need to "patch" the eBPF program at loading/instantiation time. For the "patching" to work, "y" has to be a global parameter, meaning it will be stored in the binary elf bss section which we can edit at load time.
summary: we want the ebpf code to actually specify the param "y" to be global.

It's as if the above program was written as:

bit<32> y = 123;
action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(y);
}

I am not suggesting we force the user rewrite the P4 program as above rather internally within the generated eBPF code, that's how it should be. I realize that this is ambiguous - because there are cases (probably majority) where 123 is "operationally correct". One option is to perhaps use annotations which are explicitly stated by the P4 program author as such:

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123) @tc_may_override;
}

tagging @usha1830 @komaljai @vbnogueira @tammela

jhsmt · 2024-02-22T17:39:24Z

Second comment @Sosutha (tagging: @usha1830 @komaljai @vbnogueira @tammela)

For the earlier cited annotated example:

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123) @tc_may_override;
}

At the template we would like to add a flag as such (disclaimer, we may change the specification a little, but consider that there is small syntax change here):

$TC p4template update table/... default_miss_action action b param y flags runtime

This tells us something along the lines "the default action value is determined by the datapath runtime and is NOT retrieved from the P4TC domain". IOW, the eBPF code determines the values.

Another place where this template flag would be required is in cases where the action attribute value is computed by the datapath at runtime, example:

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(z);
}
...
if (hdr.x = 5)
   z = 4;
else
  z = 3;

t.apply();

We are also considering perhaps instead of making these action attributes global within the eBPF code to rather put them in our existing filter structure you are looking at on the other issue:

struct p4tc_filter_fields { 
        __u32 pipeid;
        __u32 handle;
        __u32 classid;
        __u32 chain;
        __u32 blockid;
        __be16 proto;
        __u16 prio;
};

As an example, this structure could be dynamically generated to include these variables we determine to be global, example for the earlier use case of:

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123) @tc_may_override;
}

The compiler would generate:

struct p4tc_filter_fields { 
        __u32 pipeid;
        __u32 handle;
        __u32 classid;
        __u32 chain;
        __u32 blockid;
        __be16 proto;
        __u16 prio;
        __u32 y;
};

This would simplify our control plane code.
The important part here is we want to discuss this with you - and if it can be part of the same fix, that would be great.
If you think its too much work then we can make it a different PR. Sorry for not being very clear earlier on.

Sosutha · 2024-02-23T07:06:53Z

@jhsmt We can have these changes as separate PR.
Few points and queries on the examples given.

bit<32> y = 123;
action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(y);
}

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(z);
}
...
if (hdr.x = 5)
   z = 4;
else
  z = 3;

t.apply();

These examples are not allowed by P4 syntax. default_action should have compile time constant, variables are not allowed.
Compiler frontend will not allow variables, hence these cases can be ignored.

Now there are three queries.

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123);
}

If only the constant is specified in the P4 Program without the annotation, should we emit the value in the template as implemented now?

$TC p4template update table/... default_miss_action action b param y 123

Now the second case, where annotation is specified

action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123); @tc_may_overrride
}

Should the template look like this? It has the constant from the P4 and also the flag to mention that annotation is given in P4 that this value might change in runtime. Ignoring the constant fully and having only the flag does not make since value is given in P4. Please confirm

$TC p4template update table/... default_miss_action action b param y 123 flags runtime

For this case with annotation, as you mentioned, compiler can generate the placeholder variable inside the p4tc_filter_fields structure.

struct p4tc_filter_fields { 
        __u32 pipeid;
        __u32 handle;
        __u32 classid;
        __u32 chain;
        __u32 blockid;
        __be16 proto;
        __u16 prio;
        __u32 y;
};

Third query is a different scenario.
Consider there are two different tables with two different default actions a and b, both with same param name as y but different types. In that case, how do we generate placeholder variable name in the structure? To make it clear, Can we generate action_param name as variable name? In this case (a_y, b_y), Will this be clear? Should we use the name in template as well, so ebpf code can communicate with TC code?

jhsmt · 2024-02-23T14:00:18Z

@jhsmt We can have these changes as separate PR.

NP.

Few points and queries on the examples given.
bit<32> y = 123;
action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(y);
}
action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(z);
}
...
if (hdr.x = 5)
   z = 4;
else
  z = 3;

t.apply();
These examples are not allowed by P4 syntax. default_action should have compile time constant, variables are not allowed. Compiler frontend will not allow variables, hence these cases can be ignored.

Thanks for catching these before we code away. The first above though looks like:
https://github.com/Sosutha/p4c/pull/1/files#diff-49f9c1d028a162b115a9e6be50f34fbda8d4c15c311be8d359b10e5fcbf04722R91

Did i miss something?
Q: Can the default action value "4" in:
https://github.com/Sosutha/p4c/pull/1/files#diff-49f9c1d028a162b115a9e6be50f34fbda8d4c15c311be8d359b10e5fcbf04722R91
be overriden later by the control plane?

Now there are three queries.
action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123);
}
If only the constant is specified in the P4 Program without the annotation, should we emit the value in the template as implemented now?
$TC p4template update table/... default_miss_action action b param y 123

Yes.

Now the second case, where annotation is specified
action a(bit<32> x) { /* body omitted */ }
action b(bit<32> y) { /* body omitted */ }
table t {
   actions = {
      a;
      b;
   }
   default_action = b(123); @tc_may_overrride
}
Should the template look like this? It has the constant from the P4 and also the flag to mention that annotation is given in P4 that this value might change in runtime. Ignoring the constant fully and having only the flag does not make since value is given in P4. Please confirm
$TC p4template update table/... default_miss_action action b param y 123 flags runtime

We dont need to specify "123"

For this case with annotation, as you mentioned, compiler can generate the placeholder variable inside the p4tc_filter_fields structure.
struct p4tc_filter_fields { 
        __u32 pipeid;
        __u32 handle;
        __u32 classid;
        __u32 chain;
        __u32 blockid;
        __be16 proto;
        __u16 prio;
        __u32 y;
};

Excellent.

Third query is a different scenario.
Consider there are two different tables with two different default actions a and b, both with same param name as y but different types. In that case, how do we generate placeholder variable name in the structure? To make it clear, Can we generate action_param name as variable name? In this case (a_y, b_y), Will this be clear?

Yes, makes sense.

Should we use the name in template as well, so ebpf code can communicate with TC code?

It is not needed for the template side.

Sosutha · 2024-02-26T05:12:02Z

The first above though looks like: https://github.com/Sosutha/p4c/pull/1/files#diff-49f9c1d028a162b115a9e6be50f34fbda8d4c15c311be8d359b10e5fcbf04722R91

Did i miss something?

@jhsmt Here variable is used in the action list. P4 allows variable to be used in action list and default_action when the parameter od the action is with direction. In this case, z is a inout parameter. In the examples you mentioned, we are only considering directionless parameter.

We will raise this PR in opensource if there are no other opens. Please confirm

Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4 to 5. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v4...v5) --- updated-dependencies: - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 3 to 5. - [Release notes](https://github.com/docker/build-push-action/releases) - [Commits](docker/build-push-action@v3...v5) --- updated-dependencies: - dependency-name: docker/build-push-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Try to handle pna_main_input_metadata_t fields better * Add support for parser errors in tc backend --------- Co-authored-by: Mouse <[email protected]>

* Add aligned new / delete libgc overloads. Also cleaned some old stuff that is not relevant in year 2024 * Add nothrow overrides * A bit more clear realignment

* Try to speed up the CI build process * Update install_fedora_deps.sh * Update ci-test.yml

…4463) * Bump protobuf version. Fix some missed dependencies here and there * Grab abseil using FetchContent * Disable unity builds for Abseil * Try to bumpprotobuf version in bazel * Pull abseil * Address review comments * Fix UB in json parser that remained from GMP times * Ensure sanitizer options are propagated down to dependencies (protobuf, abseil, etc.) * Some readme refines * See if we can workaround ubsan + int128 bug * Clarify ubsan issue workaround * Fix typo in README.md Co-authored-by: Vladimír Štill <[email protected]> --------- Co-authored-by: Vladimír Štill <[email protected]>

Sosutha · 2024-02-27T05:11:16Z

@vbnogueira Please confirm this implementation, so we can raise opensource PR

vbnogueira · 2024-02-27T13:15:42Z

testdata/p4tc_samples/default_action_with_param_01.p4

+            istd.input_port : exact;
+        }
+        actions = {
+            next_hop(z);


From what we decided, for the current release (v12), we are only supporting actions with parameters as variables for default actions. So this action would have to have the defaultonly annotation. Something like:

actions = { next_hop(z) @defaultonly; ... };

vbnogueira · 2024-02-27T13:15:47Z

testdata/p4tc_samples/default_action_with_param_01.p4

+            hdr.ipv4.protocol : exact;
+        }
+        actions = {
+            next_hop(z);


Same goes here.

actions = { next_hop(z) @defaultonly; ... };

@vbnogueira Updated the testcases. Please verify once so opensource PR can be raised

I'm testing again, but just a minor nit
Shouldn't the above line also have the @defaultonly annotation?

Updated that as well now

…iables. (p4lang#4398) * [P4Testgen] Implement a library for common control-plane symbolic variables. * Review comments. * Use std::tie.

* Make use of abseil flat_hash_map * Preallocate at least 16 slots * Use better hash * Do not do lookup twice to get final result * Clarify comment * Clarify docstring Co-authored-by: Glen Gibb <[email protected]> * Link abseil privately to IR * Clarify iterator usage * Refined `finalResult` semantics` --------- Co-authored-by: Glen Gibb <[email protected]>

* Try to clean up the Protobuf includes. * Review comments.

* Removed pipeline id from introspection.json for tc backend * Updated the testcases outputs

vbnogueira · 2024-02-28T11:38:56Z

testdata/p4tc_samples/default_action_with_param_01.p4

+    action next_hop(inout PortId_t vport) {
+        send_to_port(vport);
+    }
+    action default_route_drop() {


Another small nit, could you change this function's name to dflt_route_drop?
P4TC has a limit on the action name length of 32 bytes and we concatenate the control block name with the action name when sending to the kernel. So the action name for the kernel is MainControlImpl/default_route_drop which has 34 bytes.
It would also be interesting if the compiler rejected actions with names that are too large, but this is unrelated to this PR

I will change this case.

It would also be interesting if the compiler rejected actions with names that are too large, but this is unrelated to this PR

Should the compiler issue error in this case?
If yes, could you please create new issue in https://github.com/p4tc-dev/p4tc-e2e/issues. So we can track it

I will change this case.

It would also be interesting if the compiler rejected actions with names that are too large, but this is unrelated to this PR

Should the compiler issue error in this case?

Yes

If yes, could you please create new issue in https://github.com/p4tc-dev/p4tc-e2e/issues. So we can track it

Done (https://github.com/p4tc-dev/p4tc-e2e/issues/59)

Testcase updated. Please confirm once testing is completed from your side. Will raise opensource PR

vbnogueira · 2024-02-28T11:59:52Z

testdata/p4tc_samples_outputs/default_action_with_param_01.template

+TC="tc"
+$TC p4template create pipeline/default_action_with_param_01 pipeid 1 numtables 2
+
+$TC p4template create action/default_action_with_param_01/MainControlImpl/next_hop actid 1


Just to confirm, in this case the action should have a runtime parameter, as per discussion with Jamal (#1 (comment)):

$TC p4template create action/default_action_with_param_01/MainControlImpl/next_hop actid 1 param vport type dev flags runtime

However this will be done in a future PR, right?
As will the creation of a new field to struct p4tc_filter_fields to represent this parameter.

Yes. It will be a future PR

vbnogueira · 2024-02-28T12:09:05Z

testdata/p4tc_samples_outputs/default_action_with_param_01.template

+	tblid 1 \
+	type exact \
+	keysz 64 nummasks 8 tentries 2048 \
+	table_acts act name default_action_with_param_01/MainControlImpl/next_hop \


I don't know whether this is related to this PR or not, but the defaultonly flag is not being assigned for default_action_with_param_01/MainControlImpl/next_hop in the template, even though you are putting it in the P4 program

My mistake. I have updated the case correctly now. Now the flag is generated correctly. Please check

vbnogueira · 2024-02-28T12:43:18Z

testdata/p4tc_samples/default_action_with_param.p4

+    action next_hop(PortId_t vport) {
+        send_to_port(vport);
+    }
+    action default_route_drop() {


Sorry, should've noted this earlier
Please also change the action name here to dflt_route_drop

vbnogueira

With the exception of this small nit (https://github.com/Sosutha/p4c/pull/1/files#r1505904206), LGTM

* tc template code change for default miss action * NoAction case generation in c code for tc backend * default case deletion in c code for tc backend * p4tc testcases output updated

Sosutha force-pushed the p4tc_default_action_fix branch 3 times, most recently from 5a35c09 to 491c85a Compare February 16, 2024 07:47

Sosutha force-pushed the p4tc_default_action_fix branch 3 times, most recently from 9790d3a to 2dab97f Compare February 21, 2024 10:44

syogender approved these changes Feb 22, 2024

View reviewed changes

usha1830 reviewed Feb 22, 2024

View reviewed changes

Sosutha force-pushed the p4tc_default_action_fix branch 2 times, most recently from ccf7c06 to fae6e99 Compare February 22, 2024 09:37

usha1830 approved these changes Feb 22, 2024

View reviewed changes

Sosutha force-pushed the p4tc_default_action_fix branch 2 times, most recently from 36af294 to 316168f Compare February 26, 2024 08:13

dependabot bot and others added 6 commits February 26, 2024 13:22

Add support for parser errors in tc backend (p4lang#4443)

bbc74eb

* Try to handle pna_main_input_metadata_t fields better * Add support for parser errors in tc backend --------- Co-authored-by: Mouse <[email protected]>

Add some more global operator new / delete overrides (p4lang#4465)

11e3bf3

* Add aligned new / delete libgc overloads. Also cleaned some old stuff that is not relevant in year 2024 * Add nothrow overrides * A bit more clear realignment

Try to speed up the CI build process (p4lang#4470)

c533f06

* Try to speed up the CI build process * Update install_fedora_deps.sh * Update ci-test.yml

vbnogueira reviewed Feb 27, 2024

View reviewed changes

asl and others added 4 commits February 27, 2024 13:34

Dowload pre-built Z3 on Macs as well (p4lang#4476)

6108a79

[P4Testgen] Implement a library for common control-plane symbolic var…

81ac3e7

…iables. (p4lang#4398) * [P4Testgen] Implement a library for common control-plane symbolic variables. * Review comments. * Use std::tie.

Try to clean up the Protobuf includes. (p4lang#4474)

f473fc6

* Try to clean up the Protobuf includes. * Review comments.

Sosutha force-pushed the p4tc_default_action_fix branch from 316168f to a07f507 Compare February 28, 2024 05:44

* Removed pipeline id from template for tc backend (p4lang#4480)

eeee08e

* Removed pipeline id from introspection.json for tc backend * Updated the testcases outputs

vbnogueira reviewed Feb 28, 2024

View reviewed changes

Sosutha force-pushed the p4tc_default_action_fix branch 2 times, most recently from 0c83eec to 9082d16 Compare February 28, 2024 11:47

vbnogueira reviewed Feb 28, 2024

View reviewed changes

Sosutha force-pushed the p4tc_default_action_fix branch from 9082d16 to 1c24973 Compare February 28, 2024 12:25

vbnogueira reviewed Feb 28, 2024

View reviewed changes

vbnogueira approved these changes Feb 28, 2024

View reviewed changes

Sosutha force-pushed the p4tc_default_action_fix branch 2 times, most recently from 6432404 to e7485f6 Compare February 29, 2024 05:38

Fix for default_action

81242f6

* tc template code change for default miss action * NoAction case generation in c code for tc backend * default case deletion in c code for tc backend * p4tc testcases output updated

Sosutha force-pushed the p4tc_default_action_fix branch from e7485f6 to 81242f6 Compare February 29, 2024 06:33

Sosutha mentioned this pull request Mar 4, 2024

tc_may_override annotation implementation #4

Closed

Sosutha closed this Apr 1, 2024

Sosutha deleted the p4tc_default_action_fix branch April 1, 2024 12:26

Fix for default_action #1

Fix for default_action #1

Conversation

Sosutha commented Feb 9, 2024 • edited Loading

syogender left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usha1830 left a comment

Choose a reason for hiding this comment

Sosutha commented Feb 22, 2024

jhsmt commented Feb 22, 2024 • edited Loading

jhsmt commented Feb 22, 2024

Sosutha commented Feb 23, 2024 • edited Loading

jhsmt commented Feb 23, 2024

Sosutha commented Feb 26, 2024 • edited Loading

Sosutha commented Feb 27, 2024

vbnogueira Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

vbnogueira Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbnogueira Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbnogueira left a comment

Choose a reason for hiding this comment

Sosutha commented Feb 9, 2024 •

edited

Loading

jhsmt commented Feb 22, 2024 •

edited

Loading

Sosutha commented Feb 23, 2024 •

edited

Loading

Sosutha commented Feb 26, 2024 •

edited

Loading

vbnogueira Feb 27, 2024 •

edited

Loading

vbnogueira Feb 27, 2024 •

edited

Loading

vbnogueira Feb 28, 2024 •

edited

Loading