Re: [PATCH net-next v3 09/12] iavf: switch to Page Pool

[Alexander Lobakin]

From: Alexander Duyck <alexander.duyck@xxxxxxxxx>
Date: Fri, 2 Jun 2023 11:00:07 -0700

> On Fri, Jun 2, 2023 at 9:31 AM Alexander Lobakin
> <aleksander.lobakin@xxxxxxxxx> wrote:

[...]

>>> Not a fan of this switching back and forth between being a page pool
>>> pointer and a dev pointer. Seems problematic as it is easily
>>> misinterpreted. I would say that at a minimum stick to either it is
>>> page_pool(Rx) or dev(Tx) on a ring type basis.
>>
>> The problem is that page_pool has lifetime from ifup to ifdown, while
>> its ring lives longer. So I had to do something with this, but also I
>> didn't want to have 2 pointers at the same time since it's redundant and
>> +8 bytes to the ring for nothing.
> 
> It might be better to just go with NULL rather than populating it w/
> two different possible values. Then at least you know if it is an
> rx_ring it is a page_pool and if it is a tx_ring it is dev. You can
> reset to the page pool when you repopulate the rest of the ring.

IIRC I did that to have struct device pointer at the moment of creating
page_pools. But sounds reasonable, I'll take a look.

> 
>>> This setup works for iavf, however for i40e/ice you may run into issues
>>> since the setup_rx_descriptors call is also used to setup the ethtool
>>> loopback test w/o a napi struct as I recall so there may not be a
>>> q_vector.
>>
>> I'll handle that. Somehow :D Thanks for noticing, I'll take a look
>> whether I should do something right now or it can be done later when
>> switching the actual mentioned drivers.
>>
>> [...]
>>
>>>> @@ -240,7 +237,10 @@ struct iavf_rx_queue_stats {
>>>>  struct iavf_ring {
>>>>      struct iavf_ring *next;         /* pointer to next ring in q_vector */
>>>>      void *desc;                     /* Descriptor ring memory */
>>>> -    struct device *dev;             /* Used for DMA mapping */
>>>> +    union {
>>>> +            struct page_pool *pool; /* Used for Rx page management */
>>>> +            struct device *dev;     /* Used for DMA mapping on Tx */
>>>> +    };
>>>>      struct net_device *netdev;      /* netdev ring maps to */
>>>>      union {
>>>>              struct iavf_tx_buffer *tx_bi;
>>>
>>> Would it make more sense to have the page pool in the q_vector rather
>>> than the ring? Essentially the page pool is associated per napi
>>> instance so it seems like it would make more sense to store it with the
>>> napi struct rather than potentially have multiple instances per napi.
>>
>> As per Page Pool design, you should have it per ring. Plus you have
>> rxq_info (XDP-related structure), which is also per-ring and
>> participates in recycling in some cases. So I wouldn't complicate.
>> I went down the chain and haven't found any place where having more than
>> 1 PP per NAPI would break anything. If I got it correctly, Jakub's
>> optimization discourages having 1 PP per several NAPIs (or scheduling
>> one NAPI on different CPUs), but not the other way around. The goal was
>> to exclude concurrent access to one PP from different threads, and here
>> it's impossible.
> 
> The xdp_rxq can be mapped many:1 to the page pool if I am not mistaken.
> 
> The only reason why I am a fan of trying to keep the page_pool tightly
> associated with the napi instance is because the napi instance is what
> essentially is guaranteeing the page_pool is consistent as it is only
> accessed by that one napi instance.

Here we can't have more than one NAPI instance accessing one page_pool,
so I did that unconditionally. I'm a fan of what you've said, too :p

> 
>> Lemme know. I can always disable NAPI optimization for cases when one
>> vector is shared by several queues -- and it's not a usual case for
>> these NICs anyway -- but I haven't found a reason for that.
> 
> I suppose we should be fine if we have a many to one mapping though I
> suppose. As you said the issue would be if multiple NAPI were
> accessing the same page pool.

Thanks,
Olek