On 2025-01-06 13:44, Mina Almasry wrote:
> On Tue, Dec 17, 2024 at 4:38 PM David Wei <dw@xxxxxxxxxxx> wrote:
>>
>> From: Pavel Begunkov <asml.silence@xxxxxxxxx>
>>
>> Devmem TCP needs a hook in unregister_netdevice_many_notify() to upkeep
>> the set tracking queues it's bound to, i.e. ->bound_rxqs. Instead of
>> devmem sticking directly out of the genetic path, add a mp function.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
>> Signed-off-by: David Wei <dw@xxxxxxxxxxx>
>> ---
>> include/net/page_pool/types.h | 3 +++
>> net/core/dev.c | 15 ++++++++++++++-
>> net/core/devmem.c | 36 ++++++++++++++++-------------------
>> net/core/devmem.h | 5 -----
>> 4 files changed, 33 insertions(+), 26 deletions(-)
>>
>> 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.
>
> 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. */
Sounds good, I'll make the change in the next version.
>
>> };
>>
>> 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
> along with p->mp_priv. I guess that is fine. It would have been nicer,
> maybe, to have all the mp stuff behind one pointer/struct. I would
> dread some code path setting one of mp_[ops|priv] but forgetting to
> set the other... :shrug:
We can follow up with helpers that do the set/unset/check to make sure
it is consistent.
>
> Anyhow:
>
> Reviewed-by: Mina Almasry <almasrymina@xxxxxxxxxx>
>
