Re: [Xen-devel] [PATCH 4/8] xen: add locks with timeouts for keyhandlers

[Jürgen Groß <jgross@xxxxxxxx>]

On 06.03.20 09:08, Jürgen Groß wrote:
> On 05.03.20 16:25, Jan Beulich wrote:
> > On 13.02.2020 13:54, Juergen Gross wrote:
> > > Most keyhandlers are used to dump hypervisor data to the console and
> > > they are used mostly for debugging purposes. In those cases it might
> > > happen that some data structures are locked and thus are blocking the
> > > handler to access the data.
> > >
> > > In order to be able to still get some information don't use plain
> > > locking functions in the keyhandlers, but a variant of trylocks with
> > > a timeout value. This allows to wait for some time and to give up in
> > > case the lock was not obtained.
> > >
> > > Add the main infrastructure for this feature including a new runtime
> > > parameter allowing to specify the timeout value in milliseconds.
> > >
> > > Use the new locking scheme in the handlers defined in keyhandler.c.
> > Personally I think trylock (as already used in some places) is the
> > way to go. Iirc others disagreed, but also didn't like the approach
> > taken here. I'm not intending to stand in the way if a majority
> > approves of this model, but I'm not going to ack these changes
> > myself.
> Fair enough.
>
> BTW, trylock is used at exactly three places: for dumping vNUMA, MSI and
> livepatch info.
>
> And TBH: the vNUMA case is really strange, as this is a rwlock which is
> held as writer only in one place for a very brief time period when
> freeing the domain's vnuma data.
>
> The MSI case is more complicated and looking at it in more detail I've
> realized that there is another trylock hidden in a subfunction:
> vpci_msix_arch_print(). As vpci_msix_arch_print() will drop the lock in
> the error case the interface to this function is rather weird. In
> addition the occasional softirq processing is erro prone, too, as it
> will happen only if a single domain has at least 64 MSI entries. In case
> of lots of domains with up to 63 entries watchdog timeouts can still
> happen, so I'll send a patch repairing this issue by letting
> vpci_msix_arch_print() dump only one entry and putting the loop and
> softirq handling into vpci_dump_msi().
Oh, sorry, softirqs are processed often enough, I missed one call.

Nevertheless the locking interface wants to be corrected IMO.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel