Message-ID: <20201124095812.539b9d1e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 24 Nov 2020 09:58:12 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Po Liu <po.liu@....com>
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>
Subject: Re: [PATCH net] enetc: Advance the taprio base time in the future

On Tue, 24 Nov 2020 02:40:29 +0000 Po Liu wrote:
> > The tc-taprio base time indicates the beginning of the tc-taprio schedule,
> > which is cyclic by definition (where the length of the cycle in nanoseconds
> > is called the cycle time). The base time is a 64-bit PTP time in the TAI
> > domain.
> > 
> > Logically, the base-time should be a future time. But that imposes some
> > restrictions to user space, which has to retrieve the current PTP time first
> > from the NIC first, then calculate a base time that will still be larger than

Says first twice.

> > the base time by the time the kernel driver programs this value into the
> > hardware. Actually ensuring that the programmed base time is in the
> > future is still a problem even if the kernel alone deals with this - what the
> > proposed patch does is to "reserve" 100 ms for potential delays, but
> > otherwise this is an unsolved problem in the general case.
> > 
> > Nonetheless, what is important for tc-taprio in a LAN is not precisely the
> > base-time value, but rather the fact that the taprio schedules are
> > synchronized across all nodes in the network, or at least have a given
> > phase offset.
> > 
> > Therefore, the expectation for user space is that specifying a base-time of
> > 0 would mean that the tc-taprio schedule should start "right away", with
> > one twist: the effective base-time written into the NIC is still congruent
> > with the originally specified base-time. Otherwise stated, if the current PTP
> > time of the NIC is 2.123456789, the base-time of the schedule is
> > 0.000000000 and the cycle-time is 0.500000000, then the effective base-
> > time should be 2.500000000, since that is the first beginning of a new cycle
> > starting at base-time 0.000000000, with a cycle time of 500 ms, that is
> > larger than the current PTP time.
> > 
> > So in short, the kernel driver, or the hardware, should allow user space to
> > skip the calculation of the future base time, and transparently allow a PTP
> > time in the past. The formula for advancing the base time should be:
> > 
> > effective-base-time = base-time + N x cycle-time
> > 
> > where N is the smallest integer number of cycles such that effective-base-
> > time >= now.
> > 
> > Actually, the base-time of 0.000000000 is not special in any way.
> > Reiterating the example above, just with a base-time of 0.000500000. The
> > effective base time in this case should be 2.500500000, according to the
> > formula. There are use cases for applying phase shifts like this.
> > 
> > The enetc driver is not doing that. It special-cases the case where the
> > specified base time is zero, and it replaces that with a plain "current PTP
> > time".
> > 
> > Such an implementation is unusable for applications that expect the
> > phase to be preserved. We already have drivers in the kernel that comply
> > to the behavior described above (maybe more than just the ones listed
> > below):
> > - the software implementation of taprio does it in taprio_get_start_time:
> > 
> > 	/* Schedule the start time for the beginning of the next
> > 	 * cycle.
> > 	 */
> > 	n = div64_s64(ktime_sub_ns(now, base), cycle);
> > 	*start = ktime_add_ns(base, (n + 1) * cycle);
> >   
> 
> This is the right way for calculation. For the ENETC,  hardware also do the same calculation before send to Operation State Machine. 
> For some TSN IP, like Felix and DesignWare TSN in RT1170 and IMX8MP require the basetime limite the range not less than the current time 8 cycles, software may do calculation before setting to the hardware.
> Actually, I do suggest this calculation to sch_taprio.c, but I found same calculation only for the TXTIME by taprio_get_start_time().
> Which means: 
> If (currenttime < basetime)
>        Admin_basetime = basetime;
> Else
>        Admin_basetime =  basetime + (n+1)* cycletime;
> N is the minimal value which make Admin_basetime is larger than the currenttime.
> 
> User space never to get the current time. Just set a value as offset OR future time user want.
> For example: set basetime = 1000000ns, means he want time align to 1000000ns, and on the other device, also set the basetime = 1000000ns, then the two devices are aligned cycle.
> If user want all the devices start at 11.24.2020 11:00 then set basetime = 1606273200.0 s.
> 
> > - the sja1105 offload does it via future_base_time()
> > - the ocelot/felix offload does it via vsc9959_new_base_time()
> > 
> > As for the obvious question: doesn't the hardware just "do the right thing"
> > if passed a time in the past? I've tested and it doesn't look like it. I cannot  
> 
> So hardware already do calculation same way.

So the patch is unnecessary? Or correct? Not sure what you're saying..

> > determine what base-time it uses in that case, however traffic does not
> > have the correct phase alignment.
> > 
> > Fixes: 34c6adf1977b ("enetc: Configure the Time-Aware Scheduler via tc-
> > taprio offload")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> >  .../net/ethernet/freescale/enetc/enetc_qos.c  | 34 +++++++++++++------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> > b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> > index aeb21dc48099..379deef5d9e0 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> > @@ -45,6 +45,20 @@ void enetc_sched_speed_set(struct enetc_ndev_priv
> > *priv, int speed)
> >  		      | pspeed);
> >  }
> > 
> > +static inline s64 future_base_time(s64 base_time, s64 cycle_time, s64
> > +now) {

nit: no need for this inline

> > +	s64 a, b, n;
> > +
> > +	if (base_time >= now)
> > +		return base_time;
> > +
> > +	a = now - base_time;
> > +	b = cycle_time;
> > +	n = div_s64(a + b - 1, b);
> > +
> > +	return base_time + n * cycle_time;
> > +}

Powered by blists - more mailing lists