[net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Alexander Duyck]

This change adds a function called skb_free_frag which is meant to
compliment the function __alloc_page_frag.  The general idea is to enable a
more lightweight version of page freeing since we don't actually need all
the overhead of a put_page, and we don't quite fit the model of __free_pages.

The model for this is based off of __free_pages since we don't actually
need to deal with all of the cases that put_page handles.  I incorporated
the virt_to_head_page call and compound_order into the function as it
actually allows for a signficant size reduction by reducing code
duplication.

In order to enable the function it was necessary to move __free_pages_ok
from being a statically defined function so that I could use it in
skbuff.c.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 include/linux/gfp.h    |    1 +
 include/linux/skbuff.h |    1 +
 mm/page_alloc.c        |    4 +---
 net/core/skbuff.c      |   29 ++++++++++++++++++++++++++---
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 97a9373e61e8..edd19a06e2ac 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -365,6 +365,7 @@ extern void __free_pages(struct page *page, unsigned int order);
 extern void free_pages(unsigned long addr, unsigned int order);
 extern void free_hot_cold_page(struct page *page, bool cold);
 extern void free_hot_cold_page_list(struct list_head *list, bool cold);
+extern void __free_pages_ok(struct page *page, unsigned int order);
 
 extern void __free_kmem_pages(struct page *page, unsigned int order);
 extern void free_kmem_pages(unsigned long addr, unsigned int order);
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9c2f793573fa..3bfe3340929e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2186,6 +2186,7 @@ static inline struct sk_buff *netdev_alloc_skb_ip_align(struct net_device *dev,
 	return __netdev_alloc_skb_ip_align(dev, length, GFP_ATOMIC);
 }
 
+void skb_free_frag(void *head);
 void *napi_alloc_frag(unsigned int fragsz);
 struct sk_buff *__napi_alloc_skb(struct napi_struct *napi,
 				 unsigned int length, gfp_t gfp_mask);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ebffa0e4a9c0..ab9ba9360730 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -165,8 +165,6 @@ bool pm_suspended_storage(void)
 int pageblock_order __read_mostly;
 #endif
 
