netdev - Re: [PATCH] net: sort queues in xps maps

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: <bf741f12-0587-5870-2c59-a52c36a1d2d6@chinatelecom.cn>
Date:   Thu, 14 Jul 2022 11:24:31 +0800
From:   Yonglong Li <liyonglong@...natelecom.cn>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, alexanderduyck@...com
Subject: Re: [PATCH] net: sort queues in xps maps

Hi Jakub,

Thanks for your feedback.

On 7/14/2022 10:07 AM, Jakub Kicinski wrote:
> On Wed, 13 Jul 2022 10:24:56 +0800 Yonglong Li wrote:
>> in the following case that set xps of each tx-queue with same cpu mask,
>> packets in the same tcp stream may be hash to different tx queue. Because
>> the order of queues in each xps map is not the same.
>>
>> first set each tx-queue with different cpu mask
>> echo 0 > /sys/class/net/eth0/queues/tx-0
>> echo 1 > /sys/class/net/eth0/queues/tx-1
>> echo 2 > /sys/class/net/eth0/queues/tx-2
>> echo 4 > /sys/class/net/eth0/queues/tx-3
>> and then set each tx-queue with same cpu mask
>> echo f > /sys/class/net/eth0/queues/tx-0
>> echo f > /sys/class/net/eth0/queues/tx-1
>> echo f > /sys/class/net/eth0/queues/tx-2
>> echo f > /sys/class/net/eth0/queues/tx-3
> 
> These commands look truncated.

I will refill the commands.

> 
>> at this point the order of each map queues is differnet, It will cause
>> packets in the same stream be hashed to diffetent tx queue:
>> attr_map[0].queues = [0,1,2,3]
>> attr_map[1].queues = [1,0,2,3]
>> attr_map[2].queues = [2,0,1,3]
>> attr_map[3].queues = [3,0,1,2]
>>
>> It is more reasonable that pacekts in the same stream be hashed to the same
>> tx queue when all tx queue bind with the same CPUs.
>>
>> Fixes: 537c00de1c9b ("net: Add functions netif_reset_xps_queue and netif_set_xps_queue")
> 
> I'd suggest treating this as a general improvement rather than fix,
> the kernel always behaved this way - it seems logical that sorted is
> better but whether it's a bug not to sort is not as clear cut.
> 
agree, will remove Fixes:tag in next version.

>> @@ -2654,6 +2660,13 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
>>  					  skip_tc);
>>  	}
>>  
>> +	for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids),
>> +	     j < nr_ids;) {
>> +		tci = j * num_tc + tc;
>> +		map = xmap_dereference(new_dev_maps->attr_map[tci]);
>> +		sort(map->queues, map->len, sizeof(u16), cmp_u16, NULL);
>> +	}
>> +
> 
> Can we instead make sure that expand_xps_map() maintains order?
> 
expand_xps_map() only alloc new_map and copy old map's queue to new_map.
I think it is not suitable to do it in expand_xps_map().
WDYT?

-- 
Li YongLong