Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib
From: Konstantin Taranov <kotaranov@microsoft.com>
Date: 2024-06-26 09:05:09
Also in:
linux-rdma
Subsystem:
infiniband subsystem, the rest · Maintainers:
Jason Gunthorpe, Leon Romanovsky, Linus Torvalds
quoted
When mc->ports[0] is not slave, use it in the set_netdev. When mana is used in netvsc, the stored net devices in mana are slaves and GIDs should be taken from their master devices. In the baremetal case, the mc->ports devices will not be slaves.I wonder, why do you have "... | IFF_SLAVE" in __netvsc_vf_setup() in a first place? Isn't IFF_SLAVE is supposed to be set by bond driver?
I guess it is just a valid use of the IFF_SLAVE bit. In the bond case it is also set as a BOND netdev. The IFF_SLAVE helps to show users that another master netdev should be used for networking. But I am not an expert in netvsc. Actually, another alternative solution for mana_ib is always set the slave device, but in the GID mgmt code we need the following patch. The problem is that it may require testing/confirmation from other ib providers as in the worst case some GIDs will not be listed.
diff --git a/drivers/infiniband/core/roce_gid_mgmt.c b/drivers/infiniband/core/roce_gid_mgmt.c
index d5131b3ba8ab..0f20b4e2d1c2 100644
--- a/drivers/infiniband/core/roce_gid_mgmt.c
+++ b/drivers/infiniband/core/roce_gid_mgmt.c@@ -141,6 +141,8 @@ static enum bonding_slave_state is_eth_active_slave_of_bonding_rcu(struct net_de return BONDING_SLAVE_STATE_NA; } +#define netdev_is_slave(dev) (((dev)->flags & IFF_SLAVE) == IFF_SLAVE) + #define REQUIRED_BOND_STATES (BONDING_SLAVE_STATE_ACTIVE | \ BONDING_SLAVE_STATE_NA) static bool
@@ -157,11 +159,14 @@ is_eth_port_of_netdev_filter(struct ib_device *ib_dev, u32 port, real_dev = rdma_vlan_dev_real_dev(cookie); if (!real_dev) real_dev = cookie; - + /* + * When rdma netdevice is used in netvsc, the master netdevice should + * be considered for GIDs. Therefore, ignore slave rdma netdevices. + */ 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 && !netdev_is_slave(real_dev))); rcu_read_unlock(); return res;
@@ -211,12 +216,14 @@ is_ndev_for_default_gid_filter(struct ib_device *ib_dev, u32 port, /* * When rdma netdevice is used in bonding, bonding master netdevice - * should be considered for default GIDs. Therefore, ignore slave rdma - * netdevices when bonding is considered. + * should be considered for default GIDs. + * When rdma netdevice is used in netvsc, the master netdevice should + * be considered for defauld GIDs. Therefore, ignore slave rdma + * netdevices. * Additionally when event(cookie) netdevice is bond master device, * make sure that it the upper netdevice of rdma netdevice. */ - res = ((cookie_ndev == rdma_ndev && !netif_is_bond_slave(rdma_ndev)) || + res = ((cookie_ndev == rdma_ndev && !netdev_is_slave(rdma_ndev)) || (netif_is_bond_master(cookie_ndev) && rdma_is_upper_dev_rcu(rdma_ndev, cookie_ndev)));
quoted
+#define mana_ndev_is_slave(dev) (((dev)->flags & IFF_SLAVE) ==IFF_SLAVE) There is no need in macro for one line of code and there is no need in "==", as the result will be boolean.
Sure, can address in v2. I just saw a similar macro in another kernel file.