RE: [PATCH V2 net-next] net: fec: add CBS offload support

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: 2023年2月14日 2:07
> To: Alexander Lobakin <alexandr.lobakin@xxxxxxxxx>
> Cc: Wei Fang <wei.fang@xxxxxxx>; Shenwei Wang <shenwei.wang@xxxxxxx>;
> Clark Wang <xiaoning.wang@xxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> simon.horman@xxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V2 net-next] net: fec: add CBS offload support
> 
> On Mon, Feb 13, 2023 at 06:44:05PM +0100, Alexander Lobakin wrote:
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Date: Mon, 13 Feb 2023 17:21:43 +0100
> >
> > >>> +	if (!speed) {
> > >>> +		netdev_err(ndev, "Link speed is 0!\n");
> > >>
> > >> ??? Is this possible? If so, why is it checked only here and why
> > >> can it be possible?
> > >
> > > The obvious way this happens is that there is no link partner, so
> > > auto-neg has not completed yet. The link speed is unknown.
> >
> > Sure, but why treat it an error path then?
> 
> You need to treat is somehow. I would actually disagree with netdev_err(),
> netdev_dbg() seems more appropriate. But if you don't know the link speed,
> you cannot program the scheduler.
> 
Yes, netdev_dbg() seems more appropriate. And as I replied before, I think
!fep->link maybe more appropriate than !speed. What do you think?

> This also comes back to my question about what should happen with a TC
> configuration which works fine for 1000BaseT, but will not work for 10BaseT.
> Should the driver accept it only if the current link speed is sufficient? Should it
> always accept it, and not program it into the hardware if the current link speed
> does not support it?
> 
Please see the previous reply.

> Since we are talking about hardware acceleration here, what does the pure
> software version do? Ideally we want the accelerated version to do the same
> as the software version.
> 
> Wei, please disable all clever stuff in the hardware, setup a pure software qdisc
> and a 10BaseT link. Oversubscribe the link and see what happens. Does other
> traffic get starved?
> 

Sorry, I'm not very familiar with the configuration of pure software implementation
of CBS. I tried to configure the CBS like the following. The bandwidth of queue 1 was
set to 30Mbps. And the queue 2 is set to 20Mbps. Then one stream were sent the
queue 1 and the rate was 50Mbps, the link speed was 1Gbps. But the result seemed that
the CBS did not take effective. And the linux packet generator was used to send the
stream. Was there something wrong with my configuration?

tc qdisc add dev eth0 parent root handle 100 mqprio num_tc 3 map 0 0 2 1 0 0 0 0 0 \
0 0 0 0 0 0 0 queues 1@0 1@1 1@2 hw 0
tc qdisc replace dev eth0 parent 100:2 cbs idleslope 30000 sendslope -970000 hicredit \
12 locredit -113 offload 0
tc qdisc replace dev eth0 parent 100:3 cbs idleslope 20000 sendslope -980000 hicredit \
12 locredit -113 offload 0