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 - qla_tgt_sess_get? Yes, does currently. But that's braindead and it should use an atomic_t or kref - 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. > > > - 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. > 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? -- 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
