Re: [PATCH net-next v9 06/20] net: page_pool: add a mp hook to unregister_netdevice*

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

On 1/6/25 21:44, Mina Almasry wrote:
...
>> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
>> index a473ea0c48c4..140fec6857c6 100644
>> --- a/include/net/page_pool/types.h
>> +++ b/include/net/page_pool/types.h
>> @@ -152,12 +152,15 @@ struct page_pool_stats {
>>    */
>>   #define PAGE_POOL_FRAG_GROUP_ALIGN     (4 * sizeof(long))
>>
>> +struct netdev_rx_queue;
>> +
>>   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);
>>          int (*nl_report)(const struct page_pool *pool, struct sk_buff *rsp);
>> +       void (*uninstall)(void *mp_priv, struct netdev_rx_queue *rxq);
> 
> nit: the other params take struct page_pool *pool as input, and find
> the mp_priv in there if they need, it may be nice for consistency to
> continue to pass the entire pool to these ops.

I think so as well, but there is simply no page pool to pass
from where it's called.

> AFAIU this is the first added non-mandatory op, right? Please specify
> it as so, maybe something like:
> 
> /* optional: called when the memory provider is uninstalled from the
> netdev_rx_queue due to the interface going down or otherwise. The
> memory provider may perform whatever cleanup necessary here if it
> needs to. */
> 
>>   };
>>
>>   struct pp_memory_provider_params {
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index c7f3dea3e0eb..aa082770ab1c 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -11464,6 +11464,19 @@ void unregister_netdevice_queue(struct net_device *dev, struct list_head *head)
>>   }
>>   EXPORT_SYMBOL(unregister_netdevice_queue);
>>
>> +static void dev_memory_provider_uninstall(struct net_device *dev)
>> +{
>> +       unsigned int i;
>> +
>> +       for (i = 0; i < dev->real_num_rx_queues; i++) {
>> +               struct netdev_rx_queue *rxq = &dev->_rx[i];
>> +               struct pp_memory_provider_params *p = &rxq->mp_params;
>> +
>> +               if (p->mp_ops && p->mp_ops->uninstall)
> 
> Previous versions of the code checked p->mp_priv to check if the queue
> is mp enabled or not, I guess you check mp_ops and that is set/cleared

The patchset converts it all to mp_ops (v9 missed one place), which is the
right thing to do as only ops are typed and strictly defined.

-- 
Pavel Begunkov