On Mon, 11 Feb 2019 08:55:51 -0800
Matthew Wilcox <wi...@infradead.org> wrote:
> On Mon, Feb 11, 2019 at 05:06:46PM +0100, Jesper Dangaard Brouer wrote:
> > The page_pool API is using page->private to store DMA addresses.
> > As pointed out by David Miller we can't use that on 32-bit architectures
> > with 64-bit DMA
> >
> > This patch adds a new dma_addr_t struct to allow storing DMA addresses
> >
> > Signed-off-by: Jesper Dangaard Brouer <bro...@redhat.com>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
>
> Reviewed-by: Matthew Wilcox <wi...@infradead.org>
>
> > + struct { /* page_pool used by netstack */
> > + /**
> > + * @dma_addr: Page_pool need to store DMA-addr, and
>
> s/need/needs/
>
> > + * cannot use @private, as DMA-mappings can be 64-bit
>
> s/DMA-mappings/DMA addresses/
>
> > + * even on 32-bit Architectures.
>
> s/A/a/
Yes, that comments needs improvement. I think I'll use AKPMs suggestion.
> > + */
> > + dma_addr_t dma_addr; /* Shares area with @lru */
>
> It also shares with @slab_list, @next, @compound_head, @pgmap and
> @rcu_head. I think it's pointless to try to document which other fields
> something shares space with; the places which do it are a legacy from
> before I rearranged struct page last year. Anyone looking at this should
> now be able to see "Oh, this is a union, only use the fields which are
> in the union for the type of struct page I have here".
I agree, I'll strip that comment.
> Are the pages allocated from this API ever supposed to be mapped to
> userspace?
I would like to know what fields on struct-page we cannot touch if we
want to keep this a possibility?
That said, I hope we don't need to do this. But as I integrate this
further into the netstack code, we might have to support this, or
at-least release the page_pool "state" (currently only DMA-addr) before
the skb_zcopy code path. First iteration will not do zero-copy stuff,
and later I'll coordinate with Willem how to add this, if needed.
My general opinion is that if an end-user want to have pages mapped to
userspace, then page_pool (MEM_TYPE_PAGE_POOL) is not the right choice,
but instead use MEM_TYPE_ZERO_COPY (see enum xdp_mem_type). We are
generally working towards allowing NIC drivers to have a different
memory type per RX-ring.
> You also say in the documentation:
>
> * If no DMA mapping is done, then it can act as shim-layer that
> * fall-through to alloc_page. As no state is kept on the page, the
> * regular put_page() call is sufficient.
>
> I think this is probably a dangerous precedent to set. Better to require
> exactly one call to page_pool_put_page() (with the understanding that the
> refcount may be elevated, so this may not be the final free of the page,
> but the page will no longer be usable for its page_pool purpose).
Yes, this actually how it is implemented today, and the comment should
be improved. Today __page_pool_put_page() in case of refcount is
elevated do call __page_pool_clean_page() to release page page_pool
state, and is in principle no longer "usable" for page_pool purposes.
BUT I have considered removing this, as it might not fit how want to
use the API. In our current RFC we found a need for (and introduced) a
page_pool_unmap_page() call (that call __page_pool_clean_page()), when
driver hits cases where the code path doesn't have a call-back to
page_pool_put_page() but instead end-up calling put_page().
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
