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] [thread-next>] [day] [month] [year] [list]

Date: Mon, 11 Dec 2023 11:32:03 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>
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 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).

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?