netdev - Re: [PATCH net-next V4 1/2] net/sched: taprio: fix picos_per

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9cc67457-dea5-5c23-a720-708827eb0d80@gmail.com>
Date:   Wed, 27 Mar 2019 18:54:15 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Leandro Dorileo <leandro.maciel.dorileo@...el.com>,
        netdev@...r.kernel.org
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S . Miller" <davem@...emloft.net>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        vedang.patel@...el.com, andre.guedes@...el.com
Subject: Re: [PATCH net-next V4 1/2] net/sched: taprio: fix picos_per_byte
 miscalculation



On 3/27/2019 4:59 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@...el.com>
> Reviewed-by: Vedang Patel <vedang.patel@...el.com>

Looks good to me, just one thing below I had not noticed earlier:

> ---

[snip]

> +static int taprio_dev_notifier(struct notifier_block *nb, unsigned long event,
> +			       void *ptr)
> +{
> +	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +	struct taprio_sched *q;
> +	struct net_device *qdev;
> +
> +	ASSERT_RTNL();
> +
> +	if (event != NETDEV_UP && event != NETDEV_CHANGE)
> +		return NOTIFY_DONE;
> +
> +	spin_lock(&taprio_list_lock);
> +	list_for_each_entry(q, &taprio_list, taprio_list) {
> +		qdev = qdisc_dev(q->root);
> +		if (qdev == dev)
> +			taprio_set_picos_per_byte(dev, q);

You can break out of the loop here instead of iterating on the entire
list of net_device maintained by taprio.

The same issue exists in the second patch.
-- 
Florian