Message ID | 20231205064806.2851305-4-avkrasnov@salutedevices.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp3251458vqy; Mon, 4 Dec 2023 22:56:55 -0800 (PST) X-Google-Smtp-Source: AGHT+IE+fuu4ub3sTWxpzqC8JuRbk0LK03Wg9guxr5R9UBuGaweue3168BVoZ+qrAN7hTzLDB1tA X-Received: by 2002:a05:6358:6f86:b0:16d:bb7b:c0a7 with SMTP id s6-20020a0563586f8600b0016dbb7bc0a7mr6021406rwn.10.1701759415155; Mon, 04 Dec 2023 22:56:55 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701759415; cv=none; d=google.com; s=arc-20160816; b=tqVoB7CXAPj7jaVrRr5rN+yWcxPgY0CHudFeM+47WBapX8Fb0Ny/3UnDhSZsEu3f6C Qzug54mY1GQO+JQQSLT+uM8oVSLnQOxQK0jxDpdua1tGNgN8VrrQUoUNsvKiXHZmXQk4 vrJd0wqn+xuZdFTXAmT/wUXewH8moeT9ch0oejo8p6JmOTbdAC7hWXC7+9Dh/YXB71bS K1C2i1WrCatDm59Qdh3qCR+t59gpGunlXPCPozDKEMhl00xorZcNNf1zW5h0VVaAr4ZZ UtQu8tZhKJBaGm9jUmZcRtO08TWcfpcKQsdUGYpT/0a9XY7zo2upL4M6xKuFB0Ze1kXZ 9XJA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature:dkim-filter; bh=hRb1OWAketAgWsS/ac4IBqv06gfS9pcaRvoWb7Do5Fg=; fh=wIHqZpOIuzcidDZ82yQbOZuyHJty7uvaWDEh/efoVzA=; b=F5mFw2dS4bN+m5jM624031GAp4zN4wpAIrppgLLTU37g/tBrqPqjEEkbaCmo6IZove G9Nqmevysy94QHPAcnvuHK+xGVj9s9yfIDc9bNQpSiqLw1brAe4Crb/uSFvqiOYqtn74 BEFbZ8CfAe+EUdfszi3ttvd0vZUW9MhCNreRcqhCk4DL1d2TFNpnhs2MRY0qibNbxUl7 Gog5gWJZ+7FUSoiGLmpWMEyJBRVFLg+j0HxKzZNXn610Gg6NDIpDuiLxl+fLfZv7y1ep e4EyntkMrFrIqk40tycR9KVUatJftJjcDpHsIthhMCBK+QGJrQw3fuhYqHNqayQG7NaS qTzw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@salutedevices.com header.s=mail header.b=OR6QVcpO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=salutedevices.com Received: from morse.vger.email (morse.vger.email. [2620:137:e000::3:1]) by mx.google.com with ESMTPS id n124-20020a632782000000b005be0ca9ca31si4703422pgn.294.2023.12.04.22.56.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 04 Dec 2023 22:56:55 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1; Authentication-Results: mx.google.com; dkim=pass header.i=@salutedevices.com header.s=mail header.b=OR6QVcpO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=salutedevices.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id A411380212FD; Mon, 4 Dec 2023 22:56:50 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344557AbjLEG4d (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Tue, 5 Dec 2023 01:56:33 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50236 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344522AbjLEG43 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 5 Dec 2023 01:56:29 -0500 Received: from mx1.sberdevices.ru (mx2.sberdevices.ru [45.89.224.132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 32C18136; Mon, 4 Dec 2023 22:56:33 -0800 (PST) Received: from p-infra-ksmg-sc-msk02 (localhost [127.0.0.1]) by mx1.sberdevices.ru (Postfix) with ESMTP id F3D85120026; Tue, 5 Dec 2023 09:56:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.sberdevices.ru F3D85120026 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=salutedevices.com; s=mail; t=1701759390; bh=hRb1OWAketAgWsS/ac4IBqv06gfS9pcaRvoWb7Do5Fg=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type:From; b=OR6QVcpOFCLTZlem67vZ3/D3KJKezBtF2A9wpCNlWLiLi6wokvoI/X5P2y0ZEcnBi bFvOM4bjUqpx1NtSc+ebn2tY6Li8s+w2hBDOvLSp9tJQrXbDg5S6NDwRebGwmD0UwQ 6VaZDjQJ1Ym+vFEA0K/F0uKPqBa+N/gH4oOsoK7SMJ5rBAQK2TLdFlddzzmRjE4K1Y qnNTg7y4PdjQLjm+zHgByCx8OZJk5GKQa2ZQGc+OdRFJOeJQ9gURrFlh5YHZWrs3JP MaFQdXlsIvBhn29eAdLzkmo6YKD8xokEjwOU9NVxdZwuko7pZeBAe4Dw5E4rKfwyeq 1xmxtwgPGuodA== Received: from p-i-exch-sc-m01.sberdevices.ru (p-i-exch-sc-m01.sberdevices.ru [172.16.192.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.sberdevices.ru (Postfix) with ESMTPS; Tue, 5 Dec 2023 09:56:29 +0300 (MSK) Received: from localhost.localdomain (100.64.160.123) by p-i-exch-sc-m01.sberdevices.ru (172.16.192.107) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.40; Tue, 5 Dec 2023 09:56:29 +0300 From: Arseniy Krasnov <avkrasnov@salutedevices.com> To: Stefan Hajnoczi <stefanha@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Bobby Eshleman <bobby.eshleman@bytedance.com> CC: <kvm@vger.kernel.org>, <virtualization@lists.linux-foundation.org>, <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <kernel@sberdevices.ru>, <oxffffaa@gmail.com>, <avkrasnov@salutedevices.com> Subject: [PATCH net-next v6 3/4] virtio/vsock: fix logic which reduces credit update messages Date: Tue, 5 Dec 2023 09:48:05 +0300 Message-ID: <20231205064806.2851305-4-avkrasnov@salutedevices.com> X-Mailer: git-send-email 2.35.0 In-Reply-To: <20231205064806.2851305-1-avkrasnov@salutedevices.com> References: <20231205064806.2851305-1-avkrasnov@salutedevices.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [100.64.160.123] X-ClientProxiedBy: p-i-exch-sc-m02.sberdevices.ru (172.16.192.103) To p-i-exch-sc-m01.sberdevices.ru (172.16.192.107) X-KSMG-Rule-ID: 10 X-KSMG-Message-Action: clean X-KSMG-AntiSpam-Lua-Profiles: 181836 [Dec 05 2023] X-KSMG-AntiSpam-Version: 6.0.0.2 X-KSMG-AntiSpam-Envelope-From: avkrasnov@salutedevices.com X-KSMG-AntiSpam-Rate: 0 X-KSMG-AntiSpam-Status: not_detected X-KSMG-AntiSpam-Method: none X-KSMG-AntiSpam-Auth: dkim=none X-KSMG-AntiSpam-Info: LuaCore: 5 0.3.5 98d108ddd984cca1d7e65e595eac546a62b0144b, {Tracking_from_domain_doesnt_match_to}, salutedevices.com:7.1.1;p-i-exch-sc-m01.sberdevices.ru:7.1.1,5.0.1;100.64.160.123:7.1.2;d41d8cd98f00b204e9800998ecf8427e.com:7.1.1;127.0.0.199:7.1.2, FromAlignment: s, ApMailHostAddress: 100.64.160.123 X-MS-Exchange-Organization-SCL: -1 X-KSMG-AntiSpam-Interceptor-Info: scan successful X-KSMG-AntiPhishing: Clean X-KSMG-LinksScanning: Clean X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 2.0.1.6960, bases: 2023/12/05 03:59:00 #22607474 X-KSMG-AntiVirus-Status: Clean, skipped X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Mon, 04 Dec 2023 22:56:51 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784424080552116489 X-GMAIL-MSGID: 1784424080552116489 |
Series |
send credit update during setting SO_RCVLOWAT
|
|
Commit Message
Arseniy Krasnov
Dec. 5, 2023, 6:48 a.m. UTC
Add one more condition for sending credit update during dequeue from
stream socket: when number of bytes in the rx queue is smaller than
SO_RCVLOWAT value of the socket. This is actual for non-default value
of SO_RCVLOWAT (e.g. not 1) - idea is to "kick" peer to continue data
transmission, because we need at least SO_RCVLOWAT bytes in our rx
queue to wake up user for reading data (in corner case it is also
possible to stuck both tx and rx sides, this is why 'Fixes' is used).
Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages")
Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
net/vmw_vsock/virtio_transport_common.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
Comments
On Tue, Dec 05, 2023 at 09:48:05AM +0300, Arseniy Krasnov wrote: >Add one more condition for sending credit update during dequeue from >stream socket: when number of bytes in the rx queue is smaller than >SO_RCVLOWAT value of the socket. This is actual for non-default value >of SO_RCVLOWAT (e.g. not 1) - idea is to "kick" peer to continue data >transmission, because we need at least SO_RCVLOWAT bytes in our rx >queue to wake up user for reading data (in corner case it is also >possible to stuck both tx and rx sides, this is why 'Fixes' is used). > >Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >--- > net/vmw_vsock/virtio_transport_common.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > >diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >index e137d740804e..461c89882142 100644 >--- a/net/vmw_vsock/virtio_transport_common.c >+++ b/net/vmw_vsock/virtio_transport_common.c >@@ -558,6 +558,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > struct virtio_vsock_sock *vvs = vsk->trans; > size_t bytes, total = 0; > struct sk_buff *skb; >+ bool low_rx_bytes; > int err = -EFAULT; > u32 free_space; > >@@ -602,6 +603,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > } > > free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >+ low_rx_bytes = (vvs->rx_bytes < >+ sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); As in the previous patch, should we avoid the update it if `fwd_cnt` and `last_fwd_cnt` are the same? Now I'm thinking if it is better to add that check directly in virtio_transport_send_credit_update(). Stefano > > spin_unlock_bh(&vvs->rx_lock); > >@@ -611,9 +614,11 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > * too high causes extra messages. Too low causes transmitter > * stalls. As stalls are in theory more expensive than extra > * messages, we set the limit to a high value. TODO: experiment >- * with different values. >+ * with different values. Also send credit update message when >+ * number of bytes in rx queue is not enough to wake up reader. > */ >- if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) >+ if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || >+ low_rx_bytes) > virtio_transport_send_credit_update(vsk); > > return total; >-- >2.25.1 >
On 05.12.2023 13:54, Stefano Garzarella wrote: > On Tue, Dec 05, 2023 at 09:48:05AM +0300, Arseniy Krasnov wrote: >> Add one more condition for sending credit update during dequeue from >> stream socket: when number of bytes in the rx queue is smaller than >> SO_RCVLOWAT value of the socket. This is actual for non-default value >> of SO_RCVLOWAT (e.g. not 1) - idea is to "kick" peer to continue data >> transmission, because we need at least SO_RCVLOWAT bytes in our rx >> queue to wake up user for reading data (in corner case it is also >> possible to stuck both tx and rx sides, this is why 'Fixes' is used). >> >> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >> --- >> net/vmw_vsock/virtio_transport_common.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >> index e137d740804e..461c89882142 100644 >> --- a/net/vmw_vsock/virtio_transport_common.c >> +++ b/net/vmw_vsock/virtio_transport_common.c >> @@ -558,6 +558,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >> struct virtio_vsock_sock *vvs = vsk->trans; >> size_t bytes, total = 0; >> struct sk_buff *skb; >> + bool low_rx_bytes; >> int err = -EFAULT; >> u32 free_space; >> >> @@ -602,6 +603,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >> } >> >> free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >> + low_rx_bytes = (vvs->rx_bytes < >> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); > > As in the previous patch, should we avoid the update it if `fwd_cnt` and `last_fwd_cnt` are the same? > > Now I'm thinking if it is better to add that check directly in virtio_transport_send_credit_update(). Good point, but I think, that it is better to keep this check here, because access to 'fwd_cnt' and 'last_fwd_cnt' requires taking rx_lock - so I guess it is better to avoid taking this lock every time in 'virtio_transport_send_credit_update()'. So may be we can do something like: fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; free_space = vvs->buf_alloc - fwd_cnt_delta; and then, after lock is released: if (fwd_cnt_delta && (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) virtio_transport_send_credit_update(vsk); WDYT? Also, I guess that next idea to update this optimization(in next patchset), is to make threshold depends on vvs->buf_alloc. Because if someone changes minimum buffer size to for example 32KB, and then sets buffer size to 32KB, then free_space will be always non-zero, thus optimization is off now and credit update is sent on every read. Thanks, Arseniy > > Stefano > >> >> spin_unlock_bh(&vvs->rx_lock); >> >> @@ -611,9 +614,11 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >> * too high causes extra messages. Too low causes transmitter >> * stalls. As stalls are in theory more expensive than extra >> * messages, we set the limit to a high value. TODO: experiment >> - * with different values. >> + * with different values. Also send credit update message when >> + * number of bytes in rx queue is not enough to wake up reader. >> */ >> - if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) >> + if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || >> + low_rx_bytes) >> virtio_transport_send_credit_update(vsk); >> >> return total; >> -- >> 2.25.1 >> >
On Tue, Dec 05, 2023 at 03:07:47PM +0300, Arseniy Krasnov wrote: > > >On 05.12.2023 13:54, Stefano Garzarella wrote: >> On Tue, Dec 05, 2023 at 09:48:05AM +0300, Arseniy Krasnov wrote: >>> Add one more condition for sending credit update during dequeue from >>> stream socket: when number of bytes in the rx queue is smaller than >>> SO_RCVLOWAT value of the socket. This is actual for non-default value >>> of SO_RCVLOWAT (e.g. not 1) - idea is to "kick" peer to continue data >>> transmission, because we need at least SO_RCVLOWAT bytes in our rx >>> queue to wake up user for reading data (in corner case it is also >>> possible to stuck both tx and rx sides, this is why 'Fixes' is used). >>> >>> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>> --- >>> net/vmw_vsock/virtio_transport_common.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >>> index e137d740804e..461c89882142 100644 >>> --- a/net/vmw_vsock/virtio_transport_common.c >>> +++ b/net/vmw_vsock/virtio_transport_common.c >>> @@ -558,6 +558,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >>> struct virtio_vsock_sock *vvs = vsk->trans; >>> size_t bytes, total = 0; >>> struct sk_buff *skb; >>> + bool low_rx_bytes; >>> int err = -EFAULT; >>> u32 free_space; >>> >>> @@ -602,6 +603,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >>> } >>> >>> free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >>> + low_rx_bytes = (vvs->rx_bytes < >>> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); >> >> As in the previous patch, should we avoid the update it if `fwd_cnt` and `last_fwd_cnt` are the same? >> >> Now I'm thinking if it is better to add that check directly in virtio_transport_send_credit_update(). > >Good point, but I think, that it is better to keep this check here, because access to 'fwd_cnt' and 'last_fwd_cnt' >requires taking rx_lock - so I guess it is better to avoid taking this lock every time in 'virtio_transport_send_credit_update()'. Yeah, I agree. >So may be we can do something like: > > >fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; >free_space = vvs->buf_alloc - fwd_cnt_delta; Pre-existing issue, but should we handle the wrap (e.g. fwd_cnt wrapped, but last_fwd_cnt not yet?). Maybe in that case we can foce the status update. > >and then, after lock is released: > >if (fwd_cnt_delta && (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || > low_rx_bytes)) > virtio_transport_send_credit_update(vsk); > >WDYT? Yep, I agree. > >Also, I guess that next idea to update this optimization(in next patchset), is to make >threshold depends on vvs->buf_alloc. Because if someone changes minimum buffer size to >for example 32KB, and then sets buffer size to 32KB, then free_space will be always >non-zero, thus optimization is off now and credit update is sent on >every read. But does it make sense to allow a buffer smaller than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE? Maybe we should fail in virtio_transport_notify_buffer_size() or use it as minimum. Stefano
On 05.12.2023 17:21, Stefano Garzarella wrote: > On Tue, Dec 05, 2023 at 03:07:47PM +0300, Arseniy Krasnov wrote: >> >> >> On 05.12.2023 13:54, Stefano Garzarella wrote: >>> On Tue, Dec 05, 2023 at 09:48:05AM +0300, Arseniy Krasnov wrote: >>>> Add one more condition for sending credit update during dequeue from >>>> stream socket: when number of bytes in the rx queue is smaller than >>>> SO_RCVLOWAT value of the socket. This is actual for non-default value >>>> of SO_RCVLOWAT (e.g. not 1) - idea is to "kick" peer to continue data >>>> transmission, because we need at least SO_RCVLOWAT bytes in our rx >>>> queue to wake up user for reading data (in corner case it is also >>>> possible to stuck both tx and rx sides, this is why 'Fixes' is used). >>>> >>>> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >>>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>>> --- >>>> net/vmw_vsock/virtio_transport_common.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >>>> index e137d740804e..461c89882142 100644 >>>> --- a/net/vmw_vsock/virtio_transport_common.c >>>> +++ b/net/vmw_vsock/virtio_transport_common.c >>>> @@ -558,6 +558,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >>>> struct virtio_vsock_sock *vvs = vsk->trans; >>>> size_t bytes, total = 0; >>>> struct sk_buff *skb; >>>> + bool low_rx_bytes; >>>> int err = -EFAULT; >>>> u32 free_space; >>>> >>>> @@ -602,6 +603,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >>>> } >>>> >>>> free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >>>> + low_rx_bytes = (vvs->rx_bytes < >>>> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); >>> >>> As in the previous patch, should we avoid the update it if `fwd_cnt` and `last_fwd_cnt` are the same? >>> >>> Now I'm thinking if it is better to add that check directly in virtio_transport_send_credit_update(). >> >> Good point, but I think, that it is better to keep this check here, because access to 'fwd_cnt' and 'last_fwd_cnt' >> requires taking rx_lock - so I guess it is better to avoid taking this lock every time in 'virtio_transport_send_credit_update()'. > > Yeah, I agree. > >> So may be we can do something like: >> >> >> fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; >> free_space = vvs->buf_alloc - fwd_cnt_delta; > > Pre-existing issue, but should we handle the wrap (e.g. fwd_cnt wrapped, but last_fwd_cnt not yet?). Maybe in that case we can foce the status > update. Agree, I'll add this logic! > >> >> and then, after lock is released: >> >> if (fwd_cnt_delta && (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || >> low_rx_bytes)) >> virtio_transport_send_credit_update(vsk); >> >> WDYT? > > Yep, I agree. > >> >> Also, I guess that next idea to update this optimization(in next patchset), is to make >> threshold depends on vvs->buf_alloc. Because if someone changes minimum buffer size to >> for example 32KB, and then sets buffer size to 32KB, then free_space will be always >> non-zero, thus optimization is off now and credit update is sent on every read. > > But does it make sense to allow a buffer smaller than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE? > > Maybe we should fail in virtio_transport_notify_buffer_size() or use it as minimum. Yes, currently there is no limitation in this transport callback - only for maximum. Thanks, Arseniy > > Stefano >
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index e137d740804e..461c89882142 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -558,6 +558,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, struct virtio_vsock_sock *vvs = vsk->trans; size_t bytes, total = 0; struct sk_buff *skb; + bool low_rx_bytes; int err = -EFAULT; u32 free_space; @@ -602,6 +603,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, } free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); + low_rx_bytes = (vvs->rx_bytes < + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); spin_unlock_bh(&vvs->rx_lock); @@ -611,9 +614,11 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, * too high causes extra messages. Too low causes transmitter * stalls. As stalls are in theory more expensive than extra * messages, we set the limit to a high value. TODO: experiment - * with different values. + * with different values. Also send credit update message when + * number of bytes in rx queue is not enough to wake up reader. */ - if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) + if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || + low_rx_bytes) virtio_transport_send_credit_update(vsk); return total;