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 6913CC19F2C for ; Mon, 1 Aug 2022 06:28:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237481AbiHAG21 (ORCPT ); Mon, 1 Aug 2022 02:28:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56040 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229589AbiHAG20 (ORCPT ); Mon, 1 Aug 2022 02:28:26 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id AEAC91260D for ; Sun, 31 Jul 2022 23:28:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1659335303; 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=UyyYryD7QGgOS3XQPZyq6yB64pJtotrkijoRHszU24s=; b=i6QYPd7G0jsn2Ch+H+4m8Y4f78Re+wYDvzBuPtJcuY8xsqNhisz2MiGufUvnxQV6GlyYZC OgOxH+g+FHbeIo7X6qhRb7TqNdBY9NgukuKkSnYU9FPjB0343pQY0Ie+AIZ9lV2dmp3cB4 ex20+Sn7NFkhqMvojL7ypVGFY6H9Zhw= Received: from mail-lf1-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-204-5jWFdIj7M0WZE7BuSo6C_A-1; Mon, 01 Aug 2022 02:27:42 -0400 X-MC-Unique: 5jWFdIj7M0WZE7BuSo6C_A-1 Received: by mail-lf1-f72.google.com with SMTP id z3-20020a19f703000000b0048ae517ee7bso1714975lfe.14 for ; Sun, 31 Jul 2022 23:27:42 -0700 (PDT) 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=UyyYryD7QGgOS3XQPZyq6yB64pJtotrkijoRHszU24s=; b=abfPPANmfG8vVkZYa95lEd4Vzvt6dFAPPUD2zRHUjNC9d7U5hACnvSvqY1LDPUs93b +6TO6M6iQqTmsztA3hzIo4TjW7/plfYSVqFFZahD4xtETRW01mAnZwT6mJJJPfhqFQMT VfwwOKeqKDJOaI0rAxoT9gEtpqMl9NqzAwjQSD4g4tWpzKMu5ofAIXtxXqn2XQrqv/Pk 1kGLosIR32TYmyGfx/pnvmB5KrMYAo573+PhD9N0H5gHhxct/Wi6E7G4EUHOqqZLSlKW /Gjf97MB84fO7z70tTooUELynoeg0VmWuzKI06NPgioB5ICvPQj3ExNJCtgjTKu/5jsX okgg== X-Gm-Message-State: ACgBeo3gsNyWm/FFcmVZtqo86BOTN+8XAJIxF3n2+QDk1OK5xigXbppn 799h1RnlMdgQ3epU+mnr2Rn/Z9nvYqmELQTXZr60iBN1A1+O7uvBzvqDMcjlLVC35Lt+4XOE4zi /B9UiyGewV1TVZZnxaFSQu97VC24XTcONzN7H2fTmXP1HqmLchg== X-Received: by 2002:ac2:43b0:0:b0:48b:1eb:d1e5 with SMTP id t16-20020ac243b0000000b0048b01ebd1e5mr272790lfl.641.1659335261113; Sun, 31 Jul 2022 23:27:41 -0700 (PDT) X-Google-Smtp-Source: AA6agR7kLCPxnca4MZdlMlO6mFwl8I9LgAWtroXVyXQq60uRjcCEDVvYfqEqtkyly+cwNyUmf+jb+aYHvUNOq8R+JIA= X-Received: by 2002:ac2:43b0:0:b0:48b:1eb:d1e5 with SMTP id t16-20020ac243b0000000b0048b01ebd1e5mr272773lfl.641.1659335260842; Sun, 31 Jul 2022 23:27:40 -0700 (PDT) MIME-Version: 1.0 References: <20220726072225.19884-1-xuanzhuo@linux.alibaba.com> <20220726072225.19884-17-xuanzhuo@linux.alibaba.com> <15aa26f2-f8af-5dbd-f2b2-9270ad873412@redhat.com> <1658907413.1860468-2-xuanzhuo@linux.alibaba.com> <1658992162.584327-1-xuanzhuo@linux.alibaba.com> <1658995783.1026692-1-xuanzhuo@linux.alibaba.com> <1659001321.5738833-2-xuanzhuo@linux.alibaba.com> <1659334300.4209104-1-xuanzhuo@linux.alibaba.com> In-Reply-To: <1659334300.4209104-1-xuanzhuo@linux.alibaba.com> From: Jason Wang Date: Mon, 1 Aug 2022 14:27:29 +0800 Message-ID: Subject: Re: [PATCH v13 16/42] virtio_ring: split: introduce virtqueue_resize_split() To: Xuan Zhuo Cc: Richard Weinberger , Anton Ivanov , Johannes Berg , "Michael S. Tsirkin" , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Hans de Goede , Mark Gross , Vadim Pasternak , Bjorn Andersson , Mathieu Poirier , Cornelia Huck , Halil Pasic , Eric Farman , Heiko Carstens , Vasily Gorbik , Alexander Gordeev , Christian Borntraeger , Sven Schnelle , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , Vincent Whitchurch , linux-um@lists.infradead.org, netdev , platform-driver-x86@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, kvm , "open list:XDP (eXpress Data Path)" , Kangjie Xu , virtualization Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: platform-driver-x86@vger.kernel.org On Mon, Aug 1, 2022 at 2:13 PM Xuan Zhuo wrote= : > > On Mon, 1 Aug 2022 12:49:12 +0800, Jason Wang wrote= : > > On Thu, Jul 28, 2022 at 7:27 PM Xuan Zhuo = wrote: > > > > > > On Thu, 28 Jul 2022 17:04:36 +0800, Jason Wang = wrote: > > > > On Thu, Jul 28, 2022 at 4:18 PM Xuan Zhuo wrote: > > > > > > > > > > On Thu, 28 Jul 2022 15:42:50 +0800, Jason Wang wrote: > > > > > > On Thu, Jul 28, 2022 at 3:24 PM Xuan Zhuo wrote: > > > > > > > > > > > > > > On Thu, 28 Jul 2022 10:38:51 +0800, Jason Wang wrote: > > > > > > > > On Wed, Jul 27, 2022 at 3:44 PM Xuan Zhuo wrote: > > > > > > > > > > > > > > > > > > On Wed, 27 Jul 2022 11:12:19 +0800, Jason Wang wrote: > > > > > > > > > > > > > > > > > > > > =E5=9C=A8 2022/7/26 15:21, Xuan Zhuo =E5=86=99=E9=81=93= : > > > > > > > > > > > virtio ring split supports resize. > > > > > > > > > > > > > > > > > > > > > > Only after the new vring is successfully allocated ba= sed on the new num, > > > > > > > > > > > we will release the old vring. In any case, an error = is returned, > > > > > > > > > > > indicating that the vring still points to the old vri= ng. > > > > > > > > > > > > > > > > > > > > > > In the case of an error, re-initialize(virtqueue_rein= it_split()) the > > > > > > > > > > > virtqueue to ensure that the vring can be used. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > > > Acked-by: Jason Wang > > > > > > > > > > > --- > > > > > > > > > > > drivers/virtio/virtio_ring.c | 34 +++++++++++++++++= +++++++++++++++++ > > > > > > > > > > > 1 file changed, 34 insertions(+) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/v= irtio/virtio_ring.c > > > > > > > > > > > index b6fda91c8059..58355e1ac7d7 100644 > > > > > > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > > > > > > @@ -220,6 +220,7 @@ static struct virtqueue *__vring_= new_virtqueue(unsigned int index, > > > > > > > > > > > void (*cal= lback)(struct virtqueue *), > > > > > > > > > > > const char= *name); > > > > > > > > > > > static struct vring_desc_extra *vring_alloc_desc_ex= tra(unsigned int num); > > > > > > > > > > > +static void vring_free(struct virtqueue *_vq); > > > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > > > > * Helpers. > > > > > > > > > > > @@ -1117,6 +1118,39 @@ static struct virtqueue *vring= _create_virtqueue_split( > > > > > > > > > > > return vq; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > +static int virtqueue_resize_split(struct virtqueue *= _vq, u32 num) > > > > > > > > > > > +{ > > > > > > > > > > > + struct vring_virtqueue_split vring_split =3D {}; > > > > > > > > > > > + struct vring_virtqueue *vq =3D to_vvq(_vq); > > > > > > > > > > > + struct virtio_device *vdev =3D _vq->vdev; > > > > > > > > > > > + int err; > > > > > > > > > > > + > > > > > > > > > > > + err =3D vring_alloc_queue_split(&vring_split, vde= v, num, > > > > > > > > > > > + vq->split.vring_ali= gn, > > > > > > > > > > > + vq->split.may_reduc= e_num); > > > > > > > > > > > + if (err) > > > > > > > > > > > + goto err; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think we don't need to do anything here? > > > > > > > > > > > > > > > > > > Am I missing something? > > > > > > > > > > > > > > > > I meant it looks to me most of the virtqueue_reinit() is un= necessary. > > > > > > > > We probably only need to reinit avail/used idx there. > > > > > > > > > > > > > > > > > > > > > In this function, we can indeed remove some code. > > > > > > > > > > > > > > > static void virtqueue_reinit_split(struct vring_virtq= ueue *vq) > > > > > > > > { > > > > > > > > int size, i; > > > > > > > > > > > > > > > > memset(vq->split.vring.desc, 0, vq->split.que= ue_size_in_bytes); > > > > > > > > > > > > > > > > size =3D sizeof(struct vring_desc_state_split= ) * vq->split.vring.num; > > > > > > > > memset(vq->split.desc_state, 0, size); > > > > > > > > > > > > > > > > size =3D sizeof(struct vring_desc_extra) * vq= ->split.vring.num; > > > > > > > > memset(vq->split.desc_extra, 0, size); > > > > > > > > > > > > > > These memsets can be removed, and theoretically it will not c= ause any > > > > > > > exceptions. > > > > > > > > > > > > Yes, otherwise we have bugs in detach_buf(). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > for (i =3D 0; i < vq->split.vring.num - 1; i+= +) > > > > > > > > vq->split.desc_extra[i].next =3D i + = 1; > > > > > > > > > > > > > > This can also be removed, but we need to record free_head tha= t will been update > > > > > > > inside virtqueue_init(). > > > > > > > > > > > > We can simply keep free_head unchanged? Otherwise it's a bug so= mewhere I guess. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > virtqueue_init(vq, vq->split.vring.num); > > > > > > > > > > > > > > There are some operations in this, which can also be skipped,= such as setting > > > > > > > use_dma_api. But I think calling this function directly will = be more convenient > > > > > > > for maintenance. > > > > > > > > > > > > I don't see anything that is necessary here. > > > > > > > > > > These three are currently inside virtqueue_init() > > > > > > > > > > vq->last_used_idx =3D 0; > > > > > vq->event_triggered =3D false; > > > > > vq->num_added =3D 0; > > > > > > > > Right. Let's keep it there. > > > > > > > > (Though it's kind of strange that the last_used_idx is not initiali= zed > > > > at the same place with avail_idx/flags_shadow, we can optimize it o= n > > > > top). > > > > > > I put free_head =3D 0 in the attach function, it is only necessary to= set > > > free_head =3D 0 when a new state/extra is attached. > > > > Ok, so I meant I tend to keep it to make this series converge soon :) > > > Ok, other than this, and what we discussed, no more fixes will be added. > > Thanks. Ack Thanks > > > > > > We can do optimization on top anyhow. > > > > Thanks > > > > > > > > In this way, when we call virtqueue_init(), we don't have to worry ab= out > > > free_head being modified. > > > > > > Rethinking this problem, I think virtqueue_init() can be rewritten an= d some > > > variables that will not change are removed from it. (use_dma_api, eve= nt, > > > weak_barriers) > > > > > > +static void virtqueue_init(struct vring_virtqueue *vq, u32 num) > > > +{ > > > + vq->vq.num_free =3D num; > > > + > > > + if (vq->packed_ring) > > > + vq->last_used_idx =3D 0 | (1 << VRING_PACKED_EVENT_F_= WRAP_CTR); > > > + else > > > + vq->last_used_idx =3D 0; > > > + > > > + vq->event_triggered =3D false; > > > + vq->num_added =3D 0; > > > + > > > +#ifdef DEBUG > > > + vq->in_use =3D false; > > > + vq->last_add_time_valid =3D false; > > > +#endif > > > +} > > > + > > > > > > Thanks. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > virtqueue_vring_init_split(&vq->split, vq); > > > > > > > > > > > > > > virtqueue_vring_init_split() is necessary. > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > Another method, we can take out all the variables to be reini= tialized > > > > > > > separately, and repackage them into a new function. I don=E2= =80=99t think it=E2=80=99s worth > > > > > > > it, because this path will only be reached if the memory allo= cation fails, which > > > > > > > is a rare occurrence. In this case, doing so will increase th= e cost of > > > > > > > maintenance. If you think so also, I will remove the above me= mset in the next > > > > > > > version. > > > > > > > > > > > > I agree. > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > + err =3D vring_alloc_state_extra_split(&vring_spli= t); > > > > > > > > > > > + if (err) { > > > > > > > > > > > + vring_free_split(&vring_split, vdev); > > > > > > > > > > > + goto err; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I suggest to move vring_free_split() into a dedicated e= rror label. > > > > > > > > > > > > > > > > > > Will change. > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + } > > > > > > > > > > > + > > > > > > > > > > > + vring_free(&vq->vq); > > > > > > > > > > > + > > > > > > > > > > > + virtqueue_vring_init_split(&vring_split, vq); > > > > > > > > > > > + > > > > > > > > > > > + virtqueue_init(vq, vring_split.vring.num); > > > > > > > > > > > + virtqueue_vring_attach_split(vq, &vring_split); > > > > > > > > > > > + > > > > > > > > > > > + return 0; > > > > > > > > > > > + > > > > > > > > > > > +err: > > > > > > > > > > > + virtqueue_reinit_split(vq); > > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > > +} > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > > > > * Packed ring specific functions - *_packed(). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >