RE: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

RE: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

[Long Li]

> >
> > 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.

Sorry, need to bring this issue up again.

This patch has broken user-space programs (e.g DPDK) that requires to export a kernel device to user-mode.

With this patch, the RDMA driver grabbed a reference from the master device, it's impossible to move the master device to user-mode.

I think the root cause is that the individual driver should not decide on which (master or slave) address should be used for GID. roce_gid_mgmt.c should handle this situation.

I think Konstantin's suggestion makes sense, how about we do this (don't need to define netdev_is_slave(dev)):

--- 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;


is_eth_port_of_netdev_filter() should not return true if this netdev is a bonded slave. In this case, only use the address of its bonded master.

Thanks,

Long

RE: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

[Parav Pandit]

> From: Long Li <longli@microsoft.com>
> Sent: Thursday, November 21, 2024 5:34 AM
> 
> > >
> > > 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.
> 
> Sorry, need to bring this issue up again.
> 
> This patch has broken user-space programs (e.g DPDK) that requires to
> export a kernel device to user-mode.
> 
> With this patch, the RDMA driver grabbed a reference from the master
> device, it's impossible to move the master device to user-mode.
> 
> I think the root cause is that the individual driver should not decide on which
> (master or slave) address should be used for GID. roce_gid_mgmt.c should
> handle this situation.
> 
> I think Konstantin's suggestion makes sense, how about we do this (don't
> need to define netdev_is_slave(dev)):
> 
> --- 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;
> 
> 
> is_eth_port_of_netdev_filter() should not return true if this netdev is a
> bonded slave. In this case, only use the address of its bonded master.
> 
Right. This change makes sense to me.
I don't have a setup presently to verify it to ensure I didn't miss a corner case.
Leon,
Can you or others please test the regression once with the formal patch?

Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

[Leon Romanovsky]

On Mon, Nov 25, 2024 at 03:56:01PM +0000, Parav Pandit wrote:
> 
> 
> > From: Long Li <longli@microsoft.com>
> > Sent: Thursday, November 21, 2024 5:34 AM
> > 
> > > >
> > > > 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.
> > 
> > Sorry, need to bring this issue up again.
> > 
> > This patch has broken user-space programs (e.g DPDK) that requires to
> > export a kernel device to user-mode.
> > 
> > With this patch, the RDMA driver grabbed a reference from the master
> > device, it's impossible to move the master device to user-mode.
> > 
> > I think the root cause is that the individual driver should not decide on which
> > (master or slave) address should be used for GID. roce_gid_mgmt.c should
> > handle this situation.
> > 
> > I think Konstantin's suggestion makes sense, how about we do this (don't
> > need to define netdev_is_slave(dev)):
> > 
> > --- 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;
> > 
> > 
> > is_eth_port_of_netdev_filter() should not return true if this netdev is a
> > bonded slave. In this case, only use the address of its bonded master.
> > 
> Right. This change makes sense to me.
> I don't have a setup presently to verify it to ensure I didn't miss a corner case.
> Leon,
> Can you or others please test the regression once with the formal patch?

Sure, once Long will send the patch, I'll make sure that it is tested.

Thanks

> 

RE: [EXTERNAL] Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

[Long Li]

