Re: [PATCH net-next v8 4/4] skbuff: Optimization of SKB coalescing for page pool

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 11 Dec 2023 09:46:55 +0200 Ilias Apalodimas wrote:
> As I said in the past the patch look correct. I don't like the fact
> that more pp internals creep into the default network stack, but
> perhaps this is fine with the bigger adoption?
> Jakub any thoughts/objections?

Now that you asked... the helper does seem to be in sort of 
a in-between state of being skb specific.

What worries me is that this:

+/**
+ * skb_pp_frag_ref() - Increase fragment reference count of a page
+ * @page:	page of the fragment on which to increase a reference
+ *
+ * Increase fragment reference count (pp_ref_count) on a page, but if it is
+ * not a page pool page, fallback to increase a reference(_refcount) on a
+ * normal page.
+ */
+static void skb_pp_frag_ref(struct page *page)
+{
+	struct page *head_page = compound_head(page);
+
+	if (likely(is_pp_page(head_page)))
+		page_pool_ref_page(head_page);
+	else
+		page_ref_inc(head_page);
+}

doesn't even document that the caller must make sure that the skb
which owns this page is marked for pp recycling. The caller added
by this patch does that, but we should indicate somewhere that doing
skb_pp_frag_ref() for frag in a non-pp-recycling skb is not correct.

We can either lean in the direction of making it less skb specific,
put the code in page_pool.c / helpers.h and make it clear that the
caller has to be careful.
Or we make it more skb specific, take a skb pointer as arg, and also
look at its recycling marking..
or just improve the kdoc.