Message-ID: <87cyeqml3d.fsf@toke.dk>
Date: Sun, 09 Mar 2025 13:42:30 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Mina Almasry <almasrymina@...gle.com>, Pavel Begunkov
 <asml.silence@...il.com>, David Wei <dw@...idwei.uk>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Jesper Dangaard Brouer
 <hawk@...nel.org>, Ilias Apalodimas <ilias.apalodimas@...aro.org>, "David
 S. Miller" <davem@...emloft.net>, Yunsheng Lin <linyunsheng@...wei.com>,
 Yonglong Liu <liuyonglong@...wei.com>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon
 Horman <horms@...nel.org>, linux-mm@...ck.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next] page_pool: Track DMA-mapped pages and
 unmap them when destroying the pool

Mina Almasry <almasrymina@...gle.com> writes:

> On Sat, Mar 8, 2025 at 6:55 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> When enabling DMA mapping in page_pool, pages are kept DMA mapped until
>> they are released from the pool, to avoid the overhead of re-mapping the
>> pages every time they are used. This causes problems when a device is
>> torn down, because the page pool can't unmap the pages until they are
>> returned to the pool. This causes resource leaks and/or crashes when
>> there are pages still outstanding while the device is torn down, because
>> page_pool will attempt an unmap of a non-existent DMA device on the
>> subsequent page return.
>>
>> To fix this, implement a simple tracking of outstanding dma-mapped pages
>> in page pool using an xarray. This was first suggested by Mina[0], and
>> turns out to be fairly straight forward: We simply store pointers to
>> pages directly in the xarray with xa_alloc() when they are first DMA
>> mapped, and remove them from the array on unmap. Then, when a page pool
>> is torn down, it can simply walk the xarray and unmap all pages still
>> present there before returning, which also allows us to get rid of the
>> get/put_device() calls in page_pool.
>
> THANK YOU!! I had been looking at the other proposals to fix this here
> and there and I had similar feelings to you. They add lots of code
> changes and the code changes themselves were hard for me to
> understand. I hope we can make this simpler approach work.

You're welcome :)
And yeah, me too!

>> Using xa_cmpxchg(), no additional
>> synchronisation is needed, as a page will only ever be unmapped once.
>>
>
> Very clever. I had been wondering how to handle the concurrency. I
> also think this works.

Thanks!

>> To avoid having to walk the entire xarray on unmap to find the page
>> reference, we stash the ID assigned by xa_alloc() into the page
>> structure itself, in the field previously called '_pp_mapping_pad' in
>> the page_pool struct inside struct page. This field overlaps with the
>> page->mapping pointer, which may turn out to be problematic, so an
>> alternative is probably needed. Sticking the ID into some of the upper
>> bits of page->pp_magic may work as an alternative, but that requires
>> further investigation. Using the 'mapping' field works well enough as
>> a demonstration for this RFC, though.
>>
>
> I'm unsure about this. I think page->mapping may be used when we map
> the page to the userspace in TCP zerocopy, but I'm really not sure.
> Yes, finding somewhere else to put the id would be ideal. Do we really
> need a full unsigned long for the pp_magic?

No, pp_magic was also my backup plan (see the other thread). Tried
actually doing that now, and while there's a bit of complication due to
the varying definitions of POISON_POINTER_DELTA across architectures,
but it seems that this can be defined at compile time. I'll send a v2
RFC with this change.

-Toke

Powered by blists - more mailing lists