[ACL]: Optimize DASH ACL by introducing cluster tag #336

Pterosaur · 2023-02-13T15:20:21Z

No description provided.

Signed-off-by: Ze Gan <ganze718@gmail.com>

documentation/general/dash-sonic-hld.md

Signed-off-by: Ze Gan <ganze718@gmail.com>

prsunny · 2023-02-15T17:22:33Z

documentation/general/dash-sonic-hld.md

@@ -311,8 +321,10 @@ DASH_ACL_RULE_TABLE:{{group_id}}:{{rule_num}}
    "action": {{action}}
    "terminating": {{bool}}
    "protocol": {{list of protocols}}
-    "src_addr": {{list of address}}
-    "dst_addr": {{list of address}}
+    "src_tag": {{list of tag name}}


Mark this optional

Should we have an explicit "optional" mark to "tag" field? IMHO, most of fields in DASH ACL RULE TABLE are optional. E.G. We can only set src_tag or dst_port and set other fields to be empty. So, should we also mark them?

marian-pritsak · 2023-02-16T05:54:14Z

dash-pipeline/bmv2/dash_acl.p4

@@ -29,6 +30,8 @@ match_kind {
    table table_name { \
        key = { \
            meta. ## table_name ##_dash_acl_group_id : exact @name("meta.dash_acl_group_id:dash_acl_group_id"); \
+            meta.dst_tag_map : ternary @name("meta.dst_tag_map:dst_tag"); \


match type: optional

Why is here optional?

We would like a mask operation.
E.G. If the source IP of packet belong to tag 1 and 2, and this ACL rule is applied to tag 2 and 3, it should be matched. If another ACL rule is applied to tag 3 and 4, it should be mismatched.

If the tag was not provided, the target rule could match all tags.

I'm not sure whether the optional match_kind can meet above behaviors.

A case is if a prefix belongs to tags, A, B, C, D (1111) and an ACL rule is applied to tags A, C(1010).
If my understanding is correct, to the match_kind, optional. the mask should be 1111, which implies tag (1111) & mask(1111) != target_tag(1010).
But to the match_kind, ternary, the mask will be 1010. So the rule will be matched due to tag(1111) & mask(1010) = target_tag(1010).

marian-pritsak · 2023-02-16T05:54:56Z

dash-pipeline/bmv2/dash_pipeline.p4

+    @name("src_tag|dash_tag")
+    table src_tag {
+        key = {
+            meta.src_ip_addr : lpm @name("meta.src_ip_addr:sip");


match type: prefix_list

I think here shouldn't be a list. AFAIK, one prefix can belong to multiple clusters with different tags, So the prefix list will be separated into the tag table. Otherwise if it's prefix_list, it's hard to solve the following case:
prefix(A, B, C)=>tag1
prefix(C,D,E)=>tag2

Because we have to dynamically create a new entry for prefix(C)=>tag1, tag2. And if the C was updated, we need also update all entries that include prefix(C).

Could you have a scenario where eg. 10.0.0.0/8 -> tag1, 10.1.0.0/16 -> tag2.. How do you expect lpm match here to resolve and assign tags?

Assumption of 1 prefix matching and 1 tag assignment. What if there are multiple prefixes matching to 1 tag (overlapping prefixes matching to 1 tag)? (A tag is simply a list of prefixes).
Tags will be in the Control Plane and could be in the Data Plane. Whether or not this tag is represented in the Fastpath is an 'implementation choice'. If a Data Plane chooses to enumerate, for example.
Assumption is that it would be pushed to the SouthBound.
Tag is just a name for a list to be pushed.
Indirection for the Control Plane to use.
@prsunny - we do want to optimize from the Data Plane side as well. Scale limits are in the HLD, however this is possible.
@vincent-xs suggests the API is flexible enough for the supplier to control
@jfingerh - remember that the bit-factor must be big enough :) but we do have scale

Signed-off-by: Ze Gan <ganze718@gmail.com>

documentation/general/dash-sonic-hld.md

prsunny · 2023-02-22T22:19:20Z

documentation/general/dash-sonic-hld.md

-    "src_port": {{list of range of ports}}
-    "dst_port": {{list of range of ports}}
+    "protocol": {{list of protocols}} (OPTIONAL)
+    "src_tag": {{list of tag name}} (OPTIONAL)


specifying a src_tag with empty prefix must not match packets (except if src_addr is also specified). When specifying a src_tag and src_prefix, it shall be a OR. Please clarify it in documentation

specifying a src_tag with empty prefix must not match packets. I add this description under the TAG table.

When specifying a src_tag and src_prefix, it shall be a OR. Please clarify it in documentation. I think this should be an AND instead of OR. If you want a OR, just to define another rule.
There are three reasons:

The tag fields are same as other fields like protocol, port and so on. The relationship between src, protocol and port should be AND.

The TAG is for identifying a packets that belongs to a specific group. In the further, maybe we can identify a packets not only from address but also from other features.

I think if you want to implement OR in P4, we have to separate the TAG and ADDR to different ACL tables, which will increase the complexity of the ACL system.

If you accept my points, I can polish and clarify the descriptions for TAG.

documentation/general/dash-sonic-hld.md

Signed-off-by: Ze Gan <ganze718@gmail.com>

mhanif · 2023-02-27T01:16:11Z

@mukeshmv could you please add your scale related comments/questions to this review. Thanks!

mukeshmv · 2023-03-01T08:19:15Z

documentation/general/dash-sonic-hld.md

-
+
+```
+DASH_PREFIX_TAG_TABLE:{{tag_name}}


Can we add some scale numbers for the ACL tags in the HLD ?

How many total tags ?

How many prefixes per tag ?

How many tags can each prefix belong to ?

We will need to update the HLD with the scale numbers.
Possibly by sometime this week 3/16/2023
@prsunny

Added the scale numbers of tags in the above scale table.

KrisNey-MSFT · 2023-03-09T18:43:05Z

@prsunny - may we have an update re: this one? There will be work needed in SAI.

Signed-off-by: Ze Gan <ganze718@gmail.com>

…l_tag

marian-pritsak · 2023-03-22T16:09:28Z

No need to generated separate APIs for SRC/DST tags. Can be joined together like ACL APIs.

vincent-xs · 2023-03-22T19:04:37Z

documentation/general/dash-sonic-hld.md

+    "protocol": {{list of protocols}} (OPTIONAL)
+    "src_tag": {{list of tag name}} (OPTIONAL)
+    "dst_tag": {{list of tag name}} (OPTIONAL)
+    "src_addr": {{list of prefix}} (OPTIONAL)


the documentation switched from address to prefix (prefix is definitely what we would expect).
BUT the P4 model is still generating IP address, not prefix. could the P4 code be fixed to match the documentation?

/** * @brief Optional matched key dip * * @type sai_ip_address_t * @flags MANDATORY_ON_CREATE | CREATE_ONLY */ SAI_DASH_ACL_RULE_ATTR_DIP, /** * @brief Optional matched key sip * * @type sai_ip_address_t * @flags MANDATORY_ON_CREATE | CREATE_ONLY */ SAI_DASH_ACL_RULE_ATTR_SIP,

Have you tried to manually define the DASH_MATCH in the file, dash_acl.p4?

#define DASH_MATCH // Please explicitly define this line in dash-pipeline/bmv2/dash_acl.p4 #ifdef DASH_MATCH #define LIST_MATCH list #define RANGE_LIST_MATCH range_list #else #ifdef TARGET_BMV2_V1MODEL #define LIST_MATCH optional #define RANGE_LIST_MATCH optional #endif // TARGET_BMV2_V1MODEL #ifdef TARGET_DPDK_PNA #define LIST_MATCH ternary #define RANGE_LIST_MATCH range #endif // TARGET_DPDK_PNA #endif

If you define it, you will get the correct SAI header.

/** * @brief List matched key dip * * @type sai_ip_prefix_list_t * @flags MANDATORY_ON_CREATE | CREATE_ONLY */ SAI_DASH_ACL_RULE_ATTR_DIP, /** * @brief List matched key sip * * @type sai_ip_prefix_list_t * @flags MANDATORY_ON_CREATE | CREATE_ONLY */ SAI_DASH_ACL_RULE_ATTR_SIP,

@marian-pritsak

is that something that could be taken care of so we align things up?
thanks

@vincent-xs will look at SAI Master for generation code
Difference between APIs for SAI and bmv2 defined match types for SIP and DIP
By default not-enabled (hidden compile to check for possibly)?
@Pterosaur possibly can also check the 'define'

Pterosaur · 2023-03-24T14:58:27Z

No need to generated separate APIs for SRC/DST tags. Can be joined together like ACL APIs.

Hi @marian-pritsak , the APIs were automatically generated from P4.
As you know, we only have a list of TAG-Prefix. So we don't know if the prefix is for SRC or DST to an actual packet. So, we have to try the SRC and DST both for tagging the packet.
I have no idea about how to combine two fields(SRC/DST) matching into one key in P4.
Like

    @name("dst_tag|dash_tag")
    table dst_tag {
        key = {
            meta.dst_ip_addr/meta.src_ip_addr : lpm @name("meta.dst/src_ip_addr:dip");
        }
        actions = {
            set_tag;
        }
    }

If you have any solution, I'm very glad to learn it.

svshah-intel · 2023-03-28T20:34:45Z

dash-pipeline/bmv2/dash_pipeline.p4

+    @name("src_tag|dash_tag")
+    table src_tag {
+        key = {
+            meta.src_ip_addr : lpm @name("meta.src_ip_addr:sip");


Could you have a scenario where eg. 10.0.0.0/8 -> tag1, 10.1.0.0/16 -> tag2.. How do you expect lpm match here to resolve and assign tags?

svshah-intel · 2023-03-28T20:51:59Z

documentation/general/dash-sonic-hld.md

-dst_addr                 = list of destination ip prefixes ',' separated
-src_port                 = list of range of source ports ',' separated
-dst_port                 = list of range of destination ports ',' separated
+src_tag                  = list of source tag name ',' separated; if not provided, match on all source TAGs or no TAG.


what does it mean by "match on all source TAGs or no TAG" ?

Intention is if the attribute is not provided, it is like any other ACL rule. If not provided, wildcard match. Match on ANY tag or NO tag (reword)?
@Pterosaur @prsunny

If packet matches /8, it will be tagged with tag1. It's not LPM match, will verify with @Pterosaur

@svshah-intel one prefix matching to multiple tags can result in multiple matches - is this the question? Comments are about lookup and actions.

marian-pritsak · 2023-03-29T16:22:22Z

No need to generated separate APIs for SRC/DST tags. Can be joined together like ACL APIs.

Hi @marian-pritsak , the APIs were automatically generated from P4. As you know, we only have a list of TAG-Prefix. So we don't know if the prefix is for SRC or DST to an actual packet. So, we have to try the SRC and DST both for tagging the packet. I have no idea about how to combine two fields(SRC/DST) matching into one key in P4. Like
    @name("dst_tag|dash_tag")
    table dst_tag {
        key = {
            meta.dst_ip_addr/meta.src_ip_addr : lpm @name("meta.dst/src_ip_addr:dip");
        }
        actions = {
            set_tag;
        }
    }
If you have any solution, I'm very glad to learn it.

@Pterosaur You need to keep two tables in P4 code, but unite them under one API name, like in the example you gave with this annotation @name("dst_tag|dash_tag")

prsunny · 2023-04-05T20:37:43Z

@Pterosaur , can you please resolve conflict?

marian-pritsak · 2023-04-11T23:01:16Z

No need to generated separate APIs for SRC/DST tags. Can be joined together like ACL APIs.

Hi @marian-pritsak , the APIs were automatically generated from P4. As you know, we only have a list of TAG-Prefix. So we don't know if the prefix is for SRC or DST to an actual packet. So, we have to try the SRC and DST both for tagging the packet. I have no idea about how to combine two fields(SRC/DST) matching into one key in P4. Like
    @name("dst_tag|dash_tag")
    table dst_tag {
        key = {
            meta.dst_ip_addr/meta.src_ip_addr : lpm @name("meta.dst/src_ip_addr:dip");
        }
        actions = {
            set_tag;
        }
    }
If you have any solution, I'm very glad to learn it.
@Pterosaur You need to keep two tables in P4 code, but unite them under one API name, like in the example you gave with this annotation @name("dst_tag|dash_tag")

@Pterosaur after considering this, I think it's better to generate separate APIs for src and dst tags, which will allow implementations keep two different LPM tables for a better utilization of resources.

KrisNey-MSFT · 2023-04-12T16:53:49Z

documentation/general/dash-sonic-hld.md

-dst_addr                 = list of destination ip prefixes ',' separated
-src_port                 = list of range of source ports ',' separated
-dst_port                 = list of range of destination ports ',' separated
+src_tag                  = list of source tag name ',' separated; if not provided, match on all source TAGs or no TAG.


@Pterosaur and @prsunny
Instead of this: 'match on all source TAGs or no TAG'
Could we have a rewording to: 'match on ANY tag or NO tag'?

Per Community conversation, the intention is that if the attribute is not provided, it is like any other ACL rule. If not provided, it will be a wildcard match. Could we have it reworded similar to the above?

OK, I will submit a PR to polish it.

Change it in PR: #359

Pterosaur added 2 commits February 13, 2023 15:18

Init tag

9048551

Signed-off-by: Ze Gan <ganze718@gmail.com>

Merge branch 'main' into dash_acl_tag

e9a38b2

prsunny reviewed Feb 13, 2023

View reviewed changes

Pterosaur force-pushed the dash_acl_tag branch 2 times, most recently from 176c305 to d0594b6 Compare February 14, 2023 09:06

Polish doc

e62ac0a

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch from d0594b6 to e62ac0a Compare February 14, 2023 09:10

Pterosaur mentioned this pull request Feb 15, 2023

[utils.h]: The prefix_len is strange in ipPrefixSetVal #338

Closed

Pterosaur force-pushed the dash_acl_tag branch from 4e27db6 to 43f33c3 Compare February 15, 2023 08:10

Fix code and add test

ee67783

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch 3 times, most recently from 46c152a to b303111 Compare February 15, 2023 10:06

Increasing wait time for verify packet

a245bf5

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch from b303111 to a245bf5 Compare February 15, 2023 10:55

polish doc

54556af

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur changed the title ~~[Draft][ACL]: Optimize DASH ACL by introducing cluster tag~~ [ACL]: Optimize DASH ACL by introducing cluster tag Feb 15, 2023

Pterosaur marked this pull request as ready for review February 15, 2023 12:17

Pterosaur requested review from prsunny and removed request for prsunny February 15, 2023 12:17

prsunny reviewed Feb 15, 2023

View reviewed changes

Pterosaur force-pushed the dash_acl_tag branch 2 times, most recently from 1b1e1d4 to e1ebff0 Compare February 16, 2023 01:16

marian-pritsak suggested changes Feb 16, 2023

View reviewed changes

Add optional mark in doc

8a1ef51

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch from e1ebff0 to 8a1ef51 Compare February 16, 2023 14:04

Pterosaur requested a review from marian-pritsak February 21, 2023 11:30

KrisNey-MSFT requested a review from mhanif February 22, 2023 17:15

prsunny reviewed Feb 22, 2023

View reviewed changes

Pterosaur commented Feb 23, 2023

View reviewed changes

documentation/general/dash-sonic-hld.md Outdated Show resolved Hide resolved

Polish doc

b84c4ef

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch from b9019e7 to b84c4ef Compare February 24, 2023 17:31

Pterosaur requested review from prsunny and removed request for marian-pritsak and mhanif February 25, 2023 23:27

mukeshmv reviewed Mar 1, 2023

View reviewed changes

Pterosaur force-pushed the dash_acl_tag branch from 75dfc79 to c4050bb Compare March 17, 2023 08:38

Polish doc

16457a5

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch from c4050bb to 16457a5 Compare March 17, 2023 08:57

Polish doc

f993969

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch from 16457a5 to f993969 Compare March 19, 2023 16:17

Merge branch 'main' into dash_acl_tag

f27ec5f

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl_tag branch from 587d01c to f27ec5f Compare March 20, 2023 00:52

Merge branch 'dash_acl_tag' of github.com:Pterosaur/DASH into dash_ac…

8cc3eb8

…l_tag

vincent-xs reviewed Mar 22, 2023

View reviewed changes

svshah-intel reviewed Mar 28, 2023

View reviewed changes

marian-pritsak approved these changes Apr 11, 2023

View reviewed changes

Merge branch 'main' into dash_acl_tag

afa951d

prsunny merged commit 62eb1a9 into sonic-net:main Apr 12, 2023
4 checks passed

KrisNey-MSFT reviewed Apr 12, 2023

View reviewed changes

KrisNey-MSFT mentioned this pull request Aug 16, 2023

Sai attributes for ACL rules should include list of IP prefixes for the Bmv2 compile #360

Closed

marian-pritsak mentioned this pull request Aug 18, 2023

Remove ACL tags from BM #425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ACL]: Optimize DASH ACL by introducing cluster tag #336

[ACL]: Optimize DASH ACL by introducing cluster tag #336

Pterosaur commented Feb 13, 2023

prsunny Feb 15, 2023

Pterosaur Feb 16, 2023

marian-pritsak Feb 16, 2023

Pterosaur Feb 16, 2023 •

edited

Pterosaur Feb 24, 2023 •

edited

marian-pritsak Feb 16, 2023

Pterosaur Feb 16, 2023

svshah-intel Mar 28, 2023

KrisNey-MSFT Apr 12, 2023

prsunny Feb 22, 2023

Pterosaur Feb 24, 2023

mhanif commented Feb 27, 2023

mukeshmv Mar 1, 2023

KrisNey-MSFT Mar 16, 2023

Pterosaur Mar 19, 2023

KrisNey-MSFT commented Mar 9, 2023

marian-pritsak commented Mar 22, 2023

vincent-xs Mar 22, 2023

Pterosaur Mar 27, 2023

vincent-xs Apr 11, 2023

KrisNey-MSFT Apr 12, 2023

Pterosaur commented Mar 24, 2023

svshah-intel Mar 28, 2023

svshah-intel Mar 28, 2023

KrisNey-MSFT Mar 29, 2023 •

edited

marian-pritsak commented Mar 29, 2023 •

edited by KrisNey-MSFT

prsunny commented Apr 5, 2023

marian-pritsak commented Apr 11, 2023

KrisNey-MSFT Apr 12, 2023

Pterosaur Apr 13, 2023

Pterosaur Apr 13, 2023



		```
		DASH_PREFIX_TAG_TABLE:{{tag_name}}

[ACL]: Optimize DASH ACL by introducing cluster tag #336

[ACL]: Optimize DASH ACL by introducing cluster tag #336

Conversation

Pterosaur commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pterosaur Feb 16, 2023 • edited

Choose a reason for hiding this comment

Pterosaur Feb 24, 2023 • edited

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

mhanif commented Feb 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisNey-MSFT commented Mar 9, 2023

marian-pritsak commented Mar 22, 2023

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

Pterosaur commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisNey-MSFT Mar 29, 2023 • edited

Choose a reason for hiding this comment

marian-pritsak commented Mar 29, 2023 • edited by KrisNey-MSFT

prsunny commented Apr 5, 2023

marian-pritsak commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pterosaur Feb 16, 2023 •

edited

Pterosaur Feb 24, 2023 •

edited

KrisNey-MSFT Mar 29, 2023 •

edited

marian-pritsak commented Mar 29, 2023 •

edited by KrisNey-MSFT