[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAC_iWjK=Frw_4kp-X+c4bN7e19ygqsg78aiiV2qJc59o7Gx8jA@mail.gmail.com>
Date: Wed, 13 Dec 2023 09:09:34 +0200
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Liang Chen <liangchen.linux@...il.com>, davem@...emloft.net, edumazet@...gle.com,
pabeni@...hat.com, hawk@...nel.org, linyunsheng@...wei.com,
netdev@...r.kernel.org, linux-mm@...ck.org, jasowang@...hat.com,
almasrymina@...gle.com
Subject: Re: [PATCH net-next v8 4/4] skbuff: Optimization of SKB coalescing
for page pool
Hi Jakub,
On Mon, 11 Dec 2023 at 22:14, Jakub Kicinski <kuba@...nel.org> wrote:
>
> 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.
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.
I've mentioned this in the past, but I generally try to prevent people
from shooting themselves in the foot when creating APIs. Unless
there's a proven performance hit, I'd move the pp_recycle checking in
skb_pp_frag_ref().
Thanks
/Ilias
/Ilias
Powered by blists - more mailing lists