From: Dominic Curran <dominic.curran@citrix.com>
Date: Fri, 11 May 2012 12:24:42 +0100
Subject: [PATCH] xen: Fix a grant table resource leak
If the resource (my testing shows an object in TX/RX ring) still has a
reference held on it, then add to a list and kick-off a timer to try
and free later.
Signed-off-by: Dominic Curran <dominic.curran@citrix.com>
---
drivers/xen/grant-table.c | 72
+++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 68 insertions(+), 4 deletions(-)
Index: linux-2.6/drivers/xen/grant-table.c
==================================================================---
linux-2.6.orig/drivers/xen/grant-table.c 2012-05-08
13:52:54.000000000 +0100
+++ linux-2.6/drivers/xen/grant-table.c 2012-05-08 16:53:52.000000000
+0100
@@ -33,6 +33,7 @@
#include <linux/module.h>
#include <linux/sched.h>
+#include <linux/timer.h>
#include <linux/mm.h>
#include <linux/slab.h>
#include <linux/vmalloc.h>
@@ -57,6 +58,7 @@
(grant_table_version == 1 ? \
(PAGE_SIZE / sizeof(struct grant_entry_v1)) : \
(PAGE_SIZE / sizeof(union grant_entry_v2)))
+#define GNTTAB_CLEANUP_TIMER 10
static grant_ref_t **gnttab_list;
static unsigned int nr_grant_frames;
@@ -66,6 +68,9 @@ static grant_ref_t gnttab_free_head;
static DEFINE_SPINLOCK(gnttab_list_lock);
unsigned long xen_hvm_resume_frames;
EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
+static LIST_HEAD(gnttab_used_ref_list);
+static DEFINE_SPINLOCK(gnttab_used_ref_lock);
+static struct timer_list gnttab_timer;
static union {
struct grant_entry_v1 *v1;
@@ -145,6 +150,16 @@ struct gnttab_ops {
domid_t trans_domid, grant_ref_t trans_gref);
};
+/* This structure is used to hold references to pages until they become
+ ready to free.
+*/
+struct gnttab_used_ref {
+ struct list_head list;
+ grant_ref_t ref;
+ unsigned long page;
+ int readonly;
+};
+
static struct gnttab_ops *gnttab_interface;
/*This reflects status of grant entries, so act as a global value*/
@@ -464,18 +479,62 @@ int gnttab_end_foreign_access_ref(grant_
}
EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);
+static void gnttab_attempt_clean_used_refs(unsigned long arg)
+{
+ struct gnttab_used_ref *u;
+ struct list_head *pos, *q;
+ unsigned long flags;
+
+ spin_lock_irqsave(&gnttab_used_ref_lock, flags);
+
+ list_for_each_safe(pos, q, &gnttab_used_ref_list) {
+ u = list_entry(pos, struct gnttab_used_ref, list);
+
+ if (gnttab_end_foreign_access_ref(u->ref, u->readonly)) {
+ list_del(pos);
+
+ put_free_entry(u->ref);
+ if (u->page != 0)
+ free_page(u->page);
+
+ kfree(u);
+ }
+ }
+ spin_unlock_irqrestore(&gnttab_used_ref_lock, flags);
+
+ if (!list_empty(&gnttab_used_ref_list))
+ mod_timer(&gnttab_timer, jiffies + HZ);
+}
+
void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
unsigned long page)
{
+ struct gnttab_used_ref *u;
+ unsigned long flags;
+
if (gnttab_end_foreign_access_ref(ref, readonly)) {
put_free_entry(ref);
if (page != 0)
free_page(page);
} else {
- /* XXX This needs to be fixed so that the ref and page are
- placed on a list to be freed up later. */
- printk(KERN_WARNING
- "WARNING: leaking g.e. and page still in use!\n");
+ /* References that aren''t cleaned up immediately sit on
+ * a list until they are abandoned & can be cleaned safely.
+ */
+ u = kmalloc(sizeof(struct gnttab_used_ref), GFP_KERNEL);
+ if (!u)
+ return;
+
+ u->page = page;
+ u->ref = ref;
+ u->readonly = readonly;
+
+ spin_lock_irqsave(&gnttab_used_ref_lock, flags);
+ list_add(&(u->list), &gnttab_used_ref_list);
+ spin_unlock_irqrestore(&gnttab_used_ref_lock, flags);
+
+ /* Kick off the cleanup timer. */
+ if (!timer_pending(&gnttab_timer))
+ add_timer(&gnttab_timer);
}
}
EXPORT_SYMBOL_GPL(gnttab_end_foreign_access);
@@ -1068,6 +1127,11 @@ int gnttab_init(void)
gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES;
gnttab_free_head = NR_RESERVED_ENTRIES;
+ init_timer(&gnttab_timer);
+ gnttab_timer.data = 0;
+ gnttab_timer.function = gnttab_attempt_clean_used_refs;
+ gnttab_timer.expires = jiffies +
msecs_to_jiffies(GNTTAB_CLEANUP_TIMER);
+
printk("Grant table initialized\n");
return 0;
>>> On 11.05.12 at 16:20, Dominic Curran <dominic.curran@citrix.com> wrote: > From: Dominic Curran <dominic.curran@citrix.com> > Date: Fri, 11 May 2012 12:24:42 +0100 > Subject: [PATCH] xen: Fix a grant table resource leak > > If the resource (my testing shows an object in TX/RX ring) still has a > reference held on it, then add to a list and kick-off a timer to try > and free later.Did you see http://lists.xen.org/archives/html/xen-devel/2012-04/msg00406.html Jan> Signed-off-by: Dominic Curran <dominic.curran@citrix.com> > --- > drivers/xen/grant-table.c | 72 > +++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 68 insertions(+), 4 deletions(-) > > Index: linux-2.6/drivers/xen/grant-table.c > ==================================================================> --- linux-2.6.orig/drivers/xen/grant-table.c 2012-05-08 > 13:52:54.000000000 +0100 > +++ linux-2.6/drivers/xen/grant-table.c 2012-05-08 16:53:52.000000000 > +0100 > @@ -33,6 +33,7 @@ > > #include <linux/module.h> > #include <linux/sched.h> > +#include <linux/timer.h> > #include <linux/mm.h> > #include <linux/slab.h> > #include <linux/vmalloc.h> > @@ -57,6 +58,7 @@ > (grant_table_version == 1 ? \ > (PAGE_SIZE / sizeof(struct grant_entry_v1)) : \ > (PAGE_SIZE / sizeof(union grant_entry_v2))) > +#define GNTTAB_CLEANUP_TIMER 10 > > static grant_ref_t **gnttab_list; > static unsigned int nr_grant_frames; > @@ -66,6 +68,9 @@ static grant_ref_t gnttab_free_head; > static DEFINE_SPINLOCK(gnttab_list_lock); > unsigned long xen_hvm_resume_frames; > EXPORT_SYMBOL_GPL(xen_hvm_resume_frames); > +static LIST_HEAD(gnttab_used_ref_list); > +static DEFINE_SPINLOCK(gnttab_used_ref_lock); > +static struct timer_list gnttab_timer; > > static union { > struct grant_entry_v1 *v1; > @@ -145,6 +150,16 @@ struct gnttab_ops { > domid_t trans_domid, grant_ref_t trans_gref); > }; > > +/* This structure is used to hold references to pages until they become > + ready to free. > +*/ > +struct gnttab_used_ref { > + struct list_head list; > + grant_ref_t ref; > + unsigned long page; > + int readonly; > +}; > + > static struct gnttab_ops *gnttab_interface; > > /*This reflects status of grant entries, so act as a global value*/ > @@ -464,18 +479,62 @@ int gnttab_end_foreign_access_ref(grant_ > } > EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref); > > +static void gnttab_attempt_clean_used_refs(unsigned long arg) > +{ > + struct gnttab_used_ref *u; > + struct list_head *pos, *q; > + unsigned long flags; > + > + spin_lock_irqsave(&gnttab_used_ref_lock, flags); > + > + list_for_each_safe(pos, q, &gnttab_used_ref_list) { > + u = list_entry(pos, struct gnttab_used_ref, list); > + > + if (gnttab_end_foreign_access_ref(u->ref, u->readonly)) { > + list_del(pos); > + > + put_free_entry(u->ref); > + if (u->page != 0) > + free_page(u->page); > + > + kfree(u); > + } > + } > + spin_unlock_irqrestore(&gnttab_used_ref_lock, flags); > + > + if (!list_empty(&gnttab_used_ref_list)) > + mod_timer(&gnttab_timer, jiffies + HZ); > +} > + > void gnttab_end_foreign_access(grant_ref_t ref, int readonly, > unsigned long page) > { > + struct gnttab_used_ref *u; > + unsigned long flags; > + > if (gnttab_end_foreign_access_ref(ref, readonly)) { > put_free_entry(ref); > if (page != 0) > free_page(page); > } else { > - /* XXX This needs to be fixed so that the ref and page are > - placed on a list to be freed up later. */ > - printk(KERN_WARNING > - "WARNING: leaking g.e. and page still in use!\n"); > + /* References that aren''t cleaned up immediately sit on > + * a list until they are abandoned & can be cleaned safely. > + */ > + u = kmalloc(sizeof(struct gnttab_used_ref), GFP_KERNEL); > + if (!u) > + return; > + > + u->page = page; > + u->ref = ref; > + u->readonly = readonly; > + > + spin_lock_irqsave(&gnttab_used_ref_lock, flags); > + list_add(&(u->list), &gnttab_used_ref_list); > + spin_unlock_irqrestore(&gnttab_used_ref_lock, flags); > + > + /* Kick off the cleanup timer. */ > + if (!timer_pending(&gnttab_timer)) > + add_timer(&gnttab_timer); > } > } > EXPORT_SYMBOL_GPL(gnttab_end_foreign_access); > @@ -1068,6 +1127,11 @@ int gnttab_init(void) > gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES; > gnttab_free_head = NR_RESERVED_ENTRIES; > > + init_timer(&gnttab_timer); > + gnttab_timer.data = 0; > + gnttab_timer.function = gnttab_attempt_clean_used_refs; > + gnttab_timer.expires = jiffies + > msecs_to_jiffies(GNTTAB_CLEANUP_TIMER); > + > printk("Grant table initialized\n"); > return 0; > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk
2012-May-11 14:30 UTC
Re: [PATCH] xen: Fix a grant table resource leak
On Fri, May 11, 2012 at 03:30:05PM +0100, Jan Beulich wrote:> >>> On 11.05.12 at 16:20, Dominic Curran <dominic.curran@citrix.com> wrote: > > From: Dominic Curran <dominic.curran@citrix.com> > > Date: Fri, 11 May 2012 12:24:42 +0100 > > Subject: [PATCH] xen: Fix a grant table resource leak > > > > If the resource (my testing shows an object in TX/RX ring) still has a > > reference held on it, then add to a list and kick-off a timer to try > > and free later. > > Did you see > > http://lists.xen.org/archives/html/xen-devel/2012-04/msg00406.htmlWhich is on the 3.5 queue list: http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a> > Jan > > > Signed-off-by: Dominic Curran <dominic.curran@citrix.com> > > --- > > drivers/xen/grant-table.c | 72 > > +++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 68 insertions(+), 4 deletions(-) > > > > Index: linux-2.6/drivers/xen/grant-table.c > > ==================================================================> > --- linux-2.6.orig/drivers/xen/grant-table.c 2012-05-08 > > 13:52:54.000000000 +0100 > > +++ linux-2.6/drivers/xen/grant-table.c 2012-05-08 16:53:52.000000000 > > +0100 > > @@ -33,6 +33,7 @@ > > > > #include <linux/module.h> > > #include <linux/sched.h> > > +#include <linux/timer.h> > > #include <linux/mm.h> > > #include <linux/slab.h> > > #include <linux/vmalloc.h> > > @@ -57,6 +58,7 @@ > > (grant_table_version == 1 ? \ > > (PAGE_SIZE / sizeof(struct grant_entry_v1)) : \ > > (PAGE_SIZE / sizeof(union grant_entry_v2))) > > +#define GNTTAB_CLEANUP_TIMER 10 > > > > static grant_ref_t **gnttab_list; > > static unsigned int nr_grant_frames; > > @@ -66,6 +68,9 @@ static grant_ref_t gnttab_free_head; > > static DEFINE_SPINLOCK(gnttab_list_lock); > > unsigned long xen_hvm_resume_frames; > > EXPORT_SYMBOL_GPL(xen_hvm_resume_frames); > > +static LIST_HEAD(gnttab_used_ref_list); > > +static DEFINE_SPINLOCK(gnttab_used_ref_lock); > > +static struct timer_list gnttab_timer; > > > > static union { > > struct grant_entry_v1 *v1; > > @@ -145,6 +150,16 @@ struct gnttab_ops { > > domid_t trans_domid, grant_ref_t trans_gref); > > }; > > > > +/* This structure is used to hold references to pages until they become > > + ready to free. > > +*/ > > +struct gnttab_used_ref { > > + struct list_head list; > > + grant_ref_t ref; > > + unsigned long page; > > + int readonly; > > +}; > > + > > static struct gnttab_ops *gnttab_interface; > > > > /*This reflects status of grant entries, so act as a global value*/ > > @@ -464,18 +479,62 @@ int gnttab_end_foreign_access_ref(grant_ > > } > > EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref); > > > > +static void gnttab_attempt_clean_used_refs(unsigned long arg) > > +{ > > + struct gnttab_used_ref *u; > > + struct list_head *pos, *q; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&gnttab_used_ref_lock, flags); > > + > > + list_for_each_safe(pos, q, &gnttab_used_ref_list) { > > + u = list_entry(pos, struct gnttab_used_ref, list); > > + > > + if (gnttab_end_foreign_access_ref(u->ref, u->readonly)) { > > + list_del(pos); > > + > > + put_free_entry(u->ref); > > + if (u->page != 0) > > + free_page(u->page); > > + > > + kfree(u); > > + } > > + } > > + spin_unlock_irqrestore(&gnttab_used_ref_lock, flags); > > + > > + if (!list_empty(&gnttab_used_ref_list)) > > + mod_timer(&gnttab_timer, jiffies + HZ); > > +} > > + > > void gnttab_end_foreign_access(grant_ref_t ref, int readonly, > > unsigned long page) > > { > > + struct gnttab_used_ref *u; > > + unsigned long flags; > > + > > if (gnttab_end_foreign_access_ref(ref, readonly)) { > > put_free_entry(ref); > > if (page != 0) > > free_page(page); > > } else { > > - /* XXX This needs to be fixed so that the ref and page are > > - placed on a list to be freed up later. */ > > - printk(KERN_WARNING > > - "WARNING: leaking g.e. and page still in use!\n"); > > + /* References that aren''t cleaned up immediately sit on > > + * a list until they are abandoned & can be cleaned safely. > > + */ > > + u = kmalloc(sizeof(struct gnttab_used_ref), GFP_KERNEL); > > + if (!u) > > + return; > > + > > + u->page = page; > > + u->ref = ref; > > + u->readonly = readonly; > > + > > + spin_lock_irqsave(&gnttab_used_ref_lock, flags); > > + list_add(&(u->list), &gnttab_used_ref_list); > > + spin_unlock_irqrestore(&gnttab_used_ref_lock, flags); > > + > > + /* Kick off the cleanup timer. */ > > + if (!timer_pending(&gnttab_timer)) > > + add_timer(&gnttab_timer); > > } > > } > > EXPORT_SYMBOL_GPL(gnttab_end_foreign_access); > > @@ -1068,6 +1127,11 @@ int gnttab_init(void) > > gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES; > > gnttab_free_head = NR_RESERVED_ENTRIES; > > > > + init_timer(&gnttab_timer); > > + gnttab_timer.data = 0; > > + gnttab_timer.function = gnttab_attempt_clean_used_refs; > > + gnttab_timer.expires = jiffies + > > msecs_to_jiffies(GNTTAB_CLEANUP_TIMER); > > + > > printk("Grant table initialized\n"); > > return 0; > > > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel >
On 11/05/12 15:30, Jan Beulich wrote:>>>> On 11.05.12 at 16:20, Dominic Curran<dominic.curran@citrix.com> wrote: >> From: Dominic Curran<dominic.curran@citrix.com> >> Date: Fri, 11 May 2012 12:24:42 +0100 >> Subject: [PATCH] xen: Fix a grant table resource leak >> >> If the resource (my testing shows an object in TX/RX ring) still has a >> reference held on it, then add to a list and kick-off a timer to try >> and free later. > Did you see > > http://lists.xen.org/archives/html/xen-devel/2012-04/msg00406.html > > JanOh!