-static void __free_pages_ok(struct page *page, unsigned int order);
-
 /*
  * results with 256, 32 in the lowmem_reserve sysctl:
  *	1G machine -> (16M dma, 800M-16M normal, 1G-800M high)
@@ -815,7 +813,7 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	return true;
 }
 
-static void __free_pages_ok(struct page *page, unsigned int order)
+void __free_pages_ok(struct page *page, unsigned int order)
 {
 	unsigned long flags;
 	int migratetype;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1e4278a4dd7e..754842557fb0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -428,6 +428,27 @@ refill:
 	return page_address(page) + offset;
 }
 
+/**
+ * skb_free_frag - free a page fragment
+ * @head: virtual address of page fragment
+ *
+ * Frees a page fragment allocated out of either a compound or order 0 page.
+ * The function itself is a hybrid between free_pages and free_compound_page
+ * which can be found in mm/page_alloc.c
+ */
+void skb_free_frag(void *head)
+{
+	struct page *page = virt_to_head_page(head);
+
+	if (unlikely(put_page_testzero(page))) {
+		if (likely(PageHead(page)))
+			__free_pages_ok(page, compound_order(page));
+		else
+			free_hot_cold_page(page, false);
+	}
+}
+EXPORT_SYMBOL(skb_free_frag);
+
 static void *__netdev_alloc_frag(unsigned int fragsz, gfp_t gfp_mask)
 {
 	unsigned long flags;
@@ -499,7 +520,7 @@ static struct sk_buff *__alloc_rx_skb(unsigned int length, gfp_t gfp_mask,
 		if (likely(data)) {
 			skb = build_skb(data, fragsz);
 			if (unlikely(!skb))
-				put_page(virt_to_head_page(data));
+				skb_free_frag(data);
 		}
 	} else {
 		skb = __alloc_skb(length, gfp_mask,
@@ -611,10 +632,12 @@ static void skb_clone_fraglist(struct sk_buff *skb)
 
 static void skb_free_head(struct sk_buff *skb)
 {
+	unsigned char *head = skb->head;
+
 	if (skb->head_frag)
-		put_page(virt_to_head_page(skb->head));
+		skb_free_frag(head);
 	else
-		kfree(skb->head);
+		kfree(head);
 }
 
 static void skb_release_data(struct sk_buff *skb)

[net-next PATCH 0/6] Add skb_free_frag to replace put_page(virt_to_head_page(ptr))

[Alexander Duyck]

This patch set cleans up some of the handling of page frags used in the skb
allocation.  The issue was we were having to use a number of calls to
virt_to_head_page in a number of places and then following that up with
put_page.  Both calls end up being expensive, the first due to size, and
the second due to the fact that we end up having to call a number of other
functions before we finally see the page freed in the case of compound
pages.

The skb_free_frag function is meant to resolve that by providing a
centralized location for the virt_to_head_page call and by coalesing
several checks such as the check for PageHead into a single check so that
we can keep the instruction cound minimal when freeing the page frag.

With this change I am seeing an improvement of about 5% in a simple
receive/drop test.

---

Alexander Duyck (6):
      net: Add skb_free_frag to replace use of put_page in freeing skb->head
      netcp: Replace put_page(virt_to_head_page(ptr)) w/ skb_free_frag
      mvneta: Replace put_page(virt_to_head_page(ptr)) w/ skb_free_frag
      e1000: Replace e1000_free_frag with skb_free_frag
      hisilicon: Replace put_page(virt_to_head_page()) with skb_free_frag()
      bnx2x, tg3: Replace put_page(virt_to_head_page()) with skb_free_frag()


 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |    2 +-
 drivers/net/ethernet/broadcom/tg3.c             |    2 +-
 drivers/net/ethernet/hisilicon/hip04_eth.c      |    2 +-
 drivers/net/ethernet/intel/e1000/e1000_main.c   |   19 ++++++---------
 drivers/net/ethernet/marvell/mvneta.c           |    2 +-
 drivers/net/ethernet/ti/netcp_core.c            |    2 +-
 include/linux/gfp.h                             |    1 +
 include/linux/skbuff.h                          |    1 +
 mm/page_alloc.c                                 |    4 +--
 net/core/skbuff.c                               |   29 +++++++++++++++++++++--
 10 files changed, 41 insertions(+), 23 deletions(-)

--

[net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Alexander Duyck]

This change adds a function called skb_free_frag which is meant to
compliment the function __alloc_page_frag.  The general idea is to enable a
more lightweight version of page freeing since we don't actually need all
the overhead of a put_page, and we don't quite fit the model of __free_pages.

The model for this is based off of __free_pages since we don't actually
need to deal with all of the cases that put_page handles.  I incorporated
the virt_to_head_page call and compound_order into the function as it
actually allows for a signficant size reduction by reducing code
duplication.

In order to enable the function it was necessary to move __free_pages_ok
from being a statically defined function so that I could use it in
skbuff.c.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 include/linux/gfp.h    |    1 +
 include/linux/skbuff.h |    1 +
 mm/page_alloc.c        |    4 +---
 net/core/skbuff.c      |   29 ++++++++++++++++++++++++++---
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 97a9373e61e8..edd19a06e2ac 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -365,6 +365,7 @@ extern void __free_pages(struct page *page, unsigned int order);
 extern void free_pages(unsigned long addr, unsigned int order);
 extern void free_hot_cold_page(struct page *page, bool cold);
 extern void free_hot_cold_page_list(struct list_head *list, bool cold);
+extern void __free_pages_ok(struct page *page, unsigned int order);
 
 extern void __free_kmem_pages(struct page *page, unsigned int order);
 extern void free_kmem_pages(unsigned long addr, unsigned int order);
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9c2f793573fa..3bfe3340929e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2186,6 +2186,7 @@ static inline struct sk_buff *netdev_alloc_skb_ip_align(struct net_device *dev,
 	return __netdev_alloc_skb_ip_align(dev, length, GFP_ATOMIC);
 }
 
+void skb_free_frag(void *head);
 void *napi_alloc_frag(unsigned int fragsz);
 struct sk_buff *__napi_alloc_skb(struct napi_struct *napi,
 				 unsigned int length, gfp_t gfp_mask);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ebffa0e4a9c0..ab9ba9360730 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -165,8 +165,6 @@ bool pm_suspended_storage(void)
 int pageblock_order __read_mostly;
 #endif
 
-static void __free_pages_ok(struct page *page, unsigned int order);
-
 /*
  * results with 256, 32 in the lowmem_reserve sysctl:
  *	1G machine -> (16M dma, 800M-16M normal, 1G-800M high)
@@ -815,7 +813,7 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	return true;
 }
 
-static void __free_pages_ok(struct page *page, unsigned int order)
+void __free_pages_ok(struct page *page, unsigned int order)
 {
 	unsigned long flags;
 	int migratetype;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1e4278a4dd7e..754842557fb0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -428,6 +428,27 @@ refill:
 	return page_address(page) + offset;
 }
 
+/**
+ * skb_free_frag - free a page fragment
+ * @head: virtual address of page fragment
+ *
+ * Frees a page fragment allocated out of either a compound or order 0 page.
+ * The function itself is a hybrid between free_pages and free_compound_page
+ * which can be found in mm/page_alloc.c
+ */
+void skb_free_frag(void *head)
+{
+	struct page *page = virt_to_head_page(head);
+
+	if (unlikely(put_page_testzero(page))) {
+		if (likely(PageHead(page)))
+			__free_pages_ok(page, compound_order(page));
+		else
+			free_hot_cold_page(page, false);
+	}
+}
+EXPORT_SYMBOL(skb_free_frag);
+
 static void *__netdev_alloc_frag(unsigned int fragsz, gfp_t gfp_mask)
 {
 	unsigned long flags;
@@ -499,7 +520,7 @@ static struct sk_buff *__alloc_rx_skb(unsigned int length, gfp_t gfp_mask,
 		if (likely(data)) {
 			skb = build_skb(data, fragsz);
 			if (unlikely(!skb))
-				put_page(virt_to_head_page(data));
+				skb_free_frag(data);
 		}
 	} else {
 		skb = __alloc_skb(length, gfp_mask,
@@ -611,10 +632,12 @@ static void skb_clone_fraglist(struct sk_buff *skb)
 
 static void skb_free_head(struct sk_buff *skb)
 {
+	unsigned char *head = skb->head;
+
 	if (skb->head_frag)
-		put_page(virt_to_head_page(skb->head));
+		skb_free_frag(head);
 	else
-		kfree(skb->head);
+		kfree(head);
 }
 
 static void skb_release_data(struct sk_buff *skb)

[net-next PATCH 2/6] netcp: Replace put_page(virt_to_head_page(ptr)) w/ skb_free_frag

[Alexander Duyck]

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/ti/netcp_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 43efc3a0cda5..0a28c07361cf 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -537,7 +537,7 @@ int netcp_unregister_rxhook(struct netcp_intf *netcp_priv, int order,
 static void netcp_frag_free(bool is_frag, void *ptr)
 {
 	if (is_frag)
-		put_page(virt_to_head_page(ptr));
+		skb_free_frag(ptr);
 	else
 		kfree(ptr);
 }

[net-next PATCH 3/6] mvneta: Replace put_page(virt_to_head_page(ptr)) w/ skb_free_frag

[Alexander Duyck]

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/marvell/mvneta.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..ecce8261ce3b 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1359,7 +1359,7 @@ static void *mvneta_frag_alloc(const struct mvneta_port *pp)
 static void mvneta_frag_free(const struct mvneta_port *pp, void *data)
 {
 	if (likely(pp->frag_size <= PAGE_SIZE))
-		put_page(virt_to_head_page(data));
+		skb_free_frag(data);
 	else
 		kfree(data);
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[net-next PATCH 4/6] e1000: Replace e1000_free_frag with skb_free_frag

[Alexander Duyck]

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/intel/e1000/e1000_main.c |   19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 983eb4e6f7aa..74dc15055971 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -2079,11 +2079,6 @@ static void *e1000_alloc_frag(const struct e1000_adapter *a)
 	return data;
 }
 
-static void e1000_free_frag(const void *data)
-{
-	put_page(virt_to_head_page(data));
-}
-
 /**
  * e1000_clean_rx_ring - Free Rx Buffers per Queue
  * @adapter: board private structure
@@ -2107,7 +2102,7 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
 						 adapter->rx_buffer_len,
 						 DMA_FROM_DEVICE);
 			if (buffer_info->rxbuf.data) {
-				e1000_free_frag(buffer_info->rxbuf.data);
+				skb_free_frag(buffer_info->rxbuf.data);
 				buffer_info->rxbuf.data = NULL;
 			}
 		} else if (adapter->clean_rx == e1000_clean_jumbo_rx_irq) {
@@ -4594,28 +4589,28 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
 			data = e1000_alloc_frag(adapter);
 			/* Failed allocation, critical failure */
 			if (!data) {
-				e1000_free_frag(olddata);
+				skb_free_frag(olddata);
 				adapter->alloc_rx_buff_failed++;
 				break;
 			}
 
 			if (!e1000_check_64k_bound(adapter, data, bufsz)) {
 				/* give up */
-				e1000_free_frag(data);
-				e1000_free_frag(olddata);
+				skb_free_frag(data);
+				skb_free_frag(olddata);
 				adapter->alloc_rx_buff_failed++;
 				break;
 			}
 
 			/* Use new allocation */
-			e1000_free_frag(olddata);
+			skb_free_frag(olddata);
 		}
 		buffer_info->dma = dma_map_single(&pdev->dev,
 						  data,
 						  adapter->rx_buffer_len,
 						  DMA_FROM_DEVICE);
 		if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
-			e1000_free_frag(data);
+			skb_free_frag(data);
 			buffer_info->dma = 0;
 			adapter->alloc_rx_buff_failed++;
 			break;
@@ -4637,7 +4632,7 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
 					 adapter->rx_buffer_len,
 					 DMA_FROM_DEVICE);
 
-			e1000_free_frag(data);
+			skb_free_frag(data);
 			buffer_info->rxbuf.data = NULL;
 			buffer_info->dma = 0;
 

[net-next PATCH 5/6] hisilicon: Replace put_page(virt_to_head_page()) with skb_free_frag()

[Alexander Duyck]

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 3b39fdddeb57..d49bee38cd31 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -798,7 +798,7 @@ static void hip04_free_ring(struct net_device *ndev, struct device *d)
 
 	for (i = 0; i < RX_DESC_NUM; i++)
 		if (priv->rx_buf[i])
-			put_page(virt_to_head_page(priv->rx_buf[i]));
+			skb_free_frag(priv->rx_buf[i]);
 
 	for (i = 0; i < TX_DESC_NUM; i++)
 		if (priv->tx_skb[i])

[net-next PATCH 6/6] bnx2x, tg3: Replace put_page(virt_to_head_page()) with skb_free_frag()

[Alexander Duyck]

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |    2 +-
 drivers/net/ethernet/broadcom/tg3.c             |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index a8bb8f664d3d..b10d1744e5ae 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -662,7 +662,7 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 static void bnx2x_frag_free(const struct bnx2x_fastpath *fp, void *data)
 {
 	if (fp->rx_frag_size)
-		put_page(virt_to_head_page(data));
+		skb_free_frag(data);
 	else
 		kfree(data);
 }
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 069952fa5d64..73c934cf6c61 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -6618,7 +6618,7 @@ static void tg3_tx(struct tg3_napi *tnapi)
 static void tg3_frag_free(bool is_frag, void *data)
 {
 	if (is_frag)
-		put_page(virt_to_head_page(data));
+		skb_free_frag(data);
 	else
 		kfree(data);
 }

Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Eric Dumazet]

On Mon, 2015-05-04 at 16:14 -0700, Alexander Duyck wrote:
> This change adds a function called skb_free_frag which is meant to
> compliment the function __alloc_page_frag.  The general idea is to enable a
> more lightweight version of page freeing since we don't actually need all
> the overhead of a put_page, and we don't quite fit the model of __free_pages.

Could you describe what are the things that put_page() handle that we
don't need for skb frags ?

It looks the change could benefit to other users (outside of networking)

Re: [net-next PATCH 4/6] e1000: Replace e1000_free_frag with skb_free_frag

[Jeff Kirsher]

On Mon, May 4, 2015 at 4:15 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>  drivers/net/ethernet/intel/e1000/e1000_main.c |   19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Alexander Duyck]

On 05/04/2015 05:16 PM, Eric Dumazet wrote:
> On Mon, 2015-05-04 at 16:14 -0700, Alexander Duyck wrote:
>> This change adds a function called skb_free_frag which is meant to
>> compliment the function __alloc_page_frag.  The general idea is to enable a
>> more lightweight version of page freeing since we don't actually need all
>> the overhead of a put_page, and we don't quite fit the model of __free_pages.
>
> Could you describe what are the things that put_page() handle that we
> don't need for skb frags ?

A large part of it is just all the extra code flow with each level 
having to retest flags.  So if you follow the calls for put_page w/ a 
page frag you end up with something like:
skb_free_head - virt_to_head_page
   put_page() - (Head | Tail)
     put_compound_page() - Tail, Head, _count
       __put_compound_page() - compound_dtor
         __page_cache_release() - LRU, mem_cgroup
           free_compound_page() - Head, compound_order
             __free_pages_ok()

If I free the same page frag in skb_frag_free the path ends up looking like:
skb_free_head - inlined by compiler
   skb_free_frag - virt_to_head_page, count, head, order
     __free_pages_ok()

> It looks the change could benefit to other users (outside of networking)

It could, but there are also other mechanisms already in place for 
freeing pages.  For example in the Intel drivers I had gotten into the 
habit of using __free_pages for Rx pages since it does exactly the same 
thing but you need to know the order of the pages you are freeing before 
you can use it.  In the case of head frags we don't really know that 
since it can be a page fragment given to us by any of the drivers using 
alloc_pages.

The motivation behind creating a centralized function was to take care 
of the virt_to_head_page and compound_order portions of this in one 
centralized spot.  It avoids bloating things as I'm able to get away 
with little tricks like combining the compound_order Head flag check 
with the one to determine if I call the compound freeing function or the 
order 0 one.

- Alex

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [net-next PATCH 0/6] Add skb_free_frag to replace put_page(virt_to_head_page(ptr))

[David Miller]

From: Alexander Duyck <alexander.h.duyck@redhat.com>
Date: Mon, 04 May 2015 16:14:42 -0700

> This patch set cleans up some of the handling of page frags used in the skb
> allocation.  The issue was we were having to use a number of calls to
> virt_to_head_page in a number of places and then following that up with
> put_page.  Both calls end up being expensive, the first due to size, and
> the second due to the fact that we end up having to call a number of other
> functions before we finally see the page freed in the case of compound
> pages.
> 
> The skb_free_frag function is meant to resolve that by providing a
> centralized location for the virt_to_head_page call and by coalesing
> several checks such as the check for PageHead into a single check so that
> we can keep the instruction cound minimal when freeing the page frag.
> 
> With this change I am seeing an improvement of about 5% in a simple
> receive/drop test.

I'm going to need to see some buyin from the mm folks on this series.

Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Andrew Morton]

On Mon, 04 May 2015 16:14:48 -0700 Alexander Duyck <alexander.h.duyck@redhat.com> wrote:

> +/**
> + * skb_free_frag - free a page fragment
> + * @head: virtual address of page fragment
> + *
> + * Frees a page fragment allocated out of either a compound or order 0 page.
> + * The function itself is a hybrid between free_pages and free_compound_page
> + * which can be found in mm/page_alloc.c
> + */
> +void skb_free_frag(void *head)
> +{
> +	struct page *page = virt_to_head_page(head);
> +
> +	if (unlikely(put_page_testzero(page))) {
> +		if (likely(PageHead(page)))
> +			__free_pages_ok(page, compound_order(page));
> +		else
> +			free_hot_cold_page(page, false);
> +	}
> +}

Why are we testing for PageHead in here?  If the code were to simply do

	if (unlikely(put_page_testzero(page)))
		__free_pages_ok(page, compound_order(page));

that would still work?


There's nothing networking-specific in here.  I suggest the function be
renamed and moved to page_alloc.c.  Add an inlined skb_free_frag() in a
net header which calls it.  This way the mm developers know about it
and will hopefully maintain it.  It would need a comment explaining
when and why people should and shouldn't use it.

The term "page fragment" is a net thing and isn't something we know
about.  What is it?  From context I'm thinking a definition would look
something like

  An arbitrary-length arbitrary-offset area of memory which resides
  within a 0 or higher order page.  Multiple fragments within that page
  are individually refcounted, in the page's reference counter.

Is that correct and complete?

Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Alexander Duyck]

On 05/06/2015 12:38 PM, Andrew Morton wrote:
> On Mon, 04 May 2015 16:14:48 -0700 Alexander Duyck <alexander.h.duyck@redhat.com> wrote:
>
>> +/**
>> + * skb_free_frag - free a page fragment
>> + * @head: virtual address of page fragment
>> + *
>> + * Frees a page fragment allocated out of either a compound or order 0 page.
>> + * The function itself is a hybrid between free_pages and free_compound_page
>> + * which can be found in mm/page_alloc.c
>> + */
>> +void skb_free_frag(void *head)
>> +{
>> +	struct page *page = virt_to_head_page(head);
>> +
>> +	if (unlikely(put_page_testzero(page))) {
>> +		if (likely(PageHead(page)))
>> +			__free_pages_ok(page, compound_order(page));
>> +		else
>> +			free_hot_cold_page(page, false);
>> +	}
>> +}
> Why are we testing for PageHead in here?  If the code were to simply do
>
> 	if (unlikely(put_page_testzero(page)))
> 		__free_pages_ok(page, compound_order(page));
>
> that would still work?

My assumption was that there was a performance difference between 
__free_pages_ok and free_hot_cold_page for order 0 pages.  From what I 
can tell free_hot_cold_page will do bulk cleanup via free_pcppages_bulk 
while __free_pages_ok just calls free_one_page.

> There's nothing networking-specific in here.  I suggest the function be
> renamed and moved to page_alloc.c.  Add an inlined skb_free_frag() in a
> net header which calls it.  This way the mm developers know about it
> and will hopefully maintain it.  It would need a comment explaining
> when and why people should and shouldn't use it.

That's true.  While I am at it I should probably pull the allocation out 
as well just so it is all in one location.

> The term "page fragment" is a net thing and isn't something we know
> about.  What is it?  From context I'm thinking a definition would look
> something like
>
>    An arbitrary-length arbitrary-offset area of memory which resides
>    within a 0 or higher order page.  Multiple fragments within that page
>    are individually refcounted, in the page's reference counter.
>
> Is that correct and complete?

Yeah that is pretty complete.  I've added Eric who originally authored 
this to the Cc in case there is something he wants to add. I'll see 
about updating this and will likely have a v2 ready in the next couple 
of hours.

- Alex

Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Andrew Morton]

