Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230526054621.18371-1-liangchen.linux@gmail.com>
 <20230526054621.18371-3-liangchen.linux@gmail.com> <992eb402-43f9-0289-f95c-b41cb17f2e59@huawei.com>
In-Reply-To: <992eb402-43f9-0289-f95c-b41cb17f2e59@huawei.com>
From:   Liang Chen <liangchen.linux@gmail.com>
Date:   Mon, 29 May 2023 15:30:33 +0800
Message-ID: <CAKhg4tL9PrUebqQHL+s7A6-xqNnju3erNQejMr7UFjwTaOduZw@mail.gmail.com>
Subject: Re: [PATCH net-next 3/5] virtio_net: Add page pool fragmentation support
To:     Yunsheng Lin <linyunsheng@huawei.com>
Cc:     jasowang@redhat.com, mst@redhat.com,
        virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, xuanzhuo@linux.alibaba.com,
        kuba@kernel.org, edumazet@google.com, davem@davemloft.net,
        pabeni@redhat.com, alexander.duyck@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, May 29, 2023 at 9:33=E2=80=AFAM Yunsheng Lin <linyunsheng@huawei.co=
m> wrote:
>
> On 2023/5/26 13:46, Liang Chen wrote:
>
> ...
>
> > 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.

> >
>
> ...
>
> > @@ -1769,13 +1788,29 @@ static int add_recvbuf_mergeable(struct virtnet=
_info *vi,
> >        */
> >       len =3D 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_p=
ool,
> > +                                                            &pp_frag_o=
ffset, len + room)))
> > +                             return -ENOMEM;
> > +                     buf =3D (char *)page_address(rq->page_pool->frag_=
page) +
> > +                             pp_frag_offset;
> > +                     buf +=3D headroom; /* advance address leaving hol=
e at front of pkt */
> > +                     hole =3D (PAGE_SIZE << rq->page_pool->p.order)
> > +                             - rq->page_pool->frag_offset;
> > +                     if (hole < len + room) {
> > +                             if (!headroom)
> > +                                     len +=3D hole;
> > +                             rq->page_pool->frag_offset +=3D hole;
>
> Is there any reason why the driver need to be aware of page_pool->frag_of=
fset?
> Isn't the page_pool_dev_alloc_frag() will drain the last page for you whe=
n
> 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 repo=
rt
> for skb, but I am not sure it matters that much as 'struct page_frag_cach=
e'
> 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.

> > +                     }
> > +             } else {
> > +                     struct page *page;
> >
> > -             page =3D page_pool_dev_alloc_pages(rq->page_pool);
> > -             if (unlikely(!page))
> > -                     return -ENOMEM;
> > -             buf =3D (char *)page_address(page);
> > -             buf +=3D headroom; /* advance address leaving hole at fro=
nt of pkt */
> > +                     page =3D page_pool_dev_alloc_pages(rq->page_pool)=
;
> > +                     if (unlikely(!page))
> > +                             return -ENOMEM;
> > +                     buf =3D (char *)page_address(page);
> > +                     buf +=3D headroom; /* advance address leaving hol=
e 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 rece=
ive_queue *rq)
> >       struct virtio_device *vdev =3D rq->vq->vdev;
> >
> >       struct page_pool_params pp_params =3D {
> > -             .order =3D 0,
> > +             .order =3D page_pool_frag ? SKB_FRAG_PAGE_ORDER : 0,
> >               .pool_size =3D 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.


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