[Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[longli]

From: Long Li <longli@microsoft.com>

When running under Hyper-V, the master device to the RDMA device is always
bonded to this RDMA device if it's present in the kernel. This is not
user-configurable.

The master device can be unbind/bind from the kernel. During those events,
the RDMA device should set to the current netdev to relect the change of
master device from those events.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/infiniband/hw/mana/device.c  | 35 ++++++++++++++++++++++++++++
 drivers/infiniband/hw/mana/mana_ib.h |  1 +
 2 files changed, 36 insertions(+)

diff --git a/drivers/infiniband/hw/mana/device.c b/drivers/infiniband/hw/mana/device.c
index 3416a85f8738..3e4f069c2258 100644
--- a/drivers/infiniband/hw/mana/device.c
+++ b/drivers/infiniband/hw/mana/device.c
@@ -51,6 +51,37 @@ static const struct ib_device_ops mana_ib_dev_ops = {
 			   ib_ind_table),
 };
 
+static int mana_ib_netdev_event(struct notifier_block *this,
+				unsigned long event, void *ptr)
+{
+	struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev, nb);
+	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
+	struct gdma_context *gc = dev->gdma_dev->gdma_context;
+	struct mana_context *mc = gc->mana.driver_data;
+	struct net_device *ndev;
+
+	if (event_dev != mc->ports[0])
+		return NOTIFY_DONE;
+
+	switch (event) {
+	case NETDEV_CHANGEUPPER:
+		rcu_read_lock();
+		ndev = mana_get_primary_netdev_rcu(mc, 0);
+		rcu_read_unlock();
+
+		/*
+		 * RDMA core will setup GID based on updated netdev.
+		 * It's not possible to race with the core as rtnl lock is being
+		 * held.
+		 */
+		ib_device_set_netdev(&dev->ib_dev, ndev, 1);
+
+		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
+	}
+}
+
 static int mana_ib_probe(struct auxiliary_device *adev,
 			 const struct auxiliary_device_id *id)
 {
@@ -109,6 +140,9 @@ static int mana_ib_probe(struct auxiliary_device *adev,
 	}
 	dev->gdma_dev = &mdev->gdma_context->mana_ib;
 
+	dev->nb.notifier_call = mana_ib_netdev_event;
+	register_netdevice_notifier(&dev->nb);
+
 	ret = mana_ib_gd_query_adapter_caps(dev);
 	if (ret) {
 		ibdev_err(&dev->ib_dev, "Failed to query device caps, ret %d",
@@ -159,6 +193,7 @@ static void mana_ib_remove(struct auxiliary_device *adev)
 {
 	struct mana_ib_dev *dev = dev_get_drvdata(&adev->dev);
 
+	unregister_netdevice_notifier(&dev->nb);
 	ib_unregister_device(&dev->ib_dev);
 	xa_destroy(&dev->qp_table_wq);
 	mana_ib_gd_destroy_rnic_adapter(dev);
diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
index b53a5b4de908..d88187072899 100644
--- a/drivers/infiniband/hw/mana/mana_ib.h
+++ b/drivers/infiniband/hw/mana/mana_ib.h
@@ -64,6 +64,7 @@ struct mana_ib_dev {
 	struct gdma_queue **eqs;
 	struct xarray qp_table_wq;
 	struct mana_ib_adapter_caps adapter_caps;
+	struct notifier_block nb;
 };
 
 struct mana_ib_wq {
-- 
2.34.1

RE: [EXTERNAL] [Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Long Li]

> Subject: [Patch rdma-next] RDMA/mana_ib: handle net event for
> pointing to the current netdev
> 
> From: Long Li <longli@microsoft.com>
> 
> When running under Hyper-V, the master device to the RDMA device is always
> bonded to this RDMA device if it's present in the kernel. This is not user-
> configurable.
> 
> The master device can be unbind/bind from the kernel. During those events,
> the RDMA device should set to the current netdev to relect the change of
> master device from those events.
> 
> Signed-off-by: Long Li <longli@microsoft.com>

This is not the correct patch. Please discard this patch. 

> ---
>  drivers/infiniband/hw/mana/device.c  | 35
> ++++++++++++++++++++++++++++
> drivers/infiniband/hw/mana/mana_ib.h |  1 +
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/mana/device.c
> b/drivers/infiniband/hw/mana/device.c
> index 3416a85f8738..3e4f069c2258 100644
> --- a/drivers/infiniband/hw/mana/device.c
> +++ b/drivers/infiniband/hw/mana/device.c
> @@ -51,6 +51,37 @@ static const struct ib_device_ops mana_ib_dev_ops = {
>  			   ib_ind_table),
>  };
> 
> +static int mana_ib_netdev_event(struct notifier_block *this,
> +				unsigned long event, void *ptr)
> +{
> +	struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev,
> nb);
> +	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +	struct gdma_context *gc = dev->gdma_dev->gdma_context;
> +	struct mana_context *mc = gc->mana.driver_data;
> +	struct net_device *ndev;
> +
> +	if (event_dev != mc->ports[0])
> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case NETDEV_CHANGEUPPER:
> +		rcu_read_lock();
> +		ndev = mana_get_primary_netdev_rcu(mc, 0);
> +		rcu_read_unlock();
> +
> +		/*
> +		 * RDMA core will setup GID based on updated netdev.
> +		 * It's not possible to race with the core as rtnl lock is being
> +		 * held.
> +		 */
> +		ib_device_set_netdev(&dev->ib_dev, ndev, 1);
> +
> +		return NOTIFY_OK;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +}
> +
>  static int mana_ib_probe(struct auxiliary_device *adev,
>  			 const struct auxiliary_device_id *id)  { @@ -109,6
> +140,9 @@ static int mana_ib_probe(struct auxiliary_device *adev,
>  	}
>  	dev->gdma_dev = &mdev->gdma_context->mana_ib;
> 
> +	dev->nb.notifier_call = mana_ib_netdev_event;
> +	register_netdevice_notifier(&dev->nb);
> +
>  	ret = mana_ib_gd_query_adapter_caps(dev);
>  	if (ret) {
>  		ibdev_err(&dev->ib_dev, "Failed to query device caps, ret %d",
> @@ -159,6 +193,7 @@ static void mana_ib_remove(struct auxiliary_device
> *adev)  {
>  	struct mana_ib_dev *dev = dev_get_drvdata(&adev->dev);
> 
> +	unregister_netdevice_notifier(&dev->nb);
>  	ib_unregister_device(&dev->ib_dev);
>  	xa_destroy(&dev->qp_table_wq);
>  	mana_ib_gd_destroy_rnic_adapter(dev);
> diff --git a/drivers/infiniband/hw/mana/mana_ib.h
> b/drivers/infiniband/hw/mana/mana_ib.h
> index b53a5b4de908..d88187072899 100644
> --- a/drivers/infiniband/hw/mana/mana_ib.h
> +++ b/drivers/infiniband/hw/mana/mana_ib.h
> @@ -64,6 +64,7 @@ struct mana_ib_dev {
>  	struct gdma_queue **eqs;
>  	struct xarray qp_table_wq;
>  	struct mana_ib_adapter_caps adapter_caps;
> +	struct notifier_block nb;
>  };
> 
>  struct mana_ib_wq {
> --
> 2.34.1

Re: [Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Jason Gunthorpe]

On Fri, Feb 28, 2025 at 02:41:59PM -0800, longli@linuxonhyperv.com wrote:
> +	struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev, nb);
> +	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +	struct gdma_context *gc = dev->gdma_dev->gdma_context;
> +	struct mana_context *mc = gc->mana.driver_data;
> +	struct net_device *ndev;
> +
> +	if (event_dev != mc->ports[0])
> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case NETDEV_CHANGEUPPER:
> +		rcu_read_lock();
> +		ndev = mana_get_primary_netdev_rcu(mc, 0);
> +		rcu_read_unlock();

That locking sure looks weird/wrong.

Jason

RE: [EXTERNAL] Re: [Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Long Li]

> Subject: [EXTERNAL] Re: [Patch rdma-next] RDMA/mana_ib: handle net event
> for pointing to the current netdev
> 
> On Fri, Feb 28, 2025 at 02:41:59PM -0800, longli@linuxonhyperv.com wrote:
> > +	struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev,
> nb);
> > +	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > +	struct gdma_context *gc = dev->gdma_dev->gdma_context;
> > +	struct mana_context *mc = gc->mana.driver_data;
> > +	struct net_device *ndev;
> > +
> > +	if (event_dev != mc->ports[0])
> > +		return NOTIFY_DONE;
> > +
> > +	switch (event) {
> > +	case NETDEV_CHANGEUPPER:
> > +		rcu_read_lock();
> > +		ndev = mana_get_primary_netdev_rcu(mc, 0);
> > +		rcu_read_unlock();
> 
> That locking sure looks weird/wrong.
> 
> Jason

I think the locking is necessary as mana_get_primary_netdev_rcu() requires a RCU lock. It's safe to nest rcu_read_lock() if it's already held.

I have sent v2. v1 is from an earlier branch and is missing some error handling code.

Thanks,
Long

Re: [Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Ratheesh Kannoth]

On 2025-03-01 at 04:11:59, longli@linuxonhyperv.com (longli@linuxonhyperv.com) wrote:
> From: Long Li <longli@microsoft.com>
>
> When running under Hyper-V, the master device to the RDMA device is always
> bonded to this RDMA device if it's present in the kernel. This is not
> user-configurable.
>
> The master device can be unbind/bind from the kernel. During those events,
> the RDMA device should set to the current netdev to relect the change of
> master device from those events.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/infiniband/hw/mana/device.c  | 35 ++++++++++++++++++++++++++++
>  drivers/infiniband/hw/mana/mana_ib.h |  1 +
>  2 files changed, 36 insertions(+)
>
> diff --git a/drivers/infiniband/hw/mana/device.c b/drivers/infiniband/hw/mana/device.c
> index 3416a85f8738..3e4f069c2258 100644
> --- a/drivers/infiniband/hw/mana/device.c
> +++ b/drivers/infiniband/hw/mana/device.c
> @@ -51,6 +51,37 @@ static const struct ib_device_ops mana_ib_dev_ops = {
>  			   ib_ind_table),
>  };
>
> +static int mana_ib_netdev_event(struct notifier_block *this,
> +				unsigned long event, void *ptr)
> +{
> +	struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev, nb);
> +	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +	struct gdma_context *gc = dev->gdma_dev->gdma_context;
> +	struct mana_context *mc = gc->mana.driver_data;
> +	struct net_device *ndev;
> +
> +	if (event_dev != mc->ports[0])
> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case NETDEV_CHANGEUPPER:
> +		rcu_read_lock();
> +		ndev = mana_get_primary_netdev_rcu(mc, 0);
> +		rcu_read_unlock();
...
> +
> +		/*
> +		 * RDMA core will setup GID based on updated netdev.
> +		 * It's not possible to race with the core as rtnl lock is being
> +		 * held.
> +		 */
> +		ib_device_set_netdev(&dev->ib_dev, ndev, 1);
rcu_read_unlock() should be here, right ?
> +
> +		return NOTIFY_OK;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +}
> +
>

RE: [EXTERNAL] Re: [Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Long Li]

> On 2025-03-01 at 04:11:59, longli@linuxonhyperv.com
> (longli@linuxonhyperv.com) wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > When running under Hyper-V, the master device to the RDMA device is
> > always bonded to this RDMA device if it's present in the kernel. This
> > is not user-configurable.
> >
> > The master device can be unbind/bind from the kernel. During those
> > events, the RDMA device should set to the current netdev to relect the
> > change of master device from those events.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >  drivers/infiniband/hw/mana/device.c  | 35
> > ++++++++++++++++++++++++++++  drivers/infiniband/hw/mana/mana_ib.h |
> > 1 +
> >  2 files changed, 36 insertions(+)
> >
> > diff --git a/drivers/infiniband/hw/mana/device.c
> > b/drivers/infiniband/hw/mana/device.c
> > index 3416a85f8738..3e4f069c2258 100644
> > --- a/drivers/infiniband/hw/mana/device.c
> > +++ b/drivers/infiniband/hw/mana/device.c
> > @@ -51,6 +51,37 @@ static const struct ib_device_ops mana_ib_dev_ops = {
> >                          ib_ind_table),  };
> >
> > +static int mana_ib_netdev_event(struct notifier_block *this,
> > +                             unsigned long event, void *ptr) {
> > +     struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev, nb);
> > +     struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > +     struct gdma_context *gc = dev->gdma_dev->gdma_context;
> > +     struct mana_context *mc = gc->mana.driver_data;
> > +     struct net_device *ndev;
> > +
> > +     if (event_dev != mc->ports[0])
> > +             return NOTIFY_DONE;
> > +
> > +     switch (event) {
> > +     case NETDEV_CHANGEUPPER:
> > +             rcu_read_lock();
> > +             ndev = mana_get_primary_netdev_rcu(mc, 0);
> > +             rcu_read_unlock();
> ...
> > +
> > +             /*
> > +              * RDMA core will setup GID based on updated netdev.
> > +              * It's not possible to race with the core as rtnl lock is being
> > +              * held.
> > +              */
> > +             ib_device_set_netdev(&dev->ib_dev, ndev, 1);
> rcu_read_unlock() should be here, right ?

It can't.  ib_device_set_netdev() is calling alloc_port_data() and may sleep.

I think this locking is okay. This event only comes in when:
1. the master device has changed to netvsc. In this case ndev is guaranteed to be valid as this notification is triggered by netvsc.
2. the master device has changed to itself (the ethernet device parent for the IB device).  In this case, ndev is valid because mana_ib is an auxiliary device to ndev and it can't unload itself at this time.

Re: [EXTERNAL] Re: [Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Jason Gunthorpe]

On Tue, Mar 04, 2025 at 06:26:03PM +0000, Long Li wrote:
> > On 2025-03-01 at 04:11:59, longli@linuxonhyperv.com
> > (longli@linuxonhyperv.com) wrote:
> > > From: Long Li <longli@microsoft.com>
> > >
> > > When running under Hyper-V, the master device to the RDMA device is
> > > always bonded to this RDMA device if it's present in the kernel. This
> > > is not user-configurable.
> > >
> > > The master device can be unbind/bind from the kernel. During those
> > > events, the RDMA device should set to the current netdev to relect the
> > > change of master device from those events.
> > >
> > > Signed-off-by: Long Li <longli@microsoft.com>
> > > ---
> > >  drivers/infiniband/hw/mana/device.c  | 35
> > > ++++++++++++++++++++++++++++  drivers/infiniband/hw/mana/mana_ib.h |
> > > 1 +
> > >  2 files changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/hw/mana/device.c
> > > b/drivers/infiniband/hw/mana/device.c
> > > index 3416a85f8738..3e4f069c2258 100644
> > > --- a/drivers/infiniband/hw/mana/device.c
> > > +++ b/drivers/infiniband/hw/mana/device.c
> > > @@ -51,6 +51,37 @@ static const struct ib_device_ops mana_ib_dev_ops = {
> > >                          ib_ind_table),  };
> > >
> > > +static int mana_ib_netdev_event(struct notifier_block *this,
> > > +                             unsigned long event, void *ptr) {
> > > +     struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev, nb);
> > > +     struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > > +     struct gdma_context *gc = dev->gdma_dev->gdma_context;
> > > +     struct mana_context *mc = gc->mana.driver_data;
> > > +     struct net_device *ndev;
> > > +
> > > +     if (event_dev != mc->ports[0])
> > > +             return NOTIFY_DONE;
> > > +
> > > +     switch (event) {
> > > +     case NETDEV_CHANGEUPPER:
> > > +             rcu_read_lock();
> > > +             ndev = mana_get_primary_netdev_rcu(mc, 0);
> > > +             rcu_read_unlock();
> > ...
> > > +
> > > +             /*
> > > +              * RDMA core will setup GID based on updated netdev.
> > > +              * It's not possible to race with the core as rtnl lock is being
> > > +              * held.
> > > +              */
> > > +             ib_device_set_netdev(&dev->ib_dev, ndev, 1);
> > rcu_read_unlock() should be here, right ?
> 
> It can't.  ib_device_set_netdev() is calling alloc_port_data() and may sleep.
> 
> I think this locking is okay. This event only comes in when:
> 1. the master device has changed to netvsc. In this case ndev is guaranteed to be valid as this notification is triggered by netvsc.
> 2. the master device has changed to itself (the ethernet device parent for the IB device).  In this case, ndev is valid because mana_ib is an auxiliary device to ndev and it can't unload itself at this time.


Why not return with the netdev refcount held so you don't need this
weirdo rcu thing?

Jason

RE: [EXTERNAL] Re: [Patch rdma-next] RDMA/mana_ib: handle net event for pointing to the current netdev

[Long Li]

> Subject: Re: [EXTERNAL] Re: [Patch rdma-next] RDMA/mana_ib: handle net
> event for pointing to the current netdev
> 
> On Tue, Mar 04, 2025 at 06:26:03PM +0000, Long Li wrote:
> > > On 2025-03-01 at 04:11:59, longli@linuxonhyperv.com
> > > (longli@linuxonhyperv.com) wrote:
> > > > From: Long Li <longli@microsoft.com>
> > > >
> > > > When running under Hyper-V, the master device to the RDMA device
> > > > is always bonded to this RDMA device if it's present in the
> > > > kernel. This is not user-configurable.
> > > >
> > > > The master device can be unbind/bind from the kernel. During those
> > > > events, the RDMA device should set to the current netdev to relect
> > > > the change of master device from those events.
> > > >
> > > > Signed-off-by: Long Li <longli@microsoft.com>
> > > > ---
> > > >  drivers/infiniband/hw/mana/device.c  | 35
> > > > ++++++++++++++++++++++++++++  drivers/infiniband/hw/mana/mana_ib.h
> > > > ++++++++++++++++++++++++++++ |
> > > > 1 +
> > > >  2 files changed, 36 insertions(+)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mana/device.c
> > > > b/drivers/infiniband/hw/mana/device.c
> > > > index 3416a85f8738..3e4f069c2258 100644
> > > > --- a/drivers/infiniband/hw/mana/device.c
> > > > +++ b/drivers/infiniband/hw/mana/device.c
> > > > @@ -51,6 +51,37 @@ static const struct ib_device_ops mana_ib_dev_ops =
> {
> > > >                          ib_ind_table),  };
> > > >
> > > > +static int mana_ib_netdev_event(struct notifier_block *this,
> > > > +                             unsigned long event, void *ptr) {
> > > > +     struct mana_ib_dev *dev = container_of(this, struct mana_ib_dev, nb);
> > > > +     struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > > > +     struct gdma_context *gc = dev->gdma_dev->gdma_context;
> > > > +     struct mana_context *mc = gc->mana.driver_data;
> > > > +     struct net_device *ndev;
> > > > +
> > > > +     if (event_dev != mc->ports[0])
> > > > +             return NOTIFY_DONE;
> > > > +
> > > > +     switch (event) {
> > > > +     case NETDEV_CHANGEUPPER:
> > > > +             rcu_read_lock();
> > > > +             ndev = mana_get_primary_netdev_rcu(mc, 0);
> > > > +             rcu_read_unlock();
> > > ...
> > > > +
> > > > +             /*
> > > > +              * RDMA core will setup GID based on updated netdev.
> > > > +              * It's not possible to race with the core as rtnl lock is being
> > > > +              * held.
> > > > +              */
> > > > +             ib_device_set_netdev(&dev->ib_dev, ndev, 1);
> > > rcu_read_unlock() should be here, right ?
> >
> > It can't.  ib_device_set_netdev() is calling alloc_port_data() and may sleep.
> >
> > I think this locking is okay. This event only comes in when:
> > 1. the master device has changed to netvsc. In this case ndev is guaranteed to
> be valid as this notification is triggered by netvsc.
> > 2. the master device has changed to itself (the ethernet device parent for the IB
> device).  In this case, ndev is valid because mana_ib is an auxiliary device to ndev
> and it can't unload itself at this time.
> 
> 
> Why not return with the netdev refcount held so you don't need this weirdo rcu
> thing?
> 
> Jason

I sent v3 with netdev refcount held. Thank you.

Long