Message-ID: <88b3c1de-b11c-ee9b-e251-43e1ac47592a@solarflare.com>
Date:   Wed, 15 May 2019 20:39:15 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
        "Pablo Neira Ayuso" <pablo@...filter.org>,
        David Miller <davem@...emloft.net>
CC:     netdev <netdev@...r.kernel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Vishal Kulkarni <vishal@...lsio.com>
Subject: [RFC PATCH v2 net-next 0/3] flow_offload: Re-add per-action
 statistics

When the flow_offload infrastructure was added, per-action statistics,
 which were previously possible for drivers to support in TC offload,
 were not plumbed through, perhaps because the drivers in the tree did
 not implement them.
In TC (and in the previous offload API) statistics are per-action,
 though generally only on 'delivery' actions like mirred, ok and shot.
 Actions also have an index, which may be supplied by the user, which
 allows the sharing of entities such as counters between multiple rules.
 (These indices are per action type, so 'drop index 1' and 'mirred index
 1' are different actions.  However, this means that a mirror and a
 redirect action are in the same index namespace, since they're both
 mirred actions.)  The existing driver implementations did not support
 this, however, instead allocating a single counter per rule.  The
 flow_offload API did not support this either, as (a) the action index
 never reached the driver, and (b) the TC_CLSFLOWER_STATS callback was
 only able to return a single set of stats which were added to all
 counters for actions on the rule.  Patch #1 of this series fixes (a) by
 storing tcfa_index in a new action_index member of struct
 flow_action_entry, while patch #2 fixes (b) by changing the .stats
 member of struct tc_cls_flower_offload from a single set of stats to an
 array.  Existing drivers are changed to retain their current behaviour,
 by adding their one set of stats to every element of this array (note
 however that this will behave oddly if an action appears more than once
 on a rule; I'm not sure whether that's a problem).  I have not tested
 these driver changes on corresponding hardware.
Patch #3 ensures that flow_offload.h is buildable in isolation, rather
 than relying on linux/kernel.h having already been included elsewhere;
 this is logically separable from the rest of the series.

A point for discussion: would it be better if, instead of the tcfa_index
 (for which the driver has to know the rules about which flow_action
 types share a namespace), we had some kind of globally unique cookie?
 In the same way that rule->cookie is really a pointer, could we use the
 address of the TC-internal data structure representing the action?  Do
 rules that share an action all point to the same struct tc_action in
 their tcf_exts, for instance?

Changed in v2:
* use array instead of new callback for getting stats
* remove CVLAN patch (posted separately for net)
* add header inclusion fix

Edward Cree (3):
  flow_offload: copy tcfa_index into flow_action_entry
  flow_offload: restore ability to collect separate stats per action
  flow_offload: include linux/kernel.h from flow_offload.h

 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c     |  6 ++++--
 .../net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c | 10 ++++++----
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c  |  4 +++-
 .../ethernet/mellanox/mlxsw/spectrum_flower.c    |  5 +++--
 .../net/ethernet/netronome/nfp/flower/offload.c  |  8 ++++++--
 .../net/ethernet/netronome/nfp/flower/qos_conf.c |  6 ++++--
 include/net/flow_offload.h                       | 13 +++++++++++--
 include/net/pkt_cls.h                            | 13 +++++++++----
 net/core/flow_offload.c                          | 16 ++++++++++++++++
 net/sched/cls_api.c                              |  1 +
 net/sched/cls_flower.c                           |  9 ++++++---
 net/sched/cls_matchall.c                         |  7 +++++--
 12 files changed, 74 insertions(+), 24 deletions(-)

Powered by blists - more mailing lists