[PATCH net-next 1/1] [net] bonding: Add NUMA notice

[PATCH net-next 1/1] [net] bonding: Add NUMA notice

[Patrick Talbert]

Network performance can suffer when a load balancing bond uses slave
interfaces which are in different NUMA domains.

This compares the NUMA domain of a newly enslaved interface against any
existing enslaved interfaces and prints a warning if they do not match.

Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
---
:100644 100644 b19dc03... 250a969... M	drivers/net/bonding/bond_main.c
 drivers/net/bonding/bond_main.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b19dc03..250a969 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -55,6 +55,7 @@
 #include <asm/dma.h>
 #include <linux/uaccess.h>
 #include <linux/errno.h>
+#include <linux/device.h>
 #include <linux/netdevice.h>
 #include <linux/inetdevice.h>
 #include <linux/igmp.h>
@@ -1450,6 +1451,21 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
+	if (bond_has_slaves(bond)) {
+		struct list_head *iter;
+		struct slave *slave;
+
+		bond_for_each_slave(bond, slave, iter) {
+			if (slave_dev->dev.numa_node !=
+			    slave->dev->dev.numa_node) {
+				netdev_warn(bond_dev,
+					    "%s does not match NUMA domain of existing slaves. This could have a performance impact.",
+					    slave_dev->name);
+				break;
+			}
+		}
+	}
+
 	call_netdevice_notifiers(NETDEV_JOIN, slave_dev);
 
 	/* If this is the first slave, then we need to set the master's hardware
-- 
1.8.3.1

Re: [PATCH net-next 1/1] [net] bonding: Add NUMA notice

[Andrew Lunn]

On Thu, Oct 05, 2017 at 04:23:45PM -0400, Patrick Talbert wrote:
> Network performance can suffer when a load balancing bond uses slave
> interfaces which are in different NUMA domains.
> 
> This compares the NUMA domain of a newly enslaved interface against any
> existing enslaved interfaces and prints a warning if they do not match.

Hi Patrick

Is there a bonding mode which might actually want to do this? Send on
the local domain, unless it is overloaded, in which case send it to
the other domain?

There is also this talk for netdev:

https://netdevconf.org/2.2/session.html?shochat-devicemgmt-talk

	Andrew

Re: [PATCH net-next 1/1] [net] bonding: Add NUMA notice

[Patrick Talbert]

On Thu, Oct 5, 2017 at 5:46 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Oct 05, 2017 at 04:23:45PM -0400, Patrick Talbert wrote:
>> Network performance can suffer when a load balancing bond uses slave
>> interfaces which are in different NUMA domains.
>>
>> This compares the NUMA domain of a newly enslaved interface against any
>> existing enslaved interfaces and prints a warning if they do not match.
>
> Hi Patrick
>
> Is there a bonding mode which might actually want to do this? Send on
> the local domain, unless it is overloaded, in which case send it to
> the other domain?
>

I suppose there could theoretically be a bonding mode that could do
that, but currently no such mode exists.

> There is also this talk for netdev:
>
> https://netdevconf.org/2.2/session.html?shochat-devicemgmt-talk

>From reading the abstract there, it sounds like such a device driver
would want to abstract away the numa location of the underlying
devices from the "unified" net device it presents to the kernel.

>
>         Andrew


My goal with the patch is not to prevent some one from bonding
whichever interfaces they want, only to notify them that what they are
doing is *likely* to be less than ideal from a performance
perspective. Even if some theoretical load balancing bonding mode was
intelligent enough to consider NUMA when choosing a transmit
interface, it never has control over the interface traffic is received
on (excluding the strange balance-alb mode).

Patrick

Re: [PATCH net-next 1/1] [net] bonding: Add NUMA notice

[Eric Dumazet]

On Fri, 2017-10-06 at 13:54 -0400, Patrick Talbert wrote:

> 
> My goal with the patch is not to prevent some one from bonding
> whichever interfaces they want, only to notify them that what they are
> doing is *likely* to be less than ideal from a performance
> perspective. Even if some theoretical load balancing bonding mode was
> intelligent enough to consider NUMA when choosing a transmit
> interface, it never has control over the interface traffic is received
> on (excluding the strange balance-alb mode).

Note that following the NUMA node of current process would lead to
reordering for TCP flows.

XPS makes sure we stick to one TXQ to avoid reorders.

I am not sure your patch will really help, since you do not warn if a
process from another NUMA node tries to send packet to a bonding driver
using slaves on another NUMA node.

Re: [PATCH net-next 1/1] [net] bonding: Add NUMA notice

[David Miller]

From: Patrick Talbert <ptalbert@redhat.com>
Date: Thu,  5 Oct 2017 16:23:45 -0400

> Network performance can suffer when a load balancing bond uses slave
> interfaces which are in different NUMA domains.
> 
> This compares the NUMA domain of a newly enslaved interface against any
> existing enslaved interfaces and prints a warning if they do not match.
> 
> Signed-off-by: Patrick Talbert <ptalbert@redhat.com>

This is a bit over the top, and doesn't even handle cases where
the device has no specific NUMA node (-1).

Re: [PATCH net-next 1/1] [net] bonding: Add NUMA notice

[Patrick Talbert]

On Sat, Oct 7, 2017 at 6:20 PM, David Miller <davem@davemloft.net> wrote:
> From: Patrick Talbert <ptalbert@redhat.com>
> Date: Thu,  5 Oct 2017 16:23:45 -0400
>
>> Network performance can suffer when a load balancing bond uses slave
>> interfaces which are in different NUMA domains.
>>
>> This compares the NUMA domain of a newly enslaved interface against any
>> existing enslaved interfaces and prints a warning if they do not match.
>>
>> Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
>
> This is a bit over the top, and doesn't even handle cases where
> the device has no specific NUMA node (-1).

Hello David,

The motivation was simply to have a notification in place if the
interface to-be-enslaved does not match the existing one(s). We have
seen performance issues with bonding which were eventually tracked
down to this mismatched NUMA domain issue. I thought it was worth
having the *mild* warning because it can have a decent impact yet is
probably not an obvious thing to check for most users.

Though I now see your point. If the bonded interfaces are VLANs, and
their underlying physical interfaces happen to be in different NUMA
domains, then my check will not know as the VLAN interface numa_node
member will be -1 no matter what. That's probably a pretty common
setup but adding the logic to check for it probably isn't worth it.

Patrick