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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHS8izMPKJZz=n2CoEiQv+HsC_QKRLm3Wk4V-cq7Jvv=Vr=y9g@mail.gmail.com>
Date: Mon, 11 Dec 2023 12:07:06 -0800
From: Mina Almasry <almasrymina@...gle.com>
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, ilias.apalodimas@...aro.org, 
	linyunsheng@...wei.com, netdev@...r.kernel.org, linux-mm@...ck.org, 
	jasowang@...hat.com
Subject: Re: [PATCH net-next v7 4/4] skbuff: Optimization of SKB coalescing
 for page pool

On Mon, Dec 11, 2023 at 11:32 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Sun, 10 Dec 2023 20:21:21 -0800 Mina Almasry wrote:
> > Is it possible/desirable to add a comment to skb_frag_ref() that it
> > should not be used with skb->pp_recycle? At least I was tripped by
> > this, but maybe it's considered obvious somehow.
> >
> > But I feel like this maybe needs to be fixed. Why does the page_pool
> > need a separate page->pp_ref_count? Why not use page->_refcount like
> > the rest of the code? Is there a history here behind this decision
> > that you can point me to? It seems to me that
> > incrementing/decrementing page->pp_ref_count may be equivalent to
> > doing the same on page->_refcount.
>
> Does reading the contents of the comment I proposed here:
> https://lore.kernel.org/all/20231208173816.2f32ad0f@kernel.org/
> elucidate it? The pp_ref_count means the holder is aware that
> they can't release the reference by calling put_page().
> Because (a) we may need to clean up the pp state, unmap DMA etc.
> and (b) one day it may not even be a real page (your work).
>

Thank you, that makes sense.

> TBH I'm partial to the rename from patch 1, so I wouldn't delay this
> work any more :) But you have a point that we should inspect the code
> and consider making the semantics of skb_frag_ref() stronger all by
> itself, without the need to add a new flavor of the helper..
> Are you okay with leaving that as a follow up or do you reckon it's
> easy enough we should push for it now?

I think the rename from pp_frag_count -> pp_ref_count is a huge
improvement, and I think the fact that the netstack has a way to
obtain a reference on a pp frag is also a huge improvement. Please go
ahead mearging this if you like, I was asking questions for my own
education/follow up work to consider.

-- 
Thanks,
Mina