Re: [PATCH net-next 1/2] sfc: default config to 1 channel/core in local NUMA node only

From: "Íñigo Huguet" <ihuguet@redhat.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Edward Cree <ecree.xilinx@gmail.com>,
	habetsm.xilinx@gmail.com, "David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next 1/2] sfc: default config to 1 channel/core in local NUMA node only
Date: Fri, 11 Feb 2022 12:05:19 +0100	[thread overview]
Message-ID: <CACT4ouepk83kxTGd6S3gVyFAjofofwQfxsmhe97vGP+twkoW1g@mail.gmail.com> (raw)
In-Reply-To: <20220210082249.0e50668b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On Thu, Feb 10, 2022 at 5:22 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 10 Feb 2022 10:35:53 +0100 Íñigo Huguet wrote:
> > On Mon, Feb 7, 2022 at 5:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Mon, 7 Feb 2022 16:03:01 +0100 Íñigo Huguet wrote:
> > > > 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