On Mon, Feb 21, 2011 at 11:48 AM, Nicholas A. Bellinger
[off-list ref] 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