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:   Tue, 18 Apr 2023 06:55:26 +0200
From:   Oleksij Rempel <o.rempel@pengutronix.de>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Petr Machata <petrm@nvidia.com>,
        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: <20230418045526.GB30964@pengutronix.de>
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>
 <20230417121917.cfixzwoqds6wwlyu@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230417121917.cfixzwoqds6wwlyu@skbuf>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Mon, Apr 17, 2023 at 03:19:17PM +0300, Vladimir Oltean wrote:
> 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?

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.

ok I added it to my todo list. My next time slot for this project will
be in two months.

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

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