Re: [PATCH net-next v7 3/5] page_pool: Allow drivers to hint on SKB recycling

On Fri, Jun 04, 2021 at 08:41:52PM +0100, Matthew Wilcox wrote:

On Fri, Jun 04, 2021 at 08:33:47PM +0200, Matteo Croce wrote:

quoted

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 7fcfea7e7b21..057b40ad29bd 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -40,6 +40,9 @@
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 #include <linux/netfilter/nf_conntrack_common.h>
 #endif
+#ifdef CONFIG_PAGE_POOL
+#include <net/page_pool.h>
+#endif

I'm not a huge fan of conditional includes ... any reason to not include
it always?

I think we can. I'll check and change it. 

quoted

@@ -3088,7 +3095,13 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f)
  */
 static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
 {
-	put_page(skb_frag_page(frag));
+	struct page *page = skb_frag_page(frag);
+
+#ifdef CONFIG_PAGE_POOL
+	if (recycle && page_pool_return_skb_page(page_address(page)))
+		return;

It feels weird to have a page here, convert it back to an address,
then convert it back to a head page in page_pool_return_skb_page().
How about passing 'page' here, calling compound_head() in
page_pool_return_skb_page() and calling virt_to_page() in skb_free_head()?

Sure, sounds reasonable. 

quoted

@@ -251,4 +253,11 @@ static inline void page_pool_ring_unlock(struct page_pool *pool)
 		spin_unlock_bh(&pool->ring.producer_lock);
 }
 
+/* Store mem_info on struct page and use it while recycling skb frags */
+static inline
+void page_pool_store_mem_info(struct page *page, struct page_pool *pp)
+{
+	page->pp = pp;

I'm not sure this wrapper needs to exist.

quoted

+}
+
 #endif /* _NET_PAGE_POOL_H */

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index e1321bc9d316..a03f48f45696 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -628,3 +628,26 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
 	}
 }
 EXPORT_SYMBOL(page_pool_update_nid);
+
+bool page_pool_return_skb_page(void *data)
+{
+	struct page_pool *pp;
+	struct page *page;
+
+	page = virt_to_head_page(data);
+	if (unlikely(page->pp_magic != PP_SIGNATURE))
+		return false;
+
+	pp = (struct page_pool *)page->pp;

You don't need the cast any more.

True

quoted

+	/* Driver set this to memory recycling info. Reset it on recycle.
+	 * This will *not* work for NIC using a split-page memory model.
+	 * The page will be returned to the pool here regardless of the
+	 * 'flipped' fragment being in use or not.
+	 */
+	page->pp = NULL;
+	page_pool_put_full_page(pp, page, false);
+
+	return true;
+}
+EXPORT_SYMBOL(page_pool_return_skb_page);

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 12b7e90dd2b5..f769f08e7b32 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -70,6 +70,9 @@
 #include <net/xfrm.h>
 #include <net/mpls.h>
 #include <net/mptcp.h>
+#ifdef CONFIG_PAGE_POOL
+#include <net/page_pool.h>
+#endif
 
 #include <linux/uaccess.h>
 #include <trace/events/skb.h>

@@ -645,10 +648,15 @@ static void skb_free_head(struct sk_buff *skb)
 {
 	unsigned char *head = skb->head;
 
-	if (skb->head_frag)
+	if (skb->head_frag) {
+#ifdef CONFIG_PAGE_POOL
+		if (skb->pp_recycle && page_pool_return_skb_page(head))
+			return;
+#endif

put this in a header file:

static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
{
	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
		return false;
	return page_pool_return_skb_page(virt_to_page(data));
}

then this becomes:

	if (skb->head_frag) {
		if (skb_pp_recycle(skb, head))
			return;

quoted

 		skb_free_frag(head);
-	else
+	} else {
 		kfree(head);
+	}
 }
 

ok


Thanks for having a look

Cheers
/Ilias