On Tue, Nov 8, 2011 at 1:42 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > 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. agree that the locking model needs to be defined. In fact the upstream qla2xxx driver is pretty crazy: look in qla_isr.c at the msix interrupt handlers -- they all take hardware_lock! So you can have multiple interrupt vectors going to different CPUs, all serializing on the same lock to avoid any chance of scalability. For target mode additions, probably we should look at hardware_lock as protecting the request queue and use other well-defined locking for other structures (eg session database). >> 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. There's nothing wrong with hardware_lock in process context per se... for example we probably *do* want to use hardware_lock to protect the request queue, so when allocating response (CTIO) IOCBs we should take the hardware_lock. >> 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? In my opinion we should keep the dependency one-way, that is tcm_qla2xxx depends on qla2xxx. However the way things should work IMHO is that tcm_qla2xxx should call a qla2xxx function to register itself as a target handler, and that registration function should take a struct that tcm_qla2xxx fills in. And we can put whatever we need in that struct, so there's no reason why qla2xxx can't call arbitrary target stack functions -- and in fact having to register things through a struct like this forces us to define the API clearly and keep it sane and small. (Doing things this way would let us fix the broken way tcm_qla2xxx pokes into the private data of pci_devs to find qla2xxx structs -- instead tcm_qla2xxx should pass in a callback that qla2xxx calls once for each device) - R. -- 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
