Messages in this thread | | | Date | Mon, 17 Apr 2023 13:03:11 +0200 | From | Oleksij Rempel <> | Subject | Re: [PATCH net-next v1 2/2] net: dsa: microchip: Add partial ACL support for ksz9477 switches |
| |
On Mon, Apr 17, 2023 at 01:12:09PM +0300, Vladimir Oltean wrote: > On Mon, Apr 17, 2023 at 06:57:10AM +0200, Oleksij Rempel wrote: > > > On Tue, Apr 11, 2023 at 07:24:55PM +0200, Oleksij Rempel wrote: > > > > The ACL also implements a count function, generating an interrupt > > > > instead of a forwarding action. It can be used as a watchdog timer or an > > > > event counter. > > > > > > Is the interrupt handled here? I didn't see cls_flower_stats(). > > > > No, it is not implemented in this patch. It is generic description of things > > ACL should be able to do. Is it confusing? Should I remove it? > > No, it's confusing that the ACL statistics are not reported even though > it's mentioned that it's possible...
Certain aspects of the chip specification appeared ambiguous, leading me to decide to allocate a separate time slot for investigating the counter topic if necessary.
For example, according to the KSZ9477 4.4.18 ACCESS CONTROL LIST (ACL) FILTERING:
"It is also possible to configure the ACL table so that multiple processing entries specify the same action rule. In this way, the final matching result is the OR of the matching results from each of the multiple RuleSets. The 16 ACL rules represent an ordered list, with entry #0 having the highest priority and entry #15 having the lowest priority. All matching rules are evaluated. If there are multiple true match results and multiple corresponding actions, the highest priority (lowest numbered) of those actions will be the one taken."
A summary of this part of documentation is: 1. ACL table can have multiple entries specifying the same action rule. 2. Final matching result is the OR of multiple RuleSets' results. 3. 16 ACL rules form an ordered list, with priority descending from #0 to #15. 4. All matching rules are evaluated. 5. When multiple true matches and actions occur, the highest priority action is executed.
Considering this, there is a possibility that separate action rules would not be executed, as they might not be the highest priority match. Since counters would have separation action rules, they would not be executed or prevent other action rules from execution.
To confirm my hypothesis, additional time and testing will be required. Nonetheless, I hope this issue does not impede the progress of this patch.
> > > Have you considered the "skbedit priority" action as opposed to hw_tc? > > > > I had already thought of that, but since bridging is offloaded in the HW > > no skbs are involved, i thought it will be confusing. Since tc-flower seems to > > already support hw_tc remapping, I decided to use it. I hope it will not harm, > > to use it for now as mandatory option and make it optional later if other > > actions are added, including skbedit. > > Well, skbedit is offloadable, so in that sense, its behavior is defined > even when no skbs are involved. OTOH, skbedit also has a software data > path (sets skb->priority), as opposed to hw_tc, which last time I checked, > did not.
Alright, having tc rules be portable is certainly a benefit. I presume that in this situation, it's not an exclusive "either...or" choice. Both variants can coexist, and the skbedit action can be incorporated at a later time. Is that accurate?
Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
| |