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

[Leandro Dorileo <l@dorileo.org>]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

> 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/sche
> d/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_h
> ead 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(&ne
> xt->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.

It's being useful to me on debugging it, however I agree users will not be much
interested on it.

Thanks...

--
Dorileo