Message-ID: <7312be3ff6b6a42b2cd68e8c2d403a6ab16e5ef3.camel@inf.elte.hu>
Date:   Mon, 20 Feb 2023 17:14:44 +0100
From:   Ferenc Fejes <fejes@....elte.hu>
To:     Péter Antal <peti.antal99@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, John Fastabend <john.fastabend@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        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!

On Mon, 2023-02-20 at 17:01 +0100, Péter Antal wrote:
> 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.

+1, also taprio's manpage using capitals too, but with a space between
TC and the index like "TC 0". Great think if they would harmonize.

> > 
> > > +
> > > +.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.

I think thats just saving some space and not to scare the mqprio
manpage reader even more :-)
But yes, having tables itself 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.

Yes, if you carry this forward in your patches I think just better to
omit the storytelling. Péter probably ACK it too.

> > 
> > > +.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

Best,
Ferenc

Powered by blists - more mailing lists