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

From: "Jürgen Groß" <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	xen-devel@lists.xenproject.org,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 4/8] xen: add locks with timeouts for keyhandlers
Date: Fri, 6 Mar 2020 09:08:33 +0100	[thread overview]
Message-ID: <c9a90a26-b318-b93a-90fd-6df8292b3b55@suse.com> (raw)
In-Reply-To: <6f6bf0ce-045f-6c6c-fd98-bba72bcf89d1@suse.com>

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().

Juergen

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