* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <201102142109.19150.bvanassche-HInyCGIudOg@public.gmane.org> @ 2011-02-14 20:14 ` Bart Van Assche 2011-02-15 0:18 ` Nicholas A. Bellinger 2011-02-16 20:49 ` Bart Van Assche 2 siblings, 0 replies; 8+ messages in thread From: Bart Van Assche @ 2011-02-14 20:14 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA Cc: Vu Pham, Roland Dreier, David Dillow, Nicholas A. Bellinger On Mon, Feb 14, 2011 at 9:09 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote: > > This patch adds the kernel module ib_srpt, which is a SCSI RDMA Protocol > (SRP) target implementation conforming to the SRP r16a specification. Note: this patch is also available in branch srpt of this git repository: https://github.com/bvanassche/srpt Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <201102142109.19150.bvanassche-HInyCGIudOg@public.gmane.org> 2011-02-14 20:14 ` [PATCH] ib_srpt: An IB SRP target Bart Van Assche @ 2011-02-15 0:18 ` Nicholas A. Bellinger 2011-02-16 20:49 ` Bart Van Assche 2 siblings, 0 replies; 8+ messages in thread From: Nicholas A. Bellinger @ 2011-02-15 0:18 UTC (permalink / raw) To: Bart Van Assche Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vu Pham, Roland Dreier, David Dillow On Mon, 2011-02-14 at 21:09 +0100, Bart Van Assche wrote: > This patch adds the kernel module ib_srpt, which is a SCSI RDMA Protocol > (SRP) target implementation conforming to the SRP r16a specification. > > This driver was originally developed by Vu Pham and has been optimized by > Bart Van Assche. > > Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> > Signed-off-by: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> > Cc: Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> > Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> > Cc: Nicholas A. Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> > --- > drivers/infiniband/Kconfig | 2 + > 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 | 4012 +++++++++++++++++++++++++++++++ > drivers/infiniband/ulp/srpt/ib_srpt.h | 406 ++++ > 7 files changed, 4574 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 > Hi Bart, Thank you for your contibution. I will plan to spend time in the next days reviewing the TCM interaction pieces, but have gone ahead and pushed as-is into a working branch @ lio-core-2.6.git/tcm_ib_srpt (.38-rc4). Please send along any further code updates, and I will make sure they hit this branch. Also just a FYI, in order to compile ib_srpt against the latest LIO upstream tree, the original srp_tmr_to_tcm() code was modified with the following patch to use the new TMR_* prefixed definitions from include/target/target_core_tmr.h. Thanks! --nab diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index e63775b..cfcb1bc 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -1860,15 +1860,15 @@ static int srp_tmr_to_tcm(int fn) { switch (fn) { case SRP_TSK_ABORT_TASK: - return ABORT_TASK; + return TMR_ABORT_TASK; case SRP_TSK_ABORT_TASK_SET: - return ABORT_TASK_SET; + return TMR_ABORT_TASK_SET; case SRP_TSK_CLEAR_TASK_SET: - return CLEAR_TASK_SET; + return TMR_CLEAR_TASK_SET; case SRP_TSK_LUN_RESET: - return LUN_RESET; + return TMR_LUN_RESET; case SRP_TSK_CLEAR_ACA: - return CLEAR_ACA; + return TMR_CLEAR_ACA; default: return -1; } -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <201102142109.19150.bvanassche-HInyCGIudOg@public.gmane.org> 2011-02-14 20:14 ` [PATCH] ib_srpt: An IB SRP target Bart Van Assche 2011-02-15 0:18 ` Nicholas A. Bellinger @ 2011-02-16 20:49 ` Bart Van Assche [not found] ` <AANLkTinQJCfGZxbCkS_rxp=viee2T935ObUCFdoBazSH-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2 siblings, 1 reply; 8+ messages in thread From: Bart Van Assche @ 2011-02-16 20:49 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA Cc: Vu Pham, Roland Dreier, David Dillow, Nicholas A. Bellinger On Mon, Feb 14, 2011 at 9:09 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote: > > This patch adds the kernel module ib_srpt, which is a SCSI RDMA Protocol > (SRP) target implementation conforming to the SRP r16a specification. Since no documentation is available for the target core, the unavoidable happened: I had misinterpret the meaning of one target core function. I just found out that queue_data_in must operate synchronously. So this follow-up patch is necessary in order to make this IB SRP target implementation work correctly: diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index 7fe9d3b..b92c0e3 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -1320,6 +1320,7 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch) ioctx->n_rdma_ius = 0; ioctx->rdma_ius = NULL; ioctx->mapped_sg_count = 0; + init_completion(&ioctx->tx_done); /* * transport_init_se_cmd() does not initialize all fields, so do it * here. @@ -1429,6 +1430,7 @@ static int srpt_abort_cmd(struct srpt_send_ioctx *ioctx) * not been received in time. */ srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx); + complete(&ioctx->tx_done); atomic_set(&ioctx->cmd.t_task->transport_lun_stop, 1); kref_put(&ioctx->kref, srpt_put_send_ioctx_kref); break; @@ -1492,6 +1494,9 @@ static void srpt_handle_send_comp(struct srpt_rdma_ch *ch, && state != SRPT_STATE_DONE)) pr_debug("state = %d\n", state); + if (state == SRPT_STATE_CMD_RSP_SENT) + complete(&ioctx->tx_done); + if (state != SRPT_STATE_DONE) kref_put(&ioctx->kref, srpt_put_send_ioctx_kref); else @@ -3097,7 +3102,9 @@ static int srpt_queue_data_in(struct se_cmd *cmd) resp_len = srpt_build_cmd_rsp(ch, ioctx, ioctx->tag, cmd->scsi_status); ret = srpt_post_send(ch, ioctx, resp_len); - if (ret) { + if (ret == 0) + wait_for_completion(&ioctx->tx_done); + else { printk(KERN_ERR "sending cmd response failed for tag %llu\n", ioctx->tag); srpt_unmap_sg_to_ib_sge(ch, ioctx); diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h index 321b097..54d2382 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.h +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h @@ -218,6 +218,7 @@ struct srpt_send_ioctx { spinlock_t spinlock; enum srpt_command_state state; struct se_cmd cmd; + struct completion tx_done; u64 tag; int sg_cnt; int mapped_sg_count; -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <AANLkTinQJCfGZxbCkS_rxp=viee2T935ObUCFdoBazSH-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <AANLkTinQJCfGZxbCkS_rxp=viee2T935ObUCFdoBazSH-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2011-02-21 10:48 ` Nicholas A. Bellinger [not found] ` <1298285331.26616.100.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Nicholas A. Bellinger @ 2011-02-21 10:48 UTC (permalink / raw) To: Bart Van Assche Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vu Pham, Roland Dreier, David Dillow On Wed, 2011-02-16 at 21:49 +0100, Bart Van Assche wrote: > On Mon, Feb 14, 2011 at 9:09 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote: > > > > This patch adds the kernel module ib_srpt, which is a SCSI RDMA Protocol > > (SRP) target implementation conforming to the SRP r16a specification. > > Since no documentation is available for the target core, the unavoidable > happened: I had misinterpret the meaning of one target core function. I just > found out that queue_data_in must operate synchronously. So this follow-up > patch is necessary in order to make this IB SRP target implementation work > correctly: > Greetings Bart, Using a completion queue here with possibility of the extra context switch is IMHO an unnecessary overhead. The main issue here is that target core needs to be able to clear the struct se_cmd descriptor after invoking the struct target_core_fabric_ops->send_data_in() callback, but before the call to srpt_handle_send_comp() -> release of the fabric dependent descriptor containing our struct se_cmd can happen. I prefer something along the lines of the patch below for lio-core-2.6.git/tcm_ib_srpt to follow what other TCM v4 HW fabric modules are currently doing wrt to TFO->check_stop_free() usage. This patch has been compile tested only at this point, but I will be testing this on IB hardware in the upcoming week. Please review. Thanks, --nab diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index cfcb1bc..cf42db8 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -1326,6 +1326,8 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch) */ memset(&ioctx->cmd, 0, sizeof(ioctx->cmd)); memset(&ioctx->sense_data, 0, sizeof(ioctx->sense_data)); + atomic_set(&ioctx->cmd_done, 0); + atomic_set(&ioctx->cmd_stop_free, 0); return ioctx; } @@ -1486,6 +1488,15 @@ static void srpt_handle_send_comp(struct srpt_rdma_ch *ch, atomic_inc(&ch->sq_wr_avail); state = srpt_set_cmd_state(ioctx, SRPT_STATE_DONE); + /* + * If srpt_check_stop_free() has already been called, we + * are safe to go ahead and call transport_generic_free_cmd() + * to release the descriptor. + */ + atomic_set(&ioctx->cmd_done, 1); + + if (atomic_read(&ioctx->cmd_stop_free) != 0) + transport_generic_free_cmd(&ioctx->cmd, 0, 1, 0); if (WARN_ON(state != SRPT_STATE_CMD_RSP_SENT && state != SRPT_STATE_MGMT_RSP_SENT @@ -1737,13 +1748,26 @@ out_err: static void srpt_check_stop_free(struct se_cmd *cmd) { - struct srpt_send_ioctx *ioctx; + struct srpt_send_ioctx *ioctx = container_of(cmd, + struct srpt_send_ioctx, cmd); + + if (cmd->se_tmr_req) + return; - ioctx = container_of(cmd, struct srpt_send_ioctx, cmd); - transport_generic_free_cmd(cmd, 0, 1, 0); if (srpt_get_cmd_state(ioctx) != SRPT_STATE_MGMT_RSP_SENT) srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx); - kref_put(&ioctx->kref, srpt_put_send_ioctx_kref); + + /* + * If srpt_handle_send_comp() has already been called from the LLD, + * it's safe to call transport_generic_free_cmd() to release the + * descriptor. Otherwise set cmd->cmd_stop_free=1 and let + * srpt_handle_send_comp() call transport_generic_free_cmd(). + */ + if (atomic_read(&ioctx->cmd_done) != 0) { + transport_generic_free_cmd(&ioctx->cmd, 0, 1, 0); + kref_put(&ioctx->kref, srpt_put_send_ioctx_kref); + } else + atomic_set(&ioctx->cmd_stop_free, 1); } /** diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h index 321b097..02be818 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.h +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h @@ -218,6 +218,8 @@ struct srpt_send_ioctx { spinlock_t spinlock; enum srpt_command_state state; struct se_cmd cmd; + atomic_t cmd_done; + atomic_t cmd_stop_free; u64 tag; int sg_cnt; int mapped_sg_count; -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <1298285331.26616.100.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org>]
* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <1298285331.26616.100.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org> @ 2011-02-21 16:21 ` Roland Dreier [not found] ` <AANLkTiksjO7vO7t4MNkr77AWSmbKUSLC_J9CN3y0qVGn-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-02-21 18:08 ` Bart Van Assche 1 sibling, 1 reply; 8+ messages in thread From: Roland Dreier @ 2011-02-21 16:21 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: Bart Van Assche, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vu Pham, Roland Dreier, David Dillow On Mon, Feb 21, 2011 at 2:48 AM, Nicholas A. Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote: > + atomic_t cmd_done; > + atomic_t cmd_stop_free; What's the point of using atomic_t here? AFAICT, you only use atomic_set and atomic_read with them, which have no special ordering powers (look at what those macros expand to). - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <AANLkTiksjO7vO7t4MNkr77AWSmbKUSLC_J9CN3y0qVGn-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <AANLkTiksjO7vO7t4MNkr77AWSmbKUSLC_J9CN3y0qVGn-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2011-02-21 22:59 ` Nicholas A. Bellinger 0 siblings, 0 replies; 8+ messages in thread From: Nicholas A. Bellinger @ 2011-02-21 22:59 UTC (permalink / raw) To: Roland Dreier Cc: Bart Van Assche, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vu Pham, Roland Dreier, David Dillow On Mon, 2011-02-21 at 08:21 -0800, Roland Dreier wrote: > On Mon, Feb 21, 2011 at 2:48 AM, Nicholas A. Bellinger > <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote: > > + atomic_t cmd_done; > > + atomic_t cmd_stop_free; > > What's the point of using atomic_t here? AFAICT, you only use > atomic_set and atomic_read with them, which have no special > ordering powers (look at what those macros expand to). > Ugh, thanks for pointing this out. For atomic_t inc/dec I always use smp_mb__after_atomic_[inc,dec](), but I forgot that atomic_set() is a simple/dumb assignment wrapper that also expects an explict barrier() to enforce CONFIG_SMP ordering.. So adding the extra explict barrier here is currently required to address the potential race between TFO->check_stop_free() usage after TFO->queue_data_in(), and the explict ioctx and ioctl->cmd (struct se_cmd) release from IB completion queue context. However, I am starting to wonder if using TFO->check_stop_free() here with HW target perhaps does not make the most sense.. The ->check_stop_free() target fabric callback was originally used for the TCM_Loop SCSI LLD module in order to immmediate complete and release a struct scsi_cmnd after TFO->queue_data_in() has been called. In the case of using IB completion queues, I think checking for a zero of T_TASK(cmd)->t_transport_active before release the ioctx->cmd might may more sense.. To give more of an idea of how this currently works, the following in target_core_transport.c:transport_cmd_check_stop() is called from TCM backend struct se_device thread context after TFO->queue_data_in() has been called: spin_lock_irqsave(&T_TASK(cmd)->t_state_lock, flags); ..... if (transport_off) { atomic_set(&T_TASK(cmd)->t_transport_active, 0); if (transport_off == 2) { transport_all_task_dev_remove_state(cmd); /* * Clear struct se_cmd->se_lun before the transport_off == 2 * handoff to fabric module. */ cmd->se_lun = NULL; /* * Some fabric modules like tcm_loop can release * their internally allocated I/O refrence now and * struct se_cmd now. */ if (CMD_TFO(cmd)->check_stop_free != NULL) { spin_unlock_irqrestore( &T_TASK(cmd)->t_state_lock, flags); CMD_TFO(cmd)->check_stop_free(cmd); return 1; } } spin_unlock_irqrestore(&T_TASK(cmd)->t_state_lock, flags); return 0; } ..... spin_unlock_irqrestore(&T_TASK(cmd)->t_state_lock, flags); So given this logic, I think waiting for T_TASK(cmd)->t_transport_active to return to zero before releasing ioctx->cmd in the IB completion path might be a better way handle this.. However, to do this properly w/o holding T_TASK(cmd)->t_state_lock in the completion path is going to require a small re-org of the above from transport_cmd_check_stop() for the transport_off == 2 case that TFO->queue_data_in() is invoking.. Thoughts Roland..? --nab -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <1298285331.26616.100.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org> 2011-02-21 16:21 ` Roland Dreier @ 2011-02-21 18:08 ` Bart Van Assche [not found] ` <AANLkTikL-5Az6tQuPxg77T5H5xNKYvL4r9kBjyt87QBg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 8+ messages in thread From: Bart Van Assche @ 2011-02-21 18:08 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vu Pham, Roland Dreier, David Dillow On Mon, Feb 21, 2011 at 11:48 AM, Nicholas A. Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote: > Using a completion queue here with possibility of the extra context > switch is IMHO an unnecessary overhead. The main issue here is that > target core needs to be able to clear the struct se_cmd descriptor after > invoking the struct target_core_fabric_ops->send_data_in() callback, but > before the call to srpt_handle_send_comp() -> release of the fabric > dependent descriptor containing our struct se_cmd can happen. > > I prefer something along the lines of the patch below for > lio-core-2.6.git/tcm_ib_srpt to follow what other TCM v4 HW fabric > modules are currently doing wrt to TFO->check_stop_free() usage. This > patch has been compile tested only at this point, but I will be testing > this on IB hardware in the upcoming week. Please review. Hello Nicholas, Thanks for the review. Regarding the two issues you mentioned: - As far as I understood it is safe to send back data to the initiator concurrently with the target core processing new commands ? In that case no new counters are necessary - the existing kref in srpt_send_ioctx is sufficient. The patch that re-enables concurrent SCSI READs can be found here: https://github.com/bvanassche/srpt/commit/cd8d5e9a6501f5b3a4315c261e945c201c4ce0ae. Please let me know if anyone wants to see that patch posted on the list. Also, please note that your patch contained the same race as my original patch: directly invoking srpt_unmap_sg_to_ib_sge() from the check_stop_free() callback makes that call race with the ongoing RDMA write. To Roland: I haven't found a way to invoke srpt_put_send_ioctx() without using an atomic counter. If you see an approach to avoid that atomic counter, suggestions are welcome. - Regarding making the check_stop_free() callback a NOOP for task management commands: are you sure that that's a good idea ? I have tried to do that, and the following appeared in the kernel log during module unload: CORE_HBA[0] - Detached HBA from Generic Target Core ============================================================================= BUG se_tmr_cache: Objects remaining on kmem_cache_close() ----------------------------------------------------------------------------- INFO: Slab 0xffffea0000119c00 objects=16 used=1 fp=0xffff880005080100 flags=0x1000000000000080 Pid: 18117, comm: rmmod Not tainted 2.6.38-rc5-dbg+ #4 Call Trace: [<ffffffff81123f76>] ? slab_err+0x96/0xb0 [<ffffffff81126da4>] ? __slab_alloc+0x124/0x540 [<ffffffff811277c8>] ? __kmalloc+0xf8/0x1b0 [<ffffffff81081a0d>] ? trace_hardirqs_off+0xd/0x10 [<ffffffff811277e8>] ? __kmalloc+0x118/0x1b0 [<ffffffff81128cf7>] ? kmem_cache_destroy+0x177/0x3f0 [<ffffffffa02e53c6>] ? release_se_global+0x36/0xb0 [target_core_mod] [<ffffffffa02d10ab>] ? target_core_exit_configfs+0x17b/0x190 [target_core_mod] [<ffffffff8109340f>] ? sys_delete_module+0x16f/0x260 [<ffffffff81002fdc>] ? sysret_check+0x27/0x62 [<ffffffff81087a8d>] ? trace_hardirqs_on_caller+0x14d/0x190 [<ffffffff81002fab>] ? system_call_fastpath+0x16/0x1b INFO: Object 0xffff880005080000 @offset=0 INFO: Allocated in core_tmr_alloc_req+0x33/0x80 [target_core_mod] age=2442 cpu=1 pid=18088 SLUB se_tmr_cache: kmem_cache_destroy called for cache that still has objects. Pid: 18117, comm: rmmod Not tainted 2.6.38-rc5-dbg+ #4 Call Trace: [<ffffffff81128ee4>] ? kmem_cache_destroy+0x364/0x3f0 [<ffffffffa02e53c6>] ? release_se_global+0x36/0xb0 [target_core_mod] [<ffffffffa02d10ab>] ? target_core_exit_configfs+0x17b/0x190 [target_core_mod] [<ffffffff8109340f>] ? sys_delete_module+0x16f/0x260 [<ffffffff81002fdc>] ? sysret_check+0x27/0x62 [<ffffffff81087a8d>] ? trace_hardirqs_on_caller+0x14d/0x190 [<ffffffff81002fab>] ? system_call_fastpath+0x16/0x1b Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <AANLkTikL-5Az6tQuPxg77T5H5xNKYvL4r9kBjyt87QBg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] ib_srpt: An IB SRP target [not found] ` <AANLkTikL-5Az6tQuPxg77T5H5xNKYvL4r9kBjyt87QBg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2011-02-21 23:10 ` Nicholas A. Bellinger 0 siblings, 0 replies; 8+ messages in thread From: Nicholas A. Bellinger @ 2011-02-21 23:10 UTC (permalink / raw) To: Bart Van Assche Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vu Pham, David Dillow, Roland Dreier On Mon, 2011-02-21 at 19:08 +0100, Bart Van Assche wrote: > On Mon, Feb 21, 2011 at 11:48 AM, Nicholas A. Bellinger > <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote: > > Using a completion queue here with possibility of the extra context > > switch is IMHO an unnecessary overhead. The main issue here is that > > target core needs to be able to clear the struct se_cmd descriptor after > > invoking the struct target_core_fabric_ops->send_data_in() callback, but > > before the call to srpt_handle_send_comp() -> release of the fabric > > dependent descriptor containing our struct se_cmd can happen. > > > > I prefer something along the lines of the patch below for > > lio-core-2.6.git/tcm_ib_srpt to follow what other TCM v4 HW fabric > > modules are currently doing wrt to TFO->check_stop_free() usage. This > > patch has been compile tested only at this point, but I will be testing > > this on IB hardware in the upcoming week. Please review. > > Hello Nicholas, > > Thanks for the review. Regarding the two issues you mentioned: > - As far as I understood it is safe to send back data to the initiator > concurrently with the target core processing new commands ? Yes, > 1 TCQ depth is not the issue. This issue at hand is how to clear the struct se_cmd in transport_cmd_check_stop() when using the optional TFO->check_stop_free() call after TFO->queue_data_in() has been invoked. > In that > case no new counters are necessary - the existing kref in > srpt_send_ioctx is sufficient. The patch that re-enables concurrent > SCSI READs can be found here: > https://github.com/bvanassche/srpt/commit/cd8d5e9a6501f5b3a4315c261e945c201c4ce0ae. > Please let me know if anyone wants to see that patch posted on the > list. Also, please note that your patch contained the same race as my > original patch: directly invoking srpt_unmap_sg_to_ib_sge() from the > check_stop_free() callback makes that call race with the ongoing RDMA > write. To Roland: I haven't found a way to invoke > srpt_put_send_ioctx() without using an atomic counter. Ok, so this sounds like srpt_unmap_sg_to_ib_sge() should be getting called from the IB completion queue context then.. > If you see an > approach to avoid that atomic counter, suggestions are welcome. > - Regarding making the check_stop_free() callback a NOOP for task > management commands: are you sure that that's a good idea ? I have > tried to do that, and the following appeared in the kernel log during > module unload: > > CORE_HBA[0] - Detached HBA from Generic Target Core > ============================================================================= > BUG se_tmr_cache: Objects remaining on kmem_cache_close() > ----------------------------------------------------------------------------- > > INFO: Slab 0xffffea0000119c00 objects=16 used=1 fp=0xffff880005080100 > flags=0x1000000000000080 > Pid: 18117, comm: rmmod Not tainted 2.6.38-rc5-dbg+ #4 > Call Trace: > [<ffffffff81123f76>] ? slab_err+0x96/0xb0 > [<ffffffff81126da4>] ? __slab_alloc+0x124/0x540 > [<ffffffff811277c8>] ? __kmalloc+0xf8/0x1b0 > [<ffffffff81081a0d>] ? trace_hardirqs_off+0xd/0x10 > [<ffffffff811277e8>] ? __kmalloc+0x118/0x1b0 > [<ffffffff81128cf7>] ? kmem_cache_destroy+0x177/0x3f0 > [<ffffffffa02e53c6>] ? release_se_global+0x36/0xb0 [target_core_mod] > [<ffffffffa02d10ab>] ? target_core_exit_configfs+0x17b/0x190 [target_core_mod] > [<ffffffff8109340f>] ? sys_delete_module+0x16f/0x260 > [<ffffffff81002fdc>] ? sysret_check+0x27/0x62 > [<ffffffff81087a8d>] ? trace_hardirqs_on_caller+0x14d/0x190 > [<ffffffff81002fab>] ? system_call_fastpath+0x16/0x1b > INFO: Object 0xffff880005080000 @offset=0 > INFO: Allocated in core_tmr_alloc_req+0x33/0x80 [target_core_mod] > age=2442 cpu=1 pid=18088 > SLUB se_tmr_cache: kmem_cache_destroy called for cache that still has objects. > Pid: 18117, comm: rmmod Not tainted 2.6.38-rc5-dbg+ #4 > Call Trace: > [<ffffffff81128ee4>] ? kmem_cache_destroy+0x364/0x3f0 > [<ffffffffa02e53c6>] ? release_se_global+0x36/0xb0 [target_core_mod] > [<ffffffffa02d10ab>] ? target_core_exit_configfs+0x17b/0x190 [target_core_mod] > [<ffffffff8109340f>] ? sys_delete_module+0x16f/0x260 > [<ffffffff81002fdc>] ? sysret_check+0x27/0x62 > [<ffffffff81087a8d>] ? trace_hardirqs_on_caller+0x14d/0x190 > [<ffffffff81002fab>] ? system_call_fastpath+0x16/0x1b > Hmmm, I don't recall seeing this with TCM_Loop with sg_reset, but I will double check with the lastest for-39 WIP fabric module code using TFO->check_stop_free(). Thanks! --nab -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-02-21 23:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201102142109.19150.bvanassche@acm.org>
[not found] ` <201102142109.19150.bvanassche-HInyCGIudOg@public.gmane.org>
2011-02-14 20:14 ` [PATCH] ib_srpt: An IB SRP target Bart Van Assche
2011-02-15 0:18 ` Nicholas A. Bellinger
2011-02-16 20:49 ` Bart Van Assche
[not found] ` <AANLkTinQJCfGZxbCkS_rxp=viee2T935ObUCFdoBazSH-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-02-21 10:48 ` Nicholas A. Bellinger
[not found] ` <1298285331.26616.100.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org>
2011-02-21 16:21 ` Roland Dreier
[not found] ` <AANLkTiksjO7vO7t4MNkr77AWSmbKUSLC_J9CN3y0qVGn-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-02-21 22:59 ` Nicholas A. Bellinger
2011-02-21 18:08 ` Bart Van Assche
[not found] ` <AANLkTikL-5Az6tQuPxg77T5H5xNKYvL4r9kBjyt87QBg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-02-21 23:10 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox