Re: [PATCH v2 00/14] IB/srpt: Add RDMA/CM support

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

On Tue, 2018-01-30 at 12:43 -0500, Doug Ledford wrote:
> On Tue, 2018-01-30 at 00:23 +0000, Bart Van Assche wrote:
> > On Wed, 2018-01-17 at 18:14 -0500, Doug Ledford wrote:
> > > On Tue, 2018-01-16 at 16:14 -0800, Bart Van Assche wrote:
> > > > Hello Jason and Doug,
> > > > 
> > > > This patch series not only adds RDMA/CM support to the SRP target driver but
> > > > also fixes a number of race conditions in that driver.
> > > > 
> > > > The RDMA/CM listener port number has to be specified as an ib_srpt kernel
> > > > module parameter. The default value for that parameter is zero which means
> > > > that RDMA/CM support is disabled.
> > > 
> > > Since srpt is already configured via the lIO framework, wouldn't that be
> > > a better place for the listen port?  In fact, shouldn't it be part of a
> > > portal like you have for iSERt?
> > 
> > Wouldn't that be overkill to have one listen port per RDMA port? I think
> > it will be easier for users if they have to configure the RDMA/CM port once
> > instead of one time per RDMA port. How about using the following location in
> > configfs for the RDMA/CM port:
> > 
> > /sys/kernel/config/target/srpt/discovery_auth/rdma_cm_port
> 
> Hmmm...maybe the real answer here is to start considering how serious we
> are about the RDMA_CM support in SRP.  If we're serious, should someone
> contact IANNA about a reserving port number and just use whatever they
> give us?  If we want to do the simple thing, a module option is fine,
> while we decide on this issue, then I can see that being OK.  I'm more
> OK with using configfs if there's a chance we won't get a well known
> reserved port and the config option will stick around long term.  And I
> tend to agree, per interface port configuration is probably not that
> interesting.  But the ability to specify something other than the
> wildcard IP address to listen on, and the ability to specify more than
> one IP address to listen on, are.

Hello Doug,

My motivation for posting the RDMA/CM patches on the linux-rdma mailing list
for the SRP initiator and target drivers was to allow others to run the
srp-test software on a setup that does not have any RDMA adapters, namely by
using the SoftRoCE driver. Since IANA has not yet assigned a port number to
SRP over RoCE I choose to make the port number configurable.

Personally I don't think we have to postpone the ib_srpt RDMA/CM patch until
IANA has assigned a port number. Once IANA has assigned a port number we
could e.g. modify the ib_srpt driver to enable RDMA/CM support by default
with the assigned port number (the patch I posted leaves RDMA/CM support
disabled by default).

Regarding configuring on which IP address to listen, one possibile approach
is to move the rdma_cm_port port number configfs attribute from
/sys/kernel/config/target/srpt/discovery_auth/rdma_cm_port to
/sys/kernel/config/target/srpt/$port/$port/attrib/rdma_cm_port where $port
is an RDMA port GUID or GID (both are accepted). That would allow to
enable/disable RDMA/CM support per RDMA adapter port. The ib_srpt driver
could e.g. create one listening rdma_cm_id per IP address that is associated
with an RDMA port.

One approach I have considered for allowing to configure per IP address
whether or not to accept incoming connections is to accept IP addresses as
RDMA port identifier ($port). However, that would complicate the ib_srpt
driver because there is code in that driver that assumes that there is a 1:1
relationship between RDMA ports and struct srpt_port instances. As an
example, the port[] array in struct srpt_device is an array with two elements.
Code like srpt_event_handler() and srpt_cm_req_recv() uses that array to
translate a port number into a struct srpt_port pointer.

Yet another approach would be to introduce a new directory hierarchy in
configfs for configuring IP access control. However, I don't think that would
be possible without modifying the LIO target core. And as you probably know
since several months the LIO target core maintainer is mostly interested in
bug fixes and not in new features or any other kind of patch that changes the
target core significantly.

Thanks,

Bart.