Daniel Kiper
2013-Mar-04 21:14 UTC
[PATCH 1/1] xen/balloon: Enforce various limits on target
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 It does not allow balloon driver to execute infinite loops when target exceeds limits in other cases too. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- 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; + } else { + rc = xenbus_scanf(XBT_NIL, "memory", "static-max", + "%llu", &host_limit); + + if (rc != 1) { + pr_info("xen_balloon: %s: Guest domain target limit " + "could not be established: %i\n", __func__, rc); + goto no_host_limit; + } + + /* + * The given memory target limit value is in KiB, so it needs + * converting to pages. PAGE_SHIFT converts bytes to pages, + * hence PAGE_SHIFT - 10. + */ + host_limit >>= (PAGE_SHIFT - 10); + } + + /* Enforce hypervisor/host defined limit. */ + target = min(target, (unsigned long)host_limit); + +no_host_limit: /* No need for lock. Not read-modify-write updates. */ balloon_stats.target_pages = target; + schedule_delayed_work(&balloon_worker, 0); } EXPORT_SYMBOL_GPL(balloon_set_new_target); -- 1.7.10.4
Konrad Rzeszutek Wilk
2013-Mar-05 19:27 UTC
Re: [PATCH 1/1] xen/balloon: Enforce various limits on target
On Mon, Mar 04, 2013 at 10:14:51PM +0100, 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 > > It does not allow balloon driver to execute infinite > loops when target exceeds limits in other cases too. > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>One nitpick below; David, could you take a look just for a extra set of eyes.> --- > 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);Probably pr_debug. As this means the user booted without dom0_mem_max argument. (B/c rc == UINT_MAX) I would say if you can check for that and it as part of if (rc == -EPERM) check.> + goto no_host_limit; > + } > + > + host_limit = rc; > + } else { > + rc = xenbus_scanf(XBT_NIL, "memory", "static-max", > + "%llu", &host_limit); > + > + if (rc != 1) { > + pr_info("xen_balloon: %s: Guest domain target limit " > + "could not be established: %i\n", __func__, rc); > + goto no_host_limit; > + } > + > + /* > + * The given memory target limit value is in KiB, so it needs > + * converting to pages. PAGE_SHIFT converts bytes to pages, > + * hence PAGE_SHIFT - 10. > + */ > + host_limit >>= (PAGE_SHIFT - 10); > + } > + > + /* Enforce hypervisor/host defined limit. */ > + target = min(target, (unsigned long)host_limit); > + > +no_host_limit: > /* No need for lock. Not read-modify-write updates. */ > balloon_stats.target_pages = target; > + > schedule_delayed_work(&balloon_worker, 0); > } > EXPORT_SYMBOL_GPL(balloon_set_new_target); > -- > 1.7.10.4 >
David Vrabel
2013-Mar-06 11:05 UTC
Re: [PATCH 1/1] xen/balloon: Enforce various limits on target
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: -17I 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. 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.> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> > --- > 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.> + } else { > + rc = xenbus_scanf(XBT_NIL, "memory", "static-max", > + "%llu", &host_limit); > + > + if (rc != 1) { > + pr_info("xen_balloon: %s: Guest domain target limit " > + "could not be established: %i\n", __func__, rc); > + goto no_host_limit; > + } > + > + /* > + * The given memory target limit value is in KiB, so it needs > + * converting to pages. PAGE_SHIFT converts bytes to pages, > + * hence PAGE_SHIFT - 10. > + */ > + host_limit >>= (PAGE_SHIFT - 10); > + } > + > + /* Enforce hypervisor/host defined limit. */ > + target = min(target, (unsigned long)host_limit);With the change above, you can change host_limit to unsigned long and avoid the cast. David
Daniel Kiper
2013-Mar-06 16:47 UTC
Re: [PATCH 1/1] xen/balloon: Enforce various limits on target
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.> 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.For example: - boot guest domain with memory = 2048 and maxmem = 2048, - xl mem-set 1024, - xl mem-max 1536, - echo 2097152 > /sys/devices/system/xen_memory/xen_memory0/target_kb, - ...and balloon driver without this patch attempts to increase reservation until limit is not set to at least 2048... That is why this patch is more generic.> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> > > --- > > 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.> > + } else { > > + rc = xenbus_scanf(XBT_NIL, "memory", "static-max", > > + "%llu", &host_limit); > > + > > + if (rc != 1) { > > + pr_info("xen_balloon: %s: Guest domain target limit " > > + "could not be established: %i\n", __func__, rc); > > + goto no_host_limit; > > + } > > + > > + /* > > + * The given memory target limit value is in KiB, so it needs > > + * converting to pages. PAGE_SHIFT converts bytes to pages, > > + * hence PAGE_SHIFT - 10. > > + */ > > + host_limit >>= (PAGE_SHIFT - 10); > > + } > > + > > + /* Enforce hypervisor/host defined limit. */ > > + target = min(target, (unsigned long)host_limit); > > With the change above, you can change host_limit to unsigned long and > avoid the cast.It is not possible. Please look above why... Daniel
David Vrabel
2013-Mar-06 17:52 UTC
Re: [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@oracle.com> >>> --- >>> 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
Daniel Kiper
2013-Mar-07 11:28 UTC
Re: [PATCH 1/1] xen/balloon: Enforce various limits on target
On Wed, Mar 06, 2013 at 05:52:28PM +0000, David Vrabel wrote:> On 06/03/13 16:47, Daniel Kiper wrote:[...]> >> 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 OK. [...]> >>> + 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 toNo, it was tested by me and d->max_pages is set by xl mem-set (it is current target). d->tot_pages has count of pages for a given moment. It is confusing for me but this is the reality.> 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.Please look above. If you use this value you would not be able to increase reservation. Daniel
David Vrabel
2013-Mar-07 12:07 UTC
Re: [PATCH 1/1] xen/balloon: Enforce various limits on target
On 07/03/13 11:28, Daniel Kiper wrote:> On Wed, Mar 06, 2013 at 05:52:28PM +0000, David Vrabel wrote: > >> 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. > > Please look above. If you use this value you would not > be able to increase reservation.I don''t think I''m understanding the use case you''re talking about. Do you mean we should allow a target <= d->tot_pages even if this is above d->max_pages? I agree with this. Something like this: void balloon_set_new_target(unsigned long target) { domid_t domid = DOMID_SELF; int rc; unsigned long host_limit; target = min(target, MAX_DOMAIN_PAGES); /* Prevent target from attempting the expand the reservation above the max enforced by the hypervisor. */ rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid); if (rc > 0) { host_limit = rc; target = min(target, max(host_limit, balloon_stats.current_pages)); } balloon_stats.target_pages = target; schedule_delayed_work(&balloon_worker, 0); } David
Daniel Kiper
2013-Mar-07 14:25 UTC
Re: [PATCH 1/1] xen/balloon: Enforce various limits on target
On Thu, Mar 07, 2013 at 12:07:09PM +0000, David Vrabel wrote:> On 07/03/13 11:28, Daniel Kiper wrote: > > On Wed, Mar 06, 2013 at 05:52:28PM +0000, David Vrabel wrote: > > > >> 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. > > > > Please look above. If you use this value you would not > > be able to increase reservation. > > I don''t think I''m understanding the use case you''re talking about. > > Do you mean we should allow a target <= d->tot_pages even if this is > above d->max_pages? I agree with this. > > Something like this: > > void balloon_set_new_target(unsigned long target) > { > domid_t domid = DOMID_SELF; > int rc; > unsigned long host_limit; > > target = min(target, MAX_DOMAIN_PAGES); > > /* Prevent target from attempting the expand the reservation > above the max enforced by the hypervisor. */ > rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid); > if (rc > 0) { > host_limit = rc; > target = min(target, > max(host_limit, balloon_stats.current_pages)); > } > > balloon_stats.target_pages = target; > schedule_delayed_work(&balloon_worker, 0); > }I dug deeper into Xen source and found some strange things for me. If you call xl mem-set it does two things: - sets target in xenstore, - sets d->max_pages (sic!). Last thing is very confusing because at least command comment does mention nothing about this behavior. Comment for this command states: "Set the current memory usage for a domain". It does say nothing about setting memory usage limit. IMO it is the role of xl mem-max (another strange thing is that xl mem-max sets d->max_pages but does not touch static-max; I think it should be changed too; now it is not possible to increase limit above maxmem defined at startup which makes memory hotplug practically unusable). That is why xl mem-set behavior should be changed (IIRC xm sets only target in xenstore) or comment should be aligned to what xl code doas in real. Then we could back to discussion about new balloon_set_new_target(). Daniel