Message-ID: <CA+h21hqoZdQeSxTtrEsVEHi6ZP1LrWKQGwZ9zPvjyWZ62TNfbg@mail.gmail.com>
Date:   Tue, 9 Jun 2020 15:55:31 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Allan W. Nielsen" <allan.nielsen@...rochip.com>
Cc:     Xiaoliang Yang <xiaoliang.yang_1@....com>, Po Liu <po.liu@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Li Yang <leoyang.li@....com>, Mingkai Hu <mingkai.hu@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jiri Pirko <jiri@...nulli.us>,
        Ido Schimmel <idosch@...sch.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        netdev <netdev@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Joergen Andreasen <joergen.andreasen@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        linux-devel@...ux.nxdi.nxp.com
Subject: Re: [PATCH v2 net-next 03/10] net: mscc: ocelot: allocated rules to
 different hardware VCAP TCAMs by chain index

Hi Allan,

On Mon, 8 Jun 2020 at 16:56, Allan W. Nielsen
<allan.nielsen@...rochip.com> wrote:
>
> On 03.06.2020 13:04, Vladimir Oltean wrote:
> >On Tue, 2 Jun 2020 at 11:38, Allan W. Nielsen
> ><allan.nielsen@...rochip.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@....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.
>

Impact in the sense that an IS1 rule for VLAN classification might
'steal' packets from an IS1 rule for QoS classification (since the
TCAM stops after the first matching entry in each lookup)?
Such as:

tc filter add dev swp0 ingress protocol 802.1Q flower vlan_ethtype
ipv4 action skbedit priority 3
tc filter add dev swp0 ingress protocol 802.1Q flower vlan_id 0
vlan_prio 2 action vlan modify id 12 priority 1

The trouble with this TCAM seems to be that it doesn't support the
'pipe' control, so rules on overlapping keys will have unpredictable
results. So if we had these 2 rules in the same lookup in hardware,
then an IPv4 packet tagged with VID 0 and PCP 2 would get classified
to QoS class 3 but would not get retagged to VID 12 PCP 1, right?

So we should error out if the rule's control is 'pipe' (I haven't
checked if we can do that, I hope we can), and basically only 'goto'
something.
Which brings the discussion to "goto where?".
>From the IS1 chain for QoS classification, you could goto the IS1
chain for VLAN classification, ok, that's your example. Is this goto
optional? (i.e. can you goto a PAG in IS2 directly, and in that case
would VLAN classification be skipped in hardware for the packets that
skip it in software?)
Your suggestion of having an entire chain for PAG selection suggests
to me that the rules in chain 10000 (IS1 for QoS classification) would
need to have a non-optional goto to chain 11000 (IS1 for VLAN
classification), which in turn would have a non-optional goto to chain
12000, where there would be a bunch of rules with just a goto action
that selects the IS2 policy. You can't have a goto from chains 10000
or 11000 directly into an IS2 chain (either the default policy of 0 or
a custom one).

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

No, I definitely wasn't trying to suggest that.
If I understand correctly what you're proposing, then I don't
necessarily have anything against breaking compatibility with the
current single-chain layout. Our users will definitely need an extra
book for filling in TCAM filters (the other one being on how to use tc
in general), but on the other hand, at the end of the day, having
things like the actions rigidly assigned to static id's per chain is
the more sane thing to do given the TCAM design.

My only concern was literally that duplicating filter keys in a bunch
of different chains (all of which eventually land in the same TCAM,
just in different lookups) might result in waste of TCAM space. In
Felix, VCAP IS1 has space for 512 half key entries.
Consider just the trivial example when the user wants to do something like:

tc filter add dev swp0 ingress protocol 802.1Q flower \
        vlan_ethtype ipv4 src_ip 192.168.1.1 \
        action skbedit priority 1 \
        action vlan modify id 101 priority 1
tc filter add dev swp0 ingress protocol 802.1Q flower \
        vlan_ethtype ipv4 src_ip 192.168.1.2 \
        action skbedit priority 2 \
        action vlan modify id 101 priority 2

and 100 more rules like that.
With a forced "1-action-per-chain" split, the rules would have to look
like this:

Chain 10000:
tc filter add dev swp0 ingress protocol 802.1Q flower \
        vlan_ethtype ipv4 src_ip 192.168.1.1 \
        action skbedit priority 1
tc filter add dev swp0 ingress protocol 802.1Q flower \
        vlan_ethtype ipv4 src_ip 192.168.1.2 \
        action skbedit priority 2

Chain 11000:
tc filter add dev swp0 ingress protocol 802.1Q flower \
        vlan_ethtype ipv4 src_ip 192.168.1.1 \
        action vlan modify id 101 priority 1
tc filter add dev swp0 ingress protocol 802.1Q flower \
        vlan_ethtype ipv4 src_ip 192.168.1.2 \
        action vlan modify id 101 priority 2

So I would just like to avoid this latter approach consuming 200
entries. It sounds to me like it would need pretty complicated
bookkeeping in the driver.

Also, another topic: would the lookups in VCAP IS2 be ever exposed to
the user? Looking at the VSC7514 datasheet Table 60 "MASK_MODE and
PORT_MASK Combinations" (for how the action vectors resulted from
different lookups combine into a single result), I think it's
relatively difficult to make sense out of it.

And last question: why the large spacing between chain IDs in your example?

Thanks,
-Vladimir

Powered by blists - more mailing lists