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

[Jamal Hadi Salim]

On Fri, Apr 7, 2023 at 12:41 PM Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote:
>
> On Fri, Apr 07, 2023 at 12:22:26PM -0400, Jamal Hadi Salim wrote:
> > > +enum {
> > > +       TC_FP_EXPRESS = 1,
> > > +       TC_FP_PREEMPTIBLE = 2,
> > > +};
> >
> > Suggestion: Add a MAX to this enum (as is traditionally done)..
>
> Max what? This doesn't count anything, it just expresses whether the
> quality of one traffic class, from the Frame Preemption standard's
> perspective, is express or preemptible...
>
> > > @@ -145,13 +149,94 @@ static int mqprio_parse_opt(struct net_device *dev, struct tc_mqprio_qopt *qopt,
> > >         return 0;
> > >  }
> > >
> > > +static const struct
> > > +nla_policy mqprio_tc_entry_policy[TCA_MQPRIO_TC_ENTRY_MAX + 1] = {
> > > +       [TCA_MQPRIO_TC_ENTRY_INDEX]     = NLA_POLICY_MAX(NLA_U32,
> > > +                                                        TC_QOPT_MAX_QUEUE),
> >
> > And use it here...
>
> Where? Above or below the comment? I think you mean below (for the
> policy of TCA_MQPRIO_TC_ENTRY_FP)?
>

That was what I meant. I misread that code thinking it was a nested
TLV range check. If it is only going to be those two specific values,
I understand - but then wondering why you
need a u32; wouldnt a u8 be sufficient? The only reason you would need
a MAX is if it is possible that new values greater than
TC_FP_PREEMPTIBLE showing up in the future.

> > Out of curiosity, could you have more that 16 queues in this spec? I
> > noticed 802.1p mentioned somewhere (typically 3 bits).
>
> "This spec" is IEEE 802.1Q :) It doesn't say how many "queues" (struct
> netdev_queue) there are, and this UAPI doesn't work with those, either.
> The standard defines 8 priority values, groupable in (potentially fewer)
> traffic classes. Linux liked to extend the number of traffic classes to
> 16 (and the skb->priority values are arbitrarily large IIUC) and this is
> where that number 16 came from. The number of 16 traffic classes still
> allows for more than 16 TXQs though.
>
> > Lead up question: if the max is 16 then can preemptible_tcs for example be u32?
>
> I don't understand this question, sorry. preemptible_tcs is declared as
> "unsigned long", which IIUC is at least 32-bit.

I meant: if you only had 16 possible values, meaning 16 bits are
sufficient, (although i may be misunderstanding the goal of those
bits) why not be explicit and use the proper type/size?

cheers,
jamal

> >
> > > +       [TCA_MQPRIO_TC_ENTRY_FP]        = NLA_POLICY_RANGE(NLA_U32,
> > > +                                                          TC_FP_EXPRESS,
> > > +                                                          TC_FP_PREEMPTIBLE),