On Wed, Jun 26, 2024 at 09:05:05AM +0000, Konstantin Taranov wrote: > > > 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. The thing is that netvsc is virtual device like many others, but it is the only one who uses IFF_SLAVE bit. The comment around that bit says "slave of a load balancer.", which is not the case according to the Hyper-V documentation. https://learn.microsoft.com/en-us/windows-hardware/drivers/network/overview-of-hyper-v You will need to get Ack from netdev maintainers to rely on IFF_SLAVE bit in the way you are relying on it now. > > 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. is_eth_active_slave_of_bonding_rcu() is for bonding. > > 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))); > > > > +#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. I grepped too and this is why it caused me to wonder why it is not used except small number of places. Thanks > >
