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;
IronPort-SDR: C8Xr94BcqocK4nupxF/eU6NtUd/Pt+swdFdKKd1as8NeZNtO+mEnOwZQ9wMZLtv41zGoD82pQO
 riVvhJyXZr6usnhzjq01Vfcm8OCNPlA5W3K50XjPC78GIxlfIEe2QjlLE0PbWBDt06xpFPOnY7
 2a3RmTZtHJLmcEC7L9V9GPhLEgo+wpHzR9rXNiObtN92tm/6swfRNpl5i3Qg+TzF9Y+kTWlZAL
 uRPIK1a9qc/JDDno7tU59OD3Araj9k0JLKJraLFJuPsN1PRBVTrSDJX17uIC5A1ihISmRJu7CA
 d3I=
Date:   Mon, 8 Jun 2020 15:56:33 +0200
From:   "Allan W. Nielsen" <allan.nielsen@microchip.com>
To:     Vladimir Oltean <olteanv@gmail.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>
Subject: Re: [PATCH v2 net-next 03/10] net: mscc: ocelot: allocated rules to
 different hardware VCAP TCAMs by chain index
Message-ID: <20200608135633.jznoxwny6qtzxjng@ws.localdomain>
References: <20200602051828.5734-1-xiaoliang.yang_1@nxp.com>
 <20200602051828.5734-4-xiaoliang.yang_1@nxp.com>
 <20200602083613.ddzjh54zxtbklytw@ws.localdomain>
 <CA+h21hocBOyuDFvnLq-sBEG5phaJPxbhvZ_P5H8HnTkBDv1x+w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Disposition: inline
In-Reply-To: <CA+h21hocBOyuDFvnLq-sBEG5phaJPxbhvZ_P5H8HnTkBDv1x+w@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 03.06.2020 13:04, Vladimir Oltean wrote:
>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).

Hi, I realize I forgot to answer this one - sorry.

The reason for this design is that we have use-cases where the rules to
do QoS classification must not impact VLAN classification. The easiest
way to do that, it to have it in separated lookups.

But we could make this more flexible to support your use-case better. A
alternative approach would be to assign exclusive-right-to-action on
first use. If the user choose to use the VLAN update in a given loopup,
then it cannot be used in others.

If the user attempt to use a given action across different lookups we
need to return an error.

Would that work for you? Any downside in such approach?

/Allan