[Patch v2 0/3] IB/core: Fix GID cache for bonded net devices

[Patch v2 0/3] IB/core: Fix GID cache for bonded net devices

[longli]

From: Long Li <longli@microsoft.com>

When populating GID cache for net devices in a bonded setup, it should use the master device's
address whenever applicable.

The current code has some incorrect behaviors when dealing with bonded devices:
1. It adds IP of bonded slave to the GID cache when the device is already bonded
2. It adds IP of bonded slave to the GID cache when the device becomes bonded (via NETDEV_CHANGEUPPER notifier)
3. When a bonded slave device is unbonded, it doesn't add its IP to the default table in GID cache.

The patchset fixes those issues.

Changes log:
v2: Added cover letter explaining the overall problem and current behaviors.

Long Li (3):
  IB/core: Do not use netdev IP if it is a bonded slave
  IB/core: Use upper_device_filter to add upper ips
  IB/core: Add default IP when a slave is unlinked

 drivers/infiniband/core/roce_gid_mgmt.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

-- 
2.34.1

[Patch v2 1/3] IB/core: Do not use netdev IP if it is a bonded slave

[longli]

From: Long Li <longli@microsoft.com>

Filter function is_eth_port_of_netdev_filter() is used to determine if a
netdev should be used for assigning its IP to GID cache. This function
should filter out bonded slave netdevs.

For bonded slaves, their master netdevs should be used to cache GIDs.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/infiniband/core/roce_gid_mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/roce_gid_mgmt.c b/drivers/infiniband/core/roce_gid_mgmt.c
index a9f2c6b1b29e..27a3ffed11b9 100644
--- a/drivers/infiniband/core/roce_gid_mgmt.c
+++ b/drivers/infiniband/core/roce_gid_mgmt.c
@@ -161,7 +161,7 @@ is_eth_port_of_netdev_filter(struct ib_device *ib_dev, u32 port,
 	res = ((rdma_is_upper_dev_rcu(rdma_ndev, cookie) &&
 	       (is_eth_active_slave_of_bonding_rcu(rdma_ndev, real_dev) &
 		REQUIRED_BOND_STATES)) ||
-	       real_dev == rdma_ndev);
+	       (real_dev == rdma_ndev && !netif_is_bond_slave(rdma_ndev)));
 
 	rcu_read_unlock();
 	return res;
-- 
2.34.1

[Patch v2 2/3] IB/core: Use upper_device_filter to add upper ips

[longli]

From: Long Li <longli@microsoft.com>

