Re: [PATCH iproute2 net-next v2] tc: implement support for action flags

[Vlad Buslov <vladbu@mellanox.com>]

On Mon 04 Nov 2019 at 18:49, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> On Wed, Oct 30, 2019 at 7:20 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Implement setting and printing of action flags with single available flag
>> value "no_percpu" that translates to kernel UAPI TCA_ACT_FLAGS value
>> TCA_ACT_FLAGS_NO_PERCPU_STATS. Update man page with information regarding
>> usage of action flags.
>>
>> Example usage:
>>
>>  # tc actions add action gact drop no_percpu
>>  # sudo tc actions list action gact
>>  total acts 1
>>
>>         action order 0: gact action drop
>>          random type none pass val 0
>>          index 1 ref 1 bind 0
>>         no_percpu
>
> would be nice to just call it no_percpu_stats to match the flag name ?.
> Current choice of word leaves room for possible conflict with other
> percpu flags in the future..

I didn't find any other places in action code that uses percpu allocator
directly and decided to name it "no_percpu" since it seems to me that
iproute2 developers prefer brevity when naming such things (notice "val"
and "ref" in example output).

>
>
>>
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> ---
>>
>> Notes:
>>     Changes V1 -> V2:
>>
>>     - Rework the change to use action API TCA_ACT_FLAGS instead of
>>       per-action flags implementation.
>>
>>  include/uapi/linux/pkt_cls.h |  5 +++++
>>  man/man8/tc-actions.8        | 14 ++++++++++++++
>>  tc/m_action.c                | 19 +++++++++++++++++++
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index a6aa466fac9e..c6ad22f76ede 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -16,9 +16,14 @@ enum {
>>         TCA_ACT_STATS,
>>         TCA_ACT_PAD,
>>         TCA_ACT_COOKIE,
>> +       TCA_ACT_FLAGS,
>>         __TCA_ACT_MAX
>>  };
>>
>> +#define TCA_ACT_FLAGS_NO_PERCPU_STATS 1 /* Don't use percpu allocator for
>> +                                        * actions stats.
>> +                                        */
>> +
>>  #define TCA_ACT_MAX __TCA_ACT_MAX
>>  #define TCA_OLD_COMPAT (TCA_ACT_MAX+1)
>>  #define TCA_ACT_MAX_PRIO 32
>> diff --git a/man/man8/tc-actions.8 b/man/man8/tc-actions.8
>> index f46166e3f685..bee59f7247fa 100644
>> --- a/man/man8/tc-actions.8
>> +++ b/man/man8/tc-actions.8
>> @@ -47,6 +47,8 @@ actions \- independently defined actions in tc
>>  ] [
>>  .I COOKIESPEC
>>  ] [
>> +.I FLAGS
>> +] [
>>  .I CONTROL
>>  ]
>>
>> @@ -71,6 +73,10 @@ ACTNAME
>>  :=
>>  .BI cookie " COOKIE"
>>
>> +.I FLAGS
>> +:=
>> +.I no_percpu
>> +
>>  .I ACTDETAIL
>>  :=
>>  .I ACTNAME ACTPARAMS
>> @@ -186,6 +192,14 @@ As such, it can be used as a correlating value for maintaining user state.
>>  The value to be stored is completely arbitrary and does not require a specific
>>  format. It is stored inside the action structure itself.
>>
>> +.TP
>> +.I FLAGS
>> +Action-specific flags. Currently, the only supported flag is
>> +.I no_percpu
>> +which indicates that action is expected to have minimal software data-path
>> +traffic and doesn't need to allocate stat counters with percpu allocator.
>> +This option is intended to be used by hardware-offloaded actions.
>> +
>>  .TP
>>  .BI since " MSTIME"
>>  When dumping large number of actions, a millisecond time-filter can be
>> diff --git a/tc/m_action.c b/tc/m_action.c
>> index 36c744bbe374..4da810c8c0aa 100644
>> --- a/tc/m_action.c
>> +++ b/tc/m_action.c
>> @@ -250,6 +250,16 @@ done0:
>>                                 addattr_l(n, MAX_MSG, TCA_ACT_COOKIE,
>>                                           &act_ck, act_ck_len);
>>
>> +                       if (*argv && strcmp(*argv, "no_percpu") == 0) {
>> +                               struct nla_bitfield32 flags =
>> +                                       { TCA_ACT_FLAGS_NO_PERCPU_STATS,
>> +                                         TCA_ACT_FLAGS_NO_PERCPU_STATS };
>> +
>> +                               addattr_l(n, MAX_MSG, TCA_ACT_FLAGS, &flags,
>> +                                         sizeof(struct nla_bitfield32));
>> +                               NEXT_ARG_FWD();
>> +                       }
>> +
>>                         addattr_nest_end(n, tail);
>>                         ok++;
>>                 }
>> @@ -318,6 +328,15 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg)
>>                                            strsz, b1, sizeof(b1)));
>>                 print_string(PRINT_FP, NULL, "%s", _SL_);
>>         }
>> +       if (tb[TCA_ACT_FLAGS]) {
>> +               struct nla_bitfield32 *flags = RTA_DATA(tb[TCA_ACT_FLAGS]);
>> +
>> +               if (flags->selector & TCA_ACT_FLAGS_NO_PERCPU_STATS)
>> +                       print_bool(PRINT_ANY, "no_percpu", "\tno_percpu",
>> +                                  flags->value &
>> +                                  TCA_ACT_FLAGS_NO_PERCPU_STATS);
>> +               print_string(PRINT_FP, NULL, "%s", _SL_);
>> +       }
>>
>>         return 0;
>>  }
>> --
>> 2.21.0
>>