tc_may_override annotation implementation #4

Sosutha · 2024-03-01T11:57:04Z

tc_may_override annotation implementation
* tc_may_override annotation for tc backend
* Added testcases for annotation

#1 (comment)

jhsmt · 2024-03-08T20:59:06Z

@Sosutha question:
Is it possible (in P4) to have a case where we have two tables with the same default action but different different values? For example testdata/p4tc_samples_outputs/tc_may_override_example_01.template has:

$TC p4template create action/tc_may_override_example_01/MainControlImpl/next_hop actid 1 \
	param vport type bit32
$TC p4template update table/tc_may_override_example_01/MainControlImpl/ipv4_tbl_1 default_miss_action action tc_may_override_example_01/MainControlImpl/next_hop param vport flags runtime

This syntax will be fine if we had another table ipv4_tbl_2 and therefore we could say:
$TC p4template update table/tc_may_override_example_01/MainControlImpl/ipv4_tbl_2 default_miss_action action tc_may_override_example_01/MainControlImpl/next_hop param vport flags runtime

Assuming the ebpf code will generate different variables for the two default values so we can override them. Currently for one of them you would have in testdata/p4tc_samples_outputs/tc_may_override_example_01_parser.h:

This structure:
struct p4tc_filter_fields {
    __u32 pipeid;
    __u32 handle;
    __u32 classid;
    __u32 chain;
    __u32 blockid;
    __be16 proto;
    __u16 prio;
    __u32 MainControlImpl_next_hop_vport;
};

will now contain an extra field for the second table:

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

Assuming this (P4) scenario is legit and above would work, it would require some change from our side.
OTOH if we assume sucha default action will only be used in one table then the template would need to change to:

$TC p4template create action/tc_may_override_example_01/MainControlImpl/next_hop actid 1 \
	param vport type bit32 flags runtime
$TC p4template update table/tc_may_override_example_01/MainControlImpl/ipv4_tbl_1 default_miss_action action tc_may_override_example_01/MainControlImpl/next_hop param vport

note the "flags runtime" is move to the action creation line above. With this approach there is no need for any change from our side.

Sosutha · 2024-03-11T05:54:21Z

@jhsmt, It is possible for two P4 tables to have same default action with different values.
Current implementation will have only one variable as common for both table's default action variable.
If two different variables are needed if there are two different tables with same default action, then implementation needs to be changed. In such case, for example ipv4Tbl1, ipv4Tbl2 are two tables with next_hop as default action, we can create structure like this

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

Please confirm.

syogender

Looks Good

jhsmt · 2024-03-11T12:26:37Z

@jhsmt, It is possible for two P4 tables to have same default action with different values. Current implementation will have only one variable as common for both table's default action variable. If two different variables are needed if there are two different tables with same default action, then implementation needs to be changed. In such case, for example ipv4Tbl1, ipv4Tbl2 are two tables with next_hop as default action, we can create structure like this
struct p4tc_filter_fields {
    __u32 pipeid;
    __u32 handle;
    __u32 classid;
    __u32 chain;
    __u32 blockid;
    __be16 proto;
    __u16 prio;
    __u32 ipv4Tbl1_next_hop_vport;
    __u32 ipv4Tbl2_next_hop_vport;
};
Please confirm.

Confirmed..

psivanup

Looks fine to me

psivanup · 2024-03-12T10:23:59Z

backends/tc/backend.cpp

