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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 26 Oct 2019 12:06:43 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Roman Mashak <mrv@...atatu.com>
Cc:     Vlad Buslov <vladbu@...lanox.com>, Jiri Pirko <jiri@...nulli.us>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "mleitner@...hat.com" <mleitner@...hat.com>,
        "dcaratti@...hat.com" <dcaratti@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next 00/13] Control action percpu counters allocation
 by netlink flag

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