On Mon, May 29, 2023 at 9:33 AM Yunsheng Lin [off-list ref] wrote:

On 2023/5/26 13:46, Liang Chen wrote:

...

quoted

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 99c0ca0c1781..ac40b8c66c59 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -32,7 +32,9 @@ module_param(gso, bool, 0444);
 module_param(napi_tx, bool, 0644);

 static bool page_pool_enabled;
+static bool page_pool_frag;
 module_param(page_pool_enabled, bool, 0400);
+module_param(page_pool_frag, bool, 0400);

The below patchset unifies the frag and non-frag page for
page_pool_alloc_frag() API, perhaps it would simplify the
driver's support of page pool.

https://patchwork.kernel.org/project/netdevbpf/cover/20230526092616.40355-1-linyunsheng@huawei.com/

Thanks for the information and the work to make driver support easy. I
will rebase accordingly after it lands.

quoted

...

quoted

@@ -1769,13 +1788,29 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
       */
      len = get_mergeable_buf_len(rq, &rq->mrg_avg_pkt_len, room);
      if (rq->page_pool) {
-             struct page *page;
+             if (rq->page_pool->p.flags & PP_FLAG_PAGE_FRAG) {
+                     if (unlikely(!page_pool_dev_alloc_frag(rq->page_pool,
+                                                            &pp_frag_offset, len + room)))
+                             return -ENOMEM;
+                     buf = (char *)page_address(rq->page_pool->frag_page) +
+                             pp_frag_offset;
+                     buf += headroom; /* advance address leaving hole at front of pkt */
+                     hole = (PAGE_SIZE << rq->page_pool->p.order)
+                             - rq->page_pool->frag_offset;
+                     if (hole < len + room) {
+                             if (!headroom)
+                                     len += hole;
+                             rq->page_pool->frag_offset += hole;

Is there any reason why the driver need to be aware of page_pool->frag_offset?
Isn't the page_pool_dev_alloc_frag() will drain the last page for you when
page_pool_dev_alloc_frag() is called with size being 'len + room' later?
One case I can think of needing this is to have an accurate truesize report
for skb, but I am not sure it matters that much as 'struct page_frag_cache'
and 'page_frag' implementation both have a similar problem.

Yeah, as you pointed out page_pool_dev_alloc_frag will drain the page
itself, so does skb_page_frag_refill. This is trying to keep the logic
consistent with non page pool case where the hole was skipped and
included in buffer len.

quoted

+                     }
+             } else {
+                     struct page *page;

-             page = page_pool_dev_alloc_pages(rq->page_pool);
-             if (unlikely(!page))
-                     return -ENOMEM;
-             buf = (char *)page_address(page);
-             buf += headroom; /* advance address leaving hole at front of pkt */
+                     page = page_pool_dev_alloc_pages(rq->page_pool);
+                     if (unlikely(!page))
+                             return -ENOMEM;
+                     buf = (char *)page_address(page);
+                     buf += headroom; /* advance address leaving hole at front of pkt */
+             }
      } else {
              if (unlikely(!skb_page_frag_refill(len + room, alloc_frag, gfp)))
                      return -ENOMEM;

@@ -3800,13 +3835,16 @@ static void virtnet_alloc_page_pool(struct receive_queue *rq)
      struct virtio_device *vdev = rq->vq->vdev;

      struct page_pool_params pp_params = {
-             .order = 0,
+             .order = page_pool_frag ? SKB_FRAG_PAGE_ORDER : 0,
              .pool_size = rq->vq->num_max,

If it using order SKB_FRAG_PAGE_ORDER page, perhaps pool_size does
not have to be rq->vq->num_max? Even for order 0 page, perhaps the
pool_size does not need to be as big as rq->vq->num_max?

Thanks for pointing this out! pool_size will be lowered to a more
appropriate value on v2.

quoted

              .nid = dev_to_node(vdev->dev.parent),
              .dev = vdev->dev.parent,
              .offset = 0,
      };

+     if (page_pool_frag)
+             pp_params.flags |= PP_FLAG_PAGE_FRAG;
+
      rq->page_pool = page_pool_create(&pp_params);
      if (IS_ERR(rq->page_pool)) {
              dev_warn(&vdev->dev, "page pool creation failed: %ld\n",