| Message ID | 20200930094013.156839-1-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-rc,v2] RDMA/ipoib: Set rtnl_link_ops for ipoib interfaces | expand |
On Wed, Sep 30, 2020 at 12:40:13PM +0300, Kamal Heib wrote: > To avoid inconsistent user experience for PKey interfaces that are > created via netlink vs PKey interfaces that are created via sysfs and the > base interface, make sure to set the rtnl_link_ops for all ipoib network > devices, so the ipoib attributes will be reported/modified via iproute2 > for all ipoib interfaces regardless of how they are created. > > Also, after setting the rtnl_link_ops for the base interface, implement > the dellink() callback to block users from trying to remove it. > > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support") > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > v2: Update commit message. > drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++ > drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 11 +++++++++++ > drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 2 ++ > 3 files changed, 15 insertions(+) > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > index ab75b7f745d4..96b6be5d507d 100644 > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c > @@ -2477,6 +2477,8 @@ static struct net_device *ipoib_add_port(const char *format, > /* call event handler to ensure pkey in sync */ > queue_work(ipoib_workqueue, &priv->flush_heavy); > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > + > result = register_netdev(ndev); Why do we need this one? I understand fixing the sysfs but not this part. > if (result) { > pr_warn("%s: couldn't register ipoib port %d; error %d\n", > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c > index 38c984d16996..d5a90a66b45c 100644 > +++ b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c > @@ -144,6 +144,16 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev, > return 0; > } > > +static void ipoib_del_child_link(struct net_device *dev, struct list_head *head) > +{ > + struct ipoib_dev_priv *priv = ipoib_priv(dev); > + > + if (!priv->parent) > + return; > + > + unregister_netdevice_queue(dev, head); > +} The above is why this hunk was added right? > static size_t ipoib_get_size(const struct net_device *dev) > { > return nla_total_size(2) + /* IFLA_IPOIB_PKEY */ > @@ -158,6 +168,7 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = { > .priv_size = sizeof(struct ipoib_dev_priv), > .setup = ipoib_setup_common, > .newlink = ipoib_new_child_link, > + .dellink = ipoib_del_child_link, > .changelink = ipoib_changelink, > .get_size = ipoib_get_size, > .fill_info = ipoib_fill_info, > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > index 30865605e098..c60db9f3f5ac 100644 > +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > @@ -129,6 +129,8 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, > goto out_early; > } > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > + If this is only for the sysfs case why isn't it in ipoib_vlan_add() which is the sysfs only flow? Jason
On Fri, Oct 02, 2020 at 09:02:36AM -0300, Jason Gunthorpe wrote: > On Wed, Sep 30, 2020 at 12:40:13PM +0300, Kamal Heib wrote: > > To avoid inconsistent user experience for PKey interfaces that are > > created via netlink vs PKey interfaces that are created via sysfs and the > > base interface, make sure to set the rtnl_link_ops for all ipoib network > > devices, so the ipoib attributes will be reported/modified via iproute2 > > for all ipoib interfaces regardless of how they are created. > > > > Also, after setting the rtnl_link_ops for the base interface, implement > > the dellink() callback to block users from trying to remove it. > > > > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support") > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > v2: Update commit message. > > drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++ > > drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 11 +++++++++++ > > drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 2 ++ > > 3 files changed, 15 insertions(+) > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > > index ab75b7f745d4..96b6be5d507d 100644 > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c > > @@ -2477,6 +2477,8 @@ static struct net_device *ipoib_add_port(const char *format, > > /* call event handler to ensure pkey in sync */ > > queue_work(ipoib_workqueue, &priv->flush_heavy); > > > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > > + > > result = register_netdev(ndev); > > Why do we need this one? I understand fixing the sysfs but not this part. > We need this one to report/modify the ipoib attributes for the parent interface (please take a look at pkey, mode, and umcast after applying this patch): $ ip -d link show dev mlx5_ib0 29: mlx5_ib0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc mq state UP mode DEFAULT group default qlen 256 link/infiniband 00:00:1f:e4:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a3:19:64 brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 65520 ipoib pkey 0xffff mode datagram umcast 0000 addrgenmode none numtxqueues 256 numrxqueues 32 gso_max_size 65536 gso_max_segs 65535 > > if (result) { > > pr_warn("%s: couldn't register ipoib port %d; error %d\n", > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c > > index 38c984d16996..d5a90a66b45c 100644 > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c > > @@ -144,6 +144,16 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev, > > return 0; > > } > > > > +static void ipoib_del_child_link(struct net_device *dev, struct list_head *head) > > +{ > > + struct ipoib_dev_priv *priv = ipoib_priv(dev); > > + > > + if (!priv->parent) > > + return; > > + > > + unregister_netdevice_queue(dev, head); > > +} > > The above is why this hunk was added right? This hunk was added to block users from trying to remove the parent interface. > > > static size_t ipoib_get_size(const struct net_device *dev) > > { > > return nla_total_size(2) + /* IFLA_IPOIB_PKEY */ > > @@ -158,6 +168,7 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = { > > .priv_size = sizeof(struct ipoib_dev_priv), > > .setup = ipoib_setup_common, > > .newlink = ipoib_new_child_link, > > + .dellink = ipoib_del_child_link, > > .changelink = ipoib_changelink, > > .get_size = ipoib_get_size, > > .fill_info = ipoib_fill_info, > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > > index 30865605e098..c60db9f3f5ac 100644 > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > > @@ -129,6 +129,8 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, > > goto out_early; > > } > > > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > > + > > If this is only for the sysfs case why isn't it in ipoib_vlan_add() > which is the sysfs only flow? > Basically, I was making sure set the rtnl_link_ops before calling register_netdevice(), but you are right this can be moved to the ipoib_vlan_add() as the setting of the rtnl_link_ops for pkey that is created via netlink is done before calling newlink callback, I'll send a v3 that move it to ipoib_vlan_add(). Thanks, Kamal > Jason
On Sun, Oct 04, 2020 at 03:51:07PM +0300, Kamal Heib wrote: > On Fri, Oct 02, 2020 at 09:02:36AM -0300, Jason Gunthorpe wrote: > > On Wed, Sep 30, 2020 at 12:40:13PM +0300, Kamal Heib wrote: > > > To avoid inconsistent user experience for PKey interfaces that are > > > created via netlink vs PKey interfaces that are created via sysfs and the > > > base interface, make sure to set the rtnl_link_ops for all ipoib network > > > devices, so the ipoib attributes will be reported/modified via iproute2 > > > for all ipoib interfaces regardless of how they are created. > > > > > > Also, after setting the rtnl_link_ops for the base interface, implement > > > the dellink() callback to block users from trying to remove it. > > > > > > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support") > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > v2: Update commit message. > > > drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++ > > > drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 11 +++++++++++ > > > drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 2 ++ > > > 3 files changed, 15 insertions(+) > > > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > > > index ab75b7f745d4..96b6be5d507d 100644 > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c > > > @@ -2477,6 +2477,8 @@ static struct net_device *ipoib_add_port(const char *format, > > > /* call event handler to ensure pkey in sync */ > > > queue_work(ipoib_workqueue, &priv->flush_heavy); > > > > > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > > > + > > > result = register_netdev(ndev); > > > > Why do we need this one? I understand fixing the sysfs but not this part. > > We need this one to report/modify the ipoib attributes for the parent > interface (please take a look at pkey, mode, and umcast after applying > this patch): > > $ ip -d link show dev mlx5_ib0 > 29: mlx5_ib0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc mq state UP mode DEFAULT group default qlen 256 > link/infiniband 00:00:1f:e4:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a3:19:64 brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 65520 > ipoib pkey 0xffff mode datagram umcast 0000 addrgenmode none numtxqueues 256 numrxqueues 32 gso_max_size 65536 gso_max_segs 65535 So this is what you mean by "base interface" ? Hmm OK. Highlight this a bit more in the commit message since this is something everyone will see > > > static size_t ipoib_get_size(const struct net_device *dev) > > > { > > > return nla_total_size(2) + /* IFLA_IPOIB_PKEY */ > > > @@ -158,6 +168,7 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = { > > > .priv_size = sizeof(struct ipoib_dev_priv), > > > .setup = ipoib_setup_common, > > > .newlink = ipoib_new_child_link, > > > + .dellink = ipoib_del_child_link, > > > .changelink = ipoib_changelink, > > > .get_size = ipoib_get_size, > > > .fill_info = ipoib_fill_info, > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > > > index 30865605e098..c60db9f3f5ac 100644 > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > > > @@ -129,6 +129,8 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, > > > goto out_early; > > > } > > > > > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > > > + > > > > If this is only for the sysfs case why isn't it in ipoib_vlan_add() > > which is the sysfs only flow? > > > > Basically, I was making sure set the rtnl_link_ops before calling > register_netdevice(), but you are right this can be moved to the > ipoib_vlan_add() as the setting of the rtnl_link_ops for pkey that is > created via netlink is done before calling newlink callback, I'll send a > v3 that move it to ipoib_vlan_add(). Please Jason
On Sun, Oct 04, 2020 at 10:04:54AM -0300, Jason Gunthorpe wrote: > On Sun, Oct 04, 2020 at 03:51:07PM +0300, Kamal Heib wrote: > > On Fri, Oct 02, 2020 at 09:02:36AM -0300, Jason Gunthorpe wrote: > > > On Wed, Sep 30, 2020 at 12:40:13PM +0300, Kamal Heib wrote: > > > > To avoid inconsistent user experience for PKey interfaces that are > > > > created via netlink vs PKey interfaces that are created via sysfs and the > > > > base interface, make sure to set the rtnl_link_ops for all ipoib network > > > > devices, so the ipoib attributes will be reported/modified via iproute2 > > > > for all ipoib interfaces regardless of how they are created. > > > > > > > > Also, after setting the rtnl_link_ops for the base interface, implement > > > > the dellink() callback to block users from trying to remove it. > > > > > > > > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support") > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > > v2: Update commit message. > > > > drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++ > > > > drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 11 +++++++++++ > > > > drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 2 ++ > > > > 3 files changed, 15 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > > > > index ab75b7f745d4..96b6be5d507d 100644 > > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c > > > > @@ -2477,6 +2477,8 @@ static struct net_device *ipoib_add_port(const char *format, > > > > /* call event handler to ensure pkey in sync */ > > > > queue_work(ipoib_workqueue, &priv->flush_heavy); > > > > > > > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > > > > + > > > > result = register_netdev(ndev); > > > > > > Why do we need this one? I understand fixing the sysfs but not this part. > > > > We need this one to report/modify the ipoib attributes for the parent > > interface (please take a look at pkey, mode, and umcast after applying > > this patch): > > > > $ ip -d link show dev mlx5_ib0 > > 29: mlx5_ib0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc mq state UP mode DEFAULT group default qlen 256 > > link/infiniband 00:00:1f:e4:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a3:19:64 brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 65520 > > ipoib pkey 0xffff mode datagram umcast 0000 addrgenmode none numtxqueues 256 numrxqueues 32 gso_max_size 65536 gso_max_segs 65535 > > So this is what you mean by "base interface" ? Hmm OK. Highlight this > a bit more in the commit message since this is something everyone > will see > Yes, correct. I'll update the commit message. Thanks, Kamal > > > > static size_t ipoib_get_size(const struct net_device *dev) > > > > { > > > > return nla_total_size(2) + /* IFLA_IPOIB_PKEY */ > > > > @@ -158,6 +168,7 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = { > > > > .priv_size = sizeof(struct ipoib_dev_priv), > > > > .setup = ipoib_setup_common, > > > > .newlink = ipoib_new_child_link, > > > > + .dellink = ipoib_del_child_link, > > > > .changelink = ipoib_changelink, > > > > .get_size = ipoib_get_size, > > > > .fill_info = ipoib_fill_info, > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > > > > index 30865605e098..c60db9f3f5ac 100644 > > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c > > > > @@ -129,6 +129,8 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, > > > > goto out_early; > > > > } > > > > > > > > + ndev->rtnl_link_ops = ipoib_get_link_ops(); > > > > + > > > > > > If this is only for the sysfs case why isn't it in ipoib_vlan_add() > > > which is the sysfs only flow? > > > > > > > Basically, I was making sure set the rtnl_link_ops before calling > > register_netdevice(), but you are right this can be moved to the > > ipoib_vlan_add() as the setting of the rtnl_link_ops for pkey that is > > created via netlink is done before calling newlink callback, I'll send a > > v3 that move it to ipoib_vlan_add(). > > Please > > Jason
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index ab75b7f745d4..96b6be5d507d 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -2477,6 +2477,8 @@ static struct net_device *ipoib_add_port(const char *format, /* call event handler to ensure pkey in sync */ queue_work(ipoib_workqueue, &priv->flush_heavy); + ndev->rtnl_link_ops = ipoib_get_link_ops(); + result = register_netdev(ndev); if (result) { pr_warn("%s: couldn't register ipoib port %d; error %d\n", diff --git a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c index 38c984d16996..d5a90a66b45c 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c @@ -144,6 +144,16 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev, return 0; } +static void ipoib_del_child_link(struct net_device *dev, struct list_head *head) +{ + struct ipoib_dev_priv *priv = ipoib_priv(dev); + + if (!priv->parent) + return; + + unregister_netdevice_queue(dev, head); +} + static size_t ipoib_get_size(const struct net_device *dev) { return nla_total_size(2) + /* IFLA_IPOIB_PKEY */ @@ -158,6 +168,7 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = { .priv_size = sizeof(struct ipoib_dev_priv), .setup = ipoib_setup_common, .newlink = ipoib_new_child_link, + .dellink = ipoib_del_child_link, .changelink = ipoib_changelink, .get_size = ipoib_get_size, .fill_info = ipoib_fill_info, diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c index 30865605e098..c60db9f3f5ac 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c @@ -129,6 +129,8 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, goto out_early; } + ndev->rtnl_link_ops = ipoib_get_link_ops(); + result = register_netdevice(ndev); if (result) { ipoib_warn(priv, "failed to initialize; error %i", result);
To avoid inconsistent user experience for PKey interfaces that are created via netlink vs PKey interfaces that are created via sysfs and the base interface, make sure to set the rtnl_link_ops for all ipoib network devices, so the ipoib attributes will be reported/modified via iproute2 for all ipoib interfaces regardless of how they are created. Also, after setting the rtnl_link_ops for the base interface, implement the dellink() callback to block users from trying to remove it. Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support") Signed-off-by: Kamal Heib <kamalheib1@gmail.com> --- v2: Update commit message. --- drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++ drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 11 +++++++++++ drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 2 ++ 3 files changed, 15 insertions(+)