linux-kernel - Re: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1320574170.5859.370.camel@haakon2.linux-iscsi.org>
Date:	Sun, 06 Nov 2011 02:09:30 -0800
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	Bart Van Assche <bvanassche@....org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	target-devel <target-devel@...r.kernel.org>,
	linux-rdma <linux-rdma@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Roland Dreier <roland@...estorage.com>,
	Christoph Hellwig <hch@....de>
Subject: Re: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

On Sun, 2011-11-06 at 08:34 +0100, Bart Van Assche wrote:
> On Sat, Nov 5, 2011 at 7:04 PM, Nicholas A. Bellinger
> <nab@...ux-iscsi.org> wrote:
> > On Sat, 2011-11-05 at 08:37 +0100, Bart Van Assche wrote:
> >> On Fri, Nov 4, 2011 at 9:10 PM, Nicholas A. Bellinger
> >> <nab@...ux-iscsi.org> wrote:
> >> > This is the PULL request for an initial merge of the ib_srpt driver
> >> > using mainline target infrastructure into v3.2-rc1.
> >>
> >> In case anyone is interested, the most important unaddressed comments
> >> for this version of ib_srpt are:
> >> - There are still too many module parameters. This makes ib_srpt
> >> harder to use than necessary because several of these parameters can
> >> only be set at module load time.
> >
> > Can you be more specific here..?  I went through the module parameters
> > and made the ones that where used in a per-port specific context into
> > configfs attributes as Roland recommended, but it seems like you are
> > saying that more should be made into attributes.  Which ones..?
> 
> Several read-only kernel module parameters remain. That's fine in
> out-of-tree code since it will be compiled as a kernel module only.
> But anyone who tries to build with CONFIG_INFINIBAND_SRPT=y will
> notice that it's impossible to change the value of these read-only
> kernel module parameters.
> 

So after the cleanups, srp_max_req_size, srpt_srq_size and
srpt_service_guid appear as the remaining module parameters.  The first
two are required at srpt_add_one() -> srpt_device allocation time
(usually at initial load time), and should remain as global scope
parameters correct..?  To address the point with
CONFIG_INFINIBAND_SRPT=y, how about assigning these two values for each
srpt_device during srpt_add_one(), and reference them directly in
srpt_device so their respective module parameter can become S_IRUGO|
S_IWUSR...?

We have also discussed srpt_service_guid a bit before, which you
indicated needed to stay in current code as global scope, and presumably
should not change value after loading.  Looking at the actual usage, one
post merge improvement we can consider is seeing if it's possible to
move ib_cm_listen() out of srpt_add_one() and have it driven instead by
configfs context in order to optionally set srpt_service_guid on a per
target endpoint basis to get us some more flexibility. 

> >> - The last WQE event can arrive before the queue pair is reset,
> >> resulting in a hanging session and blocking future logins
> >> (http://www.mail-archive.com/linux-rdma@vger.kernel.org/msg09678.html).
> >
> > Yep, was going to get this resolved post merge along with my patch now
> > in lio-core.git to address active I/O shutdown with ib_srpt.
> 
> Your "active I/O shutdown" patch does not address the "last WQE" issue.

The current patch in lio-core allows ib_srpt to push active I/O and
perform shutdown + unload without hanging (or crashing) at this point.
Active I/O tests with physical link layer failures also seems to work,
but mixing to two together still obviously needs more testing.

For the "last WQE" you've mentioned, I'm happy to accept a patch to
address this as you know the code better than me.  Otherwise, I'll be
digging this out of your out-of-tree code as a seperate item and
figuring out how to reproduce and test this special case.

> 
> >> - The "ib_srpt: Convert srp_max_rdma_size into per port configfs
> >> attribute" contradicts the I/O controller concept. This is a bug.
> >> (http://www.mail-archive.com/linux-rdma@vger.kernel.org/msg09677.html).
> >
> > As mentioned in the thread, I deferred to Roland's input on that one.
> > So you're saying this should be made back into a module parameter now,
> > or what..?
> 
> I see two possible ways forward:
> - Convert the per-port max_rdma_size configfs variable into a
> per-module configfs variable.

We don't need to add any per-module configfs target variables, we
already have /sys/module/$NAME/parameters/ for that.  ;)

> - Remove the max_rdma_size variable from configfs entirely and compute
> the max_rdma_size from the HCA properties in struct ib_device_attr.
> 

What did you have in mind..?  This should be based on ->max_sge *
PAGE_SIZE, or something else from ib_device_attr..?  

Thanks,

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/