netdev - Re: [PATCH net-next v1] page_pool: check for dma_sync

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]

Message-ID: <14627cec-d54a-4732-8a99-3b1b5757987d@huawei.com>
Date: Mon, 14 Oct 2024 20:38:55 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Furong Xu <0x1207@...il.com>
CC: Ilias Apalodimas <ilias.apalodimas@...aro.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Jesper Dangaard Brouer <hawk@...nel.org>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<xfr@...look.com>
Subject: Re: [PATCH net-next v1] page_pool: check for dma_sync_size earlier

On 2024/10/14 14:35, Furong Xu wrote:
> Hi Yunsheng,
> 
> On Sat, 12 Oct 2024 14:14:41 +0800, Yunsheng Lin <linyunsheng@...wei.com> wrote:
> 
>> I would prefer to add a new api to do that, as it makes the semantic
>> more obvious and may enable removing some checking in the future.
>>
>> And we may need to disable this 'feature' for frag relate API for now,
>> as currently there may be multi callings to page_pool_put_netmem() for
>> the same page, and dma_sync is only done for the last one, which means
>> it might cause some problem for those usecases when using frag API.
> 
> I am not an expert on page_pool.
> So would you mind sending a new patch to add a non-dma-sync version of
> page_pool_put_page() and CC it to me?

As I have at least two patchsets pending for the net-next, which seems
it might take a while, so it might take a while for me to send another
new patch.

Perhaps just add something like page_pool_put_page_nosync() as
page_pool_put_full_page() does for the case of dma_sync_size being
-1? and leave removing of extra checking as later refactoring and
optimization.

As for the frag related API like page_pool_alloc_frag() and
page_pool_alloc(), we don't really have a corresponding free side
API for them, instead we reuse page_pool_put_page() for the free
side, and don't really do any dma sync unless it is the last frag
user of the same page, see the page_pool_is_last_ref() checking in
page_pool_put_netmem().

So it might require more refactoring to support the usecase of
this patch for frag API, for example we might need to pull the
dma_sync operation out of __page_pool_put_page(), and put it in
page_pool_put_netmem() so that dma_sync is also done for the
non-last frag user too.
Or not support it for frag API for now as stmmac driver does not
seem to be using frag API, and put a warning to catch the case of
misusing of the 'feature' for frag API in the 'if' checking in
page_pool_put_netmem() before returning? something like below:

--- a/include/net/page_pool/helpers.h
+++ b/include/net/page_pool/helpers.h
@@ -317,8 +317,10 @@ static inline void page_pool_put_netmem(struct page_pool *pool,
         * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
         */
 #ifdef CONFIG_PAGE_POOL
-       if (!page_pool_is_last_ref(netmem))
+       if (!page_pool_is_last_ref(netmem)) {
+		/* Big comment why frag API is not support yet */
+               DEBUG_NET_WARN_ON_ONCE(!dma_sync_size);
                return;
+       }

        page_pool_put_unrefed_netmem(pool, netmem, dma_sync_size, allow_direct);
 #endif

> I am so glad to test it on my device ;)
> Thanks.
> 
>