Date:   Mon, 20 Feb 2023 17:01:57 +0100
From:   Péter Antal <peti.antal99@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, John Fastabend <john.fastabend@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Ferenc Fejes <fejes@....elte.hu>,
        Ferenc Fejes <ferenc.fejes@...csson.com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Péter Antal <antal.peti99@...il.com>
Subject: Re: [PATCH iproute2] man: tc-mqprio: extend prio-tc-queue mapping
 with examples

Hi Vladimir,

Vladimir Oltean <vladimir.oltean@....com> ezt írta (időpont: 2023.
febr. 20., H, 16:29):
>
> Hi Péter,
>
> On Mon, Feb 20, 2023 at 04:05:48PM +0100, Péter Antal wrote:
> > The current mqprio manual is not detailed about queue mapping
> > and priorities, this patch adds some examples to it.
> >
> > Suggested-by: Ferenc Fejes <fejes@....elte.hu>
> > Signed-off-by: Péter Antal <peti.antal99@...il.com>
> > ---
>
> I think it's great that you are doing this. However, with all due respect,
> this conflicts with the man page restructuring I am already doing for the
> frame preemption work. Do you mind if I fix up some things and I pick your
> patch up, and submit it as part of my series? I have some comments below.

That's all right, thank you for doing this, just please carry my
signoff as co-developer if possible.
I agree with most of your suggestions.

>
> >  man/man8/tc-mqprio.8 | 96 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> >
> > diff --git a/man/man8/tc-mqprio.8 b/man/man8/tc-mqprio.8
> > index 4b9e942e..16ecb9a1 100644
> > --- a/man/man8/tc-mqprio.8
> > +++ b/man/man8/tc-mqprio.8
> > @@ -98,6 +98,7 @@ belong to an application. See kernel and cgroup documentation for details.
> >  .TP
> >  num_tc
> >  Number of traffic classes to use. Up to 16 classes supported.
> > +You cannot have more classes than queues
>
> More impersonal: "There cannot be more traffic classes than TX queues".
>
> >
> >  .TP
> >  map
> > @@ -119,6 +120,8 @@ Set to
> >  to support hardware offload. Set to
> >  .B 0
> >  to configure user specified values in software only.
> > +The default value of this parameter is
> > +.B 1
> >
> >  .TP
> >  mode
> > @@ -146,5 +149,98 @@ max_rate
> >  Maximum value of bandwidth rate limit for a traffic class.
> >
> >
> > +.SH EXAMPLE
> > +
> > +The following example shows how to attach priorities to 4 traffic classes ("num_tc 4"),
> > +and then how to pair these traffic classes with 4 hardware queues with mqprio,
>
> "with mqprio" is understated I think, this is the mqprio man page
>
> > +with hardware coordination ("hw 1", or does not specified, because 1 is the default value).
>
> Just leave it at that, "hw 1".
>
> > +Traffic class 0 (tc0) is mapped to hardware queue 0 (q0), tc1 is mapped to q1,
> > +tc2 is mapped to q2, and tc3 is mapped q3.
>
> Would prefer saying TC0, TXQ0 etc if you don't mind.
>
> > +
> > +.EX
> > +# tc qdisc add dev eth0 root mqprio \
> > +              num_tc 4 \
> > +              map 0 0 0 0 1 1 1 1 2 2 2 2 3 3 3 3 \
> > +              queues 1@0 1@1 1@2 1@3 \
> > +              hw 1
> > +.EE
> > +
> > +The next example shows how to attach priorities to 3 traffic classes ("num_tc 3"),
> > +and how to pair these traffic classes with 4 queues,
> > +without hardware coordination ("hw 0").
> > +Traffic class 0 (tc0) is mapped to hardware queue 0 (q0), tc1 is mapped to q1,
> > +tc2 and is mapped to q2 and q3, where the queue selection between these
> > +two queues is somewhat randomly decided.
>
> Would rather say that packets are hashed arbitrarily between TXQ3 and
> TXQ4, which have the same scheduling priority.

If I understand it correctly, this depends on the "hw" parameter.
So if the driver handles this (hw 1), then it may prioritize a queue
over another,
even if they have the same priority.

>
> We should probably clarify what "hardware coordination" means, exactly.
> Without hardware coordination, the device driver is not notified of the
> number of traffic classes and their mapping to TXQs. The device is not
> expected to prioritize between traffic classes without hardware
> coordination.
>
> We should also probably clarify that with hardware coordination, the
> device driver can install a different TXQ configuration than requested,
> and that the default TC to TXQ mapping is:
>
> TC 0: 0 queues @ offset 0
> TC 1: 0 queues @ offset 0
> ...
> TC N: 0 queues @ offset 0
>

First time I see hardware coordination in this manual is the hw parameter,
so I think we should clarify this there.