On Wed, 06 May 2015 13:27:43 -0700 Alexander Duyck <alexander.h.duyck@redhat.com> wrote:

> >> +void skb_free_frag(void *head)
> >> +{
> >> +	struct page *page = virt_to_head_page(head);
> >> +
> >> +	if (unlikely(put_page_testzero(page))) {
> >> +		if (likely(PageHead(page)))
> >> +			__free_pages_ok(page, compound_order(page));
> >> +		else
> >> +			free_hot_cold_page(page, false);
> >> +	}
> >> +}
> > Why are we testing for PageHead in here?  If the code were to simply do
> >
> > 	if (unlikely(put_page_testzero(page)))
> > 		__free_pages_ok(page, compound_order(page));
> >
> > that would still work?
> 
> My assumption was that there was a performance difference between 
> __free_pages_ok and free_hot_cold_page for order 0 pages.  From what I 
> can tell free_hot_cold_page will do bulk cleanup via free_pcppages_bulk 
> while __free_pages_ok just calls free_one_page.

Could be.  Plus there's hopefully some performance advantage if the
page is genuinely cache-hot.  I don't think that anyone has verified
the benefits of the hot/cold optimisation in the last decade or two,
and it was always pretty marginal..

Is the PageHead thing really "likely"?  We're usually dealing with
order>0 pages here?

Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of put_page in freeing skb->head

