RE: [PATCH net] net: enetc: correct the indexes of highest and 2nd highest TCs

[Wei Fang]

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> Sent: 2023年6月6日 17:17
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: Claudiu Manoil <claudiu.manoil@xxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net] net: enetc: correct the indexes of highest and 2nd
> highest TCs
> 
> On Tue, Jun 06, 2023 at 04:46:18PM +0800, wei.fang@xxxxxxx wrote:
> > From: Wei Fang <wei.fang@xxxxxxx>
> >
> > For ENETC hardware, the TCs are numbered from 0 to N-1, where N
> > is the number of TCs. Numerically higher TC has higher priority.
> > It's obvious that the highest priority TC index should be N-1 and
> > the 2nd highest priority TC index should be N-2.
> > However, the previous logic uses netdev_get_prio_tc_map() to get
> > the indexes of highest priority and 2nd highest priority TCs, it
> > does not make sense and is incorrect. It may get wrong indexes of
> > the two TCs and make the CBS unconfigurable. e.g.
> 
> Well, you need to consider that prior to commit 1a353111b6d4 ("net:
> enetc: act upon the requested mqprio queue configuration"), the driver
> would always set up an identity mapping between priorities, traffic
> classes, rings and netdev queues.
> 
I also considered the situation prior to the commit 1a353111b6d4. The
problem also existed.
e.g.
$ tc qdisc add dev eno0 root handle 1: mqprio num_tc 8 \
	map 0 1 2 3 6 7 4 5 queues 1@0 1@1 \
			  ^ ^ ^ ^
	1@2 1@3 1@4 1@5 1@6 1@7 hw 1
The driver would deem the indexes of the two highest TCs are 5 and 4,
rather than 7 and 6.

> So, yes, giving a "tc" argument to netdev_get_prio_tc_map() is
> semantically incorrect, but it only started being a problem when the
> identity mapping started being configurable.
> 
In my opinion, "unconfigurable" is also a problem.

> > $ tc qdisc add dev eno0 parent root handle 100: mqprio num_tc 6 \
> > 	map 0 0 1 1 2 3 4 5 queues 1@0 1@1 1@2 1@3 2@4 2@6 hw 1
> > $ tc qdisc replace dev eno0 parent 100:6 cbs idleslope 100000 \
> > 	sendslope -900000 hicredit 12 locredit -113 offload 1
> > $ Error: Specified device failed to setup cbs hardware offload.
> >   ^^^^^
> 
> ok.
> 
> >
> > Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for
> hardware offload")
> 
> In principle, there shouldn't be an issue with backporting the fix that
> far (v5.5), even if it is unnecessary beyond commit 1a353111b6d4 (v6.3).
> If you want to respin the patch to clarify the situation, fine. If not,
> also fine.
> 
> > Signed-off-by: Wei Fang <wei.fang@xxxxxxx>
> > ---
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>