Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200602051828.5734-1-xiaoliang.yang_1@nxp.com>
 <20200602051828.5734-4-xiaoliang.yang_1@nxp.com> <20200602083613.ddzjh54zxtbklytw@ws.localdomain>
In-Reply-To: <20200602083613.ddzjh54zxtbklytw@ws.localdomain>
From:   Vladimir Oltean <olteanv@gmail.com>
Date:   Wed, 3 Jun 2020 13:04:26 +0300
Message-ID: <CA+h21hocBOyuDFvnLq-sBEG5phaJPxbhvZ_P5H8HnTkBDv1x+w@mail.gmail.com>
Subject: Re: [PATCH v2 net-next 03/10] net: mscc: ocelot: allocated rules to
 different hardware VCAP TCAMs by chain index
To:     "Allan W. Nielsen" <allan.nielsen@microchip.com>
Cc:     Xiaoliang Yang <xiaoliang.yang_1@nxp.com>, Po Liu <po.liu@nxp.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Li Yang <leoyang.li@nxp.com>, Mingkai Hu <mingkai.hu@nxp.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jiri Pirko <jiri@resnulli.us>,
        Ido Schimmel <idosch@idosch.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Vinicius Costa Gomes <vinicius.gomes@intel.com>,
        Nikolay Aleksandrov <nikolay@cumulusnetworks.com>,
        Roopa Prabhu <roopa@cumulusnetworks.com>,
        netdev <netdev@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Joergen Andreasen <joergen.andreasen@microchip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
        linux-devel@linux.nxdi.nxp.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Allan,

On Tue, 2 Jun 2020 at 11:38, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> Hi Xiaoliang,
>
> Happy to see that you are moving in the directions of multi chain - this
> seems ilke a much better fit to me.
>
>
> On 02.06.2020 13:18, Xiaoliang Yang wrote:
> >There are three hardware TCAMs for ocelot chips: IS1, IS2 and ES0. Each
> >one supports different actions. The hardware flow order is: IS1->IS2->ES0.
> >
> >This patch add three blocks to store rules according to chain index.
> >chain 0 is offloaded to IS1, chain 1 is offloaded to IS2, and egress chain
> >0 is offloaded to ES0.
>
> Using "static" allocation to to say chain-X goes to TCAM Y, also seems
> like the right approach to me. Given the capabilities of the HW, this
> will most likely be the easiest scheme to implement and to explain to
> the end-user.
>
> But I think we should make some adjustments to this mapping schema.
>
> Here are some important "things" I would like to consider when defining
> this schema:
>
> - As you explain, we have 3 TCAMs (IS1, IS2 and ES0), but we have 3
>    parallel lookups in IS1 and 2 parallel lookups in IS2 - and also these
>    TCAMs has a wide verity of keys.
>
> - We can utilize these multiple parallel lookups such that it seems like
>    they are done in serial (that is if they do not touch the same
>    actions), but as they are done in parallel they can not influence each
>    other.
>
> - We can let IS1 influence the IS2 lookup (like the GOTO actions was
>    intended to be used).
>
> - The chip also has other QoS classification facilities which sits
>    before the TCAM (take a look at 3.7.3 QoS, DP, and DSCP Classification
>    in vsc7514 datasheet). It we at some point in time want to enable
>    this, then I think we need to do that in the same tc-flower framework.
>
> Here is my initial suggestion for an alternative chain-schema:
>
> Chain 0:           The default chain - today this is in IS2. If we proceed
>                     with this as is - then this will change.
> Chain 1-9999:      These are offloaded by "basic" classification.
> Chain 10000-19999: These are offloaded in IS1
>                     Chain 10000: Lookup-0 in IS1, and here we could limit the
>                                  action to do QoS related stuff (priority
>                                  update)
>                     Chain 11000: Lookup-1 in IS1, here we could do VLAN
>                                  stuff
>                     Chain 12000: Lookup-2 in IS1, here we could apply the
>                                  "PAG" which is essentially a GOTO.
>
> Chain 20000-29999: These are offloaded in IS2
>                     Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID -
>                                        20000 is the PAG value.
>                     Chain 21000-21000: Lookup-1 in IS2.
>
> All these chains should be optional - users should only need to
> configure the chains they need. To make this work, we need to configure
> both the desired actions (could be priority update) and the goto action.
> Remember in HW, all packets goes through this process, while in SW they
> only follow the "goto" path.
>
> An example could be (I have not tested this yet - sorry):
>
> tc qdisc add dev eth0 ingress
>
> # Activate lookup 11000. We can not do any other rules in chain 0, also
> # this implicitly means that we do not want any chains <11000.
> tc filter add dev eth0 parent ffff: chain 0
>     action
>     matchall goto 11000
>
> tc filter add dev eth0 parent ffff: chain 11000 \
>     flower src_mac 00:01:00:00:00:00/00:ff:00:00:00:00 \
>     action \
>     vlan modify id 1234 \
>     pipe \
>     goto 20001
>
> tc filter add dev eth0 parent ffff: chain 20001 ...
>
> Maybe it would be an idea to create some use-cases, implement them in a
> test which can pass with today's SW, and then once we have a common
> understanding of what we want, we can implement it?
>
> /Allan
>
> >Using action goto chain to express flow order as follows:
> >        tc filter add dev swp0 chain 0 parent ffff: flower skip_sw \
> >        action goto chain 1
> >
> >Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> >---
> > drivers/net/ethernet/mscc/ocelot_ace.c    | 51 +++++++++++++++--------
> > drivers/net/ethernet/mscc/ocelot_ace.h    |  7 ++--
> > drivers/net/ethernet/mscc/ocelot_flower.c | 46 +++++++++++++++++---
> > include/soc/mscc/ocelot.h                 |  2 +-
> > include/soc/mscc/ocelot_vcap.h            |  4 +-
> > 5 files changed, 81 insertions(+), 29 deletions(-)

> /Allan

What would be the advantage, from a user perspective, in exposing the
3 IS1 lookups as separate chains with orthogonal actions?
If the user wants to add an IS1 action that performs QoS
classification, VLAN classification and selects a custom PAG, they
would have to install 3 separate filters with the same key, each into
its own chain. Then the driver would be smart enough to figure out
that the 3 keys are actually the same, so it could merge them.
In comparison, we could just add a single filter to the IS1 chain,
with 3 actions (skbedit priority, vlan modify, goto is2).

Thanks,
-Vladimir