Date:   Tue, 25 Feb 2020 18:06:59 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Joergen Andreasen <joergen.andreasen@...rochip.com>,
        "Allan W. Nielsen" <allan.nielsen@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        netdev <netdev@...r.kernel.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "Y.b. Lu" <yangbo.lu@....com>, Po Liu <po.liu@....com>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...sch.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 00/10] Wire up Ocelot tc-flower to Felix DSA

Hi Horatiu,

On Tue, 25 Feb 2020 at 16:45, Horatiu Vultur
<horatiu.vultur@...rochip.com> wrote:
>
> The 02/24/2020 15:08, Vladimir Oltean wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > From: Vladimir Oltean <vladimir.oltean@....com>
> >
> > This series is a proposal on how to wire up the tc-flower callbacks into
> > DSA. The example taken is the Microchip Felix switch, whose core
> > implementation is actually located in drivers/net/ethernet/mscc/.
> >
> > The proposal is largely a compromise solution. The DSA middle layer
> > handles just enough to get to the interesting stuff (FLOW_CLS_REPLACE,
> > FLOW_CLS_DESTROY, FLOW_CLS_STATS), but also thin enough to let drivers
> > decide what filter keys and actions they support without worrying that
> > the DSA middle layer will grow exponentially. I am far from being an
> > expert, so I am asking reviewers to please voice your opinion if you
> > think it can be done differently, with better results.
> >
> > The bulk of the work was actually refactoring the ocelot driver enough
> > to allow the VCAP (Versatile Content-Aware Processor) code for vsc7514
> > and the vsc9959 switch cores to live together.
> >
> > Flow block offloads have not been tested yet, only filters attached to a
> > single port. It might be as simple as replacing ocelot_ace_rule_create
> > with something smarter, it might be more complicated, I haven't tried
> > yet.
> >
> > I should point out that the tc-matchall filter offload is not
> > implemented in the same manner in current mainline. Florian has already
> > went all the way down into exposing actual per-action callbacks,
> > starting with port mirroring. Because currently only mirred is supported
> > by this DSA mid layer, everything else will return -EOPNOTSUPP. So even
> > though ocelot supports matchall (aka port-based) policers, we don't have
> > a call path to call into them.  Personally I think that this is not
> > going to scale for tc-matchall (there may be policers, traps, drops,
> > VLAN retagging, etc etc), and that we should consider replacing the port
> > mirroring callbacks in DSA with simple accessors to
> > TC_CLSMATCHALL_REPLACE and TC_CLSMATCHALL_DESTROY, just like for flower.
> > That means that drivers which currently implement the port mirroring
> > callbacks will need to have some extra "if" conditions now, in order for
> > them to call their port mirroring implementations.
> >
> > Vladimir Oltean (9):
> >   net: mscc: ocelot: simplify tc-flower offload structures
> >   net: mscc: ocelot: replace "rule" and "ocelot_rule" variable names
> >     with "ace"
> >   net: mscc: ocelot: return directly in
> >     ocelot_cls_flower_{replace,destroy}
> >   net: mscc: ocelot: don't rely on preprocessor for vcap key/action
> >     packing
> >   net: mscc: ocelot: remove port_pcs_init indirection for VSC7514
> >   net: mscc: ocelot: parameterize the vcap_is2 properties
> >   net: dsa: Refactor matchall mirred action to separate function
> >   net: dsa: Add bypass operations for the flower classifier-action
> >     filter
> >   net: dsa: felix: Wire up the ocelot cls_flower methods
> >
> > Yangbo Lu (1):
> >   net: mscc: ocelot: make ocelot_ace_rule support multiple ports
> >
> >  drivers/net/dsa/ocelot/felix.c            |  31 ++
> >  drivers/net/dsa/ocelot/felix.h            |   3 +
> >  drivers/net/dsa/ocelot/felix_vsc9959.c    | 126 ++++++
> >  drivers/net/ethernet/mscc/ocelot.c        |  20 +-
> >  drivers/net/ethernet/mscc/ocelot_ace.c    | 472 +++++++++++-----------
> >  drivers/net/ethernet/mscc/ocelot_ace.h    |  26 +-
> >  drivers/net/ethernet/mscc/ocelot_board.c  | 151 +++++--
> >  drivers/net/ethernet/mscc/ocelot_flower.c | 256 ++++--------
> >  drivers/net/ethernet/mscc/ocelot_tc.c     |  22 +-
> >  drivers/net/ethernet/mscc/ocelot_vcap.h   | 403 ------------------
> >  include/net/dsa.h                         |   6 +
> >  include/soc/mscc/ocelot.h                 |  20 +-
> >  include/soc/mscc/ocelot_vcap.h            | 205 ++++++++++
> >  net/dsa/slave.c                           | 128 ++++--
> >  14 files changed, 954 insertions(+), 915 deletions(-)
> >  delete mode 100644 drivers/net/ethernet/mscc/ocelot_vcap.h
> >  create mode 100644 include/soc/mscc/ocelot_vcap.h
> >
> > --
> > 2.17.1
> >
>
> Hi Vladimir,
>
> From my point, it looks OK the changes to Ocelot.
> Also I managed to run some tests and they are passing.
>

Thanks, I really appreciate you trying out the patches. Having the
VCAP IS2 already implemented and working for Felix with minimal
changes is also huge.
I'll wait for further comments from the flow offload people, DSA
people or anybody else, and if the timeout expires I'll send a v2
addressing the Kbuild and David's reverse Christmas tree complaints.

> --
> /Horatiu

Regards,
-Vladimir

Powered by blists - more mailing lists