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]

Date:   Wed, 13 Jul 2022 19:07:48 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Yonglong Li <liyonglong@...natelecom.cn>
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

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.

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

> @@ -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?