@@ -354,29 +354,52 @@ void ConvertToBackendIR::updateDefaultMissAction(const IR::P4Table *t, IR::TCTab
                if (defaultActionProperty->isConstant) {
                    tabledef->setDefaultMissConst(true);
                }
+                auto annoList = defaultActionProperty->getAnnotations()->annotations;
+                bool isTCMayOverride = false;


getSingle function shall be used instead of the loop.
overrideAnno = defaultActionProperty->getAnnotations()->getSingle(ParseTCAnnoations::tcMayOverride)

Updated the code

psivanup

Looks fine

usha1830

Looks good

usha1830 · 2024-03-12T12:01:38Z

backends/tc/ebpfCodeGen.cpp

+            cstring tblName = table->getTableName();
+            cstring defaultActionName = table->defaultMissAction->getActionName();
+            auto actionNameStr = defaultActionName.c_str();
+            for (long unsigned int i = 0; i < defaultActionName.size(); i++) {


Why use long unsigned int? long and int are both 4 bytes

Compiler issues warning because of the return type of size() function

usha1830 · 2024-03-12T12:07:50Z

backends/tc/tc.def

@@ -231,12 +265,18 @@ class TCTable {
    void setDefaultMissConst(bool i) {
        isDefaultMissConst = i;
    }
+    void setTcMayOverride(bool i) {


Are you setting this with value other than "true". Is the argument needed?

Updated the function without argument

usha1830 · 2024-03-12T12:08:18Z

backends/tc/tc.def

@@ -309,9 +350,12 @@ class TCTable {
                tcTable += " permissions 0x1024";
            }
            tcTable += " action " + defaultMissAction->getName();
-            for (auto param : defaultMissActionParams) {
+            if (!defaultMissActionParams.empty())
+            tcTable += " param";


indentation within if is incorrect

Updated the code

* tc_may_override annotation for tc backend * Added testcases for annotation

jhsmt · 2024-03-13T21:27:44Z

@jhsmt, It is possible for two P4 tables to have same default action with different values. Current implementation will have only one variable as common for both table's default action variable. If two different variables are needed if there are two different tables with same default action, then implementation needs to be changed. In such case, for example ipv4Tbl1, ipv4Tbl2 are two tables with next_hop as default action, we can create structure like this
struct p4tc_filter_fields {
    __u32 pipeid;
    __u32 handle;
    __u32 classid;
    __u32 chain;
    __u32 blockid;
    __be16 proto;
    __u16 prio;
    __u32 ipv4Tbl1_next_hop_vport;
    __u32 ipv4Tbl2_next_hop_vport;
};
Please confirm.
Confirmed..

@Sosutha i just noticed one thing, for this situation the action value should be retrieved from struct p4tc_filter_fields.
Example for this:

switch (value->action) {
                            case MAINCONTROLIMPL_IPV4_TBL_1_ACT_MAINCONTROLIMPL_NEXT_HOP: 
                                {
/* send_to_port(value->u.MainControlImpl_next_hop.vport) */
                                    compiler_meta__->drop = false;
                                    send_to_port(value->u.MainControlImpl_next_hop.vport);
                                }
                                break;

should be:

switch (value->action) {
                            case MAINCONTROLIMPL_IPV4_TBL_1_ACT_MAINCONTROLIMPL_NEXT_HOP: 
                                {
                                  /* send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport); */
                                    compiler_meta__->drop = false;
                                    send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport);
                                }
                                break;

Sorry for not catching this sooner..

Sosutha · 2024-03-14T05:11:05Z

@jhsmt Jamal, I will implement this change in separate PR. I have already raised opensource PR for this implementation.
I have a query regarding this change. These statements will be executed for table entries which have this action not only for default action. Will this change hold fine for both the cases?

Sosutha · 2024-03-15T09:24:27Z

@jhsmt Jamal, I will implement this change in separate PR. I have already raised opensource PR for this implementation. I have a query regarding this change. These statements will be executed for table entries which have this action not only for default action. Will this change hold fine for both the cases?

@jhsmt , could you please clarify the query for proceeding with the implementation of the change

jhsmt · 2024-03-15T19:38:25Z

@jhsmt Jamal, I will implement this change in separate PR. I have already raised opensource PR for this implementation. I have a query regarding this change. These statements will be executed for table entries which have this action not only for default action. Will this change hold fine for both the cases?

@jhsmt , could you please clarify the query for proceeding with the implementation of the change

@Sosutha I apologize I thought i responded. Sure lets have a second PR for the fix.

Sosutha · 2024-03-18T07:59:05Z

@jhsmt Jamal, I will implement this change in separate PR. I have already raised opensource PR for this implementation. I have a query regarding this change. These statements will be executed for table entries which have this action not only for default action. Will this change hold fine for both the cases?

@jhsmt , could you please clarify the query for proceeding with the implementation of the change

@Sosutha I apologize I thought i responded. Sure lets have a second PR for the fix.

@jhsmt I have a query regarding this change. These statements will be executed for table entries which have this next_hop action as designated action for that entry and also for default action when there is no action given. Will this change hold fine for both the cases? When these statements gets executed for table entries that have this action, Will p4tc_filter_fields.ipv4_tbl_1_next_hop_vport hold the correct value for that entry, not the value->u.MainControlImpl_next_hop.vport?

jhsmt · 2024-03-18T23:14:26Z

@jhsmt Jamal, I will implement this change in separate PR. I have already raised opensource PR for this implementation. I have a query regarding this change. These statements will be executed for table entries which have this action not only for default action. Will this change hold fine for both the cases?

@jhsmt , could you please clarify the query for proceeding with the implementation of the change

@Sosutha I apologize I thought i responded. Sure lets have a second PR for the fix.

@jhsmt I have a query regarding this change. These statements will be executed for table entries which have this next_hop action as designated action for that entry and also for default action when there is no action given. Will this change hold fine for both the cases? When these statements gets executed for table entries that have this action, Will p4tc_filter_fields.ipv4_tbl_1_next_hop_vport hold the correct value for that entry, not the value->u.MainControlImpl_next_hop.vport?

@Sosutha excellent catch. So if the value is for the default miss/hit action then the earlier switch statement we discussed i.e.

switch (value->action) {
                            case MAINCONTROLIMPL_IPV4_TBL_1_ACT_MAINCONTROLIMPL_NEXT_HOP: 
                                {
                                  /* send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport); */
                                    compiler_meta__->drop = false;
                                    send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport);
                                }
                                break;

changes to:

switch (value->action) {
                    case MAINCONTROLIMPL_IPV4_TBL_1_ACT_MAINCONTROLIMPL_NEXT_HOP: 
                                {
                                  if (value->is_default_miss_act || value->is_default_hit_act ) {
                                           /* send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport); */
                                             compiler_meta__->drop = false;
                                           send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport);
                                } else { /* we get the value from the table entry */
                                       /* send_to_port(value->u.MainControlImpl_next_hop.vport) */
                                    compiler_meta__->drop = false;
                                    send_to_port(value->u.MainControlImpl_next_hop.vport);
                                } 
                    }
                     break;

But i would say the if statement is only necessary if we have a case such as this where the action is used both in the table entry and as a default hit/miss action

Sosutha · 2024-03-19T05:17:39Z

@jhsmt We can generate if statement as you suggested. But so far all the implementation done related to default action has considered only default_miss action. For default_hit action( specific only to tc), we need to a similar implementation with different design. Since default_hit is annotation based and default_miss was statement based, Different design is needed for default_hit. In order to complete default_miss implementation fully, I will make the change you proposed in one PR. In that we can have implementation for if statement as below

if (value->is_default_miss_act)
    send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport);

For default_hit related changes, we can have separate PR. I will propose design related to default_hit annotation in the issue https://github.com/p4tc-dev/p4tc-e2e/issues/60. Please confirm

jhsmt · 2024-03-19T21:03:36Z

@jhsmt We can generate if statement as you suggested. But so far all the implementation done related to default action has considered only default_miss action. For default_hit action( specific only to tc), we need to a similar implementation with different design. Since default_hit is annotation based and default_miss was statement based, Different design is needed for default_hit. In order to complete default_miss implementation fully, I will make the change you proposed in one PR. In that we can have implementation for if statement as below
if (value->is_default_miss_act)
    send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport);
For default_hit related changes, we can have separate PR. I will propose design related to default_hit annotation in the issue p4tc-dev/p4tc-e2e#60. Please confirm

Confirmed for this and for p4tc-dev/p4tc-e2e#60

Sosutha force-pushed the p4tc_default_action_annotation branch 5 times, most recently from 4bbf2bb to 5fef734 Compare March 7, 2024 08:09

syogender approved these changes Mar 11, 2024

View reviewed changes

psivanup approved these changes Mar 12, 2024

View reviewed changes

Sosutha force-pushed the p4tc_default_action_annotation branch from 5fef734 to e173ee5 Compare March 12, 2024 11:42

Sosutha requested review from psivanup and syogender March 12, 2024 11:44

psivanup approved these changes Mar 12, 2024

View reviewed changes

usha1830 approved these changes Mar 12, 2024

View reviewed changes

Sosutha force-pushed the p4tc_default_action_annotation branch from e173ee5 to 38ff070 Compare March 12, 2024 12:34

Sosutha requested a review from usha1830 March 12, 2024 12:35

usha1830 approved these changes Mar 12, 2024

View reviewed changes

tc_may_override annotation implementation

9b0ec6b

* tc_may_override annotation for tc backend * Added testcases for annotation

Sosutha force-pushed the p4tc_default_action_annotation branch from 38ff070 to 9b0ec6b Compare March 13, 2024 08:56

Sosutha mentioned this pull request Mar 20, 2024

Added default action code for tc backend #6

Closed

Sosutha closed this Apr 1, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tc_may_override annotation implementation #4

tc_may_override annotation implementation #4

Sosutha commented Mar 1, 2024 •

edited

Loading

jhsmt commented Mar 8, 2024

Sosutha commented Mar 11, 2024

syogender left a comment

jhsmt commented Mar 11, 2024

psivanup left a comment

psivanup Mar 12, 2024

Sosutha Mar 12, 2024

psivanup left a comment

usha1830 left a comment

usha1830 Mar 12, 2024

Sosutha Mar 12, 2024

usha1830 Mar 12, 2024

Sosutha Mar 12, 2024

usha1830 Mar 12, 2024

Sosutha Mar 12, 2024

jhsmt commented Mar 13, 2024

Sosutha commented Mar 14, 2024

Sosutha commented Mar 15, 2024

jhsmt commented Mar 15, 2024

Sosutha commented Mar 18, 2024

jhsmt commented Mar 18, 2024

Sosutha commented Mar 19, 2024 •

edited

Loading

jhsmt commented Mar 19, 2024

tc_may_override annotation implementation #4

tc_may_override annotation implementation #4

Conversation

Sosutha commented Mar 1, 2024 • edited Loading

jhsmt commented Mar 8, 2024

Sosutha commented Mar 11, 2024

syogender left a comment

Choose a reason for hiding this comment

jhsmt commented Mar 11, 2024

psivanup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psivanup left a comment

Choose a reason for hiding this comment

usha1830 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

jhsmt commented Mar 13, 2024

Sosutha commented Mar 14, 2024

Sosutha commented Mar 15, 2024

jhsmt commented Mar 15, 2024

Sosutha commented Mar 18, 2024

jhsmt commented Mar 18, 2024

Sosutha commented Mar 19, 2024 • edited Loading

jhsmt commented Mar 19, 2024

Sosutha commented Mar 1, 2024 •

edited

Loading

Sosutha commented Mar 19, 2024 •

edited

Loading