Messages in this thread | | | From | Alexander Duyck <> | Date | Thu, 1 Jun 2023 11:14:45 -0700 | Subject | Re: [PATCH net-next v2 2/3] page_pool: support non-frag page for page_pool_alloc_frag() |
| |
On Wed, May 31, 2023 at 5:19 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > On 2023/5/30 23:07, Alexander H Duyck wrote: > ... > > >> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) { > >> + *offset = 0; > >> + return page_pool_alloc_pages(pool, gfp); > >> + } > >> + > > > > This is a recipe for pain. Rather than doing this I would say we should > > stick with our existing behavior and not allow page pool fragments to > > be used when the DMA address is consuming the region. Otherwise we are > > going to make things very confusing. > > Are there any other concern other than confusing? we could add a > big comment to make it clear. > > The point of adding that is to avoid the driver handling the > PAGE_POOL_DMA_USE_PP_FRAG_COUNT when using page_pool_alloc_frag() > like something like below: > > if (!PAGE_POOL_DMA_USE_PP_FRAG_COUNT) > page = page_pool_alloc_frag() > else > page = XXXXX; > > Or do you perfer the driver handling it? why? > > > > > If we have to have both version I would much rather just have some > > inline calls in the header wrapped in one #ifdef for > > PAGE_POOL_DMA_USE_PP_FRAG_COUNT that basically are a wrapper for > > page_pool pages treated as pp_frag. > > Do you have a good name in mind for that wrapper. > In addition to the naming, which API should I use when I am a driver > author wanting to add page pool support?
When I usually have to deal with these sort of things I just rename the original with a leading underscore or two and then just name the inline the same as the original function.
> > > >> size = ALIGN(size, dma_get_cache_alignment()); > >> - *offset = pool->frag_offset; > >> > > > > If we are going to be allocating mono-frag pages they should be > > allocated here based on the size check. That way we aren't discrupting > > the performance for the smaller fragments and the code below could > > function undisturbed. > > It is to allow possible optimization as below.
What optimization? From what I can tell you are taking extra steps for non-page pool pages.
> > > >> - if (page && *offset + size > max_size) { > >> + if (page) { > >> + *offset = pool->frag_offset; > >> + > >> + if (*offset + size <= max_size) { > >> + pool->frag_users++; > >> + pool->frag_offset = *offset + size; > >> + alloc_stat_inc(pool, fast); > >> + return page; > > Note that we still allow frag page here when '(size << 1 > max_size)'.
You are creating what I call a mono-frag. I am not a huge fan.
> >> + } > >> + > >> + pool->frag_page = NULL; > >> page = page_pool_drain_frag(pool, page); > >> if (page) { > >> alloc_stat_inc(pool, fast); > >> @@ -714,26 +727,24 @@ struct page *page_pool_alloc_frag(struct page_pool *pool, > >> } > >> } > >> > >> - if (!page) { > >> - page = page_pool_alloc_pages(pool, gfp); > >> - if (unlikely(!page)) { > >> - pool->frag_page = NULL; > >> - return NULL; > >> - } > >> - > >> - pool->frag_page = page; > >> + page = page_pool_alloc_pages(pool, gfp); > >> + if (unlikely(!page)) > >> + return NULL; > >> > >> frag_reset: > >> - pool->frag_users = 1; > >> + /* return page as non-frag page if a page is not able to > >> + * hold two frags for the current requested size. > >> + */ > > > > This statement ins't exactly true since you make all page pool pages > > into fragmented pages. > > Any suggestion to describe it more accurately? > I wrote that thinking frag_count being one as non-frag page.
I wouldn't consider that to be the case. The problem is if frag count == 1 then you have a fragmented page. It is no different from a page where you had either freed earlier instances.
> > > > > >> + if (unlikely(size << 1 > max_size)) { > > > > This should happen much sooner so you aren't mixing these allocations > > with the smaller ones and forcing the fragmented page to be evicted. > > As mentioned above, it is to allow a possible optimization
Maybe you should point out exactly what you think the optimization is. I don't see it as such. If you are going to evict anything that has a size that is over half your max_size then you might as well just skip using this entirely and just output a non-fragmented/mono frag page rather than evicting the previously fragmented page.
| |