Re: [PATCH v5 2/2] rdma_rxe: use netlink messages to add/delete links

[Yanjun Zhu <yanjun.zhu@xxxxxxxxxx>]

On 2018/12/4 0:26, Steve Wise wrote:
> On 12/3/2018 12:34 AM, Yanjun Zhu wrote:
> > On 2018/12/1 5:58, Steve Wise wrote:
> > > Add support for the RDMA_NLDEV_CMD_NEWLINK/DELLINK messages which allow
> > > dynamically adding new RXE links.  Deprecate the old module options
> > > for now.
> > >
> > > Cc: Moni Shoua <monis@xxxxxxxxxxxx>
> > > Signed-off-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/infiniband/sw/rxe/rxe.c       | 62
> > > +++++++++++++++++++++++++++++++++--
> > >    drivers/infiniband/sw/rxe/rxe.h       |  2 +-
> > >    drivers/infiniband/sw/rxe/rxe_net.c   |  4 +--
> > >    drivers/infiniband/sw/rxe/rxe_net.h   |  2 +-
> > >    drivers/infiniband/sw/rxe/rxe_sysfs.c |  6 ++--
> > >    drivers/infiniband/sw/rxe/rxe_verbs.c |  4 +--
> > >    drivers/infiniband/sw/rxe/rxe_verbs.h |  2 +-
> > >    7 files changed, 70 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe.c
> > > b/drivers/infiniband/sw/rxe/rxe.c
> > > index 383e65c7bbc0..c09534b513ff 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe.c
> > > @@ -31,6 +31,7 @@
> > >     * SOFTWARE.
> > >     */
> > >    +#include <rdma/rdma_netlink.h>
> > >    #include <net/addrconf.h>
> > >    #include "rxe.h"
> > >    #include "rxe_loc.h"
> > > @@ -308,7 +309,7 @@ void rxe_set_mtu(struct rxe_dev *rxe, unsigned
> > > int ndev_mtu)
> > >    /* called by ifc layer to create new rxe device.
> > >     * The caller should allocate memory for rxe by calling
> > > ib_alloc_device.
> > >     */
> > > -int rxe_add(struct rxe_dev *rxe, unsigned int mtu)
> > > +int rxe_add(struct rxe_dev *rxe, unsigned int mtu, const char
> > > *ibdev_name)
> > >    {
> > >        int err;
> > >    @@ -320,7 +321,7 @@ int rxe_add(struct rxe_dev *rxe, unsigned int mtu)
> > >          rxe_set_mtu(rxe, mtu);
> > >    -    err = rxe_register_device(rxe);
> > > +    err = rxe_register_device(rxe, ibdev_name);
> > >        if (err)
> > >            goto err1;
> > >    @@ -339,6 +340,59 @@ void rxe_remove(struct rxe_dev *rxe)
> > >        rxe_dev_put(rxe);
> > >    }
> > >    +static struct ib_device *rxe_newlink(const char *ibdev_name,
> > > +                     const char *ndev_name)
> > > +{
> > > +    struct net_device *ndev = NULL;
> > > +    struct rxe_dev *rxe;
> > > +    int err = 0;
> > > +
> > > +    ndev = dev_get_by_name(&init_net, ndev_name);
> > > +    if (!ndev) {
> > > +        pr_err("interface %s not found\n", ndev_name);
> > > +        err = -ENODEV;
> > > +        goto err;
> > > +    }
> > > +
> > > +    if (net_to_rxe(ndev)) {
> > > +        pr_err("already configured on %s\n", ndev_name);
> > > +        err = -EEXIST;
> > > +        goto err;
> > > +    }
> > > +
> > > +    rxe = rxe_net_add(ibdev_name, ndev);
> > > +    if (!rxe) {
> > > +        pr_err("failed to add %s\n", ndev_name);
> > > +        err = -EINVAL;
> > > +        goto err;
> > > +    }
> > > +
> > > +    if (netif_running(ndev) && netif_carrier_ok(ndev))
> > > +        rxe_port_up(rxe);
> > > +    else
> > > +        rxe_port_down(rxe);
> > > +    pr_info("added %s to %s\n", rxe->ib_dev.name, ndev->name);
> > > +err:
> > > +    if (ndev)
> > > +        dev_put(ndev);
> > > +    return err ? ERR_PTR(err) : &rxe->ib_dev;
> > > +}
> > > +
> > > +static int rxe_dellink(struct ib_device *device)
> > > +{
> > > +    struct rxe_dev *rxe = to_rdev(device);
> > > +
> > diff --git a/drivers/infiniband/sw/rxe/rxe.c
> > b/drivers/infiniband/sw/rxe/rxe.c
> > index c09534b513ff..e19d575d276f 100644
> > --- a/drivers/infiniband/sw/rxe/rxe.c
> > +++ b/drivers/infiniband/sw/rxe/rxe.c
> > @@ -382,7 +382,7 @@ static int rxe_dellink(struct ib_device *device)
> >   {
> >          struct rxe_dev *rxe = to_rdev(device);
> >
> > -       list_del(&rxe->list);
> > +       rxe_net_remove(rxe);
> >          rxe_remove(rxe);
> >          return 0;
> >   }
> > diff --git a/drivers/infiniband/sw/rxe/rxe_net.c
> > b/drivers/infiniband/sw/rxe/rxe_net.c
> > index 10a38b76f24b..1b57710008d3 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_net.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> > @@ -573,6 +573,13 @@ struct rxe_dev *rxe_net_add(const char
> > *ibdev_name, struct net_device *ndev)
> >          return rxe;
> >   }
> >
> > +void rxe_net_remove(struct rxe_dev *rxe)
> > +{
> > +       spin_lock_bh(&dev_list_lock);
> > +       list_del(&rxe->list);
> > +       spin_unlock_bh(&dev_list_lock);
> > +}
> > +
> >   void rxe_remove_all(void)
> >   {
> >          spin_lock_bh(&dev_list_lock);
> > diff --git a/drivers/infiniband/sw/rxe/rxe_net.h
> > b/drivers/infiniband/sw/rxe/rxe_net.h
> > index f8e00e6d5d38..45bf15fab87f 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_net.h
> > +++ b/drivers/infiniband/sw/rxe/rxe_net.h
> > @@ -44,6 +44,7 @@ struct rxe_recv_sockets {
> >   };
> >
> >   struct rxe_dev *rxe_net_add(const char *ibdev_name, struct net_device
> > *ndev);
> > +void rxe_net_remove(struct rxe_dev *rxe);
> >
> >   int rxe_net_init(void);
> >   void rxe_net_exit(void);
> >
> > > +    list_del(&rxe->list);
> > To avoid race, list_del(&rxe->list); should be replace the above.
> >
> > If I am wrong, please correct me.
> >
> > Zhu Yanjun
> Yes, you're correct.  This code is not serialized at all for device
> removal.  As Jason requested, I'll address this all in the next version
> and hopefully get it right. :)
>
> Thanks for the review.  May I add a reviewed-by tag from you?

Yes. You can:-).

Zhu Yanjun

> Steve.