Re: [PATCH 3/3] xen/evtchn: Clean up teardown handling

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 3/3] xen/evtchn: Clean up teardown handling
Date: Tue, 22 Dec 2020 11:28:24 +0000	[thread overview]
Message-ID: <683f7808-aad7-1c42-e9e9-3e251e1a4561@citrix.com> (raw)
In-Reply-To: <3d72bcb6-dabf-2b26-cecd-5f2d36505bd5@suse.com>

On 22/12/2020 10:48, Jan Beulich wrote:
> On 21.12.2020 19:14, Andrew Cooper wrote:
>> First of all, rename the evtchn APIs:
>>  * evtchn_destroy       => evtchn_teardown
>>  * evtchn_destroy_final => evtchn_destroy
> I wonder in how far this is going to cause confusion with backports
> down the road. May I suggest to do only the first of the two renames,
> at least until in a couple of year's time? Or make the second rename
> to e.g. evtchn_cleanup() or evtchn_deinit()?

I considered backports, but I don't think it will be an issue.  The
contents of the two functions are very different, and we're not likely
to be moving the callers in backports.

I'm not fussed about the exact naming, so long as we can make and
agreement and adhere to it strictly.  The current APIs are a total mess.

I used teardown/destroy because that seems to be one common theme in the
APIs, but it will require some to change their name.

>> RFC.  While testing this, I observed this, after faking up an -ENOMEM in
>> dom0's construction:
>>
>>   (XEN) [2020-12-21 16:31:20] NX (Execute Disable) protection active
>>   (XEN) [2020-12-21 16:33:04]
>>   (XEN) [2020-12-21 16:33:04] ****************************************
>>   (XEN) [2020-12-21 16:33:04] Panic on CPU 0:
>>   (XEN) [2020-12-21 16:33:04] Error creating domain 0
>>   (XEN) [2020-12-21 16:33:04] ****************************************
>>
>> XSA-344 appears to have added nearly 2 minutes of wallclock time into the
>> domain_create() error path, which isn't ok.
>>
>> Considering that event channels haven't even been initialised in this
>> particular scenario, it ought to take ~0 time.  Even if event channels have
>> been initalised, none can be active as the domain isn't visible to the system.
> evtchn_init() sets d->valid_evtchns to EVTCHNS_PER_BUCKET. Are you
> suggesting cleaning up one bucket's worth of unused event channels
> takes two minutes? If this is really the case, and considering there
> could at most be unbound Xen channels, perhaps we could avoid
> calling evtchn_teardown() from domain_create()'s error path? We'd
> need to take care of the then missing accounting (->active_evtchns
> and ->xen_evtchns), but this should be doable.

Actually, its a bug in this patch.  evtchn_init() hasn't been called, so
d->valid_evtchns is 0, so the loop is 2^32 iterations long.  Luckily,
this is easy to fix.

As for avoiding calling, specifically not.  Part of the problem we're
trying to fix is that we've got two different destruction paths, and
making domain_teardown() be fully idempotent is key to fixing that.

~Andrew