Re: [PATCH net-next 00/13] Control action percpu counters allocation by netlink flag

[Vlad Buslov <vladbu@mellanox.com>]

On Sat 26 Oct 2019 at 19:06, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2019-10-26 10:52 a.m., Roman Mashak wrote:
> [..]
>>
>> But why do we need to have two attributes, one at the root level
>> TCA_ROOT_FLAGS and the other at the inner TCA_ACT_* level, but in fact
>> serving the same purpose -- passing flags for optimizations?
>>
>>
>> The whole nest of action attributes including root ones is passed as 3rd
>> argument of tcf_exts_validate(), so it can be validated and extracted at
>> that level and passed to tcf_action_init_1() as pointer to 32-bit flag,
>> admittedly it's ugly given the growing number of arguments to
>> tcf_action_init_1(). With old iproute2 the pointer will always be NULL,
>> so I think backward compatibilty will be preserved.
>
> Note: we only call tcf_action_init_1() at that level for very
> old policer api for backward compatibility reasons. I think what
> would make sense is to be able to call tcf_action_init()(the else
> statement in tcf_exts_validate()) from that level with a global flag
> but for that we would need to introduce something like TCA_ROOT_FLAGS
> under this space:
> ---
> enum {
>         TCA_UNSPEC,
>         TCA_KIND,
>         TCA_OPTIONS,
>         TCA_STATS,
>         TCA_XSTATS,
>         TCA_RATE,
>         TCA_FCNT,
>         TCA_STATS2,
>         TCA_STAB,
>         TCA_PAD,
>         TCA_DUMP_INVISIBLE,
>         TCA_CHAIN,
>         TCA_HW_OFFLOAD,
>         TCA_INGRESS_BLOCK,
>         TCA_EGRESS_BLOCK,
>         __TCA_MAX
> };
> ---
>
> which would be a cleaner solution but would require
> _a lot more code_ both in user/kernel.
> Thats why i feel Vlad's suggestion is a reasonable compromise
> because it gets rid of the original issue of per-specific-action
> TLVs.
>
> On optimization:
> The current suggestion from Vlad is a bit inefficient,
> example, if was trying to batch 100 actions i now have 1200
> bytes of overhead instead of 12 bytes.
>
> cheers,
> jamal

Hmm, yes, this looks quite redundant. At the same time having basically
same flags in two different spaces is ugly. Maybe correct approach would
be not to add act API at all and only extend tcf_action_init_1() to be
used from cls API? I don't see a use for act API at the moment because
the only flag value (skip percpu allocation) is hardware offloads
specific and such clients generally create action through cls new filter
API. WDYT?

Regards,
Vlad