netdev - Re: [PATCH RFC net-next] net: core: Pass XPS select queue decision to skb_tx

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: <56FBD35D.4090805@dev.mellanox.co.il>
Date:	Wed, 30 Mar 2016 16:23:41 +0300
From:	Saeed Mahameed <saeedm@....mellanox.co.il>
To:	John Fastabend <john.fastabend@...il.com>,
	Saeed Mahameed <saeedm@...lanox.com>, netdev@...r.kernel.org
Cc:	Eric Dumazet <edumazet@...gle.com>,
	Tom Herbert <tom@...bertland.com>,
	Jiri Pirko <jiri@...nulli.us>,
	"David S. Miller" <davem@...emloft.net>,
	John Fastabend <john.r.fastabend@...el.com>
Subject: Re: [PATCH RFC net-next] net: core: Pass XPS select queue decision to
 skb_tx_hash



On 3/30/2016 3:18 AM, John Fastabend wrote:
> I would prefer to not have another strange quirk users have to 
> remember in order to do tx classification. So with this change 
> depending on the driver the queue selection precedence changes. 
This change doesn't depend on the driver it affects all drivers that 
implement the select queue ndo and use the default fallback 
"pick_tx_queue" which this patch came to fix, or any driver that doesn't 
implement the ndo (the fallback is the default in this case).
> In short I agree with the problem statement but think we can find a 
> better solution. One idea that comes to mind is we can have a tc 
> action to force the queue selection? Now that we have the egress tc 
> hook it would probably be fairly cheap to implement and if users want 
> this behavior they can ask for it explicitly. If your thinking about 
> tc stuff we could fix the tooling to set this action when ever dcb is 
> turned on or hardware rate limiting is enabled, etc. And even if we 
> wanted we could have the driver add the rule in the cases where 
> firmware protocols are configuring the QOS/etc. 
Why would you ask for a bug fix explicitly ? IMHO this how I expect the 
pick _tx_queue routine to behave, why would I disable XPS in order for 
select queue to choose according TC QoS ?
as this patch suggests we can benefit from both without any additional 
tooling !

>>   	if (skb_vlan_tag_present(skb))
>>   		up = skb_vlan_tag_get(skb) >> VLAN_PRIO_SHIFT;
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index cb0d5d0..ad81ffe 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -3130,16 +3130,16 @@ static inline int netif_set_xps_queue(struct net_device *dev,
>>   #endif
>>   
>>   u16 __skb_tx_hash(const struct net_device *dev, struct sk_buff *skb,
>> -		  unsigned int num_tx_queues);
>> +		  unsigned int num_tx_queues, int txq_hint);
>>   
> [...]
>
> And all this seems like it would only ever be called by drivers select
> queue routines which I really wish we could kill off one of these days
> instead of add to. Now if the signal is something higher in the stack
> and not the driver I think it is OK.
I agree, drivers shouldn't call this function, the only reason drivers 
call this function is to bypass get_xps_queue
and after this patch i don't think driver will need to call it, since it 
will be called even if XPS is configured.