Re: [PATCH rdma-core 1/5] Common infrastructure for auto loading rdma modules

[Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org>]

On Mon, 2017-07-24 at 14:44 -0600, Jason Gunthorpe wrote:
> +This is to avoid exposing systems not using RDMA from having RDMA enabled, for
> +instance if a system has a multi-protocol ethernet adaptor, but is only using
> +the net stack interface.

adaptor -> adapter ?

> +Finally udev will cause systemd to start RDMA specific daemons like
> +srp_deamon, rdma-ndd and iwpmd. These starts are linked to the detection of
> +the first RDMA hardware, and the daemons internally handle hot plug events for
> +other hardware.

Please change srp_deamon into srp_daemon such that the spelling matches the name
of the executable.

> +
> +## Hot Plug compatible services
> +
> +RDMA using services need to have device specific systemd dependencies in their
> +unit files, either created by hand by the admin or by using udev rules.

"RDMA using services" -> "Services using RDMA" ?

> +++ b/kernel-boot/modules/infiniband.conf
> @@ -0,0 +1,12 @@
> +# These modules are loaded by the system if any InfiniBand device is installed
> +# Infiniband over IP netdevice

Please spell "InfiniBand" consistently in the above comment.

> +ib_ipoib
> +
> +# Access to fabric management SMPs and GMPs from userspace.
> +ib_umad
> +
> +# SCSI Remote Protocol target support
> +# ib_srpt
> +
> +# ib_ucm provides the obsolete /dev/infiniband/ucm0
> +# ib_ucm

If ib_iser is loaded by default, should ib_srp also be loaded by default if the
appropriate hardware is present? I don't think that there are fewer SRP users
than iSER users.

> diff --git a/kernel-boot/rdma-description.rules b/kernel-boot/rdma-description.rules
> [ ... ]
> +# Hardware that supports RoCE
> +DRIVERS=="be2net", ENV{ID_RDMA_ROCE}="1"
> +DRIVERS=="bnxt_en", ENV{ID_RDMA_ROCE}="1"
> +DRIVERS=="hns", ENV{ID_RDMA_ROCE}="1"
> +DRIVERS=="i40e", ENV{ID_RDMA_ROCE}="1"
> +DRIVERS=="mlx4_core", ENV{ID_RDMA_ROCE}="1"
> +DRIVERS=="mlx5_core", ENV{ID_RDMA_ROCE}="1"
> +DRIVERS=="qede", ENV{ID_RDMA_ROCE}="1"

Should the "rdma_rxe" driver be added to this list?

> +ENV{ID_NET_DRIVER}=="mlx4_en", RUN{builtin}+="kmod load mlx4_ib"
> +ENV{ID_NET_DRIVER}=="mlx5_core", RUN{builtin}+="kmod load mlx5_ib"

Why this inconsistency between mlx4 and mlx5? Additionally, if these rules are
added, shouldn't the request_module() calls be removed from the mlx4 and ml5 core
drivers?

Anyway, nice work!

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html