Re: [PATCH v3 2/3] hvc_init(): Enforce one-time initialization.

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 [off-list ref] wrote:

quoted

On (Tue) 06 Dec 2011 [09:05:38], Miche Baker-Harvey wrote:

quoted

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