[Alexander Duyck]

On 05/06/2015 01:41 PM, Andrew Morton wrote:
> On Wed, 06 May 2015 13:27:43 -0700 Alexander Duyck <alexander.h.duyck@redhat.com> wrote:
>
>>>> +void skb_free_frag(void *head)
>>>> +{
>>>> +	struct page *page = virt_to_head_page(head);
>>>> +
>>>> +	if (unlikely(put_page_testzero(page))) {
>>>> +		if (likely(PageHead(page)))
>>>> +			__free_pages_ok(page, compound_order(page));
>>>> +		else
>>>> +			free_hot_cold_page(page, false);
>>>> +	}
>>>> +}
>>> Why are we testing for PageHead in here?  If the code were to simply do
>>>
>>> 	if (unlikely(put_page_testzero(page)))
>>> 		__free_pages_ok(page, compound_order(page));
>>>
>>> that would still work?
>> My assumption was that there was a performance difference between
>> __free_pages_ok and free_hot_cold_page for order 0 pages.  From what I
>> can tell free_hot_cold_page will do bulk cleanup via free_pcppages_bulk
>> while __free_pages_ok just calls free_one_page.
> Could be.  Plus there's hopefully some performance advantage if the
> page is genuinely cache-hot.  I don't think that anyone has verified
> the benefits of the hot/cold optimisation in the last decade or two,
> and it was always pretty marginal..

Either way it doesn't make much difference.  If you would prefer I can 
probably just call __free_pages_ok for all cases.

> Is the PageHead thing really "likely"?  We're usually dealing with
> order>0 pages here?

On any system that only supports 4K pages the default is to allocate an 
order 3 page (32K) and then pull the fragments out of that.  So if 
__free_pages_ok works for an order 0 page I'll just call it since it 
shouldn't be a very common occurrence anyway unless we are under memory 
pressure.