ib_srpt status

DISCUSSION Christoph Hellwig 7 participants 15 messages Oct 12, 2011 – Oct 20, 2011 4 messages not in the archive

Subsystems (3)

SCSI InfiniBand SCSI RDMA protocol (SRP) target

lore

Share & tools

lore

Christoph Hellwig

author

2011-10-12 09:23 UTC #0 lore raw

Hi Roland, hi Nick,

what is the status of queueing up ib_srpt for mainline? It's a very clean looking driver that was submitted 8 month ago and after an initial round of fixes only saw updates for changing core APIs.

Keeping it out of tree for so long is a very discuraging story for contributors.

Christoph Hellwig

author

2011-10-12 09:24 UTC #1 lore raw

[addressing to a working address for Roland]

Quoted 8 lines · Christoph Hellwig

Hi Roland, hi Nick,
what is the status of queueing up ib_srpt for mainline? It's a very clean looking driver that was submitted 8 month ago and after an initial round of fixes only saw updates for changing core APIs.
Keeping it out of tree for so long is a very discuraging story for contributors.

---end quoted text---

Roland Dreier

2011-10-12 16:17 UTC #2 lore raw

In reply to Christoph Hellwig

Quoted 8 lines · Christoph Hellwig

Hi Roland, hi Nick,
what is the status of queueing up ib_srpt for mainline? It's a very clean looking driver that was submitted 8 month ago and after an initial round of fixes only saw updates for changing core APIs.
Keeping it out of tree for so long is a very discuraging story for contributors.

Where is the latest patch? I would like to review it and at least get it into linux-next...

I seem to recall having some concerns about the interface -- ie some things were done through module params and I don't want to be stuck supporting those module params forever.

Other than that it is likely to be fine to merge.

- R.

Nicholas A. Bellinger

2011-10-13 22:07 UTC #3 lore raw

Quoted 10 lines · Roland Dreier

Quoted 8 lines · Christoph Hellwig
Hi Roland, hi Nick,
what is the status of queueing up ib_srpt for mainline? It's a very clean looking driver that was submitted 8 month ago and after an initial round of fixes only saw updates for changing core APIs.
Keeping it out of tree for so long is a very discuraging story for contributors.

Quoted 9 lines

Where is the latest patch? I would like to review it and at least get it into linux-next...
I seem to recall having some concerns about the interface -- ie some things were done through module params and I don't want to be stuck supporting those module params forever.
Other than that it is likely to be fine to merge.

Hey guys,

I'll post the latest version of ib_srpt in a bit after merging Christoph's remaining patches.. However, this will be against the v3.2 queued patches in target-pending.git/for-next, as the core cleanups in the se_cmd release path also effect current ib_srpt code.

--nab

Bart Van Assche

maintainer

2011-10-18 06:13 UTC #4 lore raw

In reply to Christoph Hellwig

Quoted 3 lines · Christoph Hellwig

what is the status of queueing up ib_srpt for mainline? It's a very clean looking driver that was submitted 8 month ago and after an initial round of fixes only saw updates for changing core APIs.

The ib_srpt driver was working fine last time I tested it. But with the current (3.1-rc9) LIO core performing I/O triggers grave memory corruption and random kernel crashes. Something must have changed in the LIO core that broke ib_srpt.

Bart.

Nicholas A. Bellinger

2011-10-18 16:41 UTC #5 lore raw

Quoted 10 lines · Bart Van Assche

Quoted 3 lines · Christoph Hellwig
what is the status of queueing up ib_srpt for mainline? It's a very clean looking driver that was submitted 8 month ago and after an initial round of fixes only saw updates for changing core APIs.
The ib_srpt driver was working fine last time I tested it. But with the current (3.1-rc9) LIO core performing I/O triggers grave memory corruption and random kernel crashes. Something must have changed in the LIO core that broke ib_srpt.

Hi Bart,

I'm having no problems with the patch posted for v3.2 review on MT26428 HCAs

Can you verify you are actually using this patch, and not your private tree from github..? The reason is that your tree did not contain the bugfix below to ib_srpt code that was originally causing problems:

ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252

Thanks,

--nab

Christoph Hellwig

author

2011-10-18 16:43 UTC #6 lore raw

Quoted 9 lines · Nicholas A. Bellinger

I'm having no problems with the patch posted for v3.2 review on MT26428 HCAs
Can you verify you are actually using this patch, and not your private tree from github..? The reason is that your tree did not contain the bugfix below to ib_srpt code that was originally causing problems:
ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252

Also it would be useful to know the workload that causes the corruption.

Nicholas A. Bellinger

2011-10-18 16:48 UTC #7 lore raw

Quoted 8 lines · Christoph Hellwig

Quoted 7 lines · Nicholas A. Bellinger
I'm having no problems with the patch posted for v3.2 review on MT26428 HCAs
Can you verify you are actually using this patch, and not your private tree from github..? The reason is that your tree did not contain the bugfix below to ib_srpt code that was originally causing problems:

Quoted 6 lines

Quoted 2 lines
ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252
Also it would be useful to know the workload that causes the corruption.
From the above commit:

