Message ID | 20231206211849.2707151-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 r17csp4389086vqy; Wed, 6 Dec 2023 13:27:33 -0800 (PST) X-Google-Smtp-Source: AGHT+IHMGumEWxJBiJREqqnRbDj8Rqml2ZG3zhtC2VAvsUat8wlPiIeo5Tp4g35UWvqn1JGU0oKX X-Received: by 2002:a05:6a20:9390:b0:188:1125:88bd with SMTP id x16-20020a056a20939000b00188112588bdmr1711906pzh.43.1701898053494; Wed, 06 Dec 2023 13:27:33 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701898053; cv=none; d=google.com; s=arc-20160816; b=Xeyq40jKXAsmA11jcviQNO0hbSEylpYq9B1/rXdces6NEf3UGD8KJXFKJFKm39eDoF 8KfE6aVHmdKr+Yuo6oibY099Tf9m2iY5yljN3TK2vAf+6su82ubjseimmiQDNXQiBc6B dqMEzCVAJHs7uFrBlrx2hKtFccibA51tEi2S3j/M1HoVwhizLC/8Q/agnPwzsrYMKeFg LlVIHxIaLg7SPNfDyNlqZ8OemQHpOL1wZ/vNLPTQ4FAFuBkowsAebrOiBYbQ62+CN151 QwTKZMjJSN7Tqi6J9/1F3a2QYUB7FtXcULFZ3/3HMIWl2bFmr6Ergis4+EkCk7l1szCR on8g== 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=GHNWtOPMCbudLrNVAc3MZd8FL3ABIjio3CjPcM+SRoM=; fh=wIHqZpOIuzcidDZ82yQbOZuyHJty7uvaWDEh/efoVzA=; b=WuObX7s6MBfOhkyH+/xD8ETo5E1INJ8grFA0YkjG3Jlxe26YpgMBZdSluxxiQgx5zJ AOJ+OQ645G+TukEY4KKru+ao8a4zUyTalVKWb+VzTEpt+KMQx3L3i4qpIEDkZAT1zyLG xtl2XoZKOc8/AZCaFQyHZ33vuD8w6kmDHtiFoOA8N4k0N/iZnx52q+uckJLMnLPs3cfg CsPirI1biuwTJoWPuOZAolHX89bNRyDuBrKhKFYjCQcV+PkJPSdT6xsEfwW8Fa+GSfzx Jx+bORu4hMphYCTiEb1rthZhOaOy0atYM6+Oa8/czAfcVxGgNKz9we3QnqNCzHoeIWZW J4BA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@salutedevices.com header.s=mail header.b=n8esJVfM; 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 j10-20020a056a00130a00b006ce980c788esi535451pfu.309.2023.12.06.13.27.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Dec 2023 13:27:33 -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=n8esJVfM; 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 D163382B8A66; Wed, 6 Dec 2023 13:27:29 -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 S1442944AbjLFV1P (ORCPT <rfc822;pusanteemu@gmail.com> + 99 others); Wed, 6 Dec 2023 16:27:15 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58764 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1379657AbjLFV1I (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 6 Dec 2023 16:27:08 -0500 Received: from mx1.sberdevices.ru (mx2.sberdevices.ru [45.89.224.132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 38CC0D5C; Wed, 6 Dec 2023 13:27:12 -0800 (PST) Received: from p-infra-ksmg-sc-msk02 (localhost [127.0.0.1]) by mx1.sberdevices.ru (Postfix) with ESMTP id 7EDFC120066; Thu, 7 Dec 2023 00:27:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.sberdevices.ru 7EDFC120066 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=salutedevices.com; s=mail; t=1701898030; bh=GHNWtOPMCbudLrNVAc3MZd8FL3ABIjio3CjPcM+SRoM=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type:From; b=n8esJVfMhQwGDoRuF912rYf1+AWbKOjfkBXWNiFGLr418uHCc8qupouUhRs7n+r0Y t1jpV7C86ZSdnY7sqEf50J1DSHQ4JkgZBqJTWO3mweXGxXiteB0lYofMESuPsscfd8 JcBjmvybZSxGoT3B2fYfN4dAAlDkhKMKZu9VLrJAkQM9NEzxH5uLtSTOW08D4lg/Ol Oa3oRBW/yB85O0s/ZJx0EY9YUl8tBY1FmiAc04yEVYEOxX/BErK8Q7Vxu6fZkOSUjd 8KRmz1MpPpGd/0zS7DIq6AjHNRL3NpuI0Rg9x/OI72bz+AeytEaiFNxhTKhyMxsqeZ GIfy5EbNOEHow== 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; Thu, 7 Dec 2023 00:27:10 +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; Thu, 7 Dec 2023 00:27:10 +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 v7 3/4] virtio/vsock: fix logic which reduces credit update messages Date: Thu, 7 Dec 2023 00:18:48 +0300 Message-ID: <20231206211849.2707151-4-avkrasnov@salutedevices.com> X-Mailer: git-send-email 2.35.0 In-Reply-To: <20231206211849.2707151-1-avkrasnov@salutedevices.com> References: <20231206211849.2707151-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: 181917 [Dec 06 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}, d41d8cd98f00b204e9800998ecf8427e.com:7.1.1;100.64.160.123:7.1.2;p-i-exch-sc-m01.sberdevices.ru:7.1.1,5.0.1;127.0.0.199:7.1.2;salutedevices.com:7.1.1, 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/06 19:07:00 #22622451 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]); Wed, 06 Dec 2023 13:27:30 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784569453445237122 X-GMAIL-MSGID: 1784569453445237122 |
Series |
send credit update during setting SO_RCVLOWAT
|
|
Commit Message
Arseniy Krasnov
Dec. 6, 2023, 9:18 p.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).
Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still
not.
Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages")
Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
Changelog:
v6 -> v7:
* Handle wrap of 'fwd_cnt'.
* Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'.
net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
Comments
On 07.12.2023 00:53, Michael S. Tsirkin wrote: > On Thu, Dec 07, 2023 at 12:18:48AM +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). >> Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still >> not. >> >> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >> --- >> Changelog: >> v6 -> v7: >> * Handle wrap of 'fwd_cnt'. >> * Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'. >> >> net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >> index e137d740804e..39f8660d825d 100644 >> --- a/net/vmw_vsock/virtio_transport_common.c >> +++ b/net/vmw_vsock/virtio_transport_common.c >> @@ -558,6 +558,8 @@ 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; >> + u32 fwd_cnt_delta; >> + bool low_rx_bytes; >> int err = -EFAULT; >> u32 free_space; >> >> @@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >> } >> } >> >> - free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >> + /* Handle wrap of 'fwd_cnt'. */ >> + if (vvs->fwd_cnt < vvs->last_fwd_cnt) >> + fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt); > > Are you sure there's no off by one here? for example if fwd_cnt is 0 > and last_fwd_cnt is 0xfffffffff then apparently delta is 0. Seems yes, I need +1 here > > >> + else >> + fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; > > I actually don't see what is wrong with just > fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt > 32 bit unsigned math will I think handle wrap around correctly. > > And given buf_alloc is also u32 - I don't see where the bug is in > the original code. I think problem is when fwd_cnt wraps, while last_fwd_cnt is not. In this case fwd_cnt_delta will be too big, so we won't send credit update which leads to stall for sender Thanks, Arseniy > > >> + >> + free_space = vvs->buf_alloc - fwd_cnt_delta; >> + low_rx_bytes = (vvs->rx_bytes < >> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); >> >> spin_unlock_bh(&vvs->rx_lock); >> >> @@ -611,9 +621,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 (fwd_cnt_delta && >> + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) >> virtio_transport_send_credit_update(vsk); >> >> return total; >> -- >> 2.25.1 >
On Thu, Dec 07, 2023 at 12:18:48AM +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). > Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still > not. > > Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") > Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> > --- > Changelog: > v6 -> v7: > * Handle wrap of 'fwd_cnt'. > * Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'. > > net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c > index e137d740804e..39f8660d825d 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -558,6 +558,8 @@ 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; > + u32 fwd_cnt_delta; > + bool low_rx_bytes; > int err = -EFAULT; > u32 free_space; > > @@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > } > } > > - free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); > + /* Handle wrap of 'fwd_cnt'. */ > + if (vvs->fwd_cnt < vvs->last_fwd_cnt) > + fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt); Are you sure there's no off by one here? for example if fwd_cnt is 0 and last_fwd_cnt is 0xfffffffff then apparently delta is 0. > + else > + fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; I actually don't see what is wrong with just fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt 32 bit unsigned math will I think handle wrap around correctly. And given buf_alloc is also u32 - I don't see where the bug is in the original code. > + > + free_space = vvs->buf_alloc - fwd_cnt_delta; > + low_rx_bytes = (vvs->rx_bytes < > + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); > > spin_unlock_bh(&vvs->rx_lock); > > @@ -611,9 +621,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 (fwd_cnt_delta && > + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) > virtio_transport_send_credit_update(vsk); > > return total; > -- > 2.25.1
On Thu, Dec 07, 2023 at 12:52:51AM +0300, Arseniy Krasnov wrote: > > > On 07.12.2023 00:53, Michael S. Tsirkin wrote: > > On Thu, Dec 07, 2023 at 12:18:48AM +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). > >> Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still > >> not. > >> > >> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") > >> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> > >> --- > >> Changelog: > >> v6 -> v7: > >> * Handle wrap of 'fwd_cnt'. > >> * Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'. > >> > >> net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++--- > >> 1 file changed, 15 insertions(+), 3 deletions(-) > >> > >> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c > >> index e137d740804e..39f8660d825d 100644 > >> --- a/net/vmw_vsock/virtio_transport_common.c > >> +++ b/net/vmw_vsock/virtio_transport_common.c > >> @@ -558,6 +558,8 @@ 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; > >> + u32 fwd_cnt_delta; > >> + bool low_rx_bytes; > >> int err = -EFAULT; > >> u32 free_space; > >> > >> @@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > >> } > >> } > >> > >> - free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); > >> + /* Handle wrap of 'fwd_cnt'. */ > >> + if (vvs->fwd_cnt < vvs->last_fwd_cnt) > >> + fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt); > > > > Are you sure there's no off by one here? for example if fwd_cnt is 0 > > and last_fwd_cnt is 0xfffffffff then apparently delta is 0. > > Seems yes, I need +1 here And then you will get a nop, because assigning U32_MAX + 1 to u32 gives you 0. Adding () does nothing to change the result, + and - are commutative. > > > > > >> + else > >> + fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; > > > > I actually don't see what is wrong with just > > fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt > > 32 bit unsigned math will I think handle wrap around correctly. > > > > And given buf_alloc is also u32 - I don't see where the bug is in > > the original code. > > I think problem is when fwd_cnt wraps, while last_fwd_cnt is not. In this > case fwd_cnt_delta will be too big, so we won't send credit update which > leads to stall for sender > > Thanks, Arseniy Care coming up with an example? > > > > > >> + > >> + free_space = vvs->buf_alloc - fwd_cnt_delta; > >> + low_rx_bytes = (vvs->rx_bytes < > >> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); > >> > >> spin_unlock_bh(&vvs->rx_lock); > >> > >> @@ -611,9 +621,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 (fwd_cnt_delta && > >> + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) > >> virtio_transport_send_credit_update(vsk); > >> > >> return total; > >> -- > >> 2.25.1 > >
On 07.12.2023 01:08, Michael S. Tsirkin wrote: > On Thu, Dec 07, 2023 at 12:52:51AM +0300, Arseniy Krasnov wrote: >> >> >> On 07.12.2023 00:53, Michael S. Tsirkin wrote: >>> On Thu, Dec 07, 2023 at 12:18:48AM +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). >>>> Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still >>>> not. >>>> >>>> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >>>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>>> --- >>>> Changelog: >>>> v6 -> v7: >>>> * Handle wrap of 'fwd_cnt'. >>>> * Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'. >>>> >>>> net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++--- >>>> 1 file changed, 15 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >>>> index e137d740804e..39f8660d825d 100644 >>>> --- a/net/vmw_vsock/virtio_transport_common.c >>>> +++ b/net/vmw_vsock/virtio_transport_common.c >>>> @@ -558,6 +558,8 @@ 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; >>>> + u32 fwd_cnt_delta; >>>> + bool low_rx_bytes; >>>> int err = -EFAULT; >>>> u32 free_space; >>>> >>>> @@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >>>> } >>>> } >>>> >>>> - free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >>>> + /* Handle wrap of 'fwd_cnt'. */ >>>> + if (vvs->fwd_cnt < vvs->last_fwd_cnt) >>>> + fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt); >>> >>> Are you sure there's no off by one here? for example if fwd_cnt is 0 >>> and last_fwd_cnt is 0xfffffffff then apparently delta is 0. >> >> Seems yes, I need +1 here > > And then you will get a nop, because assigning U32_MAX + 1 to u32 > gives you 0. Adding () does nothing to change the result, > + and - are commutative. Ahh, unsigned here, yes. @Stefano, what did You mean about wrapping here? I think Michael is right, for example vvs->fwd_cnt wraps and now == 5 vvs->last_fwd_cnt == 0xffffffff now delta before this patch will be 6 - correct value May be I didn't get your idea, so implement it very naive? Thanks, Arseniy > > >>> >>> >>>> + else >>>> + fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; >>> >>> I actually don't see what is wrong with just >>> fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt >>> 32 bit unsigned math will I think handle wrap around correctly. >>> >>> And given buf_alloc is also u32 - I don't see where the bug is in >>> the original code. >> >> I think problem is when fwd_cnt wraps, while last_fwd_cnt is not. In this >> case fwd_cnt_delta will be too big, so we won't send credit update which >> leads to stall for sender >> >> Thanks, Arseniy > > Care coming up with an example? > > >>> >>> >>>> + >>>> + free_space = vvs->buf_alloc - fwd_cnt_delta; >>>> + low_rx_bytes = (vvs->rx_bytes < >>>> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); >>>> >>>> spin_unlock_bh(&vvs->rx_lock); >>>> >>>> @@ -611,9 +621,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 (fwd_cnt_delta && >>>> + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) >>>> virtio_transport_send_credit_update(vsk); >>>> >>>> return total; >>>> -- >>>> 2.25.1 >>> >
On 11.12.2023 15:01, Stefano Garzarella wrote: > On Thu, Dec 07, 2023 at 01:50:05AM +0300, Arseniy Krasnov wrote: >> >> >> On 07.12.2023 01:08, Michael S. Tsirkin wrote: >>> On Thu, Dec 07, 2023 at 12:52:51AM +0300, Arseniy Krasnov wrote: >>>> >>>> >>>> On 07.12.2023 00:53, Michael S. Tsirkin wrote: >>>>> On Thu, Dec 07, 2023 at 12:18:48AM +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). >>>>>> Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still >>>>>> not. >>>>>> >>>>>> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >>>>>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>>>>> --- >>>>>> Changelog: >>>>>> v6 -> v7: >>>>>> * Handle wrap of 'fwd_cnt'. >>>>>> * Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'. >>>>>> >>>>>> net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++--- >>>>>> 1 file changed, 15 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >>>>>> index e137d740804e..39f8660d825d 100644 >>>>>> --- a/net/vmw_vsock/virtio_transport_common.c >>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c >>>>>> @@ -558,6 +558,8 @@ 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; >>>>>> + u32 fwd_cnt_delta; >>>>>> + bool low_rx_bytes; >>>>>> int err = -EFAULT; >>>>>> u32 free_space; >>>>>> >>>>>> @@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >>>>>> } >>>>>> } >>>>>> >>>>>> - free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >>>>>> + /* Handle wrap of 'fwd_cnt'. */ >>>>>> + if (vvs->fwd_cnt < vvs->last_fwd_cnt) >>>>>> + fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt); >>>>> >>>>> Are you sure there's no off by one here? for example if fwd_cnt is 0 >>>>> and last_fwd_cnt is 0xfffffffff then apparently delta is 0. >>>> >>>> Seems yes, I need +1 here >>> >>> And then you will get a nop, because assigning U32_MAX + 1 to u32 >>> gives you 0. Adding () does nothing to change the result, >>> + and - are commutative. >> >> Ahh, unsigned here, yes. > > Ooops, sorry I was confused here! > >> >> @Stefano, what did You mean about wrapping here? >> >> I think Michael is right, for example > > Yep, I agree! > Sorry for this wrong suggestion! Got it! I'll remove it, no problem Thanks, Arseniy > > Stefano > >> >> vvs->fwd_cnt wraps and now == 5 >> vvs->last_fwd_cnt == 0xffffffff >> >> now delta before this patch will be 6 - correct value >> >> May be I didn't get your idea, so implement it very naive? >> >> Thanks, Arseniy >> >>> >>> >>>>> >>>>> >>>>>> + else >>>>>> + fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; >>>>> >>>>> I actually don't see what is wrong with just >>>>> fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt >>>>> 32 bit unsigned math will I think handle wrap around correctly. >>>>> >>>>> And given buf_alloc is also u32 - I don't see where the bug is in >>>>> the original code. >>>> >>>> I think problem is when fwd_cnt wraps, while last_fwd_cnt is not. In this >>>> case fwd_cnt_delta will be too big, so we won't send credit update which >>>> leads to stall for sender >>>> >>>> Thanks, Arseniy >>> >>> Care coming up with an example? >>> >>> >>>>> >>>>> >>>>>> + >>>>>> + free_space = vvs->buf_alloc - fwd_cnt_delta; >>>>>> + low_rx_bytes = (vvs->rx_bytes < >>>>>> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); >>>>>> >>>>>> spin_unlock_bh(&vvs->rx_lock); >>>>>> >>>>>> @@ -611,9 +621,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 (fwd_cnt_delta && >>>>>> + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) >>>>>> virtio_transport_send_credit_update(vsk); >>>>>> >>>>>> return total; >>>>>> -- >>>>>> 2.25.1 >>>>> >>> >> >
On Thu, Dec 07, 2023 at 01:50:05AM +0300, Arseniy Krasnov wrote: > > >On 07.12.2023 01:08, Michael S. Tsirkin wrote: >> On Thu, Dec 07, 2023 at 12:52:51AM +0300, Arseniy Krasnov wrote: >>> >>> >>> On 07.12.2023 00:53, Michael S. Tsirkin wrote: >>>> On Thu, Dec 07, 2023 at 12:18:48AM +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). >>>>> Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still >>>>> not. >>>>> >>>>> Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages") >>>>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>>>> --- >>>>> Changelog: >>>>> v6 -> v7: >>>>> * Handle wrap of 'fwd_cnt'. >>>>> * Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'. >>>>> >>>>> net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++--- >>>>> 1 file changed, 15 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >>>>> index e137d740804e..39f8660d825d 100644 >>>>> --- a/net/vmw_vsock/virtio_transport_common.c >>>>> +++ b/net/vmw_vsock/virtio_transport_common.c >>>>> @@ -558,6 +558,8 @@ 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; >>>>> + u32 fwd_cnt_delta; >>>>> + bool low_rx_bytes; >>>>> int err = -EFAULT; >>>>> u32 free_space; >>>>> >>>>> @@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >>>>> } >>>>> } >>>>> >>>>> - free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); >>>>> + /* Handle wrap of 'fwd_cnt'. */ >>>>> + if (vvs->fwd_cnt < vvs->last_fwd_cnt) >>>>> + fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt); >>>> >>>> Are you sure there's no off by one here? for example if fwd_cnt is 0 >>>> and last_fwd_cnt is 0xfffffffff then apparently delta is 0. >>> >>> Seems yes, I need +1 here >> >> And then you will get a nop, because assigning U32_MAX + 1 to u32 >> gives you 0. Adding () does nothing to change the result, >> + and - are commutative. > >Ahh, unsigned here, yes. Ooops, sorry I was confused here! > >@Stefano, what did You mean about wrapping here? > >I think Michael is right, for example Yep, I agree! Sorry for this wrong suggestion! Stefano > >vvs->fwd_cnt wraps and now == 5 >vvs->last_fwd_cnt == 0xffffffff > >now delta before this patch will be 6 - correct value > >May be I didn't get your idea, so implement it very naive? > >Thanks, Arseniy > >> >> >>>> >>>> >>>>> + else >>>>> + fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; >>>> >>>> I actually don't see what is wrong with just >>>> fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt >>>> 32 bit unsigned math will I think handle wrap around correctly. >>>> >>>> And given buf_alloc is also u32 - I don't see where the bug is in >>>> the original code. >>> >>> I think problem is when fwd_cnt wraps, while last_fwd_cnt is not. In this >>> case fwd_cnt_delta will be too big, so we won't send credit update which >>> leads to stall for sender >>> >>> Thanks, Arseniy >> >> Care coming up with an example? >> >> >>>> >>>> >>>>> + >>>>> + free_space = vvs->buf_alloc - fwd_cnt_delta; >>>>> + low_rx_bytes = (vvs->rx_bytes < >>>>> + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); >>>>> >>>>> spin_unlock_bh(&vvs->rx_lock); >>>>> >>>>> @@ -611,9 +621,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 (fwd_cnt_delta && >>>>> + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) >>>>> virtio_transport_send_credit_update(vsk); >>>>> >>>>> return total; >>>>> -- >>>>> 2.25.1 >>>> >> >
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index e137d740804e..39f8660d825d 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -558,6 +558,8 @@ 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; + u32 fwd_cnt_delta; + bool low_rx_bytes; int err = -EFAULT; u32 free_space; @@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, } } - free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); + /* Handle wrap of 'fwd_cnt'. */ + if (vvs->fwd_cnt < vvs->last_fwd_cnt) + fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt); + else + fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt; + + free_space = vvs->buf_alloc - fwd_cnt_delta; + low_rx_bytes = (vvs->rx_bytes < + sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX)); spin_unlock_bh(&vvs->rx_lock); @@ -611,9 +621,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 (fwd_cnt_delta && + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes)) virtio_transport_send_credit_update(vsk); return total;