On 11/19/18 1:57 AM, Jiri Pirko wrote:

Mon, Nov 19, 2018 at 10:19:28AM CET, Manish.Chopra@cavium.com wrote:

quoted

quoted

-----Original Message-----
From: netdev-owner@vger.kernel.org <redacted> On
Behalf Of Pablo Neira Ayuso
Sent: Friday, November 16, 2018 7:11 AM
To: netdev@vger.kernel.org
Cc: davem@davemloft.net; thomas.lendacky@amd.com;
f.fainelli@gmail.com; Elior, Ariel [off-list ref];
michael.chan@broadcom.com; santosh@chelsio.com;
madalin.bucur@nxp.com; yisen.zhuang@huawei.com;
salil.mehta@huawei.com; jeffrey.t.kirsher@intel.com; tariqt@mellanox.com;
saeedm@mellanox.com; jiri@mellanox.com; idosch@mellanox.com;
jakub.kicinski@netronome.com; peppe.cavallaro@st.com;
grygorii.strashko@ti.com; andrew@lunn.ch;
vivien.didelot@savoirfairelinux.com; alexandre.torgue@st.com;
joabreu@synopsys.com; linux-net-drivers@solarflare.com;
ganeshgr@chelsio.com
Subject: [PATCH 00/10] add flow_rule infrastructure

External Email

This patchset introduces a kernel intermediate representation (IR) to express
ACL hardware offloads, this is heavily based on the existing flow dissector
infrastructure and the TC actions. This IR can be used by different frontend
ACL interfaces such as ethtool_rxnfc and tc to represent ACL hardware
offloads. Main goal is to simplify the development of ACL hardware offloads
for the existing frontend interfaces, the idea is that driver developers do not
need to add one specific parser for each ACL frontend, instead each frontend
can just generate this flow_rule IR and pass it to drivers to populate the
hardware IR.

                .   ethtool_rxnfc   tc
               |       (ioctl)    (netlink)
               |          |         |     translate native
      Frontend |          |         |  interface representation
               |          |         |      to flow_rule IR
               |          |         |
                .         \/        \/
                .         flow_rule IR
               |                |
       Drivers |                | parsing of flow_rule IR
               |                |  to populate hardware IR
               |               \/
                .      hardware IR (driver)

For design and implementation details, please have a look at:

        https://lwn.net/Articles/766695/

As an example, with this patchset, it should be possible to simplify the
existing net/qede driver which already has two parsers to populate the
hardware IR, one for ethtool_rxnfc interface and another for tc.

This batch is composed of 10 patches:

Patch #1 adds the flow_match structure, this includes the
         flow_rule_match_key() interface to check for existing selectors
         that are in used in the rule and the flow_rule_match_*()
         functions to fetch the selector value and the mask. This
         also introduces the initial flow_rule structure skeleton to
         avoid a follow up patch that would update the same LoCs.

Patch #2 makes changes to packet edit parser of mlx5e driver, to prepare
         introduction of the new flow_action to mangle packets.

Patch #3 Introduce flow_action infrastructure. This infrastructure is
         based on the TC actions. Patch #8 extends it so it also
         supports two new actions that are only available through the
         ethtool_rxnfc interface.

Patch #4 Add function to translate TC action to flow_action from
         cls_flower.

Patch #5 Add infrastructure to fetch statistics into container structure
         and synchronize them to TC actions from cls_flower. Another
         preparation patch before patch #7, so we can stop exposing the
         TC action native layout to the drivers.

Patch #6 Use flow_action infrastructure from drivers.

Patch #7 Do not expose TC actions to drivers anymore, now that drivers
         have been converted to use the flow_action infrastructure after
         patch #5.

Patch #8 Support to wake-up-on-lan and queue actions for the flow_action
         infrastructure, two actions supported by NICs. This is used by
         the ethtool_rx_flow interface.

Patch #9 Add a function to translate from ethtool_rx_flow_spec structure
         to the flow_action structure. This is a simple enough for its
         first client: the ethtool_rxnfc interface in the bcm_sf2 driver.

Patch #10 Update bcm_sf2 to use this new translator function and
          update codebase to configure hardware IR using the
          flow_action representation. This will allow later development
          of cls_flower using the same codebase from the driver.

