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]

Date:	Wed, 30 Mar 2016 10:04:35 -0700
From:	John Fastabend <john.fastabend@...il.com>
To:	Saeed Mahameed <saeedm@....mellanox.co.il>,
	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 16-03-30 06:23 AM, Saeed Mahameed wrote:
> 
> 
> 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).

Yep, sorry I read the patch to quickly and without coffee thanks!

>> 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 !
> 

OK, so let me see if I get this right now. This was the precedence
before the patch in the normal no select queue case,

	(1) socket mapping sk_tx_queue_mapping iff !ooo_okay
	(2) xps
	(3) skb->queue_mapping
	(4) qoffset/qcount (hash over tc queues)
	(5) hash over num_tx_queues

With this patch the precedence is a bit changed because
skb_tx_hash is always called.

	(1) socket mapping sk_tx_queue_mapping iff !ooo_okay
	(2) skb->queue_mapping
	(3) qoffset/qcount
	   (hash over tc queues if xps choice is > qcount)
	(4) xps
	(5) hash over num_tx_queues

Sound right? Nice thing about this with correct configuration
of tc with qcount = xps_queues it sort of works as at least
I expect it to. I think the question is are people OK with
letting skb->queue_mapping take precedence. I am at least
because it makes the skb edit queue_mapping action from tc
easier to use.

And just a comment on the code why not just move get_xps_queue
into skb_tx_hash at this point if its always being called as the
"hint". Then we avoid calling it in the case queue_mapping is
set.

>>>       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.
> 

yep.