netdev - Re: [PATCH v4 net-next 6/9] net/sched: mqprio: allow per-TC user input of FP adminStatus

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]

Message-ID: <20230407215252.x3lwkhfp4u6vptxl@skbuf>
Date:   Sat, 8 Apr 2023 00:52:52 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Gerhard Engleder <gerhard@...leder-embedded.com>,
        Amritha Nambiar <amritha.nambiar@...el.com>,
        Ferenc Fejes <ferenc.fejes@...csson.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Roger Quadros <rogerq@...nel.org>,
        Pranavi Somisetty <pranavi.somisetty@....com>,
        Harini Katakam <harini.katakam@....com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Michael Sit Wei Hong <michael.wei.hong.sit@...el.com>,
        Mohammad Athari Bin Ismail <mohammad.athari.ismail@...el.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Jacob Keller <jacob.e.keller@...el.com>,
        linux-kernel@...r.kernel.org, Ferenc Fejes <fejes@....elte.hu>,
        Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH v4 net-next 6/9] net/sched: mqprio: allow per-TC user
 input of FP adminStatus

On Fri, Apr 07, 2023 at 05:40:20PM -0400, Jamal Hadi Salim wrote:
> Yes, it is minor (and usually minor things generate the most emails;->).
> I may be misunderstanding what you mean by "doesnt justify exporting
> something to UAPI"  - those definitions are part of uapi and are
> already being exported.

In my proposed patch set there isn't any TC_FP_MAX. I'm saying it
doesn't help user space, and so, it just pollutes the name space of C
programs with no good reason.

> > The changed MAX value is only a
> > property of the kernel headers that the application is compiled with -
> > it doesn't give the capability of the running kernel.
> 
> Maybe I am missing something or not communicating effectively. What i
> am suggesting is something very trivial:
> 
> enum {
> TC_FP_EXPRESS = 1,
> TC_FP_PREEMPTIBLE = 2,
> TC_FP_MAX
> };
> 
>  [TCA_MQPRIO_TC_ENTRY_FP] = NLA_POLICY_RANGE(NLA_U32,
>     TC_FP_EXPRESS,
>     TC_FP_MAX),
> 
> And in a newer revision with TC_FP_PREEMPTIBLE_WITH_STRIPES:
> 
> enum {
> TC_FP_EXPRESS = 1,
> TC_FP_PREEMPTIBLE = 2,
> TC_FP_PREEMPTIBLE_WITH_STRIPES = 3,
> TC_FP_MAX
> };
> etc.
> 
> Saves you one line in a patch for when TC_FP_PREEMPTIBLE_WITH_STRIPES shows up.

Right, and I don't think that saving me one line in a kernel patch is a
good enough reason to add TC_FP_MAX to include/uapi/, when I can't find
a reason why user space would be interested in TC_FP_MAX anyway.

> > If you think it's valuable to change the type of preemptible_tcs from
> > unsigned long to u16 and that's a more "proper" type, I can do so.
> 
> No, no, it is a matter of taste and opinion. You may have noticed,
> trivial stuff like this gets the most comments and reviews normally(we
> just spent like 4-5 emails on this?). Poteto/potato: IOW, if i was to
> do it i would have used a u16 or u32 because i feel it would be more
> readable. I would have used NLA_U8 because i felt it is more fitting
> and i would have used a max value because it would save me one line in
> a patch in the future. I think weve spent enough electrons on this - I
> defer to you.

Ok, I won't change preemptible_tcs from unsigned long to u32.
Things like for_each_set_bit() take unsigned long, and so, I got used
to using that consistently for small bitfield types.

If there's a second opinion stating that I should prefer the smallest
netlink attribute type that fits the estimated data, then I'll transition
from NLA_U32 to NLA_U8. Otherwise, I won't :) since I would need to
change iproute2 too, and I'd have to re-test more thoroughly to make
sure I don't introduce stupid bugs.