On Wed, Sep 26, 2018 at 10:51:46AM -0600, Jason Gunthorpe wrote:
> On Wed, Sep 26, 2018 at 10:44:56AM -0500, Steve Wise wrote:
> >
> >
> > On 9/26/2018 3:05 AM, Leon Romanovsky wrote:
> > > On Tue, Sep 25, 2018 at 02:11:18PM -0600, Jason Gunthorpe wrote:
> > >> On Tue, Sep 25, 2018 at 08:55:29PM +0300, Leon Romanovsky wrote:
> > >>> On Thu, Sep 13, 2018 at 12:16:20PM -0700, Steve Wise wrote:
> > >>>> Add support for new LINK messages to allow adding and deleting rdma
> > >>>> interfaces. This will be used initially for soft rdma drivers which
> > >>>> instantiate device instances dynamically by the admin specifying a netdev
> > >>>> device to use. The rdma_rxe module will be the first user of these
> > >>>> messages.
> > >>>>
> > >>>> The design is modeled after RTNL_NEWLINK/DELLINK: rdma drivers
> > >>>> register with the rdma core if they provide link add/delete functions.
> > >>>> Each driver registers with a unique "type" string, that is used to
> > >>>> dispatch messages coming from user space. A new RDMA_NLDEV_ATTR is
> > >>>> defined for the "type" string. User mode will pass 3 attributes in a
> > >>>> NEWLINK message: RDMA_NLDEV_ATTR_IBDEV_NAME for the desired rdma device
> > >>>> name to be created, RDMA_NLDEV_ATTR_LINK_TYPE for the "type" of link
> > >>>> being added, and RDMA_NLDEV_ATTR_NDEV_NAME for the net_device interface
> > >>>> to use for this link. The DELLINK message will contain the IBDEV_NAME
> > >>>> and LINK_TYPE attributes.
> > >>>>
> > >>>> Signed-off-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
> > >>>
> > >>> <...>
> > >>>>
> > >>>> + RDMA_NLDEV_CMD_NEWLINK,
> > >>>> +
> > >>>> + RDMA_NLDEV_CMD_DELLINK,
> > >>>> +
> > >>>> RDMA_NLDEV_NUM_OPS
> > >>>> };
> > >>>>
> > >>>> @@ -427,6 +431,11 @@ enum rdma_nldev_attr {
> > >>>> RDMA_NLDEV_ATTR_DRIVER_U64, /* u64 */
> > >>>>
> > >>>> /*
> > >>>> + * Identifies the rdma driver. eg: "rxe" or "siw"
> > >>>> + */
> > >>>> + RDMA_NLDEV_ATTR_LINK_TYPE, /* string */
> > >>>
> > >>> I'll review it more deeply tomorrow, but two things caught my attention:
> > >>> 1. You need RDMA_NL_ADMIN_PERM for your netlink commands
> > >>> 2. LINK_TYPE should be index and not string, because it is why we are
> > >>> using using netlink :)
> > >>
> > >> netdev uses strings. See IFLA_INFO_KIND
> > >>
> > >> strings are not bad in netlink
> > >
> > > It is not the best example, because IFLA_INFO_KIND is used to forward
> > > filtering information and one day we will do the same with various
> > > rdmatool dumping options. However in case of ibdevice type, it is
> > > declared as number and doesn't seem to me right to convert it to string
> > > and limit our KABI to name.
> > >
> >
> > Are you suggesting rdmatool uses enum rdma_driver_id from
> > rdma/rdma_user_ioctl_cmds.h? So I add code in rdmatool to map the
> > user's string, eg "mlx5" or "rxe" into the enum and send that down?
> >
> > That works for me. If rdma_driver_id is part of the KABI, then I guess
> > its fine for rdmatool to use it and map names to enum values...
>
> I think this is a bad idea, we don't want to tie the kernel to iproute
> so tightly, flowing the string from the command line is the right way
> to handle this, just like netdev does.
Sorry, but netdev uses strings for filtering and not for types.
>
> Also, remember, the DRIVER_ID specifies the driver *NOT* the link
> type. It would be perfectly resonable for a driver like RXE to have
> multiple link types using the same user space driver. I can't imagine
> a use case for this, but is wrong to mix these two different concepts.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
