On 2023/5/31 20:19, Yunsheng Lin wrote:

On 2023/5/30 23:07, Alexander H Duyck wrote:

Hi, Alexander
    Any more comment or concern?
    I feel like we are circling back to v1 about whether it is better
add a new wrapper/API or not and where to do the "(size << 1 > max_size)"
checking. I really like to continue the discussion here instead of in the
new thread again when I post a v3, thanks.

...

quoted

quoted

+	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?

quoted

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?

quoted

quoted

 	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.

quoted

quoted

-	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)'.

quoted

quoted

+		}
+
+		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.

quoted

quoted

+	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


.