On Wed, Sep 26, 2018 at 07:57:26PM +0300, Leon Romanovsky wrote:
> 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.
I showed you, it is using a string for NEWLINK, which is what we are
copying.
Jason
