On (Mon) 12 Dec 2011 [11:11:55], Miche Baker-Harvey wrote: > So on a CONSOLE_PORT_ADD message, we would take the > (existing)ports_device::ports_lock, and for other control messages we > would justtake the (new) port::port_lock? You are concerned that just > takingthe ports_lock for all control messages could be too > restrictive? Iwouldn't have expected these messages to be frequent > occurrences, butI'll defer to your experience here. No, I mean we'll have to take the new port_lock() everywhere we currently take the port lock, plus in a few more places. I only suggest using port_lock() helper since we'll need a dependency on the portdev lock as well. > The CONSOLE_CONSOLE_PORT message calls hvc_alloc, which also > needsserialization. That's in another one of these three patches; are > youthinking we could leave that patch be, or that we would we use > theport_lock for CONSOLE_CONSOLE_PORT? Using the port_lock > wouldprovide the HVC serialization "for free" but it would be cleaner > if weput HVC related synchronization in hvc_console.c. Yes, definitely, since other users of hvc_console may get bitten in similar ways. However, I'm not too familiar with the hvc code, the people at linux-ppc can be of help. > On Thu, Dec 8, 2011 at 4:08 AM, Amit Shah <amit.shah@xxxxxxxxxx> wrote: > > On (Tue) 06 Dec 2011 [09:05:38], Miche Baker-Harvey wrote: > >> Amit, > >> > >> Ah, indeed. I am not using MSI-X, so virtio_pci::vp_try_to_find_vqs() > >> calls vp_request_intx() and sets up an interrupt callback. From > >> there, when an interrupt occurs, the stack looks something like this: > >> > >> virtio_pci::vp_interrupt() > >> virtio_pci::vp_vring_interrupt() > >> virtio_ring::vring_interrupt() > >> vq->vq.callback() <-- in this case, that's virtio_console::control_intr() > >> workqueue::schedule_work() > >> workqueue::queue_work() > >> queue_work_on(get_cpu()) <-- queues the work on the current CPU. > >> > >> I'm not doing anything to keep multiple control message from being > >> sent concurrently to the guest, and we will take those interrupts on > >> any CPU. I've confirmed that the two instances of > >> handle_control_message() are occurring on different CPUs. > > > > So let's have a new helper, port_lock() that takes the port-specific > > spinlock. There has to be a new helper, since the port lock should > > depend on the portdev lock being taken too. For the port addition > > case, just the portdev lock should be taken. For any other > > operations, the port lock should be taken. > > > > My assumption was that we would be able to serialise the work items, > > but that will be too restrictive. Taking port locks sounds like a > > better idea. > > > > We'd definitely need the port lock in the control work handler. We > > might need it in a few more places (like module removal), but we'll > > worry about that later. > > > > Does this sound fine? > > > > Amit Amit _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
