[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/1] xen/balloon: Enforce various limits on target
On 06/03/13 16:47, Daniel Kiper wrote: > On Wed, Mar 06, 2013 at 11:05:03AM +0000, David Vrabel wrote: >> On 04/03/13 21:14, Daniel Kiper wrote: >>> This patch enforces on target limit statically defined in Linux Kernel >>> source and limit defined by hypervisor or host. >>> >>> Particularly this patch fixes bug which led to flood >>> of dom0 kernel log with messages similar to: >>> >>> System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added >>> xen_balloon: reserve_additional_memory: add_memory() failed: -17 >> >> I think this helps in some cases, but because >> reserve_additional_memory() places the hotplugged memory after max_pfn >> without checking if there is anything already there, there are still >> ways it can repeatedly fail. >> >> e.g., >> >> 1. If dom0 has had its maximum reservation limited initially (with the >> dom0_mem option) /and/ the max reservation and target is subsequently >> raised then the balloon driver will attempt to hotplug memory that >> overlaps with E820_UNUSABLE regions in the e820 map and the hotplug will >> fail. >> >> 2. If a domU has passed-through PCI devices max_pfn is before the PCI >> memory window then the balloon driver will attempt to hotplug memory >> over the PCI device BARs. > > You are right. During work on this patch I discovered that but decided > to enforce limits because it is more generic. Please look below why. > However, I stated that it should be fixed too. I added it to my todo list. > It requires a bit more work because I think new algorithm should cover > many different cases. Probably add_memory() (it requires changes in > mm/memory_hotplug.c) should be modified to look for range having > sufficient size and not conflicting with others. Ok, so we're agreed, this patch doesn't fix everything and that's fine. >> I think reserve_additional_memory() should check the current resource >> map and the e820 map to find a large enough unused region. This can be >> done as an additional patch at a later date. >> >>> It does not allow balloon driver to execute infinite >>> loops when target exceeds limits in other cases too. >> >> This sentence confuses me. I'm just confused by the English. Perhaps it should say: "The balloon driver will limit target to the maximum reservation as any attempt to populate pages above the maximum reservation will always fail." ? > That is why this patch is more generic. > >>> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx> >>> --- >>> drivers/xen/balloon.c | 47 >>> ++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 46 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c >>> index a56776d..07da753 100644 >>> --- a/drivers/xen/balloon.c >>> +++ b/drivers/xen/balloon.c >>> @@ -65,6 +65,7 @@ >>> #include <xen/balloon.h> >>> #include <xen/features.h> >>> #include <xen/page.h> >>> +#include <xen/xenbus.h> >>> >>> /* >>> * balloon_process() state: >>> @@ -490,11 +491,55 @@ static void balloon_process(struct work_struct *work) >>> mutex_unlock(&balloon_mutex); >>> } >>> >>> -/* Resets the Xen limit, sets new target, and kicks off processing. */ >>> +/* Enforce limits, set new target and kick off processing. */ >>> void balloon_set_new_target(unsigned long target) >>> { >>> + domid_t domid = DOMID_SELF; >>> + int rc; >>> + unsigned long long host_limit; >>> + >>> + /* Enforce statically defined limit. */ >>> + target = min(target, MAX_DOMAIN_PAGES); >>> + >>> + if (xen_initial_domain()) { >>> + rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid); >>> + >>> + /* Limit is not enforced by hypervisor. */ >>> + if (rc == -EPERM) >>> + goto no_host_limit; >>> + >>> + if (rc <= 0) { >>> + pr_info("xen_balloon: %s: Initial domain target limit " >>> + "could not be established: %i\n", __func__, rc); >>> + goto no_host_limit; >>> + } >>> + >>> + host_limit = rc; >> >> I think you should use this method for both dom0 and domUs. No need to >> check static-max from xenstore. > > Sadly XENMEM_maximum_reservation for domU returns value which is set by xl > mem-set > not by xl mem-max :-(((... That is why I get this value from xenstore. It gets d->max_pages which the limit for d->tot_pages. d->max_pages is set by xl mem-max (and xl mem-set as it uses the enforce option to libxl_set_memory_target()). If you set the target above d->max_pages you won't be able to populate them. So, using the maximum_reservation call seems like the right thing to me. David _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |