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

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4929 bytes --]

On Tue, 2018-01-30 at 18:19 +0000, Bart Van Assche wrote:
> 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.

Sure, that makes sense.  I was just saying that if the intent is to
truly support this on anything other than an IB-like network, such as
actual iWARP or RoCE, then it's worth thinking about the long term
things that need done.

> Personally I don't think we have to postpone the ib_srpt RDMA/CM patch until
> IANA has assigned a port number.

No, we don't.

> 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.

That would work as well.

> 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.

Since you envision this mostly as a testing tool, we can worry about
things like this later if it is decided to make it a "supported" item. 
It would be worth noting the testing status in some way that makes it
clear to users, maybe in docs or somewhere else.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]