Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 15:19:17 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>,
        Petr Machata <petrm@nvidia.com>
Cc:     Woojung Huh <woojung.huh@microchip.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Arun Ramadoss <arun.ramadoss@microchip.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        UNGLinuxDriver@microchip.com, Eric Dumazet <edumazet@google.com>,
        kernel@pengutronix.de, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH net-next v1 2/2] net: dsa: microchip: Add partial ACL
 support for ksz9477 switches
Message-ID: <20230417121917.cfixzwoqds6wwlyu@skbuf>
References: <20230411172456.3003003-1-o.rempel@pengutronix.de>
 <20230411172456.3003003-1-o.rempel@pengutronix.de>
 <20230411172456.3003003-3-o.rempel@pengutronix.de>
 <20230411172456.3003003-3-o.rempel@pengutronix.de>
 <20230416165658.fuo7vwer7m7ulkg2@skbuf>
 <20230417045710.GB20350@pengutronix.de>
 <20230417101209.m5fhc7njeeomljkf@skbuf>
 <20230417110311.GA11474@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230417110311.GA11474@pengutronix.de>
Precedence: bulk

On Mon, Apr 17, 2023 at 01:03:11PM +0200, Oleksij Rempel wrote:
> 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.

This is the kind of stuff you'd have to know when adding a software model
for the rules, right? Could you consider writing a selftest that
precisely illustrates the matching pattern of the hardware? It would be
good if the same test could then be run on a software-only implementation
and if the behavior would match. The tc tool should be more than a
vendor agnostic tool of doing vendor specific stuff. It should offload
as faithfully as possible the software data path. It would also be good,
but I haven't studied or used this test personally, if the test could be
based on the existing tools/testing/selftests/net/forwarding/skbedit_priority.sh.

> > > > 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?

I believe Petr Machata (now copied) could have an opinion here too.