Re: [PATCH net V3 1/2] net/sched: taprio: fix picos_per_byte miscalculation

[Florian Fainelli <f.fainelli@gmail.com>]

On 3/15/19 2:16 PM, Leandro Dorileo wrote:
> The Time Aware Priority Scheduler is heavily dependent to link speed,
> it relies on it to calculate transmission bytes per cycle, we can't
> properly calculate the so called budget if the device has failed
> to report the link speed.
> 
> In that case we can't dequeue packets assuming a wrong budget.
> This patch makes sure we fail to dequeue case:
> 
> 1) __ethtool_get_link_ksettings() reports error or 2) the ethernet
> driver failed to set the ksettings' speed value (setting link speed
> to SPEED_UNKNOWN).
> 
> Additionally we re calculate the budget whenever the link speed is
> changed.
> 
> Fixes: 5a781ccbd19e4 ("tc: Add support for configuring the taprio scheduler")
> Signed-off-by: Leandro Dorileo <leandro.maciel.dorileo@intel.com>
> ---
>  net/sched/sch_taprio.c | 90 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 74 insertions(+), 16 deletions(-)
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 206e4dbed12f..fa237908ba09 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -20,6 +20,9 @@
>  #include <net/pkt_cls.h>
>  #include <net/sch_generic.h>
>  
> +static LIST_HEAD(taprio_list);
> +static DEFINE_SPINLOCK(taprio_list_lock);
> +
>  #define TAPRIO_ALL_GATES_OPEN -1
>  
>  struct sched_entry {
> @@ -42,9 +45,9 @@ struct taprio_sched {
>  	struct Qdisc *root;
>  	s64 base_time;
>  	int clockid;
> -	int picos_per_byte; /* Using picoseconds because for 10Gbps+
> -			     * speeds it's sub-nanoseconds per byte
> -			     */
> +	atomic64_t picos_per_byte; /* Using picoseconds because for 10Gbps+
> +				    * speeds it's sub-nanoseconds per byte
> +				    */
>  	size_t num_entries;
>  
>  	/* Protects the update side of the RCU protected current_entry */
> @@ -53,6 +56,7 @@ struct taprio_sched {
>  	struct list_head entries;
>  	ktime_t (*get_time)(void);
>  	struct hrtimer advance_timer;
> +	struct list_head taprio_list;
>  };
>  
>  static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> @@ -117,7 +121,7 @@ static struct sk_buff *taprio_peek(struct Qdisc *sch)
>  
>  static inline int length_to_duration(struct taprio_sched *q, int len)
>  {
> -	return (len * q->picos_per_byte) / 1000;
> +	return (len * atomic64_read(&q->picos_per_byte)) / 1000;
>  }
>  
>  static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
> @@ -129,6 +133,11 @@ static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
>  	u32 gate_mask;
>  	int i;
>  
> +	if (atomic64_read(&q->picos_per_byte) == -1) {
> +		WARN_ONCE(1, "taprio: dequeue() called with unknown picos per byte.");
> +		return NULL;
> +	}
> +
>  	rcu_read_lock();
>  	entry = rcu_dereference(q->current_entry);
>  	/* if there's no entry, it means that the schedule didn't
> @@ -233,7 +242,7 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
>  
>  	next->close_time = close_time;
>  	atomic_set(&next->budget,
> -		   (next->interval * 1000) / q->picos_per_byte);
> +		   (next->interval * 1000) / atomic64_read(&q->picos_per_byte));
>  
>  first_run:
>  	rcu_assign_pointer(q->current_entry, next);
> @@ -567,7 +576,8 @@ static void taprio_start_sched(struct Qdisc *sch, ktime_t start)
>  
>  	first->close_time = ktime_add_ns(start, first->interval);
>  	atomic_set(&first->budget,
> -		   (first->interval * 1000) / q->picos_per_byte);
> +		   (first->interval * 1000) /
> +		   atomic64_read(&q->picos_per_byte));
>  	rcu_assign_pointer(q->current_entry, NULL);
>  
>  	spin_unlock_irqrestore(&q->current_entry_lock, flags);
> @@ -575,6 +585,45 @@ static void taprio_start_sched(struct Qdisc *sch, ktime_t start)
>  	hrtimer_start(&q->advance_timer, start, HRTIMER_MODE_ABS);
>  }
>  
> +static void taprio_set_picos_per_byte(struct net_device *dev,
> +				      struct taprio_sched *q)
> +{
> +	struct ethtool_link_ksettings ecmd;
> +	int picos_per_byte = -1;
> +
> +	if (!__ethtool_get_link_ksettings(dev, &ecmd) &&
> +	    ecmd.base.speed != SPEED_UNKNOWN)
> +		picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8,
> +					   ecmd.base.speed * 1000 * 1000);
> +
> +	atomic64_set(&q->picos_per_byte, picos_per_byte);
> +	pr_info("taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n",
> +		dev->name, atomic64_read(&q->picos_per_byte), ecmd.base.speed);

It does not seem appropriate to use pr_info() here whenever the link
speed changes and the taprio scheduler is reconfigured to match the
network device's link speed. netdev_dbg() might be more appropriate, or
no message at all to avoid spamming the kernel log. Same thing for your
second patch.
-- 
Florian