netdev - Re: [PATCH net-next v1] net: page_pool: add page_pool_put_page

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: <c19f8761-1642-45a5-b05b-c880fb4ff3ad@intel.com>
Date: Thu, 19 Dec 2024 17:01:45 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Guowei Dang <guowei.dang@...mail.com>
CC: Jakub Kicinski <kuba@...nel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <netdev@...r.kernel.org>, Jesper Dangaard Brouer
	<hawk@...nel.org>, Ilias Apalodimas <ilias.apalodimas@...aro.org>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Paolo
 Abeni" <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Jonathan Corbet
	<corbet@....net>, Yunsheng Lin <linyunsheng@...wei.com>, Furong Xu
	<0x1207@...il.com>
Subject: Re: [PATCH net-next v1] net: page_pool: add
 page_pool_put_page_nosync()

From: Jakub Kicinski <kuba@...nel.org>
Date: Thu, 19 Dec 2024 06:24:38 -0800

(to the author of the patch)

> On Thu, 19 Dec 2024 11:11:38 +0800 Guowei Dang wrote:
>> Add page_pool_put_page_nosync() to respond to dma_sync_size being 0.

If PP_FLAG_DMA_SYNC_DEV is set, dma_sync_size == 0 can happen only when
the HW didn't write anything *and* the driver uses only one page per
frame, no frags. Very unlikely case I'd say, adding a separate wrapper
for it makes no sense.

>>
>> The purpose of this is to make the semantics more obvious and may
>> enable removing some checkings in the future.

Which checks do you want to remove?

>>
>> And in the long term, treating the nosync scenario separately provides
>> more flexibility for the user and enable removing of the
>> PP_FLAG_DMA_SYNC_DEV in the future.

Why remove SYNC_DEV?

>>
>> Since we do have a page_pool_put_full_page(), adding a variant for
>> the nosync seems reasonable.

Not really. put_full_page() is for cases when either the HW-written size
is unknown or the driver uses frags, those are common and widely-used.

> 
> You should provide an upstream user with the API.

Would be nice to see a real example as I don't understand the purpose of
this function as well.

> But IMHO this just complicates the already very large API, 
> for little benefit. 
> I'm going to leave this in patchwork for a day in case page
> pool maintainers disagree, but I vote "no".

I don't see a reason for this either.

Thanks,
Olek