lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Aug 2021 16:58:05 +0200 From: Jesper Dangaard Brouer <jbrouer@...hat.com> To: Yunsheng Lin <linyunsheng@...wei.com>, davem@...emloft.net, kuba@...nel.org Cc: brouer@...hat.com, alexander.duyck@...il.com, linux@...linux.org.uk, mw@...ihalf.com, linuxarm@...neuler.org, yisen.zhuang@...wei.com, salil.mehta@...wei.com, thomas.petazzoni@...tlin.com, hawk@...nel.org, ilias.apalodimas@...aro.org, ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com, akpm@...ux-foundation.org, peterz@...radead.org, will@...nel.org, willy@...radead.org, vbabka@...e.cz, fenghua.yu@...el.com, guro@...com, peterx@...hat.com, feng.tang@...el.com, jgg@...pe.ca, mcroce@...rosoft.com, hughd@...gle.com, jonathan.lemon@...il.com, alobakin@...me, willemb@...gle.com, wenxu@...oud.cn, cong.wang@...edance.com, haokexin@...il.com, nogikh@...gle.com, elver@...gle.com, yhs@...com, kpsingh@...nel.org, andrii@...nel.org, kafai@...com, songliubraving@...com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org, chenhao288@...ilicon.com, Linux-MM <linux-mm@...ck.org> Subject: Re: [PATCH net-next v2 2/4] page_pool: add interface to manipulate frag count in page pool On 06/08/2021 04.46, Yunsheng Lin wrote: > For 32 bit systems with 64 bit dma, dma_addr[1] is used to > store the upper 32 bit dma addr, those system should be rare > those days. > > For normal system, the dma_addr[1] in 'struct page' is not > used, so we can reuse dma_addr[1] for storing frag count, > which means how many frags this page might be splited to. > > In order to simplify the page frag support in the page pool, > the PAGE_POOL_DMA_USE_PP_FRAG_COUNT macro is added to indicate > the 32 bit systems with 64 bit dma, and the page frag support > in page pool is disabled for such system. > > The newly added page_pool_set_frag_count() is called to reserve > the maximum frag count before any page frag is passed to the > user. The page_pool_atomic_sub_frag_count_return() is called > when user is done with the page frag. > > Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com> > --- > include/linux/mm_types.h | 18 +++++++++++++----- > include/net/page_pool.h | 46 +++++++++++++++++++++++++++++++++++++++------- > net/core/page_pool.c | 4 ++++ > 3 files changed, 56 insertions(+), 12 deletions(-) > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 52bbd2b..7f8ee09 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -103,11 +103,19 @@ struct page { > unsigned long pp_magic; > struct page_pool *pp; > unsigned long _pp_mapping_pad; > - /** > - * @dma_addr: might require a 64-bit value on > - * 32-bit architectures. > - */ > - unsigned long dma_addr[2]; > + unsigned long dma_addr; > + union { > + /** > + * dma_addr_upper: might require a 64-bit > + * value on 32-bit architectures. > + */ > + unsigned long dma_addr_upper; > + /** > + * For frag page support, not supported in > + * 32-bit architectures with 64-bit DMA. > + */ > + atomic_long_t pp_frag_count; > + }; > }; > struct { /* slab, slob and slub */ > union { > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index 8d7744d..42e6997 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -45,7 +45,10 @@ > * Please note DMA-sync-for-CPU is still > * device driver responsibility > */ > -#define PP_FLAG_ALL (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV) > +#define PP_FLAG_PAGE_FRAG BIT(2) /* for page frag feature */ > +#define PP_FLAG_ALL (PP_FLAG_DMA_MAP |\ > + PP_FLAG_DMA_SYNC_DEV |\ > + PP_FLAG_PAGE_FRAG) > > /* > * Fast allocation side cache array/stack > @@ -198,19 +201,48 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, > page_pool_put_full_page(pool, page, true); > } > > +#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) > { > - dma_addr_t ret = page->dma_addr[0]; > - if (sizeof(dma_addr_t) > sizeof(unsigned long)) > - ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16; > + dma_addr_t ret = page->dma_addr; > + > + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) > + ret |= (dma_addr_t)page->dma_addr_upper << 16 << 16; I find the macro name confusing. I think it would be easier to read the code, if it was called: PAGE_POOL_DMA_CANNOT_USE_PP_FRAG_COUNT > + > return ret; > } > > static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > { > - page->dma_addr[0] = addr; > - if (sizeof(dma_addr_t) > sizeof(unsigned long)) > - page->dma_addr[1] = upper_32_bits(addr); > + page->dma_addr = addr; > + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) > + page->dma_addr_upper = upper_32_bits(addr); > +} > + > +static inline void page_pool_set_frag_count(struct page *page, long nr) > +{ > + atomic_long_set(&page->pp_frag_count, nr); > +} > + > +static inline long page_pool_atomic_sub_frag_count_return(struct page *page, > + long nr) > +{ > + long ret; > + > + /* As suggested by Alexander, atomic_long_read() may cover up the > + * reference count errors, so avoid calling atomic_long_read() in > + * the cases of freeing or draining the page_frags, where we would > + * not expect it to match or that are slowpath anyway. > + */ > + if (__builtin_constant_p(nr) && > + atomic_long_read(&page->pp_frag_count) == nr) > + return 0; > + > + ret = atomic_long_sub_return(nr, &page->pp_frag_count); > + WARN_ON(ret < 0); > + return ret; > } > > static inline bool is_page_pool_compiled_in(void) > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index 78838c6..68fab94 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -67,6 +67,10 @@ static int page_pool_init(struct page_pool *pool, > */ > } > > + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT && > + pool->p.flags & PP_FLAG_PAGE_FRAG) > + return -EINVAL; I read this as: if the page_pool use pp_frag_count and have flag set, then it is invalid/no-allowed, which seems wrong. I find this code more intuitive to read: + if (PAGE_POOL_DMA_CANNOT_USE_PP_FRAG_COUNT && + pool->p.flags & PP_FLAG_PAGE_FRAG) + return -EINVAL; --Jesper
Powered by blists - more mailing lists