Protect against CPU exhaust by event/x process during
errors by adding some delays in scheduling next event
and retry count limit.
Signed-off-by: Daniel Kiper <[email protected]>
---
drivers/xen/balloon.c | 107 +++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 90 insertions(+), 17 deletions(-)
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 4223f64..6bae013 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -66,6 +66,22 @@
#define BALLOON_CLASS_NAME "xen_memory"
+/*
+ * balloon_process() state:
+ *
+ * BP_DONE: done or nothing to do,
+ * BP_EAGAIN: error, go to sleep,
+ * BP_ECANCELED: error, balloon operation canceled.
+ */
+
+enum bp_state {
+ BP_DONE,
+ BP_EAGAIN,
+ BP_ECANCELED
+};
+
+#define RETRY_UNLIMITED 0
+
struct balloon_stats {
/* We aim for 'current allocation' == 'target allocation'. */
unsigned long current_pages;
@@ -73,6 +89,10 @@ struct balloon_stats {
/* Number of pages in high- and low-memory balloons. */
unsigned long balloon_low;
unsigned long balloon_high;
+ unsigned long schedule_delay;
+ unsigned long max_schedule_delay;
+ unsigned long retry_count;
+ unsigned long max_retry_count;
};
static DEFINE_MUTEX(balloon_mutex);
@@ -171,6 +191,36 @@ static struct page *balloon_next_page(struct page *page)
return list_entry(next, struct page, lru);
}
+static enum bp_state update_schedule(enum bp_state state)
+{
+ if (state == BP_DONE) {
+ balloon_stats.schedule_delay = 1;
+ balloon_stats.retry_count = 1;
+ return BP_DONE;
+ }
+
+ pr_info("xen_balloon: Retry count: %lu/%lu\n", balloon_stats.retry_count,
+ balloon_stats.max_retry_count);
+
+ ++balloon_stats.retry_count;
+
+ if (balloon_stats.max_retry_count != RETRY_UNLIMITED &&
+ balloon_stats.retry_count > balloon_stats.max_retry_count) {
+ pr_info("xen_balloon: Retry count limit exceeded\n"
+ "xen_balloon: Balloon operation canceled\n");
+ balloon_stats.schedule_delay = 1;
+ balloon_stats.retry_count = 1;
+ return BP_ECANCELED;
+ }
+
+ balloon_stats.schedule_delay <<= 1;
+
+ if (balloon_stats.schedule_delay > balloon_stats.max_schedule_delay)
+ balloon_stats.schedule_delay = balloon_stats.max_schedule_delay;
+
+ return BP_EAGAIN;
+}
+
static unsigned long current_target(void)
{
unsigned long target = balloon_stats.target_pages;
@@ -183,11 +233,11 @@ static unsigned long current_target(void)
return target;
}
-static int increase_reservation(unsigned long nr_pages)
+static enum bp_state increase_reservation(unsigned long nr_pages)
{
+ int rc;
unsigned long pfn, i;
struct page *page;
- long rc;
struct xen_memory_reservation reservation = {
.address_bits = 0,
.extent_order = 0,
@@ -199,7 +249,10 @@ static int increase_reservation(unsigned long nr_pages)
page = balloon_first_page();
for (i = 0; i < nr_pages; i++) {
- BUG_ON(page == NULL);
+ if (!page) {
+ nr_pages = i;
+ break;
+ }
frame_list[i] = page_to_pfn(page);
page = balloon_next_page(page);
}
@@ -207,8 +260,10 @@ static int increase_reservation(unsigned long nr_pages)
set_xen_guest_handle(reservation.extent_start, frame_list);
reservation.nr_extents = nr_pages;
rc = HYPERVISOR_memory_op(XENMEM_populate_physmap, &reservation);
- if (rc < 0)
- goto out;
+ if (rc <= 0) {
+ pr_info("xen_balloon: %s: Cannot allocate memory\n", __func__);
+ return BP_EAGAIN;
+ }
for (i = 0; i < rc; i++) {
page = balloon_retrieve();
@@ -238,15 +293,14 @@ static int increase_reservation(unsigned long nr_pages)
balloon_stats.current_pages += rc;
- out:
- return rc < 0 ? rc : rc != nr_pages;
+ return BP_DONE;
}
-static int decrease_reservation(unsigned long nr_pages)
+static enum bp_state decrease_reservation(unsigned long nr_pages)
{
+ enum bp_state state = BP_DONE;
unsigned long pfn, i;
struct page *page;
- int need_sleep = 0;
int ret;
struct xen_memory_reservation reservation = {
.address_bits = 0,
@@ -259,8 +313,9 @@ static int decrease_reservation(unsigned long nr_pages)
for (i = 0; i < nr_pages; i++) {
if ((page = alloc_page(GFP_BALLOON)) == NULL) {
+ pr_info("xen_balloon: %s: Cannot allocate memory\n", __func__);
nr_pages = i;
- need_sleep = 1;
+ state = BP_EAGAIN;
break;
}
@@ -296,7 +351,7 @@ static int decrease_reservation(unsigned long nr_pages)
balloon_stats.current_pages -= nr_pages;
- return need_sleep;
+ return state;
}
/*
@@ -307,27 +362,31 @@ static int decrease_reservation(unsigned long nr_pages)
*/
static void balloon_process(struct work_struct *work)
{
- int need_sleep = 0;
+ enum bp_state state = BP_DONE;
long credit;
mutex_lock(&balloon_mutex);
do {
credit = current_target() - balloon_stats.current_pages;
+
if (credit > 0)
- need_sleep = (increase_reservation(credit) != 0);
+ state = increase_reservation(credit);
+
if (credit < 0)
- need_sleep = (decrease_reservation(-credit) != 0);
+ state = decrease_reservation(-credit);
+
+ state = update_schedule(state);
#ifndef CONFIG_PREEMPT
if (need_resched())
schedule();
#endif
- } while ((credit != 0) && !need_sleep);
+ } while (credit && state == BP_DONE);
/* Schedule more work if there is some still to be done. */
- if (current_target() != balloon_stats.current_pages)
- schedule_delayed_work(&balloon_worker, HZ);
+ if (state == BP_EAGAIN)
+ schedule_delayed_work(&balloon_worker, balloon_stats.schedule_delay * HZ);
mutex_unlock(&balloon_mutex);
}
@@ -394,6 +453,11 @@ static int __init balloon_init(void)
balloon_stats.balloon_low = 0;
balloon_stats.balloon_high = 0;
+ balloon_stats.schedule_delay = 1;
+ balloon_stats.max_schedule_delay = 32;
+ balloon_stats.retry_count = 1;
+ balloon_stats.max_retry_count = 16;
+
register_balloon(&balloon_sysdev);
/*
@@ -447,6 +511,11 @@ BALLOON_SHOW(current_kb, "%lu\n", PAGES2KB(balloon_stats.current_pages));
BALLOON_SHOW(low_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_low));
BALLOON_SHOW(high_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_high));
+static SYSDEV_ULONG_ATTR(schedule_delay, 0444, balloon_stats.schedule_delay);
+static SYSDEV_ULONG_ATTR(max_schedule_delay, 0644, balloon_stats.max_schedule_delay);
+static SYSDEV_ULONG_ATTR(retry_count, 0444, balloon_stats.retry_count);
+static SYSDEV_ULONG_ATTR(max_retry_count, 0644, balloon_stats.max_retry_count);
+
static ssize_t show_target_kb(struct sys_device *dev, struct sysdev_attribute *attr,
char *buf)
{
@@ -508,6 +577,10 @@ static SYSDEV_ATTR(target, S_IRUGO | S_IWUSR,
static struct sysdev_attribute *balloon_attrs[] = {
&attr_target_kb,
&attr_target,
+ &attr_schedule_delay.attr,
+ &attr_max_schedule_delay.attr,
+ &attr_retry_count.attr,
+ &attr_max_retry_count.attr
};
static struct attribute *balloon_info_attrs[] = {
--
1.5.6.5
On Tue, 2011-03-08 at 21:48 +0000, Daniel Kiper wrote:
> Protect against CPU exhaust by event/x process during
> errors by adding some delays in scheduling next event
> and retry count limit.
The addition of a default retry count limit reverses the change made in
bc2c0303226ec716854d3c208c7f84fe7aa35cd7. That change was made to allow
system wide ballooning daemons to work as expected and I don't think a
strong argument has been made for undoing it here.
I think the exponential back-off element of this patch is probably all
that is needed to avoid using too much CPU.
We are talking about polling at most once a second (backing off pretty
quickly to once every 32s with this patch) -- is that really enough to
"exhaust" the CPU running event/x?
Also this patch seems to make the driver quite chatty:
> + pr_info("xen_balloon: Retry count: %lu/%lu\n", balloon_stats.retry_count,
> + balloon_stats.max_retry_count);
Not needed. The balloon driver is a best effort background thing, it
doesn't need to be spamming the system logs each time something doesn't
go quite right first time, it should just continue on silently in the
background. It should only be logging if something goes catastrophically
wrong (in which case pr_info isn't really sufficient).
> + if (rc <= 0) {
> + pr_info("xen_balloon: %s: Cannot allocate memory\n", __func__);
Likewise.
> + pr_info("xen_balloon: %s: Cannot allocate memory\n", __func__);
And again.
Ian.
On Mon, Mar 14, 2011 at 03:04:49PM +0000, Ian Campbell wrote:
> On Tue, 2011-03-08 at 21:48 +0000, Daniel Kiper wrote:
> > Protect against CPU exhaust by event/x process during
> > errors by adding some delays in scheduling next event
> > and retry count limit.
>
> The addition of a default retry count limit reverses the change made in
> bc2c0303226ec716854d3c208c7f84fe7aa35cd7. That change was made to allow
> system wide ballooning daemons to work as expected and I don't think a
> strong argument has been made for undoing it here.
It is possible to restore original balloon driver behavior by setting
balloon_stats.max_retry_count = 0 and balloon_stats.max_schedule_delay = 1
using sysfs.
> We are talking about polling at most once a second (backing off pretty
> quickly to once every 32s with this patch) -- is that really enough to
> "exhaust" the CPU running event/x?
OK, it is not precise. I will change that to:
xen/balloon: Reduce CPU utilization by event/x process
> Also this patch seems to make the driver quite chatty:
>
> > + pr_info("xen_balloon: Retry count: %lu/%lu\n", balloon_stats.retry_count,
> > + balloon_stats.max_retry_count);
>
> Not needed. The balloon driver is a best effort background thing, it
> doesn't need to be spamming the system logs each time something doesn't
> go quite right first time, it should just continue on silently in the
> background. It should only be logging if something goes catastrophically
> wrong (in which case pr_info isn't really sufficient).
Here http://lists.xensource.com/archives/html/xen-devel/2011-02/msg00649.html
Kondrad suggested to add some printk() to inform user what is going on.
I agree with him. However, If balloon driver is controlled by external
process it could pollute logs to some extent. I think that issue could
be easliy resolved by adding quiet flag.
Additionally, I think that errors which are sent to logs by balloon
driver are not critical one. That is why I decided to use pr_info(),
however, I cosidered using pr_warn(). If you think that pr_warn()
is better I could change that part of code.
Daniel
On Tue, 2011-03-15 at 15:17 +0000, Daniel Kiper wrote:
> On Mon, Mar 14, 2011 at 03:04:49PM +0000, Ian Campbell wrote:
> > On Tue, 2011-03-08 at 21:48 +0000, Daniel Kiper wrote:
> > > Protect against CPU exhaust by event/x process during
> > > errors by adding some delays in scheduling next event
> > > and retry count limit.
> >
> > The addition of a default retry count limit reverses the change made in
> > bc2c0303226ec716854d3c208c7f84fe7aa35cd7. That change was made to allow
> > system wide ballooning daemons to work as expected and I don't think a
> > strong argument has been made for undoing it here.
>
> It is possible to restore original balloon driver behavior by setting
> balloon_stats.max_retry_count = 0 and balloon_stats.max_schedule_delay = 1
> using sysfs.
If max_retry_count continues to exist at all then the default should be
0, you can't just change an interface which users (in this case host
toolstacks) rely on in this manner.
In any case there is no reason for the guest to arbitrarily stop trying
to reach the limit which it has been asked to shoot for (at least not by
default). The guest should never be asked a guest to aim for a
completely unrealistic target which it can never reach (that would be a
toolstack bug) but it is reasonable to assume that the guest will keep
trying to reach its target across any transient memory pressure.
Allowing the guest to back off (max_schedule_delay > 1) makes sense to
me though, although I think 32s is a pretty large default maximum.
> > Also this patch seems to make the driver quite chatty:
> >
> > > + pr_info("xen_balloon: Retry count: %lu/%lu\n", balloon_stats.retry_count,
> > > + balloon_stats.max_retry_count);
> >
> > Not needed. The balloon driver is a best effort background thing, it
> > doesn't need to be spamming the system logs each time something doesn't
> > go quite right first time, it should just continue on silently in the
> > background. It should only be logging if something goes catastrophically
> > wrong (in which case pr_info isn't really sufficient).
>
> Here http://lists.xensource.com/archives/html/xen-devel/2011-02/msg00649.html
> Kondrad suggested to add some printk() to inform user what is going on.
> I agree with him. However, If balloon driver is controlled by external
> process it could pollute logs to some extent. I think that issue could
> be easliy resolved by adding quiet flag.
>
> Additionally, I think that errors which are sent to logs by balloon
> driver are not critical one. That is why I decided to use pr_info(),
> however, I cosidered using pr_warn(). If you think that pr_warn()
> is better I could change that part of code.
Only the important messages should be logged and those should, by
definition, be via pr_warn (if not higher). However most of the messages
you add needn't be logged at all -- allocation failures and retries are
simply part of the normal behaviour of the balloon driver.
Perhaps some interesting statistics could be exported via sysfs, e.g.
total number of failed allocations, number of allocation failures trying
to reach the current target, how long the balloon driver has been trying
to meet its current target etc, but these don't belong in the system
logs.
The message you quoted above might be acceptable if it wasn't printed
for every retry (a first retry is not going to be all that uncommon) but
rather only periodically after some initial threshold is met without
progress being made. Note that retries without making progress is an
important distinction from just retries, since if the driver is making
some progress there is no need to say anything.
Ian.