[patch 6/6] netif_release_rx_bufs

[patch 6/6] netif_release_rx_bufs

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

  Hi,

This patch adds a netif_release_rx_bufs() function to the netfront
driver.  It intends to fix the rx buffer page leak.  Unfortunaly it
doesn't work perfectly, the reason is that reclaiming the pages granted
to the backend driver works only if the backend driver gave them back
already, filled with network data.  Reclaiming unfilled rx buffers does
NOT work.

I think we need either a way to get back pages with transfer grants
without cooperation from the backend driver, or we need some way to say
"pretty pretty please, give me back my rx buffers" to the netback driver.

comments?

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

[-- Attachment #2: netfront-release-rx --]
[-- Type: text/plain, Size: 2983 bytes --]

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
Index: source-lnx-stable-22813/drivers/xen/netfront/netfront.c
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/netfront/netfront.c	2006-08-17 15:20:17.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/netfront/netfront.c	2006-08-17 15:20:17.000000000 +0200
@@ -494,6 +494,7 @@ static int network_open(struct net_devic
 {
 	struct netfront_info *np = netdev_priv(dev);
 
+	DPRINTK("%s\n", np->xbdev->nodename);
 	memset(&np->stats, 0, sizeof(np->stats));
 
 	network_alloc_rx_buffers(dev);
@@ -1285,10 +1286,80 @@ err:
 	return more_to_do;
 }
 
+static void netif_release_rx_bufs(struct netfront_info *np)
+{
+	struct mmu_update      *mmu = np->rx_mmu;
+	struct multicall_entry *mcl = np->rx_mcl;
+	struct sk_buff *skb;
+	unsigned long mfn;
+	int bufs = 0, gnttab = 0, unused = 0;
+	int id, ref;
+
+	spin_lock(&np->rx_lock);
+
+	for (id = 0; id < NET_RX_RING_SIZE; id++) {
+		if ((ref = np->grant_rx_ref[id]) == GRANT_INVALID_REF) {
+			unused++;
+			continue;
+		}
+		if ((mfn = gnttab_end_foreign_transfer_ref(ref)) == 0) {
+			gnttab++;
+			continue;
+		}
+
+		gnttab_release_grant_reference(&np->gref_rx_head, ref);
+		np->grant_rx_ref[id] = GRANT_INVALID_REF;
+
+		skb = np->rx_skbs[id];
+		add_id_to_freelist(np->rx_skbs, id);
+
+		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+			/* Remap the page. */
+			MULTI_update_va_mapping(mcl, (unsigned long)skb->head,
+						pfn_pte_ma(mfn, PAGE_KERNEL),
+						0);
+			mcl++;
+			mmu->ptr = ((maddr_t)mfn << PAGE_SHIFT)
+				| MMU_MACHPHYS_UPDATE;
+			mmu->val = __pa(skb->head) >> PAGE_SHIFT;
+			mmu++;
+
+			set_phys_to_machine(__pa(skb->head) >> PAGE_SHIFT,
+					    mfn);
+		}
+		bufs++;
+
+#if 0 /* FIXME */
+		dev_kfree_skb(skb);
+#endif
+	}
+
+	printk("%s: %d released ok, %d gnttab errs, %d unused slots\n",
+	       __FUNCTION__, bufs, gnttab, unused);
+	if (0 == bufs)
+		return;
+
+	/* Some pages are no longer absent... */
+	balloon_update_driver_allowance(-bufs);
+
+	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+		/* Do all the remapping work, and M2P updates, in one big hypercall. */
+		mcl->op = __HYPERVISOR_mmu_update;
+		mcl->args[0] = (unsigned long)np->rx_mmu;
+		mcl->args[1] = mmu - np->rx_mmu;
+		mcl->args[2] = 0;
+		mcl->args[3] = DOMID_SELF;
+		mcl++;
+		HYPERVISOR_multicall(np->rx_mcl, mcl - np->rx_mcl);
+	}
+
+	spin_unlock(&np->rx_lock);
+}
 
 static int network_close(struct net_device *dev)
 {
 	struct netfront_info *np = netdev_priv(dev);
+	DPRINTK("%s\n", np->xbdev->nodename);
 	netif_stop_queue(np->netdev);
 	return 0;
 }