This patch fixes a bug in srpt_map_sg_to_ib_sge() that assumed a single contigious array of struct scatterlist. This was causing a OOPs when se_cmd->t_task->t_tasks_no > 1 was using a set of sg_chained struct se_task->task_sg[] memory into srpt_map_sg_to_ib_sge() code.

Bart Van Assche

maintainer

2011-10-19 16:59 UTC #8 lore raw

In reply to Nicholas A. Bellinger

Quoted 9 lines · Nicholas A. Bellinger

I'm having no problems with the patch posted for v3.2 review on MT26428 HCAs
Can you verify you are actually using this patch, and not your private tree from github..? The reason is that your tree did not contain the bugfix below to ib_srpt code that was originally causing problems:
ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252

Yes, that patch was included - sorry, but I had made another mistake. 3.1-rc10 + ib_srpt + patch below is now working fine here. I noticed only one unusual issue so far (with kernel debugging enabled) and that is that the watchdog thread CPU time was unusually high:

root         7  2.3  0.0      0     0 ?        S    18:39   0:22 [watchdog/0]
root        12  0.6  0.0      0     0 ?        S    18:39   0:06 [watchdog/1]

The patch I used to backport ib_srpt from LIO-4.1 to kernel 3.1-rc10 is:

(quoted diff) +0 -0

b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 109b412..b3947fe 100644

drivers/infiniband/ulp/srpt/ib_srpt.c +0 -0

@@ -1340,7 +1340,7 @@ static void srpt_put_send_ioctx(struct

srpt_send_ioctx *ioctx)

        WARN_ON(srpt_get_cmd_state(ioctx) != SRPT_STATE_DONE);

        srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx);

- transport_generic_free_cmd(&ioctx->cmd, 0);
+ transport_generic_free_cmd(&ioctx->cmd, 0, 0);

        if (ioctx->n_rbuf > 1) {
                kfree(ioctx->rbufs);
@@ -1899,7 +1899,7 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
                        TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
                goto process_tmr;
        }
-       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr, GFP_KERNEL);
+       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr);
        if (!cmd->se_tmr_req) {
                send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
                send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;

Bart.

Nicholas A. Bellinger

2011-10-19 17:38 UTC #9 lore raw

Quoted 5 lines · Bart Van Assche

Quoted 3 lines · Nicholas A. Bellinger
I'm having no problems with the patch posted for v3.2 review on MT26428 HCAs

Quoted 8 lines

Quoted 6 lines
Can you verify you are actually using this patch, and not your private tree from github..? The reason is that your tree did not contain the bugfix below to ib_srpt code that was originally causing problems:
ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252
Yes, that patch was included - sorry, but I had made another mistake.

Thanks for the update here.

Quoted 8 lines

3.1-rc10 + ib_srpt + patch below is now working fine here. I noticed only one unusual issue so far (with kernel debugging enabled) and that is that the watchdog thread CPU time was unusually high:
root         7  2.3  0.0      0     0 ?        S    18:39   0:22 [watchdog/0]
root        12  0.6  0.0      0     0 ?        S    18:39   0:06 [watchdog/1]

I'm not exactly sure what to make of this..

Quoted 19 lines

The patch I used to backport ib_srpt from LIO-4.1 to kernel 3.1-rc10 is:

drivers/infiniband/ulp/srpt/ib_srpt.c +1 -1

index 109b412..b3947fe 100644
@@ -1340,7 +1340,7 @@ static void srpt_put_send_ioctx(struct
 srpt_send_ioctx *ioctx)
        WARN_ON(srpt_get_cmd_state(ioctx) != SRPT_STATE_DONE);
 
        srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx);
-       transport_generic_free_cmd(&ioctx->cmd, 0);
+       transport_generic_free_cmd(&ioctx->cmd, 0, 0);
 
        if (ioctx->n_rbuf > 1) {

                kfree(ioctx->rbufs);

drivers/infiniband/ulp/srpt/ib_srpt.c static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch, · line 1905

Quoted 8 lines

drivers/infiniband/ulp/srpt/ib_srpt.c +1 -1

@@ -1899,7 +1899,7 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
                        TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
                goto process_tmr;
        }
-       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr, GFP_KERNEL);
+       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr);
        if (!cmd->se_tmr_req) {
                send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
                send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;

Yep, the ib_srpt patch I sent is against target-pending.git/for-next along with the other target-core updates for v3.2 that will be merged first.

Btw, Roland asked me to take a look at the current set of module parameters for ib_srpt, and see which could be removed and/or converted to per SPR target endpoint configfs attributes.. So on that note, a few questions for you on a couple of the module parameters:

*) use_port_guid_in_session_name: This appears to be a legacy compat
item, can this be safetly removed for mainline..?

*) srpt_service_guid: Should this really have global scope..?

Aside from those, i'm currently having a look to see which of the module parameters are not used in the initial srpt_add_one() setup path, and that using per SRP target endpoint configfs attributes would make sense.

Any input here would be appreciated.

Thanks,

--nab

A message that is not in the archive (sent off-list or not yet received): 1319045907.18132.15.camel-Y1+j5t8j3WgjMeEPmliV8E/[email protected]

Bart Van Assche

2011-10-19 18:09 UTC #11 lore raw

Quoted 7 lines · Nicholas A. Bellinger

Btw, Roland asked me to take a look at the current set of module parameters for ib_srpt, and see which could be removed and/or converted to per SPR target endpoint configfs attributes.. So on that note, a few questions for you on a couple of the module parameters:
*) use_port_guid_in_session_name: This appears to be a legacy compat item, can this be safetly removed for mainline..?

As far as I know some people are still using this parameter.

Quoted 1 line

*) srpt_service_guid: Should this really have global scope..?

I'm afraid that having different values of this parameter for different HCAs would break failing over between HCAs.

Quoted 3 lines

Aside from those, i'm currently having a look to see which of the module parameters are not used in the initial srpt_add_one() setup path, and that using per SRP target endpoint configfs attributes would make sense.

My opinion is that the parameters used in srpt_add_one() also should be converted from module parameters into something that is configurable dynamically. ib_srpt users have already been asking to make e.g. srp_max_req_size configurable dynamically. It should be doable to reallocate the receive ring when there are no active sessions.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAO+b5-pypAr6zEXY0eS8Fv06kt-FiYOLNp2PxqqTXnRTo7tfHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Nicholas A. Bellinger

2011-10-19 18:42 UTC #13 lore raw

Quoted 11 lines · Bart Van Assche

Quoted 7 lines · Nicholas A. Bellinger
Btw, Roland asked me to take a look at the current set of module parameters for ib_srpt, and see which could be removed and/or converted to per SPR target endpoint configfs attributes.. So on that note, a few questions for you on a couple of the module parameters:
*) use_port_guid_in_session_name: This appears to be a legacy compat
item, can this be safetly removed for mainline..?
As far as I know some people are still using this parameter.

Sure, but I think the question is if it's worth getting rid of for mainline if it's something that the mainline code will not be using.

AFAICT it's still something we will never end up using with mainline target infrastructure, right..?

Quoted 6 lines

Quoted 2 lines
*) srpt_service_guid: Should this really have global scope..?
I'm afraid that having different values of this parameter for different HCAs would break failing over between HCAs.

Ok, leaving srpt_service_guid as a single global module parameter.

Quoted 7 lines

Quoted 3 lines
Aside from those, i'm currently having a look to see which of the module parameters are not used in the initial srpt_add_one() setup path, and that using per SRP target endpoint configfs attributes would make sense.
My opinion is that the parameters used in srpt_add_one() also should be converted from module parameters into something that is configurable dynamically.

Sure, but the question is: Where can these parameters be set dynamically if srpt_add_one() is called directly from modprobe..?

Quoted 5 lines

ib_srpt users have already been asking to
make e.g. srp_max_req_size configurable dynamically. It should be doable to reallocate the receive ring when there are no active sessions.

Mmmm, I think I understand what you mean here, but will need to dig into the code some more to make sure. I'll try moving as much of these as possible into per SRP target endpoint attributes, and will let you know if I get stuck on any specific parameter.

Thanks,

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1319049743.18132.41.camel-Y1+j5t8j3WgjMeEPmliV8E/[email protected]

Roland Dreier

2011-10-20 16:54 UTC #15 lore raw

Quoted 4 lines · Nicholas A. Bellinger

Quoted 4 lines
Quoted 1 line
*) srpt_service_guid: Should this really have global scope..?
I'm afraid that having different values of this parameter for different HCAs would break failing over between HCAs.

If I have the possibility to set this per-whatever, I can of course still choose to set it to the same value everywhere, if I care about failover (vs. maybe providing SRP service to two different contexts).

How do I choose what value to set this to anyway?

Can we at least put this in configfs with the rest of the target config options? It seems like a hassle fo rme as a user to have to set a module parameter to control this type of config.

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Roland Dreier

2011-10-20 16:51 UTC #16 lore raw

In reply to a message not in the archive

Quoted 4 lines · Bart Van Assche

Quoted 2 lines
*) use_port_guid_in_session_name: This appears to be a legacy compat item, can this be safetly removed for mainline..?
As far as I know some people are still using this parameter.

So what exactly does this parameter do? How do I as a user know what to set this to?

For mainline can we just decide what the right value of this is and get rid of the parameter?

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAL1RGDUfzRXfbc3hpF6s8v-Qo2_okXB8J-jDgQ79ELHe-x3yvg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Bart Van Assche

2011-10-20 18:01 UTC #18 lore raw

Quoted 11 lines · Roland Dreier

Quoted 4 lines · Bart Van Assche
Quoted 2 lines
*) use_port_guid_in_session_name: This appears to be a legacy compat item, can this be safetly removed for mainline..?
As far as I know some people are still using this parameter.
So what exactly does this parameter do? How do I as a user know what to set this to?
For mainline can we just decide what the right value of this is and get rid of the parameter?

If this parameter would be removed I would prefer to use the value 0 (false) - this is the only value that is compliant with the SRP r16a draft.

If anyone is still using this parameter, I think it is a good time to speak up.

The most recent discussion I have been able to find about this subject
is: http://sourceforge.net/mailarchive/message.php?msg_id=23481074

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html