Re: [net-next] net: dsa: felix: disable always guard band bit for TAS config

[Michael Walle <michael@walle.cc>]

Am 2021-05-04 20:50, schrieb Vladimir Oltean:
> On Tue, May 04, 2021 at 08:38:29PM +0200, Michael Walle wrote:
>> Hi Vladimir,
>> 
>> Am 2021-05-04 20:18, schrieb Vladimir Oltean:
>> > On Tue, May 04, 2021 at 07:05:14PM +0200, Michael Walle wrote:
>> > > Hi,
>> > >
>> > > > ALWAYS_GUARD_BAND_SCH_Q bit in TAS config register is descripted as
>> > > > this:
>> > > > 	0: Guard band is implemented for nonschedule queues to schedule
>> > > > 	   queues transition.
>> > > > 	1: Guard band is implemented for any queue to schedule queue
>> > > > 	   transition.
>> > > >
>> > > > The driver set guard band be implemented for any queue to schedule queue
>> > > > transition before, which will make each GCL time slot reserve a guard
>> > > > band time that can pass the max SDU frame. Because guard band time could
>> > > > not be set in tc-taprio now, it will use about 12000ns to pass 1500B max
>> > > > SDU. This limits each GCL time interval to be more than 12000ns.
>> > > >
>> > > > This patch change the guard band to be only implemented for nonschedule
>> > > > queues to schedule queues transition, so that there is no need to reserve
>> > > > guard band on each GCL. Users can manually add guard band time for each
>> > > > schedule queues in their configuration if they want.
>> > >
>> > >
>> > > As explained in another mail in this thread, all queues are marked as
>> > > scheduled. So this is actually a no-op, correct? It doesn't matter if
>> > > it set or not set for now. Dunno why we even care for this bit then.
>> >
>> > It matters because ALWAYS_GUARD_BAND_SCH_Q reduces the available
>> > throughput when set.
>> 
>> Ahh, I see now. All queues are "scheduled" but the guard band only 
>> applies
>> for "non-scheduled" -> "scheduled" transitions. So the guard band is 
>> never
>> applied, right? Is that really what we want?
> 
> Xiaoliang explained that yes, this is what we want. If the end user
> wants a guard band they can explicitly add a "sched-entry 00" in the
> tc-taprio config.

You're disabling the guard band, then. I figured, but isn't that
suprising for the user? Who else implements taprio? Do they do it in the
same way? I mean this behavior is passed right to the userspace and have
a direct impact on how it is configured. Of course a user can add it
manually, but I'm not sure that is what we want here. At least it needs
to be documented somewhere. Or maybe it should be a switchable option.

Consider the following:
sched-entry S 01 25000
sched-entry S fe 175000
basetime 0

Doesn't guarantee, that queue 0 is available at the beginning of
the cycle, in the worst case it takes up to
<begin of cycle> + ~12.5us until the frame makes it through (given
gigabit and 1518b frames).

Btw. there are also other implementations which don't need a guard
band (because they are store-and-forward and cound the remaining
bytes). So yes, using a guard band and scheduling is degrading the
performance.


>> > > > Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
>> > > > ---
>> > > >  drivers/net/dsa/ocelot/felix_vsc9959.c | 8 ++++++--
>> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> > > > index 789fe08cae50..2473bebe48e6 100644
>> > > > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
>> > > > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> > > > @@ -1227,8 +1227,12 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>> > > >  	if (taprio->num_entries > VSC9959_TAS_GCL_ENTRY_MAX)
>> > > >  		return -ERANGE;
>> > > >
>> > > > -	ocelot_rmw(ocelot, QSYS_TAS_PARAM_CFG_CTRL_PORT_NUM(port) |
>> > > > -		   QSYS_TAS_PARAM_CFG_CTRL_ALWAYS_GUARD_BAND_SCH_Q,
>> > > > +	/* Set port num and disable ALWAYS_GUARD_BAND_SCH_Q, which means set
>> > > > +	 * guard band to be implemented for nonschedule queues to schedule
>> > > > +	 * queues transition.
>> > > > +	 */
>> > > > +	ocelot_rmw(ocelot,
>> > > > +		   QSYS_TAS_PARAM_CFG_CTRL_PORT_NUM(port),
>> > > >  		   QSYS_TAS_PARAM_CFG_CTRL_PORT_NUM_M |
>> > > >  		   QSYS_TAS_PARAM_CFG_CTRL_ALWAYS_GUARD_BAND_SCH_Q,
>> > > >  		   QSYS_TAS_PARAM_CFG_CTRL);
>> > >
>> > > Anyway, I don't think this the correct place for this:
>> > >  (1) it isn't per port, but a global bit, but here its done per port.
>> >
>> > I don't understand. According to the documentation, selecting the port
>> > whose time-aware shaper you are configuring is done through
>> > QSYS::TAS_PARAM_CFG_CTRL.PORT_NUM.
>> 
>> According to the LS1028A RM:
>> 
>>   PORT_NUM
>>   Specifies the port number to which the TAS_PARAMS register 
>> configurations
>>   (CFG_REG_1 to CFG_REG_5, TIME_INTERVAL and GATE_STATE) need to be 
>> applied.
>> 
>> I guess this work together with CONFIG_CHANGE and applies the mentions
>> registers
>> in an atomic way (or at a given time). There is no mention of the
>> ALWAYS_GUARD_BAND_SCH_Q bit nor the register TAS_PARAM_CFG_CTRL.
>> 
>> But the ALWAYS_GUARD_BAND_SCH_Q mention its "Global configuration". 
>> That
>> together with the fact that it can't be read back (unless I'm missing
>> something), led me to the conclusion that this bit is global for the 
>> whole
>> switch. I may be wrong.
> 
> Sorry, I don't understand what you mean to say here.

I doubt that ALWAYS_GUARD_BAND_SCH_Q is a per-port setting. But that is
only a guess. One would have to check with the IP vendor.

>> But in any case, (2) is more severe IMHO.
>> 
>> > >  (2) rmw, I presume is read-modify-write. and there is one bit CONFIG_CHAGE
>> > >      which is set by software and cleared by hardware. What happens if it
>> > > 	 will be cleared right after we read it. Then it will be set again, no?
>> > >
>> > > So if we really care about this bit, shouldn't this be moved to switch
>> > > initialization then?
> 
> Sorry, again, I don't understand. Let me copy here the procedure from
> vsc9959_qos_port_tas_set():
> 
> 	ocelot_rmw(ocelot, QSYS_TAS_PARAM_CFG_CTRL_CONFIG_CHANGE,
> 		   QSYS_TAS_PARAM_CFG_CTRL_CONFIG_CHANGE,
> 		   QSYS_TAS_PARAM_CFG_CTRL); <- set the CONFIG_CHANGE bit, keep
> everything else the same
> 
> 	ret = readx_poll_timeout(vsc9959_tas_read_cfg_status, ocelot, val,
> 				 !(val & QSYS_TAS_PARAM_CFG_CTRL_CONFIG_CHANGE),
> 				 10, 100000); <- spin until CONFIG_CHANGE clears
> 
> Should there have been a mutex at the beginning of 
> vsc9959_qos_port_tas_set,
> ensuring that two independent user space processes configuring the TAS
> of two ports cannot access the global config registers concurrently?
> Probably, although my guess is that currently, the global rtnetlink
> mutex prevents this from happening in practice.

Ah ok, I missed that.

> 
>> > May I know what drew your attention to this patch? Is there something
>> > wrong?

See private mail.

-michael