On Wed, May 31, 2023 at 5:19 AM Yunsheng Lin [off-list ref] wrote:

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

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?

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.

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.

What optimization? From what I can tell you are taking extra steps for
non-page pool pages.

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

You are creating what I call a mono-frag. I am not a huge fan.

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.

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.

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

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.