Message-ID: <20230220152942.kj7uojbgfcsowfap@skbuf>
Date:   Mon, 20 Feb 2023 17:29:42 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Péter Antal <peti.antal99@...il.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 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.

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

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

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 :)

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

Powered by blists - more mailing lists