> Should also probably clarify that there is a default prio:tc map of:
>
> .prio_tc_map = { 0, 1, 2, 3, 4, 5, 6, 7, 0, 1, 1, 1, 3, 3, 3, 3 },
>
> hmm... you gave me more work than I was intending to do :)

Sorry about that :)

>
> > +
> > +.EX
> > +# tc qdisc add dev eth0 root mqprio \
> > +              num_tc 3 \
> > +              map 0 0 0 0 1 1 1 1 2 2 2 2 2 2 2 2 \
> > +              queues 1@0 1@1 2@2 \
> > +              hw 0
> > +.EE
> > +
> > +
> > +In both cases from above the priority values from 0 to 3 (prio0-3) are
> > +mapped to tc0, prio4-7 are mapped to tc1, and the
> > +prio8-11 are mapped to tc2 ("map" attribute). The last four priority values
> > +(prio12-15) are mapped in different ways in the two examples.
> > +They are mapped to tc3 in the first example and mapped to tc2 in the second example.
> > +The values of these two examples are the following:
> > +
> > + ┌────┬────┬───────┐  ┌────┬────┬────────┐
> > + │Prio│ tc │ queue │  │Prio│ tc │  queue │
> > + ├────┼────┼───────┤  ├────┼────┼────────┤
> > + │  0 │  0 │     0 │  │  0 │  0 │      0 │
> > + │  1 │  0 │     0 │  │  1 │  0 │      0 │
> > + │  2 │  0 │     0 │  │  2 │  0 │      0 │
> > + │  3 │  0 │     0 │  │  3 │  0 │      0 │
> > + │  4 │  1 │     1 │  │  4 │  1 │      1 │
> > + │  5 │  1 │     1 │  │  5 │  1 │      1 │
> > + │  6 │  1 │     1 │  │  6 │  1 │      1 │
> > + │  7 │  1 │     1 │  │  7 │  1 │      1 │
> > + │  8 │  2 │     2 │  │  8 │  2 │ 2 or 3 │
> > + │  9 │  2 │     2 │  │  9 │  2 │ 2 or 3 │
> > + │ 10 │  2 │     2 │  │ 10 │  2 │ 2 or 3 │
> > + │ 11 │  2 │     2 │  │ 11 │  2 │ 2 or 3 │
> > + │ 12 │  3 │     3 │  │ 12 │  2 │ 2 or 3 │
> > + │ 13 │  3 │     3 │  │ 13 │  2 │ 2 or 3 │
> > + │ 14 │  3 │     3 │  │ 14 │  2 │ 2 or 3 │
> > + │ 15 │  3 │     3 │  │ 15 │  2 │ 2 or 3 │
> > + └────┴────┴───────┘  └────┴────┴────────┘
> > +       example1             example2
>
> What would you say if we didn't put the 2 examples side by side, but
> kept them separately? Also, I get the feeling that the verbiage above
> the tables is a bit too much. Having the tables is already good enough.
>
> > +
> > +Another example of queue mapping is the following.
> > +There are 5 traffic classes, and there are 8 hardware queues.
>
> I think for maintainability, the examples should be fairly independent,
> because they might be moved around in the future. This story-like
> "the following example" -> "the next example" -> "another example"
> doesn't really work.
>
> > +.EX
> > +# tc qdisc add dev eth0 root mqprio \
> > +              num_tc 5 \
> > +              map 0 0 0 1 1 1 1 2 2 3 3 4 4 4 4 4 \
> > +              queues 1@0 2@1 1@3 1@4 3@5
>
> The formatting here and everywhere doesn't look very well when viewed
> with "man -l man/man8/tc-mqprio.8". If you look at tc-taprio.8, it uses
> "\\" to render properly. If you don't mind, I'll do that here too.
>
> > +.EE
> > +
> > +The value mapping is the following for this example:
> > +
> > +        ┌───────┐
> > + tc0────┤Queue 0│◄────1@0
> > +        ├───────┤
> > +      ┌─┤Queue 1│◄────2@1
> > + tc1──┤ ├───────┤
> > +      └─┤Queue 2│
> > +        ├───────┤
> > + tc2────┤Queue 3│◄────1@3
> > +        ├───────┤
> > + tc3────┤Queue 4│◄────1@4
> > +        ├───────┤
> > +      ┌─┤Queue 5│◄────3@5
> > +      │ ├───────┤
> > + tc4──┼─┤Queue 6│
> > +      │ ├───────┤
> > +      └─┤Queue 7│
> > +        └───────┘
> > +
> > +
> >  .SH AUTHORS
> >  John Fastabend, <john.r.fastabend@...el.com>
> > --
> > 2.34.1
> >

Best,
Peter

Powered by blists - more mailing lists