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

[Pavel Begunkov <asml.silence@gmail.com>]

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@kernel.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@kernel.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@gmail.com>
>> Signed-off-by: David Wei <dw@davidwei.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@gmail.com>
>> + *	Author:	David Wei <dw@davidwei.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