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=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED 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 2AE16C48BDF for ; Tue, 22 Jun 2021 10:51:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0908F613AB for ; Tue, 22 Jun 2021 10:51:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229906AbhFVKxS (ORCPT ); Tue, 22 Jun 2021 06:53:18 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:44110 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229668AbhFVKxR (ORCPT ); Tue, 22 Jun 2021 06:53:17 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624359061; 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: in-reply-to:in-reply-to:references:references; bh=MahCvvTWod0cKiuylqNSDVrXb3r6bOZRYQZmM4hEPQc=; b=FOwlYRrSllYs17GEv5S9OGHqAcfsxvFilPPFtklbbn8bvikEZ8IT1wDt+U+zvGMmhEYHwl rB4WAQd2B7K82V1vUnFhi5OsMmphAepZm+Z4ZURKUjq8ktgDuLFSzcAgEirjzOgRckJQlG Nd22JqtQIj/FIWMPqWPjjd2bXppf5kA= Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-258-wQ-yVxbjMGmb9DKVmgr6Og-1; Tue, 22 Jun 2021 06:50:59 -0400 X-MC-Unique: wQ-yVxbjMGmb9DKVmgr6Og-1 Received: by mail-ed1-f69.google.com with SMTP id ee28-20020a056402291cb0290394a9a0bfaeso3808883edb.6 for ; Tue, 22 Jun 2021 03:50:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=MahCvvTWod0cKiuylqNSDVrXb3r6bOZRYQZmM4hEPQc=; b=Dp5pKgTJi6Vc8cKCx5xsUVxjJh6xBEOl7rQBFLmhQN8qst39OAImX7RJ7oU7upDcMl 4xXFt6z2fGFol6yNzR1iiAZx6+PTCoIxD6Y+CapSOpPmkaewt8QhkvamliO9g2out5cp 6cO04hr84exoUQKQD1o0CyNL++6TywsBHRaHwtpRzHXcea3m/nUuqF5bg6ls/SNc2wIx LVLBl+FxB+7IgKmXD3CSWo4+LSCzjtjc+3Ik6Cok3dcRziq7Bn+ZCsCO81E9bqv0FvAW RPbF8g4RCj9TwfRNU8BW1qtRmQHFosOxmBLtrlQwMbrc7mIFWqjhbUTUeoa36xk4240E YbNw== X-Gm-Message-State: AOAM533wArhOAQZogQuPMlFLQZ6Hdr9jX4AqBrQpq2AfuhfrySxgNCB6 ujU42iWdHG9BdR9fJHbi7NHah0J6+iEFyThq+iCn65FPP8M/nBGbwfWnkwm2gBWXEwxdxwsH252 G/Rri0trSHhYQ X-Received: by 2002:a17:907:7848:: with SMTP id lb8mr3393596ejc.494.1624359058673; Tue, 22 Jun 2021 03:50:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz110F+dcZSJKHiAM4Ds6UTPerNlho7sqlAGd2gqzNfyEj5VUGpE7hBK44QNGnfViisZ8+97Q== X-Received: by 2002:a17:907:7848:: with SMTP id lb8mr3393565ejc.494.1624359058443; Tue, 22 Jun 2021 03:50:58 -0700 (PDT) Received: from steredhat (host-79-18-148-79.retail.telecomitalia.it. [79.18.148.79]) by smtp.gmail.com with ESMTPSA id d6sm1638699edq.37.2021.06.22.03.50.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Jun 2021 03:50:58 -0700 (PDT) Date: Tue, 22 Jun 2021 12:50:55 +0200 From: Stefano Garzarella To: "Jiang Wang ." Cc: virtualization@lists.linux-foundation.org, Stefan Hajnoczi , "Michael S. Tsirkin" , Arseny Krasnov , cong.wang@bytedance.com, Xiongchun Duan , Yongji Xie , =?utf-8?B?5p+056iz?= , Jason Wang , "David S. Miller" , Jakub Kicinski , Steven Rostedt , Ingo Molnar , Andra Paraschiv , Norbert Slusarek , Colin Ian King , Alexander Popov , kvm@vger.kernel.org, Networking , linux-kernel@vger.kernel.org Subject: Re: [External] Re: [RFC v1 1/6] virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit Message-ID: <20210622105055.ogacdpsadazwa4wq@steredhat> References: <20210609232501.171257-1-jiang.wang@bytedance.com> <20210609232501.171257-2-jiang.wang@bytedance.com> <20210618093951.g32htj3rsu2koqi5@steredhat.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Mon, Jun 21, 2021 at 10:24:20AM -0700, Jiang Wang . wrote: >On Fri, Jun 18, 2021 at 2:40 AM Stefano Garzarella wrote: >> >> On Wed, Jun 09, 2021 at 11:24:53PM +0000, Jiang Wang wrote: >> >When this feature is enabled, allocate 5 queues, >> >otherwise, allocate 3 queues to be compatible with >> >old QEMU versions. >> > >> >Signed-off-by: Jiang Wang >> >--- >> > drivers/vhost/vsock.c | 3 +- >> > include/linux/virtio_vsock.h | 9 +++++ >> > include/uapi/linux/virtio_vsock.h | 3 ++ >> > net/vmw_vsock/virtio_transport.c | 73 +++++++++++++++++++++++++++++++++++---- >> > 4 files changed, 80 insertions(+), 8 deletions(-) >> > >> >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c >> >index 5e78fb719602..81d064601093 100644 >> >--- a/drivers/vhost/vsock.c >> >+++ b/drivers/vhost/vsock.c >> >@@ -31,7 +31,8 @@ >> > >> > enum { >> > VHOST_VSOCK_FEATURES = VHOST_FEATURES | >> >- (1ULL << VIRTIO_F_ACCESS_PLATFORM) >> >+ (1ULL << VIRTIO_F_ACCESS_PLATFORM) | >> >+ (1ULL << VIRTIO_VSOCK_F_DGRAM) >> > }; >> > >> > enum { >> >diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h >> >index dc636b727179..ba3189ed9345 100644 >> >--- a/include/linux/virtio_vsock.h >> >+++ b/include/linux/virtio_vsock.h >> >@@ -18,6 +18,15 @@ enum { >> > VSOCK_VQ_MAX = 3, >> > }; >> > >> >+enum { >> >+ VSOCK_VQ_STREAM_RX = 0, /* for host to guest data */ >> >+ VSOCK_VQ_STREAM_TX = 1, /* for guest to host data */ >> >+ VSOCK_VQ_DGRAM_RX = 2, >> >+ VSOCK_VQ_DGRAM_TX = 3, >> >+ VSOCK_VQ_EX_EVENT = 4, >> >+ VSOCK_VQ_EX_MAX = 5, >> >+}; >> >+ >> > /* Per-socket state (accessed via vsk->trans) */ >> > struct virtio_vsock_sock { >> > struct vsock_sock *vsk; >> >diff --git a/include/uapi/linux/virtio_vsock.h b/include/uapi/linux/virtio_vsock.h >> >index 1d57ed3d84d2..b56614dff1c9 100644 >> >--- a/include/uapi/linux/virtio_vsock.h >> >+++ b/include/uapi/linux/virtio_vsock.h >> >@@ -38,6 +38,9 @@ >> > #include >> > #include >> > >> >+/* The feature bitmap for virtio net */ >> >+#define VIRTIO_VSOCK_F_DGRAM 0 /* Host support dgram vsock */ >> >+ >> > struct virtio_vsock_config { >> > __le64 guest_cid; >> > } __attribute__((packed)); >> >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >> >index 2700a63ab095..7dcb8db23305 100644 >> >--- a/net/vmw_vsock/virtio_transport.c >> >+++ b/net/vmw_vsock/virtio_transport.c >> >@@ -27,7 +27,8 @@ static DEFINE_MUTEX(the_virtio_vsock_mutex); /* protects the_virtio_vsock */ >> > >> > struct virtio_vsock { >> > struct virtio_device *vdev; >> >- struct virtqueue *vqs[VSOCK_VQ_MAX]; >> >+ struct virtqueue **vqs; >> >+ bool has_dgram; >> > >> > /* Virtqueue processing is deferred to a workqueue */ >> > struct work_struct tx_work; >> >@@ -333,7 +334,10 @@ static int virtio_vsock_event_fill_one(struct virtio_vsock *vsock, >> > struct scatterlist sg; >> > struct virtqueue *vq; >> > >> >- vq = vsock->vqs[VSOCK_VQ_EVENT]; >> >+ if (vsock->has_dgram) >> >+ vq = vsock->vqs[VSOCK_VQ_EX_EVENT]; >> >+ else >> >+ vq = vsock->vqs[VSOCK_VQ_EVENT]; >> > >> > sg_init_one(&sg, event, sizeof(*event)); >> > >> >@@ -351,7 +355,10 @@ static void virtio_vsock_event_fill(struct virtio_vsock *vsock) >> > virtio_vsock_event_fill_one(vsock, event); >> > } >> > >> >- virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]); >> >+ if (vsock->has_dgram) >> >+ virtqueue_kick(vsock->vqs[VSOCK_VQ_EX_EVENT]); >> >+ else >> >+ virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]); >> > } >> > >> > static void virtio_vsock_reset_sock(struct sock *sk) >> >@@ -391,7 +398,10 @@ static void virtio_transport_event_work(struct work_struct *work) >> > container_of(work, struct virtio_vsock, event_work); >> > struct virtqueue *vq; >> > >> >- vq = vsock->vqs[VSOCK_VQ_EVENT]; >> >+ if (vsock->has_dgram) >> >+ vq = vsock->vqs[VSOCK_VQ_EX_EVENT]; >> >+ else >> >+ vq = vsock->vqs[VSOCK_VQ_EVENT]; >> > >> > mutex_lock(&vsock->event_lock); >> > >> >@@ -411,7 +421,10 @@ static void virtio_transport_event_work(struct work_struct *work) >> > } >> > } while (!virtqueue_enable_cb(vq)); >> > >> >- virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]); >> >+ if (vsock->has_dgram) >> >+ virtqueue_kick(vsock->vqs[VSOCK_VQ_EX_EVENT]); >> >+ else >> >+ virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]); >> > out: >> > mutex_unlock(&vsock->event_lock); >> > } >> >@@ -434,6 +447,10 @@ static void virtio_vsock_tx_done(struct virtqueue *vq) >> > queue_work(virtio_vsock_workqueue, &vsock->tx_work); >> > } >> > >> >+static void virtio_vsock_dgram_tx_done(struct virtqueue *vq) >> >+{ >> >+} >> >+ >> > static void virtio_vsock_rx_done(struct virtqueue *vq) >> > { >> > struct virtio_vsock *vsock = vq->vdev->priv; >> >@@ -443,6 +460,10 @@ static void virtio_vsock_rx_done(struct virtqueue *vq) >> > queue_work(virtio_vsock_workqueue, &vsock->rx_work); >> > } >> > >> >+static void virtio_vsock_dgram_rx_done(struct virtqueue *vq) >> >+{ >> >+} >> >+ >> > static struct virtio_transport virtio_transport = { >> > .transport = { >> > .module = THIS_MODULE, >> >@@ -545,13 +566,29 @@ static int virtio_vsock_probe(struct virtio_device *vdev) >> > virtio_vsock_tx_done, >> > virtio_vsock_event_done, >> > }; >> >+ vq_callback_t *ex_callbacks[] = { >> >> 'ex' is not clear, maybe better 'dgram'? >> >sure. > >> What happen if F_DGRAM is negotiated, but not F_STREAM? >> >Hmm. In my mind, F_STREAM is always negotiated. Do we want to add >support when F_STREAM is not negotiated? > Yep, I think we should support this case. The main purpose of the feature bits is to enable/disable the functionality after the negotiation. Initially we didn't want to introduce it, but then we thought it was better because there could be a device for example that wants to support only datagram. Since you're touching this part of the code, it would be very helpful to fix the problem now. But if you think it's too complex, we can do it in a second step. Thanks, Stefano >> >+ virtio_vsock_rx_done, >> >+ virtio_vsock_tx_done, >> >+ virtio_vsock_dgram_rx_done, >> >+ virtio_vsock_dgram_tx_done, >> >+ virtio_vsock_event_done, >> >+ }; >> >+ >> > static const char * const names[] = { >> > "rx", >> > "tx", >> > "event", >> > }; >> >+ static const char * const ex_names[] = { >> >+ "rx", >> >+ "tx", >> >+ "dgram_rx", >> >+ "dgram_tx", >> >+ "event", >> >+ }; >> >+ >> > struct virtio_vsock *vsock = NULL; >> >- int ret; >> >+ int ret, max_vq; >> > >> > ret = mutex_lock_interruptible(&the_virtio_vsock_mutex); >> > if (ret) >> >@@ -572,9 +609,30 @@ static int virtio_vsock_probe(struct virtio_device *vdev) >> > >> > vsock->vdev = vdev; >> > >> >- ret = virtio_find_vqs(vsock->vdev, VSOCK_VQ_MAX, >> >+ if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_DGRAM)) >> >+ vsock->has_dgram = true; >> >+ >> >+ if (vsock->has_dgram) >> >+ max_vq = VSOCK_VQ_EX_MAX; >> >+ else >> >+ max_vq = VSOCK_VQ_MAX; >> >+ >> >+ vsock->vqs = kmalloc_array(max_vq, sizeof(struct virtqueue *), GFP_KERNEL); >> >+ if (!vsock->vqs) { >> >+ ret = -ENOMEM; >> >+ goto out; >> >+ } >> >+ >> >+ if (vsock->has_dgram) { >> >+ ret = virtio_find_vqs(vsock->vdev, max_vq, >> >+ vsock->vqs, ex_callbacks, ex_names, >> >+ NULL); >> >+ } else { >> >+ ret = virtio_find_vqs(vsock->vdev, max_vq, >> > vsock->vqs, callbacks, names, >> > NULL); >> >+ } >> >+ >> > if (ret < 0) >> > goto out; >> > >> >@@ -695,6 +753,7 @@ static struct virtio_device_id id_table[] = { >> > }; >> > >> > static unsigned int features[] = { >> >+ VIRTIO_VSOCK_F_DGRAM, >> > }; >> > >> > static struct virtio_driver virtio_vsock_driver = { >> >-- >> >2.11.0 >> > >> >