On Tue, Sep 04, 2012 at 01:18:31PM +0200, Paolo Bonzini wrote: > Il 04/09/2012 13:09, Michael S. Tsirkin ha scritto: > >> > queuecommand on CPU #0 queuecommand #2 on CPU #1 > >> > -------------------------------------------------------------- > >> > atomic_inc_return(...) == 1 > >> > atomic_inc_return(...) == 2 > >> > virtscsi_queuecommand to queue #1 > >> > tgt->req_vq = queue #0 > >> > virtscsi_queuecommand to queue #0 > >> > > >> > then two requests are issued to different queues without a quiescent > >> > point in the middle. > > What happens then? Does this break correctness? > > Yes, requests to the same target should be processed in FIFO order, or > you have things like a flush issued before the write it was supposed to > flush. This is why I can only change the queue when there is no request > pending. > > Paolo I see. I guess you can rewrite this as: atomic_inc if (atomic_read() == 1) which is a bit cheaper, and make the fact that you do not need increment and return to be atomic, explicit. Another simple idea: store last processor id in target, if it is unchanged no need to play with req_vq and take spinlock. Also - some kind of comment explaining why a similar race can not happen with this lock in place would be nice: I see why this specific race can not trigger but since lock is dropped later before you submit command, I have hard time convincing myself what exactly gurantees that vq is never switched before or even while command is submitted. -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
