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