From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A7ABDC433FE for ; Tue, 14 Dec 2021 06:15:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230382AbhLNGPP (ORCPT ); Tue, 14 Dec 2021 01:15:15 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:50034 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230334AbhLNGPN (ORCPT ); Tue, 14 Dec 2021 01:15:13 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1639462513; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YwugZBIsB9O3b1tEKG2yDl1lfnlqxf61JgJVJ0aWmUQ=; b=Vk2++8tnyfBAP4YnUTj+Dm5WH1aP9yxSgCu5lPLv1iQisZFEJ2QU8AzLsldHM9XQfP9/9n XNGMvo2VNxTwqU5KzsEsfCqp2/+vFGpmTrQXyixte8IlX9VFVihFgMwc1AR/Gkb6chkcpz 2RprZaKhno5bxeXV2GS95msHLpFksbE= Received: from mail-lf1-f71.google.com (mail-lf1-f71.google.com [209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-650-VfGCTCZeNdqngVT5OUYY9g-1; Tue, 14 Dec 2021 01:15:11 -0500 X-MC-Unique: VfGCTCZeNdqngVT5OUYY9g-1 Received: by mail-lf1-f71.google.com with SMTP id 24-20020ac25f58000000b0041799ebf529so8519305lfz.1 for ; Mon, 13 Dec 2021 22:15:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=YwugZBIsB9O3b1tEKG2yDl1lfnlqxf61JgJVJ0aWmUQ=; b=frTMef1gznfLhnNYtWecVr4E6s1W+DaUDhYwJLDC7d/Ku2ARZmvp7jWXoqAZUiyRXL pKJ4BBap4MWIOI/HPs4WjqiI6i2jN2+Q18UzU27xNNoNFOx0hEJ0lTgCB58YEajoZnTJ RAUJzhhaQAmCDFgjuRjHpls5KhV6tRp01RpCax5ZnPBG0mITHBTKafSRRXk+a6akOOqi l6z7hXrStv8s2wbiPJz4GKhK91PrsinWGBCUb3YE5/6qwfSHtjMhZVAIXeb9ZvG5b0l7 NglOA9sfX0xI2Uee4Zsyskg35sQlL8JDej25Kxs1VvQz6MKjPUKNHDMA8R6MCAKkFeQk 3GhA== X-Gm-Message-State: AOAM530Ugm1mgjTPp2llzgWFBwapsaQmMvypBvNN59sI5Lnt40Ni2yHO ZHfTZ6Vo6G/e7EavDgaX4BQrxSN8lLTfFWIPYFX83Jafjz79Ji9Cv0+laNKNmbicK0sEQk76Blf ZrSYnHqKy3WPuu3CjKT4ZOeh2lO6pqOFXfA97o1Fy X-Received: by 2002:a2e:3012:: with SMTP id w18mr2978404ljw.217.1639462510183; Mon, 13 Dec 2021 22:15:10 -0800 (PST) X-Google-Smtp-Source: ABdhPJwqenw88BrR2D6Qt+sGpTwBW3+xZwJrfzDxCVORbTErFH0MhwcnexNkpqOBvtu367RvJ0qUvX4Rn9Ui+iPKu5s= X-Received: by 2002:a2e:3012:: with SMTP id w18mr2978375ljw.217.1639462509908; Mon, 13 Dec 2021 22:15:09 -0800 (PST) MIME-Version: 1.0 References: <20211213045012.12757-1-mengensun@tencent.com> In-Reply-To: From: Jason Wang Date: Tue, 14 Dec 2021 14:14:59 +0800 Message-ID: Subject: Re: [PATCH] virtio-net: make copy len check in xdp_linearize_page To: =?UTF-8?B?5a2Z6JKZ5oGp?= Cc: davem , Jakub Kicinski , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , virtualization , netdev , linux-kernel , bpf@vger.kernel.org, mengensun , MengLong Dong , ZhengXiong Jiang Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 14, 2021 at 11:48 AM =E5=AD=99=E8=92=99=E6=81=A9 wrote: > > Jason Wang =E4=BA=8E2021=E5=B9=B412=E6=9C=8814=E6= =97=A5=E5=91=A8=E4=BA=8C 11:13=E5=86=99=E9=81=93=EF=BC=9A > > > > On Mon, Dec 13, 2021 at 5:14 PM =E5=AD=99=E8=92=99=E6=81=A9 wrote: > > > > > > Jason Wang =E4=BA=8E2021=E5=B9=B412=E6=9C=8813= =E6=97=A5=E5=91=A8=E4=B8=80 15:49=E5=86=99=E9=81=93=EF=BC=9A > > > > > > > > On Mon, Dec 13, 2021 at 12:50 PM wrote: > > > > > > > > > > From: mengensun > > > > > > > > > > xdp_linearize_page asume ring elem size is smaller then page size > > > > > when copy the first ring elem, but, there may be a elem size bigg= er > > > > > then page size. > > > > > > > > > > add_recvbuf_mergeable may add a hole to ring elem, the hole size = is > > > > > not sure, according EWMA. > > > > > > > > The logic is to try to avoid dropping packets in this case, so I > > > > wonder if it's better to "fix" the add_recvbuf_mergeable(). > > > > > > > Adding lists back. > > > > > turn to XDP generic is so difficulty for me, here can "fix" the > > > add_recvbuf_mergeable link follow: > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 36a4b7c195d5..06ce8bb10b47 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -1315,6 +1315,7 @@ static int add_recvbuf_mergeable(struct virtnet= _info *vi, > > > alloc_frag->offset +=3D hole; > > > } > > > + len =3D min(len, PAGE_SIZE - room); > > > sg_init_one(rq->sg, buf, len); > > > ctx =3D mergeable_len_to_ctx(len, headroom); > > > > Then the truesize here is wrong. > many thanks!! i have known i'm wrong immediately after click the > "send" botton , now, this problem seem not only about the *hole* but > the EWMA, EWMA will give buff len bewteen min_buf_len and PAGE_SIZE, > while swith from no-attach-xdp to attach xdp, there may be some buff > already in ring and filled before xdp attach. xdp_linearize_page > assume buf size is PAGE_SIZE - VIRTIO_XDP_HEADROOM, and coped "len" > from the buff, while the buff may be **PAGE_SIZE** So the issue I see is that though add_recvbuf_mergeable() tries to make the buffer size is PAGE_SIZE, XDP might requires more on the header which makes a single page is not sufficient. > > because we have no idear when the user attach xdp prog, so, i have no > idear except make all the buff have a "header hole" len of > VIRTIO_XDP_HEADROOM(128), but it seem so expensive for no-xdp-attach > virtio eth to aways leave 128 byte not used at all. That's an requirement for XDP header adjustment so far. > > this problem is found by review code, in really test, it seemed not so > may big frame. so here we can just "fix" the xdp_linearize_page, make > it trying best to not drop frames for now? I think you can reproduce the issue by keeping sending large frames to guest and try to attach XDP in the middle. > > btw, it seem no way to fix this thoroughly, except we drained all the > buff in the ring, and flush it all to "xdp buff" when attaching xdp > prog. > > is that some mistake i have makeed again? #^_^ I see two ways to solve this: 1) reverse more room (but not headerroom) to make sure PAGE_SIZE can work in the case of linearizing 2) switch to use XDP genreic 2) looks much more easier, you may refer tun_xdp_one() to see how it works, it's as simple as call do_xdp_generic() Thanks > > > > > > > > err =3D virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, = gfp); > > > > > > it seems a rule that, length of elem giving to vring is away smaller > > > or equall then PAGE_SIZE > > > > It aims to be consistent to what EWMA tries to do: > > > > len =3D hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_p= kt_len), > > rq->min_buf_len, PAGE_SIZE - hdr_len); > > > > Thanks > > > > > > > > > > > > > Or another idea is to switch to use XDP generic here where we can u= se > > > > skb_linearize() which should be more robust and we can drop the > > > > xdp_linearize_page() logic completely. > > > > > > > > Thanks > > > > > > > > > > > > > > so, fix it by check copy len,if checked failed, just dropped the > > > > > whole frame, not make the memory dirty after the page. > > > > > > > > > > Signed-off-by: mengensun > > > > > Reviewed-by: MengLong Dong > > > > > Reviewed-by: ZhengXiong Jiang > > > > > --- > > > > > drivers/net/virtio_net.c | 6 +++++- > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index 36a4b7c195d5..844bdbd67ff7 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -662,8 +662,12 @@ static struct page *xdp_linearize_page(struc= t receive_queue *rq, > > > > > int page_off, > > > > > unsigned int *len) > > > > > { > > > > > - struct page *page =3D alloc_page(GFP_ATOMIC); > > > > > + struct page *page; > > > > > > > > > > + if (*len > PAGE_SIZE - page_off) > > > > > + return NULL; > > > > > + > > > > > + page =3D alloc_page(GFP_ATOMIC); > > > > > if (!page) > > > > > return NULL; > > > > > > > > > > -- > > > > > 2.27.0 > > > > > > > > > > > > > > >