On Mon, 2011-02-21 at 08:21 -0800, Roland Dreier wrote:
> On Mon, Feb 21, 2011 at 2:48 AM, Nicholas A. Bellinger
> <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
