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 X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0D70AC4167B for ; Wed, 9 Dec 2020 18:43:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4726F23B98 for ; Wed, 9 Dec 2020 18:43:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387408AbgLISnb (ORCPT ); Wed, 9 Dec 2020 13:43:31 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:31952 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1733201AbgLISn2 (ORCPT ); Wed, 9 Dec 2020 13:43:28 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1607539321; 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=lk3sun6J6ZaKCEXmHk4Lr251oV6cGXMTlOHoLDL99QA=; b=A2O10HKXCjhpuTl9k5oMOsmDvH6ax0cTPyUwzA7TWVA2kVqUFfIbAPIzEBR67sYFXad64B nqGng/eNFcT5jLvXeAFSAsCCEcXIM4iysP/yb2JUTmSvJI15qZlUxTjzQdLeeVi/xIu2k1 NdHFNQLNwOu+Mpq36ZkqIP1K9reKqWo= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-549-jqnx9vu5NY-cFAl3j6NrVQ-1; Wed, 09 Dec 2020 13:42:00 -0500 X-MC-Unique: jqnx9vu5NY-cFAl3j6NrVQ-1 Received: by mail-qk1-f199.google.com with SMTP id 198so1764822qkj.7 for ; Wed, 09 Dec 2020 10:42:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=lk3sun6J6ZaKCEXmHk4Lr251oV6cGXMTlOHoLDL99QA=; b=lqXp+W7XCdkYheebcG3pavKcHqucL/4KjuvWjdz6V2aduFRD1dPKKeqYhJ6FlNbNXl 9zmErGmtzFGQ6YOKEPGMqdwqBygBxptZX54q9H0m4ClaXsyd4lgVh6aGY2Enagsy9DLS Vm7lNy5+pfeXNQpIw9IOEysdGZ0GxrsJhJ1llVZSuIGvO1IYmm0jrUaw3E6M9mIqFvn9 SLNi4D1KT1s3U6L35Byg7Kb1rGBgk4nKFKy1jbHT2WiJAN5a8Mp+vatbNuZxGem63NSY DzUycWIjjhb813iHjmk0zmFBcGLbiuF4CLzFbV8Cdni6rhWLbIYbtLtQkKZ7aJg25b83 6oeQ== X-Gm-Message-State: AOAM532R9EtfUy8nW4YeSUEjj+9lAihEXFP2mWS9jjY9KJ9AoIgy8FFM FJh5GKmRd3wVpZMdvRQWKQ7T6SaVWqc2HoIft2EZubl725CLiaEEYIutIIiIJK5w7kpmBQdUf81 RGYvBdWimv7k/qLD79xs27uO21t6Y X-Received: by 2002:a37:4658:: with SMTP id t85mr4608861qka.210.1607539319543; Wed, 09 Dec 2020 10:41:59 -0800 (PST) X-Google-Smtp-Source: ABdhPJzXkGCcuiVm+epPlx+mW0vY7zLl3v75k/7Dp2jT6Sl5FNC8Dc6HdWJfZipNZNECFlpW33gUvQfa2MAbG9WCydA= X-Received: by 2002:a37:4658:: with SMTP id t85mr4608817qka.210.1607539319246; Wed, 09 Dec 2020 10:41:59 -0800 (PST) MIME-Version: 1.0 References: <20201120185105.279030-1-eperezma@redhat.com> <20201120185105.279030-14-eperezma@redhat.com> <20201208081621.GR203660@stefanha-x1.localdomain> In-Reply-To: <20201208081621.GR203660@stefanha-x1.localdomain> From: Eugenio Perez Martin Date: Wed, 9 Dec 2020 19:41:23 +0100 Message-ID: Subject: Re: [RFC PATCH 13/27] vhost: Send buffers to device To: Stefan Hajnoczi Cc: qemu-level , Lars Ganrot , virtualization@lists.linux-foundation.org, Salil Mehta , "Michael S. Tsirkin" , Liran Alon , Rob Miller , Max Gurtovoy , Alex Barba , Stefan Hajnoczi , Jim Harford , Jason Wang , Harpreet Singh Anand , Christophe Fontaine , vm , Daniel Daly , Michael Lilja , Stefano Garzarella , Nitin Shrivastav , Lee Ballard , Dmytro Kazantsev , Juan Quintela , kvm list , Howard Cai , Xiao W Wang , Sean Mooney , Parav Pandit , Eli Cohen , Siwei Liu , Stephen Finucane Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, Dec 8, 2020 at 9:16 AM Stefan Hajnoczi wrote: > > On Fri, Nov 20, 2020 at 07:50:51PM +0100, Eugenio P=C3=A9rez wrote: > > -static inline bool vhost_vring_should_kick(VhostShadowVirtqueue *vq) > > +static bool vhost_vring_should_kick_rcu(VhostShadowVirtqueue *vq) > > "vhost_vring_" is a confusing name. This is not related to > vhost_virtqueue or the vhost_vring_* structs. > > vhost_shadow_vq_should_kick_rcu()? > > > { > > - return virtio_queue_get_used_notify_split(vq->vq); > > + VirtIODevice *vdev =3D vq->vdev; > > + vq->num_added =3D 0; > > I'm surprised that a bool function modifies state. Is this assignment > intentional? > It's from the kernel code, virtqueue_kick_prepare_split function. The num_added member is internal (mutable) state, counting for the batch so the driver sends a notification in case of uint16_t wrapping in vhost_vring_add_split with no notification in between. I don't know if some actual virtio devices could be actually affected from this, since actual vqs are smaller than (uint16_t)-1 so they should be aware that more buffers have been added anyway. > > +/* virtqueue_add: > > + * @vq: The #VirtQueue > > + * @elem: The #VirtQueueElement > > The copy-pasted doc comment doesn't match this function. > Right, I will fix it. > > +int vhost_vring_add(VhostShadowVirtqueue *vq, VirtQueueElement *elem) > > +{ > > + int host_head =3D vhost_vring_add_split(vq, elem); > > + if (vq->ring_id_maps[host_head]) { > > + g_free(vq->ring_id_maps[host_head]); > > + } > > VirtQueueElement is freed lazily? Is there a reason for this approach? I > would have expected it to be freed when the used element is process by > the kick fd handler. > Maybe it has more sense to free immediately in this commit and introduce ring_id_maps in later commits, yes. > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 9352c56bfa..304e0baa61 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -956,8 +956,34 @@ static void handle_sw_lm_vq(VirtIODevice *vdev, Vi= rtQueue *vq) > > uint16_t idx =3D virtio_get_queue_index(vq); > > > > VhostShadowVirtqueue *svq =3D hdev->sw_lm_shadow_vq[idx]; > > + VirtQueueElement *elem; > > > > - vhost_vring_kick(svq); > > + /* > > + * Make available all buffers as possible. > > + */ > > + do { > > + if (virtio_queue_get_notification(vq)) { > > + virtio_queue_set_notification(vq, false); > > + } > > + > > + while (true) { > > + int r; > > + if (virtio_queue_full(vq)) { > > + break; > > + } > > Why is this check necessary? The guest cannot provide more descriptors > than there is ring space. If that happens somehow then it's a driver > error that is already reported in virtqueue_pop() below. > It's just checked because virtqueue_pop prints an error on that case, and there is no way to tell the difference between a regular error and another caused by other causes. Maybe the right thing to do is just to not to print that error? Caller should do the error printing in that case. Should we return an error code? > I wonder if you added this because the vring implementation above > doesn't support indirect descriptors? It's easy to exhaust the vhost > hdev vring while there is still room in the VirtIODevice's VirtQueue > vring. 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 X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81BF5C4361B for ; Wed, 9 Dec 2020 19:54:11 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CA8BA239D1 for ; Wed, 9 Dec 2020 19:54:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CA8BA239D1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:41200 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kn5XN-0003m2-IP for qemu-devel@archiver.kernel.org; Wed, 09 Dec 2020 14:54:09 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:60754) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kn4Pf-0007c2-2U for qemu-devel@nongnu.org; Wed, 09 Dec 2020 13:42:07 -0500 Received: from us-smtp-delivery-124.mimecast.com ([63.128.21.124]:51838) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.90_1) (envelope-from ) id 1kn4Pc-0001y8-7R for qemu-devel@nongnu.org; Wed, 09 Dec 2020 13:42:06 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1607539321; 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=lk3sun6J6ZaKCEXmHk4Lr251oV6cGXMTlOHoLDL99QA=; b=A2O10HKXCjhpuTl9k5oMOsmDvH6ax0cTPyUwzA7TWVA2kVqUFfIbAPIzEBR67sYFXad64B nqGng/eNFcT5jLvXeAFSAsCCEcXIM4iysP/yb2JUTmSvJI15qZlUxTjzQdLeeVi/xIu2k1 NdHFNQLNwOu+Mpq36ZkqIP1K9reKqWo= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-184-ybW8m4TqNimpK9v5K8HT9Q-1; Wed, 09 Dec 2020 13:42:00 -0500 X-MC-Unique: ybW8m4TqNimpK9v5K8HT9Q-1 Received: by mail-qk1-f198.google.com with SMTP id e68so1778664qkd.3 for ; Wed, 09 Dec 2020 10:42:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=lk3sun6J6ZaKCEXmHk4Lr251oV6cGXMTlOHoLDL99QA=; b=m3ZFZRz5UicYRKqRR7Ehj3mNgGDFKXtIAn8Vbh8t3SOPujarZecJBISu14uC9Lc25H DnwDuDBQAdp4zku6VTbB05ZPzVxM8IjRYjjEd5UIYUp4tlcGN1A3y4saRgVnZShyAZCk qHXNqXgZBoTy42O3pxGBZAMGVvYP7wASSLlISd0bv0rWRCF8TqUns2alciT3Dz4Wuj9k AedXSdik2fLU8x6bpxG7Y2mniwapDO8PcsE5LR9Wi1tl7+cCHQlpRAqL2UPc+vQ+nddZ 5yP+DS/nttcgu+bonElqUmkZJoxBdokptTmVigUVcO5dxxB1/IMdcVCiW8nRGYtoINg2 lXrw== X-Gm-Message-State: AOAM533ypnBKvinraiOQ+NP3ja09a8axD9/NkDluxLmNWAqtZxSj3Jwq B61ybxyrXSHqn/dStLloBB7tYT5c6STsS+h6nmpaOM2B9zEhTQ5HyTvNzRHhMK1/ActL4g+TghR 8fCFUQbiOgfkRNr/qJPiz8+g66fyW/QA= X-Received: by 2002:a37:4658:: with SMTP id t85mr4608864qka.210.1607539319543; Wed, 09 Dec 2020 10:41:59 -0800 (PST) X-Google-Smtp-Source: ABdhPJzXkGCcuiVm+epPlx+mW0vY7zLl3v75k/7Dp2jT6Sl5FNC8Dc6HdWJfZipNZNECFlpW33gUvQfa2MAbG9WCydA= X-Received: by 2002:a37:4658:: with SMTP id t85mr4608817qka.210.1607539319246; Wed, 09 Dec 2020 10:41:59 -0800 (PST) MIME-Version: 1.0 References: <20201120185105.279030-1-eperezma@redhat.com> <20201120185105.279030-14-eperezma@redhat.com> <20201208081621.GR203660@stefanha-x1.localdomain> In-Reply-To: <20201208081621.GR203660@stefanha-x1.localdomain> From: Eugenio Perez Martin Date: Wed, 9 Dec 2020 19:41:23 +0100 Message-ID: Subject: Re: [RFC PATCH 13/27] vhost: Send buffers to device To: Stefan Hajnoczi Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=eperezma@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=63.128.21.124; envelope-from=eperezma@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kvm list , "Michael S. Tsirkin" , Jason Wang , qemu-level , Daniel Daly , virtualization@lists.linux-foundation.org, Liran Alon , Eli Cohen , Nitin Shrivastav , Alex Barba , Christophe Fontaine , Juan Quintela , Lee Ballard , Lars Ganrot , Rob Miller , Stefano Garzarella , Howard Cai , Parav Pandit , vm , Salil Mehta , Stephen Finucane , Xiao W Wang , Sean Mooney , Stefan Hajnoczi , Jim Harford , Dmytro Kazantsev , Siwei Liu , Harpreet Singh Anand , Michael Lilja , Max Gurtovoy Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Tue, Dec 8, 2020 at 9:16 AM Stefan Hajnoczi wrote: > > On Fri, Nov 20, 2020 at 07:50:51PM +0100, Eugenio P=C3=A9rez wrote: > > -static inline bool vhost_vring_should_kick(VhostShadowVirtqueue *vq) > > +static bool vhost_vring_should_kick_rcu(VhostShadowVirtqueue *vq) > > "vhost_vring_" is a confusing name. This is not related to > vhost_virtqueue or the vhost_vring_* structs. > > vhost_shadow_vq_should_kick_rcu()? > > > { > > - return virtio_queue_get_used_notify_split(vq->vq); > > + VirtIODevice *vdev =3D vq->vdev; > > + vq->num_added =3D 0; > > I'm surprised that a bool function modifies state. Is this assignment > intentional? > It's from the kernel code, virtqueue_kick_prepare_split function. The num_added member is internal (mutable) state, counting for the batch so the driver sends a notification in case of uint16_t wrapping in vhost_vring_add_split with no notification in between. I don't know if some actual virtio devices could be actually affected from this, since actual vqs are smaller than (uint16_t)-1 so they should be aware that more buffers have been added anyway. > > +/* virtqueue_add: > > + * @vq: The #VirtQueue > > + * @elem: The #VirtQueueElement > > The copy-pasted doc comment doesn't match this function. > Right, I will fix it. > > +int vhost_vring_add(VhostShadowVirtqueue *vq, VirtQueueElement *elem) > > +{ > > + int host_head =3D vhost_vring_add_split(vq, elem); > > + if (vq->ring_id_maps[host_head]) { > > + g_free(vq->ring_id_maps[host_head]); > > + } > > VirtQueueElement is freed lazily? Is there a reason for this approach? I > would have expected it to be freed when the used element is process by > the kick fd handler. > Maybe it has more sense to free immediately in this commit and introduce ring_id_maps in later commits, yes. > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 9352c56bfa..304e0baa61 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -956,8 +956,34 @@ static void handle_sw_lm_vq(VirtIODevice *vdev, Vi= rtQueue *vq) > > uint16_t idx =3D virtio_get_queue_index(vq); > > > > VhostShadowVirtqueue *svq =3D hdev->sw_lm_shadow_vq[idx]; > > + VirtQueueElement *elem; > > > > - vhost_vring_kick(svq); > > + /* > > + * Make available all buffers as possible. > > + */ > > + do { > > + if (virtio_queue_get_notification(vq)) { > > + virtio_queue_set_notification(vq, false); > > + } > > + > > + while (true) { > > + int r; > > + if (virtio_queue_full(vq)) { > > + break; > > + } > > Why is this check necessary? The guest cannot provide more descriptors > than there is ring space. If that happens somehow then it's a driver > error that is already reported in virtqueue_pop() below. > It's just checked because virtqueue_pop prints an error on that case, and there is no way to tell the difference between a regular error and another caused by other causes. Maybe the right thing to do is just to not to print that error? Caller should do the error printing in that case. Should we return an error code? > I wonder if you added this because the vring implementation above > doesn't support indirect descriptors? It's easy to exhaust the vhost > hdev vring while there is still room in the VirtIODevice's VirtQueue > vring.