Re: [PATCH 00/10] add flow_rule infrastructure

[Jiri Pirko <jiri@resnulli.us>]

Mon, Nov 19, 2018 at 10:19:28AM CET, Manish.Chopra@cavium.com wrote:
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> 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 <Ariel.Elior@cavium.com>;
>> 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)
>3.  Could be more such common cases which could be the reason to combine them through single interface ?

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.

>
>Thanks !!
>
>
>