On Tue, 2011-11-08 at 16:42 -0500, Christoph Hellwig wrote: > On Tue, Nov 08, 2011 at 01:32:59PM -0800, Nicholas A. Bellinger wrote: > > Using qla_tgt_exec_cmd_work() in the main I/O path means currently > > requiring hardware_lock (again) in process context for every I/O > > submission. Also, AFAICT in the original code with QLA_TGT_SESS_WORK_CMD > > (now qla_tgt_exec_cmd_work) is only ever called when a session has > > not been found under the first hardware_lock access. > > And what exactly do we need hardware_lock for? > > - tgt_stop is just read, not needed > - reading the id from the atio structure? I can't see how that could > trace, but it needs a check from someone who needs the hardware. > - tcm_qla2xxx_find_sess_by_s_id? doesn't do any lookps, and certainly > none related to hardware This currently needs to be protected by hardware_lock when looking up an associated se_nacl pointer in vmalloc memory area. > - qla_tgt_sess_get? Yes, does currently. But that's braindead and it > should use an atomic_t or kref Agreed, the legacy usage from the original code is pretty bad. This needs to get fixed as well. > - tm_to_unknown is again another read of a simple scalar > - qla_tgt_send_cmd_to_target? Same thing as for > tcm_qla2xxx_find_sess_by_s_id applies. > > In short the locking model here (and in the whole driver) is utterly > braindead, and no one sat down trying to define it. That is something > which needs fixing, and not working around. ... > > > - make it use a qla2xxx-local workqueue > > > > Do you mean a global qla2xxx local workqueue, instead of a qla_hw_data > > specific workqueue..? Is there a reason why having a per qla_hw_data WQ > > is not a good idea..? > > The workqueues mostly are a context for when you need to flush them. > Unless you actually need to flush all items for a host there absolutely > is not reason to create more workqueues than you need. Ok, in that case converting I'll go ahead and convert to using a single qla_target.c local scope workqueue. > > > > > > - stop holding the hardware lock around qla_tgt_send_cmd_to_target > > > > Why should we reacquire hardware_lock at all in process context..? > > You shouldn't hold it at all in tons of places that can't be explained > either by utter lazyness or cargo cult programming. > Fair enough.. But I think after the initial setup in interrupt context w/ target_get_cmd_sess, we should not need to reacquire it within workqueue process context. > > To clarify, I'm not sure that adding target_core_mod symbol dependencies > > into the qla2xxx LLD makes sense. This would mean target_core_mod would > > need to be loaded even when qla2xxx is configured to run in initiator > > mode (the presumable default). > > Which symbol would you pull in? > Just making sure that whatever we call is from tcm_qla2xx to avoid the two way dependency for initiator mode. :) So the next step is making qla_target.c invoke workqueue do_work, and convert qla_hw_data->tgt_ops->handle_cmd() to be the piece of tcm-qla2xx that's run in process context from qla_target.c do_work code. Will get this converted shortly. --nab -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