add_cmd_upper_ips() in ndev_event_link() is used to add upper IPs on a
bonded slave. Its filter function should be set to the same as
upper_ips_del_cmd() in ndev_event_unlink, in that they both look for upper
netdevs for GIDs.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/infiniband/core/roce_gid_mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/roce_gid_mgmt.c b/drivers/infiniband/core/roce_gid_mgmt.c
index 27a3ffed11b9..827a50dbd308 100644
--- a/drivers/infiniband/core/roce_gid_mgmt.c
+++ b/drivers/infiniband/core/roce_gid_mgmt.c
@@ -689,7 +689,7 @@ static const struct netdev_event_work_cmd add_cmd = {
 
 static const struct netdev_event_work_cmd add_cmd_upper_ips = {
 	.cb	= add_netdev_upper_ips,
-	.filter = is_eth_port_of_netdev_filter
+	.filter = upper_device_filter
 };
 
 static void
-- 
2.34.1

[Patch v2 3/3] IB/core: Add default IP when a slave is unlinked

[longli]

From: Long Li <longli@microsoft.com>

When a bonded slave is unlinked, the current code doesn't add a default
GID for the new unlinked netdev.

Add a default GID for the netdev.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/infiniband/core/roce_gid_mgmt.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/roce_gid_mgmt.c b/drivers/infiniband/core/roce_gid_mgmt.c
index 827a50dbd308..3fa2740fa0d2 100644
--- a/drivers/infiniband/core/roce_gid_mgmt.c
+++ b/drivers/infiniband/core/roce_gid_mgmt.c
@@ -692,6 +692,11 @@ static const struct netdev_event_work_cmd add_cmd_upper_ips = {
 	.filter = upper_device_filter
 };
 
+static const struct netdev_event_work_cmd add_default_gid_cmd = {
+	.cb	= add_default_gids,
+	.filter	= is_ndev_for_default_gid_filter,
+};
+
 static void
 ndev_event_unlink(struct netdev_notifier_changeupper_info *changeupper_info,
 		  struct netdev_event_work_cmd *cmds)
@@ -704,7 +709,8 @@ ndev_event_unlink(struct netdev_notifier_changeupper_info *changeupper_info,
 
 	cmds[0] = upper_ips_del_cmd;
 	cmds[0].ndev = changeupper_info->upper_dev;
-	cmds[1] = add_cmd;
+	cmds[1] = add_default_gid_cmd;
+	cmds[2] = add_cmd;
 }
 
 static const struct netdev_event_work_cmd bonding_default_add_cmd = {
@@ -751,11 +757,6 @@ static void netdevice_event_changeupper(struct net_device *event_ndev,
 		ndev_event_unlink(changeupper_info, cmds);
 }
 
-static const struct netdev_event_work_cmd add_default_gid_cmd = {
-	.cb	= add_default_gids,
-	.filter	= is_ndev_for_default_gid_filter,
-};
-
 static int netdevice_event(struct notifier_block *this, unsigned long event,
 			   void *ptr)
 {
-- 
2.34.1

Re: [Patch v2 0/3] IB/core: Fix GID cache for bonded net devices

[Leon Romanovsky]

On Fri, Feb 07, 2025 at 01:36:15PM -0800, longli@linuxonhyperv.com wrote:
> From: Long Li <longli@microsoft.com>
> 
> When populating GID cache for net devices in a bonded setup, it should use the master device's
> address whenever applicable.
> 
> The current code has some incorrect behaviors when dealing with bonded devices:
> 1. It adds IP of bonded slave to the GID cache when the device is already bonded
> 2. It adds IP of bonded slave to the GID cache when the device becomes bonded (via NETDEV_CHANGEUPPER notifier)
> 3. When a bonded slave device is unbonded, it doesn't add its IP to the default table in GID cache.

I took a look at the patches and would like to see the reasoning why
current behaviour is incorrect and need to be changed. In addition,
there is a need to add examples of what is "broken" now and will start
to work after the fixes.

Thanks


> 
> The patchset fixes those issues.
> 
> Changes log:
> v2: Added cover letter explaining the overall problem and current behaviors.
> 
> Long Li (3):
>   IB/core: Do not use netdev IP if it is a bonded slave
>   IB/core: Use upper_device_filter to add upper ips
>   IB/core: Add default IP when a slave is unlinked
> 
>  drivers/infiniband/core/roce_gid_mgmt.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> -- 
> 2.34.1
> 

RE: [EXTERNAL] Re: [Patch v2 0/3] IB/core: Fix GID cache for bonded net devices

[Long Li]

> Subject: [EXTERNAL] Re: [Patch v2 0/3] IB/core: Fix GID cache for bonded net
> devices
> 
> On Fri, Feb 07, 2025 at 01:36:15PM -0800, longli@linuxonhyperv.com wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > When populating GID cache for net devices in a bonded setup, it should
> > use the master device's address whenever applicable.
> >
> > The current code has some incorrect behaviors when dealing with bonded
> devices:
> > 1. It adds IP of bonded slave to the GID cache when the device is
> > already bonded 2. It adds IP of bonded slave to the GID cache when the
> > device becomes bonded (via NETDEV_CHANGEUPPER notifier) 3. When a
> bonded slave device is unbonded, it doesn't add its IP to the default table in GID
> cache.
> 
> I took a look at the patches and would like to see the reasoning why current
> behaviour is incorrect and need to be changed. In addition, there is a need to add
> examples of what is "broken" now and will start to work after the fixes.
> 
> Thanks


Thanks for looking. I will work on another set of patches based on feedback from:
https://lore.kernel.org/lkml/20250211163735.18d0fd02@hermes.local/

I have some questions on the RDMA GID cache code determining GID cache based on bonded device states. Please see following.

For an IB device, the RDMA GID cache code (rdma_roce_rescan_device() in drivers/infiniband/core/roce_gid_mgmt.c) looks at the following devices for its default GIDs:
1. This IB device if it is not a bonded slave (if this IB device is a slave but not bonded, it will be used for default GIDs)
2. This IB device's bonded master devices
Please see is_ndev_for_default_gid_filter() as its filtering function

And for those devices for this IB device's non-default GIDs:
1. An upper device to this IB device that is not bonded
2. An upper device to this IB device that is bonded to this IB device, and this IB device is the current active bonded slave
3. This IB device if it's not a VLAN type
See is_eth_port_of_netdev_filter() as its filtering function

To summarize, the GID caching behavior for an IB device which is also a slave device, looks like below:
1. It seems all upper devices (bonded or not) to this IB device will be used in the GID cache, but only its bonding master is used for the default GID cache
2. The IB device will not be used in default GID cache if it's bonded
3. The IB device will always be used in non-default GID caches. (assuming it's not VLAN)

My understanding is that the default GID cache will look for bonded master when checking on a slave device and its upper devices. The non-default GID cache just takes this IB device and all its upper devices.

Why the default GID cache needs to check bonded devices?

Thanks,
Long