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

[Vladimir Oltean]

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.