This patchset has passed here functional tests of the codepath that generates
the flow_rule structure and the functions to implement the parsers that
populate the hardware IR.

Thanks.

Pablo Neira Ayuso (10):
  flow_dissector: add flow_rule and flow_match structures and use them
  net/mlx5e: support for two independent packet edit actions
  flow_dissector: add flow action infrastructure
  cls_api: add translator to flow_action representation
  cls_flower: add statistics retrieval infrastructure and use it
  drivers: net: use flow action infrastructure
  cls_flower: don't expose TC actions to drivers anymore
  flow_dissector: add wake-up-on-lan and queue to flow_action
  flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure
translator
  dsa: bcm_sf2: use flow_rule infrastructure

 drivers/net/dsa/bcm_sf2_cfp.c                      | 103 ++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c       | 252 +++----
 .../net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c   | 450 ++++++-------
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 178 ++---
 drivers/net/ethernet/intel/iavf/iavf_main.c        | 195 +++---
 drivers/net/ethernet/intel/igb/igb_main.c          |  64 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    | 743 ++++++++++--------
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c |   2 +-
 .../net/ethernet/mellanox/mlxsw/spectrum_flower.c  | 259 ++++---
drivers/net/ethernet/netronome/nfp/flower/action.c | 196 +++---
drivers/net/ethernet/netronome/nfp/flower/match.c  | 416 ++++++------
 .../net/ethernet/netronome/nfp/flower/offload.c    | 151 ++---
 drivers/net/ethernet/qlogic/qede/qede_filter.c     |  93 ++-
 include/net/flow_dissector.h                       | 185 +++++
 include/net/pkt_cls.h                              |  23 +-
 net/core/flow_dissector.c                          | 341 ++++++++++
 net/sched/cls_api.c                                | 113 ++++
 net/sched/cls_flower.c                             |  42 +-
 18 files changed, 2120 insertions(+), 1686 deletions(-)

--
2.11.0

Today, we have different kernel interfaces(aRFS/ethtool/tc) to do flow offloading.
One big question arises in my mind that why we need these all different interfaces to do flow offloading and
why can't they just be combined into a single kernel interface somehow.
Having these flow offloading done through various hooks makes driver implementation really complex.

Specially when looking at ethtool/tc flows most of the use cases seems common to me -

1.  Drop action can be done on tuples from both tc/ethtool flows.
2.  Flow redirection is done from both ethtool/tc
    (May be with just slight difference ? as Ethtool does steering on PF and VF/VF-queues
     but tc doesn't have redirection on queues which can be extended over tc)

We discussed several times before with Jiri at various conferences, and
I unfortunately don't have the time to look into doing it, but if
cls_flower gained support for ethtool_rx_flow_spec::location, we should
be good in terms of feature parity. Pablo also identified the
RX_CLS_FLOW_WAKE as something that needs to be supported, but this is a
simple thing that could be implemented as a special tc action (should?)

quoted

3.  Could be more such common cases which could be the reason to combine them through single interface ?

Main reason IMHO for going to an unified interface is that ultimately
the piece of HW that implements filtering/action is shared across all
programming interfaces. If they all have about roughly the same feature
set, yet some slight feature deviation or semantic differences, this
gets confusing for driver writers and users.

Sure you can make one interface feature complete. But that does not mean
you can remove the others. At least not easily. You woul break UAPI.

Indeed, though we can have a couple of strategies to unify and later
deprecate them:

1) Go after each application and make them use the chosen interface,
e.g: the tc/cls_flower UAPI, this does not really scale well because
while we can easily get mainstream reference applications converted such
as ethtool, iptables, xtables, we don't know what people have designed
around the UAPI interface, and usually this also does not mean bindings
to different languages (e.g: ethtool python) would be converted. Because
we have the application converted to the native interface, there is no
loss in translation or performance penalty from going to the kernel's IR.

2) We convert the non-chosen interfaces (e.g: ethtool, *tables) from
within the kernel through the chosen IR and programming interface, that
way we can easily identify which applications are still using the
deprecated interface and issue an appropriate warning in kernel version
XZY for instance. The drawback is that you have a translation so that
could mean loss of information and/or performance penalty if rule
insertion/deletion is a hot path (which is unfortunately likely the case).
-- 
Florian