Re: [PATCH] ib_srpt: An IB SRP target

Re: [PATCH] ib_srpt: An IB SRP target

[Bart Van Assche]

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

Re: [PATCH] ib_srpt: An IB SRP target

[Nicholas A. Bellinger]

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

Re: [PATCH] ib_srpt: An IB SRP target

[Bart Van Assche]

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

Re: [PATCH] ib_srpt: An IB SRP target

[Nicholas A. Bellinger]

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

Re: [PATCH] ib_srpt: An IB SRP target

[Roland Dreier]

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

Re: [PATCH] ib_srpt: An IB SRP target

[Nicholas A. Bellinger]

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

Re: [PATCH] ib_srpt: An IB SRP target

[Bart Van Assche]

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

Re: [PATCH] ib_srpt: An IB SRP target

[Nicholas A. Bellinger]

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