[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] tools: libxl: do not set the PoD target on ARM



On 01/28/2014 03:03 PM, Ian Campbell wrote:
On Tue, 2014-01-28 at 14:48 +0000, George Dunlap wrote:
On 01/28/2014 02:31 PM, Ian Campbell wrote:
On Tue, 2014-01-28 at 14:28 +0000, George Dunlap wrote:
On Tue, Jan 28, 2014 at 10:47 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
On Thu, 2014-01-16 at 15:27 +0000, Ian Campbell wrote:
ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.

The correct solution here would be to check for ENOSYS in libxl, unfortunately
xc_domain_set_pod_target suffers from the same broken error reporting as the
rest of libxc and throws away the errno.

So for now conditionally define xc_domain_set_pod_target to return success
(which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
errno==-1 and returns -1, which matches the broken error reporting of the
existing function. It appears to have no in tree callers in any case.

The conditional should be removed once libxc has been fixed.

This makes ballooning (xl mem-set) work for ARM domains.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: george.dunlap@xxxxxxxxxx
---
I'd be generally wary of modifying the error handling in a piecemeal way, but
certainly doing so for 4.4 now would be inapropriate.

IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
frame, at which point this conditional stuff could be dropped.

In terms of the 4.4 release, obviously ballooning would be very nice to have
for ARM guests, on the other hand I'm aware that while the patch is fairly
small/contained and safe it is also pretty skanky and likely wouldn't be
accepted outside of the rc period.

George -- what do you think of this?

So is this actually called in the arm domain build code at the moment?

It is common code in libxl which calls into it. I originally had the
ifdef there instead.

(I've just noticed that I forgot to update $subject when I moved the
#ifdef from libxl to libxc)

Oh, right -- yes, you normally need to call set_pod_target() every time
you update the balloon target, just in case PoD mode was activated on
boot; if it wasn't (or if all the PoD entries have gone away) this will
be a noop.

The only conceptual issue with putting it here is that
xc_domain_set_pod_target() is also called during domain creation to fill
the PoD "cache" with the domain's memory, from which to populate the p2m
on-demand.  So if "someone" were to try to add PoD to the ARM guest
creation, and forgot about this hack, they might spend a bit of time
figuring out why the initial call to fill the PoD cache was succeeding
but the guest was crashing with "PoD empty cache" anyway.

My hope is that this will get cleaned up in the 4.5 timeframe, as part
of the overdue cleanup of libxc error handling. After that then this
will properly report ENOSYS so that libxl can just DTRT. My hope is that
this will happen before anyone gets to implementing PoD on ARM.

Anyway, if this is the biggest stumbling block someone has while adding
PoD to ARM then they will have done pretty well...

(xc_domain_set_target behaves differently if there are no entries in the
p2m than if there are: if the p2m is empty, it will respond to this by
filling the cache; if it's non-empty, it will ignore changes if there
are no outstanding p2m entries.  That made sense at the time, but now it
looks like a bit of an interface trap for the unwary...)

Was there a reason to put this in libxc rather than libxc?  We don't
expect anyone to call libxc, so having it libxc isn't a big deal, but
conceptually it would probably be safer in libxl.

I just thought the hack was more contained in libxc is all. Also having
it in libxc means that when the error handling cleanup I mentioned
occurs this will cleaned up at the same time, whereas if it was an ifdef
in libxl it might get missed. Not a big deal until someone implements
PoD on ARM though...

OK -- well I guess whatever you want to do then. It's a bit of a bikeshed issue -- I've expressed my preference, go ahead and paint it whatever color you think best. :-)

If you want to check in this one:

Release-acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.