Re: RUNSTATE_runnable delta time for idle_domain accounted to HVM guest.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, May 07, 2014 at 09:07:32AM +0100, Jan Beulich wrote:
> >>> On 06.05.14 at 19:36, <konrad.wilk@oracle.com> wrote:
> > The reason we schedule a tasklet instead of continuing with an
> > 'vcpu_kick' is not yet known to me. This commit added the mechanism
> > to do it via the tasklet:

I should also add that a bit more digging and I realized that
the reason we have so many of the tasklets running (the guests
couldn't be that insane to schedule so many alarms for one-shot timers!)
is that we do PCI passthrough. And that uses the 'hvm_dirq_assist'
tasklet (hvm_do_IRQ_dpci). As in we serialize passthrough interrupts
for all of the guests via this tasklet lock. 

And in this particular setups, the other sockets are busy
doing I/O over PCI passthrough devices. Hence the lock is taken
quite frequently - over all off the sockets.

<Big sigh>
> > 
> > commit a5db2986d47fafc5e62f992616f057bfa43015d9
> > Author: Keir Fraser <keir.fraser@citrix.com>
> > Date:   Fri May 8 11:50:12 2009 +0100
> > 
> >     x86 hvm: hvm_set_callback_irq_level() must not be called in IRQ
> >     context or with IRQs disabled. Ensure this by deferring to tasklet
> >     (softirq) context if required.
> >     
> >     Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
> > 
> > But I am not sure why:
> > 
> >  a). 'must not be called in IRQ context or with IRQs disabled' is
> >     important - I haven't dug in the code yet to understand the
> >     crucial reasons for - is there a known issue about this?
> 
> Because of its use of spin_lock(), which would have the potential
> for a deadlock if the function was called in the wrong context. The
> apparent alternative would be to make all users of
> d->arch.hvm_domain.irq_lock use the IRQ-safe variant; I didn't
> check whether that would in turn cause new problems.

<chuckles> Thanks for the pointer.
> 
> >  b). Why do we have a per-cpu tasklet lists, but any manipulation of the
> >      items of them are protected by a global lock. Looking at the code in
> >      Linux and Xen the major difference is that Xen can schedule on specific CPUs
> >      (or even the tasklet can schedule itself on another CPU).
> 
> tasklet_kill() and migrate_tasklets_from_cpu() at the very least
> would need very careful modification if you were to replace the
> global lock.

<nods> That was my feeling as well.
> 
> >      I can see the need for the tasklets being on different CPUs for
> >      the microcode, and I am digging through the other ones to get
> >      a feel for it - but has anybody thought about improving this
> >      code? Has there been any suggestions/ideas tossed around in the
> >      past (the mailing list didn't help or my Google-fun sucks).
> 
> I'm not aware of any such, quite likely because no-one so far
> spotted a problem with the current implementation.

Yeey! First one to discover!

> 
> Jan
>