Re: [RFC PATCH net] net/sched: taprio: account for L1 overhead when calculating transmit time

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Hi Vladimir,

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> The taprio scheduler underestimates the packet transmission time, which
> means that packets can be scheduled for transmission in time slots in
> which they are never going to fit.
>
> When this function was added in commit 4cfd5779bd6e ("taprio: Add
> support for txtime-assist mode"), the only implication was that time
> triggered packets would overrun its time slot and eat from the next one,
> because with txtime-assist there isn't really any emulation of a "gate
> close" event that would stop a packet from being transmitted.
>
> However, commit b5b73b26b3ca ("taprio: Fix allowing too small
> intervals") started using this function too, in all modes of operation
> (software, txtime-assist and full offload). So we now accept time slots
> which we know we won't be ever able to fulfill.
>
> It's difficult to say which issue is more pressing, I'd say both are
> visible with testing, even though the second would be more obvious
> because of a black&white result (trying to send small packets in an
> insufficiently large window blocks the queue).
>
> Issue found through code inspection, the code was not even compile
> tested.
>
> The L1 overhead chosen here is an approximation, because various network
> equipment has configurable IFG, however I don't think Linux is aware of
> this.

When testing CBS, I remember using tc-stab: 

https://man7.org/linux/man-pages/man8/tc-stab.8.html

To set the 'overhead' to some value.

That value should be used in the calculation.

I agree that it's not ideal, in the ideal world we would have a way to
retrieve the link overhead from the netdevice. But I would think that it
gets complicated really quickly when using netdevices that are not
Ethernet-based.

>
> Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/sched/sch_taprio.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index b9c71a304d39..8c8681c37d4f 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -176,7 +176,10 @@ static ktime_t get_interval_end_time(struct sched_gate_list *sched,
>  
>  static int length_to_duration(struct taprio_sched *q, int len)
>  {
> -	return div_u64(len * atomic64_read(&q->picos_per_byte), 1000);
> +	/* The duration of frame transmission should account for L1 overhead
> +	 * (12 octets IFG, 7 octets of preamble, 1 octet SFD, 4 octets FCS)
> +	 */
> +	return div_u64((24 + len) * atomic64_read(&q->picos_per_byte), 1000);
>  }
>  
>  /* Returns the entry corresponding to next available interval. If
> -- 
> 2.25.1
>

-- 
Vinicius