From: Alexander Duyck <redacted>
Date: Fri, 2 Jun 2023 11:00:07 -0700

On Fri, Jun 2, 2023 at 9:31 AM Alexander Lobakin
[off-list ref] wrote:

[...]

quoted

quoted

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.

quoted

quoted

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.

[...]

quoted

quoted

@@ -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

quoted

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