netdev - Re: [PATCH net-next v10 06/22] net: page_pool: create hooks for custom memory providers

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: <e5620d54-377b-414d-8e6a-447cd9c51307@gmail.com>
Date: Thu, 16 Jan 2025 02:25:41 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, David Wei <dw@...idwei.uk>
Cc: io-uring@...r.kernel.org, netdev@...r.kernel.org,
 Jens Axboe <axboe@...nel.dk>, Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jesper Dangaard Brouer <hawk@...nel.org>, David Ahern <dsahern@...nel.org>,
 Mina Almasry <almasrymina@...gle.com>,
 Stanislav Fomichev <stfomichev@...il.com>, Joe Damato <jdamato@...tly.com>,
 Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH net-next v10 06/22] net: page_pool: create hooks for
 custom memory providers

On 1/16/25 00:44, Jakub Kicinski wrote:
> On Wed,  8 Jan 2025 14:06:27 -0800 David Wei wrote:
>> From: Jakub Kicinski <kuba@...nel.org>
>>
>> A spin off from the original page pool memory providers patch by Jakub,
>> which allows extending page pools with custom allocators. One of such
>> providers is devmem TCP, and the other is io_uring zerocopy added in
>> following patches.
>>
>> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> 
> Something odd with authorship here. You list me as author (From)
> but didn't add my SoB. Maybe add something like "Based on
> earlier work by Jakub" to the commit and reset the tags?

The intention was to change the author (failed) and put it as
suggested-by since you said before you don't care and changes pile
up, and even modification notes got deleted for unknown to me
reasons in v9. Based-on, Co-authored also sound good if you
have a preference.

> Or the Suggested-by is just for the warning on ops not being built in?
> 
>> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
>> Signed-off-by: David Wei <dw@...idwei.uk>
>> ---
>>   include/net/page_pool/memory_provider.h | 20 ++++++++++++++++++++
>>   include/net/page_pool/types.h           |  4 ++++
>>   net/core/devmem.c                       | 15 ++++++++++++++-
>>   net/core/page_pool.c                    | 23 +++++++++++++++--------
>>   4 files changed, 53 insertions(+), 9 deletions(-)
>>   create mode 100644 include/net/page_pool/memory_provider.h
>>
>> diff --git a/include/net/page_pool/memory_provider.h b/include/net/page_pool/memory_provider.h
>> new file mode 100644
>> index 000000000000..79412a8714fa
>> --- /dev/null
>> +++ b/include/net/page_pool/memory_provider.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * page_pool/memory_provider.h
>> + *	Author:	Pavel Begunkov <asml.silence@...il.com>
>> + *	Author:	David Wei <dw@...idwei.uk>
> 
> Not a customary thing in networking to list authors in comments.

I'm not used to that, but _all_ networking files I quickly looked
through before adding that had authors listed including some new
ones. Maybe I was just too lucky. I can kill it.

>> + */
>> +#ifndef _NET_PAGE_POOL_MEMORY_PROVIDER_H
>> +#define _NET_PAGE_POOL_MEMORY_PROVIDER_H
>> +
>> +#include <net/netmem.h>
>> +#include <net/page_pool/types.h>
> 
> No need? All you need is forward declarations for types at this stage.

That's getting extremely nit picky. I don't see why it's preferable
forward declaring all structures instead of including a "types.h"
file. Not like that matters or there is a dependency problem. Even
from your comments to v18, the list would likely need to grow with
an mp_params declaration.

>> +struct memory_provider_ops {
>> +	netmem_ref (*alloc_netmems)(struct page_pool *pool, gfp_t gfp);
>> +	bool (*release_netmem)(struct page_pool *pool, netmem_ref netmem);
>> +	int (*init)(struct page_pool *pool);
>> +	void (*destroy)(struct page_pool *pool);
>> +};
>> +
>> +#endif
> 
> Rest LGTM.

-- 
Pavel Begunkov