> > > I think Konstantin's suggestion makes sense, how about we do this
> > > (don't need to define netdev_is_slave(dev)):
> > >
> > > --- 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;
> > >
> > >
> > > is_eth_port_of_netdev_filter() should not return true if this netdev
> > > is a bonded slave. In this case, only use the address of its bonded master.
> > >
> > Right. This change makes sense to me.
> > I don't have a setup presently to verify it to ensure I didn't miss a corner case.
> > Leon,
> > Can you or others please test the regression once with the formal patch?
> 
> Sure, once Long will send the patch, I'll make sure that it is tested.
> 
> Thanks
> 

I posted patches for discussion.
https://lore.kernel.org/linux-rdma/1732736619-19941-1-git-send-email-longli@linuxonhyperv.com/T/#t

Thank you,
Long

Re: [EXTERNAL] Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

[Leon Romanovsky]

On Wed, Nov 27, 2024 at 07:46:39PM +0000, Long Li wrote:
> 
> > > > I think Konstantin's suggestion makes sense, how about we do this
> > > > (don't need to define netdev_is_slave(dev)):
> > > >
> > > > --- 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;
> > > >
> > > >
> > > > is_eth_port_of_netdev_filter() should not return true if this netdev
> > > > is a bonded slave. In this case, only use the address of its bonded master.
> > > >
> > > Right. This change makes sense to me.
> > > I don't have a setup presently to verify it to ensure I didn't miss a corner case.
> > > Leon,
> > > Can you or others please test the regression once with the formal patch?
> > 
> > Sure, once Long will send the patch, I'll make sure that it is tested.
> > 
> > Thanks
> > 
> 
> I posted patches for discussion.
> https://lore.kernel.org/linux-rdma/1732736619-19941-1-git-send-email-longli@linuxonhyperv.com/T/#t

Please resend these patches as series with cover letter and don't embed
extra patch (the one which is not numbered) into the series.

Thanks

> 
> Thank you,
> Long
> 

RE: [EXTERNAL] Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

[Long Li]

> Subject: Re: [EXTERNAL] Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct
> device into ib
> 
> On Wed, Nov 27, 2024 at 07:46:39PM +0000, Long Li wrote:
> >
> > > > > I think Konstantin's suggestion makes sense, how about we do
> > > > > this (don't need to define netdev_is_slave(dev)):
> > > > >
> > > > > --- 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;
> > > > >
> > > > >
> > > > > is_eth_port_of_netdev_filter() should not return true if this
> > > > > netdev is a bonded slave. In this case, only use the address of its bonded
> master.
> > > > >
> > > > Right. This change makes sense to me.
> > > > I don't have a setup presently to verify it to ensure I didn't miss a corner
> case.
> > > > Leon,
> > > > Can you or others please test the regression once with the formal patch?
> > >
> > > Sure, once Long will send the patch, I'll make sure that it is tested.
> > >
> > > Thanks
> > >
> >
> > I posted patches for discussion.
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-rdma%2F1732736619-19941-1-git-send-email-longli%40
> >
> linuxonhyperv.com%2FT%2F%23t&data=05%7C02%7Clongli%40microsoft.com%7
> C4
> >
> 20bac91521e414ff34c08dd0f909cf6%7C72f988bf86f141af91ab2d7cd011db47%7
> C1
> > %7C0%7C638683835975667120%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU
> 1hcGkiOnRy
> >
> dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D
> %
> >
> 3D%7C0%7C%7C%7C&sdata=7vTTi%2FilkYdEKNG1qwpgYYDriOPPUF%2Bp8Zh91
> 60CEVE%
> > 3D&reserved=0
> 
> Please resend these patches as series with cover letter and don't embed extra
> patch (the one which is not numbered) into the series.
> 
> Thanks


I will resend those as a series after addressing the other comments on bonding.

Thanks

RE: [EXTERNAL] Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

[Long Li]

> On Wed, Nov 27, 2024 at 07:46:39PM +0000, Long Li wrote:
> >
> > > > > I think Konstantin's suggestion makes sense, how about we do
> > > > > this (don't need to define netdev_is_slave(dev)):
> > > > >
> > > > > --- 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;
> > > > >
> > > > >
> > > > > is_eth_port_of_netdev_filter() should not return true if this
> > > > > netdev is a bonded slave. In this case, only use the address of its bonded
> master.
> > > > >
> > > > Right. This change makes sense to me.
> > > > I don't have a setup presently to verify it to ensure I didn't miss a corner
> case.
> > > > Leon,
> > > > Can you or others please test the regression once with the formal patch?
> > >
> > > Sure, once Long will send the patch, I'll make sure that it is tested.
> > >
> > > Thanks
> > >
> >
> > I posted patches for discussion.
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-rdma%2F1732736619-19941-1-git-send-email-longli%40
> >
> linuxonhyperv.com%2FT%2F%23t&data=05%7C02%7Clongli%40microsoft.com%7
> C4
> >
> 20bac91521e414ff34c08dd0f909cf6%7C72f988bf86f141af91ab2d7cd011db47%7
> C1
> >
> %7C0%7C638683835975667120%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1h
> cGkiOnRy
> >
> dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D
> %
> >
> 3D%7C0%7C%7C%7C&sdata=7vTTi%2FilkYdEKNG1qwpgYYDriOPPUF%2Bp8Zh91
> 60CEVE%
> > 3D&reserved=0
> 
> Please resend these patches as series with cover letter and don't embed extra
> patch (the one which is not numbered) into the series.
> 
> Thanks

Sorry for the late relay. I have done some more testing and sent those patches in a series with a cover letter.

Please review the series.

Thanks,
Long