On Thu, Feb 10, 2022 at 5:22 PM Jakub Kicinski [off-list ref] wrote:

On Thu, 10 Feb 2022 10:35:53 +0100 Íñigo Huguet wrote:

quoted

On Mon, Feb 7, 2022 at 5:53 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Mon, 7 Feb 2022 16:03:01 +0100 Íñigo Huguet wrote:

quoted

I have a few busy weeks coming, but I can do this after that.

With num_cores / 2 you divide by 2 because you're assuming 2 NUMA
nodes, or just the plain number 2?

Plain number 2, it's just a heuristic which seems to work okay.
One queue per core (IOW without the /2) is still way too many queues
for normal DC workloads.

Maybe it's because of being quite special workloads, but I have
encountered problems related to queues in different NUMA nodes in 2
cases: XDP performance being almost half with more RX queues because
of being in different node (the example in my patches) and a customer
losing UDP packets which was solved reducing the number of RX queues
so all them are in the same node.

Right, no argument, NUMA tuning will still be necessary.
I'm primarily concerned about providing a sensible default
for workloads which are not network heavy and therefore
nobody spends time tuning their queue configuration.
Any network-heavy workload will likely always benefit from tuning.

The status quo is that our current default returned by
netif_get_num_default_rss_queues() is 8 which is inadequate
for modern servers, and people end up implementing their own
logic in the drivers.

I'm fine with sfc doing its own thing (at least for now) and
therefore your patches as they are, but for new drivers I want
to be able to recommend netif_get_num_default_rss_queues() with
a clear conscience.

Does that make sense?

Totally. My comment was intended to be more like a question to see why
we should or shouldn't consider NUMA nodes in
netif_get_num_default_rss_queues. But now I understand your point
better.

However, would it make sense something like this for
netif_get_num_default_rss_queues, or it would be a bit overkill?
if the system has more than one NUMA node, allocate one queue per
physical core in local NUMA node.
else, allocate physical cores / 2

Another thing: this patch series appears in patchwork with state
"Changes Requested", but no changes have been requested, actually. Can
the state be changed so it has more visibility to get reviews?

Thanks!

-- 
Íñigo Huguet