Tue, May 05, 2020 at 12:08:14PM CEST, pablo@xxxxxxxxxxxxx wrote:
>On Tue, May 05, 2020 at 11:49:00AM +0200, Jiri Pirko wrote:
>> Tue, May 05, 2020 at 11:21:59AM CEST, pablo@xxxxxxxxxxxxx wrote:
>> >This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver
>> >that the frontend does not need counters, this hw stats type request
>> >never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests
>> >the driver to disable the stats, however, if the driver cannot disable
>> >counters, it bails out.
>> >
>> >Remove BUILD_BUG_ON since TCA_ACT_HW_STATS_* don't map 1:1 with
>> >FLOW_ACTION_HW_STATS_* anymore. Add tc_act_hw_stats() to perform the
>> >mapping between TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*
>> >
>> >Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
>> >Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
>> >---
>> >This is a follow up after "net: flow_offload: skip hw stats check for
>> >FLOW_ACTION_HW_STATS_DISABLED". This patch restores the netfilter hardware
>> >offloads.
>> >
>> > include/net/flow_offload.h | 9 ++++++++-
>> > net/sched/cls_api.c | 23 +++++++++++++++++------
>> > 2 files changed, 25 insertions(+), 7 deletions(-)
>> >
>> >diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> >index 3619c6acf60f..0c75163699f0 100644
>> >--- a/include/net/flow_offload.h
>> >+++ b/include/net/flow_offload.h
>> >@@ -164,12 +164,15 @@ enum flow_action_mangle_base {
>> > };
>> >
>> > enum flow_action_hw_stats_bit {
>> >+ FLOW_ACTION_HW_STATS_DISABLED_BIT,
>> > FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
>> > FLOW_ACTION_HW_STATS_DELAYED_BIT,
>> > };
>> >
>> > enum flow_action_hw_stats {
>> >- FLOW_ACTION_HW_STATS_DISABLED = 0,
>> >+ FLOW_ACTION_HW_STATS_DONT_CARE = 0,
>> >+ FLOW_ACTION_HW_STATS_DISABLED =
>> >+ BIT(FLOW_ACTION_HW_STATS_DISABLED_BIT),
>> > FLOW_ACTION_HW_STATS_IMMEDIATE =
>> > BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT),
>> > FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT),
>> >@@ -325,7 +328,11 @@ __flow_action_hw_stats_check(const struct flow_action *action,
>> > return true;
>> > if (!flow_action_mixed_hw_stats_check(action, extack))
>> > return false;
>> >+
>> > action_entry = flow_action_first_entry_get(action);
>> >+ if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DONT_CARE)
>> >+ return true;
>> >+
>> > if (!check_allow_bit &&
>> > action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
>> > NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
>> >diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> >index 55bd1429678f..8ddc16a1ca68 100644
>> >--- a/net/sched/cls_api.c
>> >+++ b/net/sched/cls_api.c
>> >@@ -3523,16 +3523,27 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
>> > #endif
>> > }
>> >
>> >+static enum flow_action_hw_stats tc_act_hw_stats_array[] = {
>> >+ [0] = FLOW_ACTION_HW_STATS_DISABLED,
>> >+ [TCA_ACT_HW_STATS_IMMEDIATE] = FLOW_ACTION_HW_STATS_IMMEDIATE,
>> >+ [TCA_ACT_HW_STATS_DELAYED] = FLOW_ACTION_HW_STATS_DELAYED,
>> >+ [TCA_ACT_HW_STATS_ANY] = FLOW_ACTION_HW_STATS_ANY,
>>
>> TCA_ACT_HW_* are bits. There can be a combination of those according
>> to the user request. For 2 bits, it is not problem, but I have a
>> patchset in pipes adding another type.
>> Then you need to have 1:1 mapping in this array for all bit
>> combinations. That is not right.
>>
>> How about putting DISABLED to the at the end of enum
>> flow_action_hw_stats? They you can just map 0 here to FLOW_ACTION_HW_STATS_DISABLED
>> as an exception, but the bits you can take 1:1.
>
>Another possibility is to remove this mapping code is to update tc
>UAPI to get it aligned with this update.
>
>This UAPI is only available in the few 5.7.0-rc kernel releases,
>I think only developers are using this at this stage?
Hmm, I think that TC-wise, the UAPI as is makes perfect sense. There is
no don't care. Each bit allows to indicate user what he is interested
in.
I would prefer to convert to flow_offload (as we do anyway) in a format
that is comfortable for flow_offload (given the fact it is used not only
for TC).
Did you check the solution I suggested? Above. Seems to be quite
straightforward.
