| Message ID | 20230105214631.3939268-4-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Split netmem from struct page | expand |
On 05/01/2023 22.46, Matthew Wilcox (Oracle) wrote: > Turn page_pool_set_dma_addr() and page_pool_get_dma_addr() into > wrappers. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > include/net/page_pool.h | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Hi Matthew On Thu, 5 Jan 2023 at 23:46, Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > > Turn page_pool_set_dma_addr() and page_pool_get_dma_addr() into > wrappers. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > include/net/page_pool.h | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index 84b4ea8af015..196b585763d9 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -449,21 +449,31 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, > #define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \ > (sizeof(dma_addr_t) > sizeof(unsigned long)) > > -static inline dma_addr_t page_pool_get_dma_addr(struct page *page) > +static inline dma_addr_t netmem_get_dma_addr(struct netmem *nmem) Ideally, we'd like to avoid having people call these directly and use the page_pool_(get|set)_dma_addr wrappers. Can we add a comment in v3? > { > - dma_addr_t ret = page->dma_addr; > + dma_addr_t ret = nmem->dma_addr; > > if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) > - ret |= (dma_addr_t)page->dma_addr_upper << 16 << 16; > + ret |= (dma_addr_t)nmem->dma_addr_upper << 16 << 16; > > return ret; > } > > -static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > +static inline dma_addr_t page_pool_get_dma_addr(struct page *page) > +{ > + return netmem_get_dma_addr(page_netmem(page)); > +} > + > +static inline void netmem_set_dma_addr(struct netmem *nmem, dma_addr_t addr) > { > - page->dma_addr = addr; > + nmem->dma_addr = addr; > if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) > - page->dma_addr_upper = upper_32_bits(addr); > + nmem->dma_addr_upper = upper_32_bits(addr); > +} > + > +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > +{ > + netmem_set_dma_addr(page_netmem(page), addr); > } > > static inline bool is_page_pool_compiled_in(void) > -- > 2.35.1 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
On Mon, 9 Jan 2023 at 19:30, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Matthew > > On Thu, 5 Jan 2023 at 23:46, Matthew Wilcox (Oracle) > <willy@infradead.org> wrote: > > > > Turn page_pool_set_dma_addr() and page_pool_get_dma_addr() into > > wrappers. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > --- > > include/net/page_pool.h | 22 ++++++++++++++++------ > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > > index 84b4ea8af015..196b585763d9 100644 > > --- a/include/net/page_pool.h > > +++ b/include/net/page_pool.h > > @@ -449,21 +449,31 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, > > #define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \ > > (sizeof(dma_addr_t) > sizeof(unsigned long)) > > > > -static inline dma_addr_t page_pool_get_dma_addr(struct page *page) > > +static inline dma_addr_t netmem_get_dma_addr(struct netmem *nmem) > > Ideally, we'd like to avoid having people call these directly and use > the page_pool_(get|set)_dma_addr wrappers. Can we add a comment in > v3? Ignore this, I just saw the changes in mlx5. This is fine as is > > > { > > - dma_addr_t ret = page->dma_addr; > > + dma_addr_t ret = nmem->dma_addr; > > > > if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) > > - ret |= (dma_addr_t)page->dma_addr_upper << 16 << 16; > > + ret |= (dma_addr_t)nmem->dma_addr_upper << 16 << 16; > > > > return ret; > > } > > > > -static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > > +static inline dma_addr_t page_pool_get_dma_addr(struct page *page) > > +{ > > + return netmem_get_dma_addr(page_netmem(page)); > > +} > > + > > +static inline void netmem_set_dma_addr(struct netmem *nmem, dma_addr_t addr) > > { > > - page->dma_addr = addr; > > + nmem->dma_addr = addr; > > if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) > > - page->dma_addr_upper = upper_32_bits(addr); > > + nmem->dma_addr_upper = upper_32_bits(addr); > > +} > > + > > +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > > +{ > > + netmem_set_dma_addr(page_netmem(page), addr); > > } > > > > static inline bool is_page_pool_compiled_in(void) > > -- > > 2.35.1 > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
On Mon, Jan 09, 2023 at 07:30:30PM +0200, Ilias Apalodimas wrote: > Hi Matthew Hey Ilias. Thanks for all the review! > > -static inline dma_addr_t page_pool_get_dma_addr(struct page *page) > > +static inline dma_addr_t netmem_get_dma_addr(struct netmem *nmem) > > Ideally, we'd like to avoid having people call these directly and use > the page_pool_(get|set)_dma_addr wrappers. Can we add a comment in > v3? I don't think this is what we want. Currently drivers call page_pool_get_dma_addr() on pages that are presumably from the page pool, but the compiler isn't going to help them out if they just get the struct page from somewhere random. They'll get garbage and presumably crash. By returning a netmem pointer from page_pool, we help drivers ensure that they're only passing around memory that was actually allocated from the page_pool and so they won't get garbage if they pass it to netmem_get_dma_addr(). The page_pool_get_dma_addr() wrapper is a temporary measure until we have all the drivers converted to use netmem alone. Does that all make sense, or have I misunderstood what you wanted from a comment?
On Tue, Jan 10, 2023 at 11:17:35AM +0200, Ilias Apalodimas wrote:
> Ignore this, I just saw the changes in mlx5. This is fine as is
I should read everything before replying ;-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h index 84b4ea8af015..196b585763d9 100644 --- a/include/net/page_pool.h +++ b/include/net/page_pool.h @@ -449,21 +449,31 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, #define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \ (sizeof(dma_addr_t) > sizeof(unsigned long)) -static inline dma_addr_t page_pool_get_dma_addr(struct page *page) +static inline dma_addr_t netmem_get_dma_addr(struct netmem *nmem) { - dma_addr_t ret = page->dma_addr; + dma_addr_t ret = nmem->dma_addr; if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) - ret |= (dma_addr_t)page->dma_addr_upper << 16 << 16; + ret |= (dma_addr_t)nmem->dma_addr_upper << 16 << 16; return ret; } -static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) +static inline dma_addr_t page_pool_get_dma_addr(struct page *page) +{ + return netmem_get_dma_addr(page_netmem(page)); +} + +static inline void netmem_set_dma_addr(struct netmem *nmem, dma_addr_t addr) { - page->dma_addr = addr; + nmem->dma_addr = addr; if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) - page->dma_addr_upper = upper_32_bits(addr); + nmem->dma_addr_upper = upper_32_bits(addr); +} + +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) +{ + netmem_set_dma_addr(page_netmem(page), addr); } static inline bool is_page_pool_compiled_in(void)
Turn page_pool_set_dma_addr() and page_pool_get_dma_addr() into wrappers. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- include/net/page_pool.h | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)