[PATCH] hv_netvsc: Set device flags for properly indicating bonding

[PATCH] hv_netvsc: Set device flags for properly indicating bonding

[longli]

From: Long Li <longli@microsoft.com>

hv_netvsc uses a subset of bonding features in that the master always
has only one active slave. But it never properly setup those flags.

Other kernel APIs (e.g those in "include/linux/netdevice.h") check for
IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used
in a master/slave setup. RDMA uses those APIs extensively when looking
for master/slave devices.

Make hv_netvsc properly setup those flags.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d6c4abfc3a28..2112fb74eb21 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2204,6 +2204,10 @@ static int netvsc_vf_join(struct net_device *vf_netdev,
 		goto rx_handler_failed;
 	}
 
+	vf_netdev->priv_flags |= IFF_BONDING;
+	ndev->priv_flags |= IFF_BONDING;
+	ndev->flags |= IFF_MASTER;
+
 	ret = netdev_master_upper_dev_link(vf_netdev, ndev,
 					   NULL, NULL, NULL);
 	if (ret != 0) {
@@ -2484,7 +2488,15 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
 
 	reinit_completion(&net_device_ctx->vf_add);
 	netdev_rx_handler_unregister(vf_netdev);
+
+	/* Unlink the slave device and clear flag */
+	vf_netdev->priv_flags &= ~IFF_BONDING;
+	ndev->priv_flags &= ~IFF_BONDING;
+	vf_netdev->flags &= ~IFF_SLAVE;
+	ndev->flags &= ~IFF_MASTER;
+
 	netdev_upper_dev_unlink(vf_netdev, ndev);
+
 	RCU_INIT_POINTER(net_device_ctx->vf_netdev, NULL);
 	dev_put(vf_netdev);
 
-- 
2.34.1

RE: [EXTERNAL] [PATCH] hv_netvsc: Set device flags for properly indicating bonding

[Haiyang Zhang]

> -----Original Message-----
> From: longli@linuxonhyperv.com <longli@linuxonhyperv.com>
> Sent: Wednesday, November 27, 2024 2:43 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui
> <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Shradha Gupta
> <shradhagupta@linux.microsoft.com>; Simon Horman <horms@kernel.org>;
> Konstantin Taranov <kotaranov@microsoft.com>; Souradeep Chakrabarti
> <schakrabarti@linux.microsoft.com>; Erick Archer
> <erick.archer@outlook.com>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> rdma@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: [EXTERNAL] [PATCH] hv_netvsc: Set device flags for properly
> indicating bonding
> 
> From: Long Li <longli@microsoft.com>
> 
> hv_netvsc uses a subset of bonding features in that the master always
> has only one active slave. But it never properly setup those flags.
> 
> Other kernel APIs (e.g those in "include/linux/netdevice.h") check for
> IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used
> in a master/slave setup. RDMA uses those APIs extensively when looking
> for master/slave devices.
> 
> Make hv_netvsc properly setup those flags.
> 
> Signed-off-by: Long Li <longli@microsoft.com>

Please add Fixes tag, Cc stable, add "net" to the subject.
Thanks.
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

Re: [PATCH] hv_netvsc: Set device flags for properly indicating bonding

[Jakub Kicinski]

On Wed, 27 Nov 2024 11:42:50 -0800 longli@linuxonhyperv.com wrote:
> hv_netvsc uses a subset of bonding features in that the master always
> has only one active slave. But it never properly setup those flags.
> 
> Other kernel APIs (e.g those in "include/linux/netdevice.h") check for
> IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used
> in a master/slave setup. 

I feel like this has been nacked 10 times already?
IFF_BONDING means the bonding driver.
There is more than one driver in the tree providing link aggregation
and only bonding uses IFF_BONDING. If some user is buggy fix the user.
-- 
pw-bot: reject

RE: [EXTERNAL] Re: [PATCH] hv_netvsc: Set device flags for properly indicating bonding

[Long Li]

> Subject: [EXTERNAL] Re: [PATCH] hv_netvsc: Set device flags for properly
> indicating bonding
> 
> On Wed, 27 Nov 2024 11:42:50 -0800 longli@linuxonhyperv.com wrote:
> > hv_netvsc uses a subset of bonding features in that the master always
> > has only one active slave. But it never properly setup those flags.
> >
> > Other kernel APIs (e.g those in "include/linux/netdevice.h") check for
> > IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used
> > in a master/slave setup.
> 
> I feel like this has been nacked 10 times already?
> IFF_BONDING means the bonding driver.
> There is more than one driver in the tree providing link aggregation and only
> bonding uses IFF_BONDING. If some user is buggy fix the user.
> --
> pw-bot: reject

Sorry I didn't know this has been discussed in other threads. As far as I know, this is probably the 1st time it is discussed in the context of netvsc.

My understanding is that netvsc is a special use-case of bonding which is implemented as an emulated device in drivers/net/bonding. It is the only non-emulated driver that sets IFF_MASTER and IFF_SLAVE flags on netdevs. After the master/slave devices are set up in this way, the behavior is very similar to that of the bonding device with a single active bonded slave.

There are code that use netif_is_bond_master() and netif_is_bond_slave() to decide how a netdev should be used when it is in a master/slave setup. One example is "drivers/infiniband/core/roce_gid_mgmt.c". Their use case is relevant to netvsc and its slave device setup.

I haven't found a good way to communicate the relationship of netvsc and its slave netdev to those code. The best solution I can think of is to use the IFF_BONDING, as it is the closest representation of this relationship. Another way would be adding a new IFF flag (e.g. IFF_PERMSLAVE) to netdev_priv_flags. I feel this is not needed for this special use-case in netvsc.

Thanks,

Long

Re: [PATCH] hv_netvsc: Set device flags for properly indicating bonding

[Stephen Hemminger]

On Wed, 27 Nov 2024 11:42:50 -0800
longli@linuxonhyperv.com wrote:

> hv_netvsc uses a subset of bonding features in that the master always
> has only one active slave. But it never properly setup those flags.
> 
> Other kernel APIs (e.g those in "include/linux/netdevice.h") check for
> IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used
> in a master/slave setup. RDMA uses those APIs extensively when looking
> for master/slave devices.
> 
> Make hv_netvsc properly setup those flags.
> 
> Signed-off-by: Long Li <longli@microsoft.com>

Linux networking has evolved a patch work of flags to network devices
but never really standardized on a way to express linked relationships
between network devices. There are several drivers do this in slighlty
different and overlapping ways: bonding, team, failover, hyperv, bridge
and others.

The current convention is to mark the primary device as IFF_MASTER
and each secondary device with IFF_SLAVE.  But not clear what the
right combination is if stacked more than one level.  Also, not clear
if userspace and other addressing should use or not use nested devices.

It would be ideal to deprecate and use different terms than
the non-inclusive terms master/slave
but probably that would have to have been done years ago (like 2.5).

For now, it makes sense for all the nested devices to use IFF_MASTER
and IFF_SLAVE consistently. It is not a good idea to set the priv_flag
for IFF_BONDING (or any of the other bits) which should be reserved
for one driver. 

If hyperv driver needs to it could add its own flag in netdev_priv_flags,
but it looks like that is running out of bits. May need to grow to 64 bits
or do some more work to add a new field for device type. I.e. there
are combinations of flags that are impossible and having one bit per
type leads to a problem. Fixing that is non trivial but not impossible
level of effort.

My thoughts, but I don't use or work on Hyper-v anymore.

RE: [EXTERNAL] Re: [PATCH] hv_netvsc: Set device flags for properly indicating bonding

[Long Li]

> Subject: [EXTERNAL] Re: [PATCH] hv_netvsc: Set device flags for properly
> indicating bonding
> 
> On Wed, 27 Nov 2024 11:42:50 -0800
> longli@linuxonhyperv.com wrote:
> 
> > hv_netvsc uses a subset of bonding features in that the master always
> > has only one active slave. But it never properly setup those flags.
> >
> > Other kernel APIs (e.g those in "include/linux/netdevice.h") check for
> > IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used
> > in a master/slave setup. RDMA uses those APIs extensively when looking
> > for master/slave devices.
> >
> > Make hv_netvsc properly setup those flags.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> 
> Linux networking has evolved a patch work of flags to network devices but never
> really standardized on a way to express linked relationships between network
> devices. There are several drivers do this in slighlty different and overlapping
> ways: bonding, team, failover, hyperv, bridge and others.
> 
> The current convention is to mark the primary device as IFF_MASTER and each
> secondary device with IFF_SLAVE.  But not clear what the right combination is if
> stacked more than one level.  Also, not clear if userspace and other addressing
> should use or not use nested devices.
> 
> It would be ideal to deprecate and use different terms than the non-inclusive
> terms master/slave but probably that would have to have been done years ago
> (like 2.5).
> 
> For now, it makes sense for all the nested devices to use IFF_MASTER and
> IFF_SLAVE consistently. It is not a good idea to set the priv_flag for IFF_BONDING
> (or any of the other bits) which should be reserved for one driver.
> 
> If hyperv driver needs to it could add its own flag in netdev_priv_flags, but it looks
> like that is running out of bits. May need to grow to 64 bits or do some more work
> to add a new field for device type. I.e. there are combinations of flags that are
> impossible and having one bit per type leads to a problem. Fixing that is non trivial
> but not impossible level of effort.
> 
> My thoughts, but I don't use or work on Hyper-v anymore.

Thank you. I have sent v2.

Long