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.
>+};
>+
>+static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats)
>+{
>+ if (WARN_ON_ONCE(hw_stats > TCA_ACT_HW_STATS_ANY))
>+ return FLOW_ACTION_HW_STATS_DONT_CARE;
>+
>+ return tc_act_hw_stats_array[hw_stats];
>+}
>+
> int tc_setup_flow_action(struct flow_action *flow_action,
> const struct tcf_exts *exts)
> {
> struct tc_action *act;
> int i, j, k, err = 0;
>
>- BUILD_BUG_ON(TCA_ACT_HW_STATS_ANY != FLOW_ACTION_HW_STATS_ANY);
>- BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE);
>- BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED != FLOW_ACTION_HW_STATS_DELAYED);
>-
> if (!exts)
> return 0;
>
>@@ -3546,7 +3557,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> if (err)
> goto err_out_locked;
>
>- entry->hw_stats = act->hw_stats;
>+ entry->hw_stats = tc_act_hw_stats(act->hw_stats);
>
> if (is_tcf_gact_ok(act)) {
> entry->id = FLOW_ACTION_ACCEPT;
>@@ -3614,7 +3625,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> entry->mangle.mask = tcf_pedit_mask(act, k);
> entry->mangle.val = tcf_pedit_val(act, k);
> entry->mangle.offset = tcf_pedit_offset(act, k);
>- entry->hw_stats = act->hw_stats;
>+ entry->hw_stats = tc_act_hw_stats(act->hw_stats);
> entry = &flow_action->entries[++j];
> }
> } else if (is_tcf_csum(act)) {
>--
>2.20.1
>
