Re: [RFC PATCH] net: add a tx_queue attribute rate_queue_limits in Mbps

[John Fastabend <john.fastabend@gmail.com>]

On 06/25/2013 12:00 PM, Ben Hutchings wrote:
> On Sun, 2013-06-23 at 20:24 -0700, John Fastabend wrote:
> [...]
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> [...]
>> +int ixgbe_set_rate_limit(struct net_device *dev, int index, u32 *tx_rate)
>> +{
>> +	struct ixgbe_adapter *a = netdev_priv(dev);
>> +	struct ixgbe_hw *hw = &a->hw;
>> +	int linkspeed = ixgbe_link_mbps(a);
>> +	u8 reg_idx = a->tx_ring[index]->reg_idx;
>> +	u32 bcnrc = ixgbe_bcnrc_from_rate(a, *tx_rate, linkspeed);
>> +
>> +	/* rate limit cannot be less than 10Mbs or greater than link speed */
>> +	if (*tx_rate && ((*tx_rate <= 10) || (*tx_rate > linkspeed)))
>> +		return -EINVAL;
>
> What if the link is not up?
>

As its written this would always be an error because link_speed is 0.
Not likely what we want.

> What if the link speed is currently higher than tx_rate, but is later
> reduced below tx_rate?

As its written you just end up with a tx_rate limit that is broken and
because the hw calculation depends on link speed the value won't be
correct any more.

Both of these cases are probably broken as is.

>
> [...]
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -574,6 +574,7 @@ struct netdev_queue {
>>   #ifdef CONFIG_BQL
>>   	struct dql		dql;
>>   #endif
>> +	unsigned long		rate_limit;
>
> Should be u32, consistent with variables in ndo_set_ratelimit and
> set_tx_rate_limit.
>
>>   } ____cacheline_aligned_in_smp;
>>
>>   static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)
>> @@ -932,6 +933,14 @@ struct netdev_fcoe_hbainfo {
>>    *	that determine carrier state from physical hardware properties (eg
>>    *	network cables) or protocol-dependent mechanisms (eg
>>    *	USB_CDC_NOTIFY_NETWORK_CONNECTION) should NOT implement this function.
>> + *
>> + * int (*ndo_set_ratelimit)(struct net_device *dev,
>> + *			    int queue_index, u32 *maxrate)
>> + *	Called to set the rate limit in Mpbs specified by maxrate of the
>> + *	specified queue_index. It is expected that hardware way may quantize
>> + *	the rate limits. In these cases the driver should guarentee the
>> + *	specfied maxrate is not exceeded and return the set value in maxrate.
>> + *	Zero should be returned on sucess otherwise use appropriate error code.
> [...]
>
> In general, how does maxrate interact with link speed?

I think the best approach is to allow a max_rate that is greater than
the link speed. This seems more straight forward than arbitrarily
resetting the value or doing something else.

ixgbe and maybe other drivers will need to reconfigure their internal
hardware limiters if they are derived from link speed.

>
> There are also several spelling and grammar errors in this text.
>

Yeah, must have been late, not enough coffee. Thanks for looking at it
Ben.

Also I was thinking about pushing it over to netlink as Stephen
preferred.

-- 
John Fastabend         Intel Corporation