Date:   Mon, 19 Nov 2018 10:57:58 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     "Chopra, Manish" <Manish.Chopra@...ium.com>
Cc:     Pablo Neira Ayuso <pablo@...filter.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "Elior, Ariel" <Ariel.Elior@...ium.com>,
        "michael.chan@...adcom.com" <michael.chan@...adcom.com>,
        "santosh@...lsio.com" <santosh@...lsio.com>,
        "madalin.bucur@....com" <madalin.bucur@....com>,
        "yisen.zhuang@...wei.com" <yisen.zhuang@...wei.com>,
        "salil.mehta@...wei.com" <salil.mehta@...wei.com>,
        "jeffrey.t.kirsher@...el.com" <jeffrey.t.kirsher@...el.com>,
        "tariqt@...lanox.com" <tariqt@...lanox.com>,
        "saeedm@...lanox.com" <saeedm@...lanox.com>,
        "jiri@...lanox.com" <jiri@...lanox.com>,
        "idosch@...lanox.com" <idosch@...lanox.com>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "grygorii.strashko@...com" <grygorii.strashko@...com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...oirfairelinux.com" 
        <vivien.didelot@...oirfairelinux.com>,
        "alexandre.torgue@...com" <alexandre.torgue@...com>,
        "joabreu@...opsys.com" <joabreu@...opsys.com>,
        "linux-net-drivers@...arflare.com" <linux-net-drivers@...arflare.com>,
        "ganeshgr@...lsio.com" <ganeshgr@...lsio.com>
Subject: Re: [PATCH 00/10] add flow_rule infrastructure

Mon, Nov 19, 2018 at 10:19:28AM CET, Manish.Chopra@...ium.com wrote:
>> -----Original Message-----
>> From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On
>> Behalf Of Pablo Neira Ayuso
>> Sent: Friday, November 16, 2018 7:11 AM
>> To: netdev@...r.kernel.org
>> Cc: davem@...emloft.net; thomas.lendacky@....com;
>> f.fainelli@...il.com; Elior, Ariel <Ariel.Elior@...ium.com>;
>> michael.chan@...adcom.com; santosh@...lsio.com;
>> madalin.bucur@....com; yisen.zhuang@...wei.com;
>> salil.mehta@...wei.com; jeffrey.t.kirsher@...el.com; tariqt@...lanox.com;
>> saeedm@...lanox.com; jiri@...lanox.com; idosch@...lanox.com;
>> jakub.kicinski@...ronome.com; peppe.cavallaro@...com;
>> grygorii.strashko@...com; andrew@...n.ch;
>> vivien.didelot@...oirfairelinux.com; alexandre.torgue@...com;
>> joabreu@...opsys.com; linux-net-drivers@...arflare.com;
>> ganeshgr@...lsio.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 !!
>
>
>

Powered by blists - more mailing lists