Received-SPF: pass (google.com: domain of linux-kernel+bounces-195499-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 30 May 2024 15:14:48 +0100
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Xiaolei Wang <xiaolei.wang@windriver.com>, Andrew Lunn <andrew@lunn.ch>,
	alexandre.torgue@foss.st.com, joabreu@synopsys.com,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, mcoquelin.stm32@gmail.com,
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [net v2 PATCH] net: stmmac: Update CBS parameters when speed
 changes after linking up
Message-ID: <ZliJ2O+bj18jQ0B8@shell.armlinux.org.uk>
References: <20240530061453.561708-1-xiaolei.wang@windriver.com>
 <f8b0843f-7900-4ad0-9e70-c16175e893d9@lunn.ch>
 <20240530132822.xv23at32wj73hzfj@skbuf>
 <ZliBzo7eETml/+bl@shell.armlinux.org.uk>
 <20240530135335.yanffjb3ketmoo7u@skbuf>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240530135335.yanffjb3ketmoo7u@skbuf>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Thu, May 30, 2024 at 04:53:35PM +0300, Vladimir Oltean wrote:
> On Thu, May 30, 2024 at 02:40:30PM +0100, Russell King (Oracle) wrote:
> > On Thu, May 30, 2024 at 04:28:22PM +0300, Vladimir Oltean wrote:
> > > On Thu, May 30, 2024 at 02:50:52PM +0200, Xiaolei Wang wrote:
> > > > When the port is relinked, if the speed changes, the CBS parameters
> > > > should be updated, so saving the user transmission parameters so
> > > > that idle_slope and send_slope can be recalculated after the speed
> > > > changes after linking up can help reconfigure CBS after the speed
> > > > changes.
> > > > 
> > > > Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> > > > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> > > > ---
> > > > v1 -> v2
> > > >  - Update CBS parameters when speed changes
> > > 
> > > May I ask what is the point of this patch? The bandwidth fraction, as
> > > IEEE 802.1Q defines it, it a function of idleSlope / portTransmitRate,
> > > the latter of which is a runtime variant. If the link speed changes at
> > > runtime, which is entirely possible, I see no alternative than to let
> > > user space figure out that this happened, and decide what to do. This is
> > > a consequence of the fact that the tc-cbs UAPI takes the raw idleSlope
> > > as direct input, rather than something more high level like the desired
> > > bandwidth for the stream itself, which could be dynamically computed by
> > > the kernel.
> > 
> > So what should be the behaviour here? Refuse setting CBS parameters if
> > the link is down, and clear the hardware configuration of the CBS
> > parameters each and every time there is a link-down event? Isn't that
> > going to make the driver's in-use settings inconsistent with what the
> > kernel thinks have been set? AFAIK, tc qdisc's don't vanish from the
> > kernel just because the link went down.
> > 
> > I think what you're proposing leads to the hardware being effectively
> > "de-programmed" for CBS while "tc qdisc show" will probably report
> > that CBS is active on the interface - which clearly would be absurd.
> 
> No, just program to hardware right away the idleSlope, sendSlope,
> loCredit and hiCredit that were communicated by user space. Those were
> computed for a specific link speed and it is user space's business to
> monitor that this link speed is maintained for as long as the streams
> are necessary (otherwise those parameters are no longer valid).
> One could even recover the portTransmitRate that the parameters were
> computed for (it should be idleSlope - sendSlope, in Kbps).
> 
> AKA keep the driver as it is.
> 
> I don't see why the CBS parameters would need to be de-programmed from
> hardware on a link down event. Is that some stmmac specific thing?

If the driver is having to do computation on the parameters based on
the link speed, then when the link speed changes, the parameters
no longer match what the kernel _thinks_ those parameters were
programmed with.

What I'm trying to get over to you is that what you propose causes
an inconsistency between how the hardware is _programmed_ to behave
for CBS and what the kernel reports the CBS settings are if the
link speed changes.

For example, if the link was operating at 10G, and the idle slope
set by userspace is A, and the send slope was B, tc qdisc show
will report an idle slope of A and send slope of B.

If the link speed now changes to 5G, then, without updating the
settings in the hardware, the multiplier for the register values
will have reduced by a factor of two, meaning they're twice as
large as they should be for values of A and B.

However, tc qdisc show continues to report that values of A and B
are being used, but the hardware is actually using 2 * A and 2 * B.

It's all very well saying that userspace should basically reconstruct
the tc settings when the link changes, but not everyone is aware of
that. I'm saying it's a problem if one isn't aware of this issue with
this hardware, and one looks at tc qdisc show output, and assumes
that reflects what is actually being used when it isn't.

It's quality of implmentation - as far as I'm concerned, the kernel
should *not* mislead the user like this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!