@@ -1427,6 +1498,8 @@ static void network_connect(struct net_d
 static void netif_uninit(struct net_device *dev)
 {
 	struct netfront_info *np = netdev_priv(dev);
+	DPRINTK("%s\n", np->xbdev->nodename);
+	netif_release_rx_bufs(np);
 	gnttab_free_grant_references(np->gref_tx_head);
 	gnttab_free_grant_references(np->gref_rx_head);
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [patch 6/6] netif_release_rx_bufs

[Keir Fraser]

On 17/8/06 3:13 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> This patch adds a netif_release_rx_bufs() function to the netfront
> driver.  It intends to fix the rx buffer page leak.  Unfortunaly it
> doesn't work perfectly, the reason is that reclaiming the pages granted
> to the backend driver works only if the backend driver gave them back
> already, filled with network data.  Reclaiming unfilled rx buffers does
> NOT work.
> 
> I think we need either a way to get back pages with transfer grants
> without cooperation from the backend driver, or we need some way to say
> "pretty pretty please, give me back my rx buffers" to the netback driver.

Unlike a map grant, you can reclaim a transfer grant at any time. The only
question is whether it has been used to transfer a page, or not.

 -- Keir

Re: [patch 6/6] netif_release_rx_bufs

[Gerd Hoffmann]

Keir Fraser wrote:
>> I think we need either a way to get back pages with transfer grants
>> without cooperation from the backend driver, or we need some way to say
>> "pretty pretty please, give me back my rx buffers" to the netback driver.
> 
> Unlike a map grant, you can reclaim a transfer grant at any time. The only
> question is whether it has been used to transfer a page, or not.

How can I do that?  gnttab_end_foreign_transfer_ref() doesn't cut it, it
will not succeed unconditionally ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

Re: [patch 6/6] netif_release_rx_bufs

[Keir Fraser]

On 17/8/06 4:13 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>>> I think we need either a way to get back pages with transfer grants
>>> without cooperation from the backend driver, or we need some way to say
>>> "pretty pretty please, give me back my rx buffers" to the netback driver.
>> 
>> Unlike a map grant, you can reclaim a transfer grant at any time. The only
>> question is whether it has been used to transfer a page, or not.
> 
> How can I do that?  gnttab_end_foreign_transfer_ref() doesn't cut it, it
> will not succeed unconditionally ...

The function will not spin unboundedly and, when it returns, the grant is
definitely no longer active. If any page was transferred via the grant, it
is returned to the caller (else zero is returned). It does just what you
need.

 -- Keir

Re: [patch 6/6] netif_release_rx_bufs

[Gerd Hoffmann]

Keir Fraser wrote:
> 
>> How can I do that?  gnttab_end_foreign_transfer_ref() doesn't cut it, it
>> will not succeed unconditionally ...
> 
> The function will not spin unboundedly and, when it returns, the grant is
> definitely no longer active. If any page was transferred via the grant, it
> is returned to the caller (else zero is returned). It does just what you
> need.

Oh, ok.  I assumed zero return means it failed.  Guess I just have to
keep track of the original mfns so I can map them back into the address
space in case no page was transfered, right?

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

Re: [patch 6/6] netif_release_rx_bufs

[Keir Fraser]

On 18/8/06 7:43 am, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>>> How can I do that?  gnttab_end_foreign_transfer_ref() doesn't cut it, it
>>> will not succeed unconditionally ...
>> 
>> The function will not spin unboundedly and, when it returns, the grant is
>> definitely no longer active. If any page was transferred via the grant, it
>> is returned to the caller (else zero is returned). It does just what you
>> need.
> 
> Oh, ok.  I assumed zero return means it failed.  Guess I just have to
> keep track of the original mfns so I can map them back into the address
> space in case no page was transfered, right?

You've given up the original mfns at this point, to ensure you have enough
reservation headroom for the transfer. You'll be wanting to give the 'empty
mfn' to the balloon driver, which can simply stick the page on its
ballooned-out list. You might need to add a new API function to balloon
driver to do this.

 -- Keir

Re: [patch 6/6] netif_release_rx_bufs

[Gerd Hoffmann]

Keir Fraser wrote:
> You've given up the original mfns at this point, to ensure you have enough
> reservation headroom for the transfer. You'll be wanting to give the 'empty
> mfn' to the balloon driver, which can simply stick the page on its
> ballooned-out list. You might need to add a new API function to balloon
> driver to do this.

Passing the page to the balloon driver works ok, but then I have trouble
releasing the skb because shinfo(skb)->frags[0].page isn't valid any
more.  Guess I better aquire a page from xen in netfront instead of
letting the ballon driver do that ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

Re: [patch 6/6] netif_release_rx_bufs

[Keir Fraser]

On 18/8/06 12:34 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> Keir Fraser wrote:
>> You've given up the original mfns at this point, to ensure you have enough
>> reservation headroom for the transfer. You'll be wanting to give the 'empty
>> mfn' to the balloon driver, which can simply stick the page on its
>> ballooned-out list. You might need to add a new API function to balloon
>> driver to do this.
> 
> Passing the page to the balloon driver works ok, but then I have trouble
> releasing the skb because shinfo(skb)->frags[0].page isn't valid any
> more.  Guess I better aquire a page from xen in netfront instead of
> letting the ballon driver do that ...

Best bet is to free the page to the balloon driver, and then set nr_frags to
zero. This is perfectly valid -- it was netfront that set it to non-zero in
the first place.

 -- Keir

Re: [patch 6/6] netif_release_rx_bufs

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

  Hi,

> Best bet is to free the page to the balloon driver, and then set nr_frags to
> zero. This is perfectly valid -- it was netfront that set it to non-zero in
> the first place.

Perfect.  New version of the patch is attached below.

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

[-- Attachment #2: netfront-release-rx --]
[-- Type: text/plain, Size: 5082 bytes --]

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
Index: source-lnx-stable-22813/drivers/xen/netfront/netfront.c
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/netfront/netfront.c	2006-08-18 10:47:12.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/netfront/netfront.c	2006-08-18 14:39:56.000000000 +0200
@@ -494,6 +494,7 @@ static int network_open(struct net_devic
 {
 	struct netfront_info *np = netdev_priv(dev);
 
+	DPRINTK("%s\n", np->xbdev->nodename);
 	memset(&np->stats, 0, sizeof(np->stats));
 
 	network_alloc_rx_buffers(dev);
@@ -1285,10 +1286,86 @@ err:
 	return more_to_do;
 }
 
+static void netif_release_rx_bufs(struct netfront_info *np)
+{
+	struct mmu_update      *mmu = np->rx_mmu;
+	struct multicall_entry *mcl = np->rx_mcl;
+	struct sk_buff *skb;
+	unsigned long mfn;
+	int xfer = 0, noxfer = 0, unused = 0;
+	int id, ref, rc;
+
+	printk("%s: enter\n", __FUNCTION__);
+
+	spin_lock(&np->rx_lock);
+
+	for (id = 0; id < NET_RX_RING_SIZE; id++) {
+		if ((ref = np->grant_rx_ref[id]) == GRANT_INVALID_REF) {
+			unused++;
+			continue;
+		}
+
+		skb = np->rx_skbs[id];
+		mfn = gnttab_end_foreign_transfer_ref(ref);
+		gnttab_release_grant_reference(&np->gref_rx_head, ref);
+		np->grant_rx_ref[id] = GRANT_INVALID_REF;
+		add_id_to_freelist(np->rx_skbs, id);
+
+		if (0 == mfn) {
+			struct page *page = skb_shinfo(skb)->frags[0].page;
+			balloon_release_driver_page(page);
+			skb_shinfo(skb)->nr_frags = 0;
+			dev_kfree_skb(skb);
+			noxfer++;
+			continue;
+		}
+
+		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+			/* Remap the page. */
+			struct page *page = skb_shinfo(skb)->frags[0].page;
+			unsigned long pfn = page_to_pfn(page);
+			void *vaddr = page_address(page);
+
+			MULTI_update_va_mapping(mcl, (unsigned long)vaddr,
+						pfn_pte_ma(mfn, PAGE_KERNEL),
+						0);
+			mcl++;
+			mmu->ptr = ((maddr_t)mfn << PAGE_SHIFT)
+				| MMU_MACHPHYS_UPDATE;
+			mmu->val = pfn;
+			mmu++;
+
+			set_phys_to_machine(pfn, mfn);
+		}
+		dev_kfree_skb(skb);
+		xfer++;
+	}
+	printk("%s: %d xfer, %d noxfer, %d unused\n",
+	       __FUNCTION__, xfer, noxfer, unused);
+
+	if (xfer) {
+		/* Some pages are no longer absent... */
+		balloon_update_driver_allowance(-xfer);
+
+		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+			/* Do all the remapping work, and M2P updates, in one big hypercall. */
+			mcl->op = __HYPERVISOR_mmu_update;
+			mcl->args[0] = (unsigned long)np->rx_mmu;
+			mcl->args[1] = mmu - np->rx_mmu;
+			mcl->args[2] = 0;
+			mcl->args[3] = DOMID_SELF;
+			mcl++;
+			HYPERVISOR_multicall(np->rx_mcl, mcl - np->rx_mcl);
+		}
+	}
+
+	spin_unlock(&np->rx_lock);
+}
 
 static int network_close(struct net_device *dev)
 {
 	struct netfront_info *np = netdev_priv(dev);
+	DPRINTK("%s\n", np->xbdev->nodename);
 	netif_stop_queue(np->netdev);
 	return 0;
 }
@@ -1427,6 +1504,8 @@ static void network_connect(struct net_d
 static void netif_uninit(struct net_device *dev)
 {
 	struct netfront_info *np = netdev_priv(dev);
+	DPRINTK("%s\n", np->xbdev->nodename);
+	netif_release_rx_bufs(np);
 	gnttab_free_grant_references(np->gref_tx_head);
 	gnttab_free_grant_references(np->gref_rx_head);
 }
Index: source-lnx-stable-22813/drivers/xen/balloon/balloon.c
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/balloon/balloon.c	2006-08-18 10:38:47.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/balloon/balloon.c	2006-08-18 12:13:44.000000000 +0200
@@ -612,8 +612,21 @@ void balloon_dealloc_empty_page_range(
 	schedule_work(&balloon_worker);
 }
 
+void balloon_release_driver_page(struct page *page)
+{
+	unsigned long flags;
+
+	balloon_lock(flags);
+	balloon_append(page);
+	driver_pages--;
+	balloon_unlock(flags);
+
+	schedule_work(&balloon_worker);
+}
+
 EXPORT_SYMBOL_GPL(balloon_update_driver_allowance);
 EXPORT_SYMBOL_GPL(balloon_alloc_empty_page_range);
 EXPORT_SYMBOL_GPL(balloon_dealloc_empty_page_range);
+EXPORT_SYMBOL_GPL(balloon_release_driver_page);
 
 MODULE_LICENSE("Dual BSD/GPL");
Index: source-lnx-stable-22813/drivers/xen/netfront/Makefile
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/netfront/Makefile	2006-08-18 11:14:35.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/netfront/Makefile	2006-08-18 12:20:14.000000000 +0200
@@ -1,3 +1,4 @@
+EXTRA_CFLAGS += -DDEBUG=1
 
 obj-$(CONFIG_XEN_NETDEV_FRONTEND)	:= xennet.o
 
Index: source-lnx-stable-22813/include/xen/balloon.h
===================================================================
--- source-lnx-stable-22813.orig/include/xen/balloon.h	2006-08-17 16:29:59.000000000 +0200
+++ source-lnx-stable-22813/include/xen/balloon.h	2006-08-18 12:13:17.000000000 +0200
@@ -52,6 +52,8 @@ extern void
 balloon_dealloc_empty_page_range(
 	struct page *page, unsigned long nr_pages);
 
+void balloon_release_driver_page(struct page *page);
+
 /*
  * Prevent the balloon driver from changing the memory reservation during
  * a driver critical region.

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel