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

November 04th, 2011 - 03:20 pm ET by Nicholas A. Bellinger | Report spam

Hi Linus,

This is the PULL request for an initial merge of the ib_srpt driver
using mainline target infrastructure into v3.2-rc1. Please go ahead and
pull from:

git://git.kernel.org/pub/scm/linux/...ending.git for-next-merge

Note that 'target: Updates for v3.2-rc1 (round two)' should be pulled
first via target-pending.git/for-next, and has gone out in a separate
PULL request.

At this point should we should have a stable userspace API for the
layout of /sys/kernel/config/target/srpt/, and a number of legacy module
parameters have been removed or converted into per endpoint attributes
as per Roland's feedback. This code has been getting build testing in
next the past week, and does not touch any external code.

There is an known issue with active I/O shutdown is currently being
addressed using the new generic active I/O shutdown bits in for-next,
and will be sent out post merge after more testing and feedback. I also
see a few patches from DanC this morning to address a handful of non
critical issues that will be included post merge as well.

Here are full set of review changes from Roland and Bart that have been
made:

v1 -> v2 review changes:

ib_srpt: Fix potential out-of-bounds array access
ib_srpt: Avoid failed multipart RDMA transfers
ib_srpt: Fix srpt_alloc_fabric_acl failure case return value
ib_srpt: Update comments to reference $driver/$port layout
ib_srpt: Fix sport->port_guid formatting code
ib_srpt: Remove legacy use_port_guid_in_session_name module parameter
ib_srpt: Convert srp_max_rdma_size into per port configfs attribute
ib_srpt: Convert srp_max_rsp_size into per port configfs attribute
ib_srpt: Convert srpt_sq_size into per port configfs attribute

v2 -> v3 review changes:

ib_srpt: Fix possible race with srp_sq_size in srpt_create_ch_ib
ib_srpt: Fix possible race with srp_max_rsp_size in srpt_release_channel_work
ib_srpt: Fix up MAX_SRPT_RDMA_SIZE define
ib_srpt: Make srpt_map_sg_to_ib_sge() failure case return -EAGAIN
ib_srpt: Convert port_guid to use subnet_prefix + interface_id formatting
ib_srpt: Make srpt_check_stop_free return kref_put status

Thanks!

Bart Van Assche (1):
ib_srpt: Initial SRP Target merge for v3.2-rc1

drivers/infiniband/Kconfig | 1 +
drivers/infiniband/Makefile | 1 +
drivers/infiniband/ulp/srpt/Kconfig | 12 +
drivers/infiniband/ulp/srpt/Makefile | 2 +
drivers/infiniband/ulp/srpt/ib_dm_mad.h | 139 ++
drivers/infiniband/ulp/srpt/ib_srpt.c | 4081 +++++++++++++++++++++++++++++++
drivers/infiniband/ulp/srpt/ib_srpt.h | 444 ++++
7 files changed, 4680 insertions(+), 0 deletions(-)
create mode 100644 drivers/infiniband/ulp/srpt/Kconfig
create mode 100644 drivers/infiniband/ulp/srpt/Makefile
create mode 100644 drivers/infiniband/ulp/srpt/ib_dm_mad.h
create mode 100644 drivers/infiniband/ulp/srpt/ib_srpt.c
create mode 100644 drivers/infiniband/ulp/srpt/ib_srpt.h

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

Follow the discussion Replies

9 replies

Make a reply

Replies

#1 Roland Dreier

November 04th, 2011 - 03:30 pm ET | Report spam

On Fri, Nov 4, 2011 at 1:10 PM, Nicholas A. Bellinger
wrote:

At this point should we should have a stable userspace API for the
layout of /sys/kernel/config/target/srpt/, and a number of legacy module
parameters have been removed or converted into per endpoint attributes
as per Roland's feedback. This code has been getting build testing in
next the past week, and does not touch any external code.

Given the confidence that the userspace ABI is stable,

Acked-by: Roland Dreier

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

Reply to this message

#2 Bart Van Assche

November 05th, 2011 - 02:40 am ET | Report spam

On Fri, Nov 4, 2011 at 9:10 PM, Nicholas A. Bellinger
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.
- 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//msg09678.html).
- 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//msg09677.html).

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

Reply to this message

#3 Nicholas A. Bellinger

November 05th, 2011 - 01:10 pm ET | Report spam

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

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

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

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

Reply to this message

#4 Bart Van Assche

November 06th, 2011 - 02:40 am ET | Report spam

On Sat, Nov 5, 2011 at 7:04 PM, Nicholas A. Bellinger
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
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.

- 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//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 "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//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.
- Remove the max_rdma_size variable from configfs entirely and compute
the max_rdma_size from the HCA properties in struct ib_device_attr.

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

Reply to this message

#5 Nicholas A. Bellinger

November 06th, 2011 - 05:10 am ET | Report spam

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
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
>> 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//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//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,

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

Reply to this message

Create a new topicNext page

Make a reply

Make your own search

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

Similar topics

Replies