On Tue, 11 May 2021 at 17:19, Matthew Wilcox [off-list ref] wrote:

On Tue, May 11, 2021 at 05:11:13PM +0300, Ilias Apalodimas wrote:

quoted

Hi Matthew,

On Tue, May 11, 2021 at 02:45:32PM +0100, Matthew Wilcox wrote:

quoted

On Tue, May 11, 2021 at 03:31:15PM +0200, Matteo Croce wrote:

quoted

@@ -101,6 +101,7 @@ struct page {
                   * 32-bit architectures.
                   */
                  unsigned long dma_addr[2];
+                 unsigned long signature;
          };
          struct {        /* slab, slob and slub */
                  union {

No.  Signature now aliases with page->mapping, which is going to go
badly wrong for drivers which map this page into userspace.

I had this as:

+                       unsigned long pp_magic;
+                       unsigned long xmi;
+                       unsigned long _pp_mapping_pad;
                        unsigned long dma_addr[2];

and pp_magic needs to be set to something with bits 0&1 clear and
clearly isn't a pointer.  I went with POISON_POINTER_DELTA + 0x40.

Regardless to the changes required, there's another thing we'd like your
opinion on.
There was a change wrt to the previous patchset. We used to store the
struct xdp_mem_info into page->private.  On the new version we store the
page_pool ptr address in page->private (there's an explanation why on the
mail thread, but the tl;dr is that we can get some more speed and keeping
xdp_mem_info is not that crucial). So since we can just store the page_pool
address directly, should we keep using page->private or it's better to
do:

+                       unsigned long pp_magic;
+                       unsigned long pp_ptr;
+                       unsigned long _pp_mapping_pad;
                        unsigned long dma_addr[2];
and use pp_ptr?

I'd rather you didn't use page_private ... Any reason not to use:

                        unsigned long pp_magic;
                        struct page_pool *pp;
                        unsigned long _pp_mapping_pad;
                        unsigned long dma_addr[2];

?

Nope not at all, either would work. we'll switch to that