Message ID | 20231108072004.1045669-3-avkrasnov@salutedevices.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:aa0b:0:b0:403:3b70:6f57 with SMTP id k11csp750225vqo; Tue, 7 Nov 2023 23:28:48 -0800 (PST) X-Google-Smtp-Source: AGHT+IHVKPj6g4Kya8DBIWspKb/MaCgVOucTrFQRCn+ytrRXjaVfK+rqS5cPBrIkyGMencFVFML+ X-Received: by 2002:a17:902:d486:b0:1cc:5f5a:5d3 with SMTP id c6-20020a170902d48600b001cc5f5a05d3mr1751893plg.22.1699428528305; Tue, 07 Nov 2023 23:28:48 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1699428528; cv=none; d=google.com; s=arc-20160816; b=WdaWBUqaGh1qVLJkHV31OSkCSmqElLRmQBCfPS0FGTPfegTXER+I2upJc+HwWF+7sQ IXSAKIa3uw1wJeTGTcsr3UoUM9RZyV7BAB+a9CAzdN6BNkNL2h8ZRETxf6Q+hFCKCiYL NgkjwZrOcM3Pu/vLAqSKIkRzOEhoeNOpub+RjYmz33slqEp9lMRKar4V7J+2sv41Mjlu 4DmTzcitLGUbviUgQIhVpwvhYkQFnn0NufQ0RnX7Y5+qgAHujt3MAJ5eMdDt7x7IgPCr 8qgSCpKsVSWsykv2vgijY5D+TYPcvALqDcBMrajJKTrsmaCuYzgpfrQmh4wsq6f+WcMg LVGA== 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=RpkC5lfXlkMFYUw1ymgu4ywMbNeocePvX94SLreWShw=; fh=wIHqZpOIuzcidDZ82yQbOZuyHJty7uvaWDEh/efoVzA=; b=XY7QYQ2JkA3LFt0rAYJcj6qNXr7qyC3J7HiSgfovDFoooV+7d+IkyvinrbelaQnvCD PqUPEVFAxbvpD2w8VhxeLLcM7M1Slc35X4izfpvhI85+Ese0ljKFJjHKj70U/0d02IGU IJ02nBkXxPyxv79zgeJza9qc6UFAJAawCfOW0d96EDdoYWcREVFHEJqkc4fVEYkvX3ZJ cu7whCsT9xvquXtk8PiVkNFlPtiN89hiuD8UBPI/zU3uXxe04UBy8cBNvBRa04oFx1BO pZhkJ1kkIKoU72kid2P8umK1pZeiM8CTI00pdfxd887KruG+4vHJErwNEemaNepEWpZh 0RNw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@salutedevices.com header.s=mail header.b=HqYz1f+q; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 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 fry.vger.email (fry.vger.email. [23.128.96.38]) by mx.google.com with ESMTPS id p14-20020a170902e74e00b001b894687ec9si1794017plf.462.2023.11.07.23.28.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Nov 2023 23:28:48 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38; Authentication-Results: mx.google.com; dkim=pass header.i=@salutedevices.com header.s=mail header.b=HqYz1f+q; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 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 fry.vger.email (Postfix) with ESMTP id 3A43D81ED50B; Tue, 7 Nov 2023 23:28:45 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343544AbjKHH17 (ORCPT <rfc822;jaysivo@gmail.com> + 32 others); Wed, 8 Nov 2023 02:27:59 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48376 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234922AbjKHH1v (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 8 Nov 2023 02:27:51 -0500 Received: from mx1.sberdevices.ru (mx1.sberdevices.ru [37.18.73.165]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 073E8170D; Tue, 7 Nov 2023 23:27:48 -0800 (PST) Received: from p-infra-ksmg-sc-msk01 (localhost [127.0.0.1]) by mx1.sberdevices.ru (Postfix) with ESMTP id 0698C100024; Wed, 8 Nov 2023 10:27:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.sberdevices.ru 0698C100024 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=salutedevices.com; s=mail; t=1699428467; bh=RpkC5lfXlkMFYUw1ymgu4ywMbNeocePvX94SLreWShw=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type:From; b=HqYz1f+qOByC8+Xdh71Jlne1jsd+gBV7aEZxjFrh2eHufZy8I7eDhlyBjD1hz0mTA r6KmRxK+VbNEzG+XL7Ip1Bw0fFqY352dq+0kKdro9chfjs2aXAEbkbKM4zNu/wyT0g mAigMZgyRx9HuJBhjUUv0QS5DQ2E+vJI09fhvmFJnNgV3PKjy4cJ22gnvNkztTjeqJ 7JdWKO2+fsOl7dr3zhWJdknPvW+Mh7wJYybC4pjOq92HhHLT33flHMfA+3RNYsT1s6 cvk0SS54bsehDigFqR9l5UbKly+BaGO6asi6xzRwgsqnEhhshG+ooNsRQ5+ewdfb/d fylXNVMS7vZkg== 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; Wed, 8 Nov 2023 10:27:46 +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.37; Wed, 8 Nov 2023 10:27:46 +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: [RFC PATCH v1 2/2] vsock/test: SO_RCVLOWAT + deferred credit update test Date: Wed, 8 Nov 2023 10:20:04 +0300 Message-ID: <20231108072004.1045669-3-avkrasnov@salutedevices.com> X-Mailer: git-send-email 2.35.0 In-Reply-To: <20231108072004.1045669-1-avkrasnov@salutedevices.com> References: <20231108072004.1045669-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: 181188 [Nov 08 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: 543 543 1e3516af5cdd92079dfeb0e292c8747a62cb1ee4, {Tracking_from_domain_doesnt_match_to}, p-i-exch-sc-m01.sberdevices.ru:5.0.1,7.1.1;salutedevices.com:7.1.1;127.0.0.199:7.1.2;d41d8cd98f00b204e9800998ecf8427e.com:7.1.1;100.64.160.123: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/11/08 04:00:00 #22424297 X-KSMG-AntiVirus-Status: Clean, skipped 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 (fry.vger.email [0.0.0.0]); Tue, 07 Nov 2023 23:28:45 -0800 (PST) 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 fry.vger.email X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781979968483531310 X-GMAIL-MSGID: 1781979968483531310 |
Series |
send credit update during setting SO_RCVLOWAT
|
|
Commit Message
Arseniy Krasnov
Nov. 8, 2023, 7:20 a.m. UTC
This adds test which checks, that updating SO_RCVLOWAT value also sends
credit update message. Otherwise mutual hungup may happen when receiver
didn't send credit update and then calls 'poll()' with non default
SO_RCVLOWAT value (e.g. waiting enough bytes to read), while sender
waits for free space at receiver's side.
Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
tools/testing/vsock/vsock_test.c | 131 +++++++++++++++++++++++++++++++
1 file changed, 131 insertions(+)
Comments
On Wed, Nov 08, 2023 at 10:20:04AM +0300, Arseniy Krasnov wrote: >This adds test which checks, that updating SO_RCVLOWAT value also sends You can avoid "This adds", and write just "Add test ...". See https://docs.kernel.org/process/submitting-patches.html#describe-your-changes Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. Also in the other patch. >credit update message. Otherwise mutual hungup may happen when receiver >didn't send credit update and then calls 'poll()' with non default >SO_RCVLOWAT value (e.g. waiting enough bytes to read), while sender >waits for free space at receiver's side. > >Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >--- > tools/testing/vsock/vsock_test.c | 131 +++++++++++++++++++++++++++++++ > 1 file changed, 131 insertions(+) > >diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >index c1f7bc9abd22..c71b3875fd16 100644 >--- a/tools/testing/vsock/vsock_test.c >+++ b/tools/testing/vsock/vsock_test.c >@@ -1180,6 +1180,132 @@ static void test_stream_shutrd_server(const struct test_opts *opts) > close(fd); > } > >+#define RCVLOWAT_CREDIT_UPD_BUF_SIZE (1024 * 128) >+#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64) What about adding a comment like the one in the cover letter about dependency with kernel values? Please add it also in the commit description. I'm thinking if we should move all the defines that depends on the kernel in some special header. >+ >+static void test_stream_rcvlowat_def_cred_upd_client(const struct test_opts *opts) >+{ >+ size_t buf_size; >+ void *buf; >+ int fd; >+ >+ fd = vsock_stream_connect(opts->peer_cid, 1234); >+ if (fd < 0) { >+ perror("connect"); >+ exit(EXIT_FAILURE); >+ } >+ >+ /* Send 1 byte more than peer's buffer size. */ >+ buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE + 1; >+ >+ buf = malloc(buf_size); >+ if (!buf) { >+ perror("malloc"); >+ exit(EXIT_FAILURE); >+ } >+ >+ /* Wait until peer sets needed buffer size. */ >+ control_expectln("SRVREADY"); >+ >+ if (send(fd, buf, buf_size, 0) != buf_size) { >+ perror("send failed"); >+ exit(EXIT_FAILURE); >+ } >+ >+ free(buf); >+ close(fd); >+} >+ >+static void test_stream_rcvlowat_def_cred_upd_server(const struct test_opts *opts) >+{ >+ size_t recv_buf_size; >+ struct pollfd fds; >+ size_t buf_size; >+ void *buf; >+ int fd; >+ >+ fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >+ if (fd < 0) { >+ perror("accept"); >+ exit(EXIT_FAILURE); >+ } >+ >+ buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE; >+ >+ if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE, >+ &buf_size, sizeof(buf_size))) { >+ perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"); >+ exit(EXIT_FAILURE); >+ } >+ >+ buf = malloc(buf_size); >+ if (!buf) { >+ perror("malloc"); >+ exit(EXIT_FAILURE); >+ } >+ >+ control_writeln("SRVREADY"); >+ >+ /* Wait until there will be 128KB of data in rx queue. */ >+ while (1) { >+ ssize_t res; >+ >+ res = recv(fd, buf, buf_size, MSG_PEEK); >+ if (res == buf_size) >+ break; >+ >+ if (res <= 0) { >+ fprintf(stderr, "unexpected 'recv()' return: %zi\n", res); >+ exit(EXIT_FAILURE); >+ } >+ } >+ >+ /* There is 128KB of data in the socket's rx queue, >+ * dequeue first 64KB, credit update is not sent. >+ */ >+ recv_buf_size = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE; >+ recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size); >+ recv_buf_size++; >+ >+ /* Updating SO_RCVLOWAT will send credit update. */ >+ if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT, >+ &recv_buf_size, sizeof(recv_buf_size))) { >+ perror("setsockopt(SO_RCVLOWAT)"); >+ exit(EXIT_FAILURE); >+ } >+ >+ memset(&fds, 0, sizeof(fds)); >+ fds.fd = fd; >+ fds.events = POLLIN | POLLRDNORM | POLLERR | >+ POLLRDHUP | POLLHUP; >+ >+ /* This 'poll()' will return once we receive last byte >+ * sent by client. >+ */ >+ if (poll(&fds, 1, -1) < 0) { >+ perror("poll"); >+ exit(EXIT_FAILURE); >+ } >+ >+ if (fds.revents & POLLERR) { >+ fprintf(stderr, "'poll()' error\n"); >+ exit(EXIT_FAILURE); >+ } >+ >+ if (fds.revents & (POLLIN | POLLRDNORM)) { >+ recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size); >+ } else { >+ /* These flags must be set, as there is at >+ * least 64KB of data ready to read. >+ */ >+ fprintf(stderr, "POLLIN | POLLRDNORM expected\n"); >+ exit(EXIT_FAILURE); >+ } >+ >+ free(buf); >+ close(fd); >+} >+ > static struct test_case test_cases[] = { > { > .name = "SOCK_STREAM connection reset", >@@ -1285,6 +1411,11 @@ static struct test_case test_cases[] = { > .run_client = test_stream_msgzcopy_empty_errq_client, > .run_server = test_stream_msgzcopy_empty_errq_server, > }, >+ { >+ .name = "SOCK_STREAM virtio SO_RCVLOWAT + deferred cred update", >+ .run_client = test_stream_rcvlowat_def_cred_upd_client, >+ .run_server = test_stream_rcvlowat_def_cred_upd_server, >+ }, > {}, > }; > >-- >2.25.1 >
On 15.11.2023 14:11, Stefano Garzarella wrote: > On Wed, Nov 08, 2023 at 10:20:04AM +0300, Arseniy Krasnov wrote: >> This adds test which checks, that updating SO_RCVLOWAT value also sends > > You can avoid "This adds", and write just "Add test ...". > > See https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > > Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour. > > Also in the other patch. > >> credit update message. Otherwise mutual hungup may happen when receiver >> didn't send credit update and then calls 'poll()' with non default >> SO_RCVLOWAT value (e.g. waiting enough bytes to read), while sender >> waits for free space at receiver's side. >> >> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >> --- >> tools/testing/vsock/vsock_test.c | 131 +++++++++++++++++++++++++++++++ >> 1 file changed, 131 insertions(+) >> >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >> index c1f7bc9abd22..c71b3875fd16 100644 >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -1180,6 +1180,132 @@ static void test_stream_shutrd_server(const struct test_opts *opts) >> close(fd); >> } >> >> +#define RCVLOWAT_CREDIT_UPD_BUF_SIZE (1024 * 128) >> +#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64) > > What about adding a comment like the one in the cover letter about > dependency with kernel values? > > Please add it also in the commit description. > > I'm thinking if we should move all the defines that depends on the > kernel in some special header. IIUC it will be new header file in tools/testing/vsock, which includes such defines. At this moment in will contain only VIRTIO_VSOCK_MAX_PKT_BUF_SIZE. Idea is that such defines are not supposed to use by user (so do not move it to uapi headers), but needed by tests to check kernel behaviour. Please correct me if i'm wrong. Thanks, Arseniy > >> + >> +static void test_stream_rcvlowat_def_cred_upd_client(const struct test_opts *opts) >> +{ >> + size_t buf_size; >> + void *buf; >> + int fd; >> + >> + fd = vsock_stream_connect(opts->peer_cid, 1234); >> + if (fd < 0) { >> + perror("connect"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Send 1 byte more than peer's buffer size. */ >> + buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE + 1; >> + >> + buf = malloc(buf_size); >> + if (!buf) { >> + perror("malloc"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Wait until peer sets needed buffer size. */ >> + control_expectln("SRVREADY"); >> + >> + if (send(fd, buf, buf_size, 0) != buf_size) { >> + perror("send failed"); >> + exit(EXIT_FAILURE); >> + } >> + >> + free(buf); >> + close(fd); >> +} >> + >> +static void test_stream_rcvlowat_def_cred_upd_server(const struct test_opts *opts) >> +{ >> + size_t recv_buf_size; >> + struct pollfd fds; >> + size_t buf_size; >> + void *buf; >> + int fd; >> + >> + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >> + if (fd < 0) { >> + perror("accept"); >> + exit(EXIT_FAILURE); >> + } >> + >> + buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE; >> + >> + if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE, >> + &buf_size, sizeof(buf_size))) { >> + perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"); >> + exit(EXIT_FAILURE); >> + } >> + >> + buf = malloc(buf_size); >> + if (!buf) { >> + perror("malloc"); >> + exit(EXIT_FAILURE); >> + } >> + >> + control_writeln("SRVREADY"); >> + >> + /* Wait until there will be 128KB of data in rx queue. */ >> + while (1) { >> + ssize_t res; >> + >> + res = recv(fd, buf, buf_size, MSG_PEEK); >> + if (res == buf_size) >> + break; >> + >> + if (res <= 0) { >> + fprintf(stderr, "unexpected 'recv()' return: %zi\n", res); >> + exit(EXIT_FAILURE); >> + } >> + } >> + >> + /* There is 128KB of data in the socket's rx queue, >> + * dequeue first 64KB, credit update is not sent. >> + */ >> + recv_buf_size = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE; >> + recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size); >> + recv_buf_size++; >> + >> + /* Updating SO_RCVLOWAT will send credit update. */ >> + if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT, >> + &recv_buf_size, sizeof(recv_buf_size))) { >> + perror("setsockopt(SO_RCVLOWAT)"); >> + exit(EXIT_FAILURE); >> + } >> + >> + memset(&fds, 0, sizeof(fds)); >> + fds.fd = fd; >> + fds.events = POLLIN | POLLRDNORM | POLLERR | >> + POLLRDHUP | POLLHUP; >> + >> + /* This 'poll()' will return once we receive last byte >> + * sent by client. >> + */ >> + if (poll(&fds, 1, -1) < 0) { >> + perror("poll"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (fds.revents & POLLERR) { >> + fprintf(stderr, "'poll()' error\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (fds.revents & (POLLIN | POLLRDNORM)) { >> + recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size); >> + } else { >> + /* These flags must be set, as there is at >> + * least 64KB of data ready to read. >> + */ >> + fprintf(stderr, "POLLIN | POLLRDNORM expected\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + free(buf); >> + close(fd); >> +} >> + >> static struct test_case test_cases[] = { >> { >> .name = "SOCK_STREAM connection reset", >> @@ -1285,6 +1411,11 @@ static struct test_case test_cases[] = { >> .run_client = test_stream_msgzcopy_empty_errq_client, >> .run_server = test_stream_msgzcopy_empty_errq_server, >> }, >> + { >> + .name = "SOCK_STREAM virtio SO_RCVLOWAT + deferred cred update", >> + .run_client = test_stream_rcvlowat_def_cred_upd_client, >> + .run_server = test_stream_rcvlowat_def_cred_upd_server, >> + }, >> {}, >> }; >> >> -- >> 2.25.1 >> >
On Fri, Nov 17, 2023 at 10:12:38AM +0300, Arseniy Krasnov wrote: > > >On 15.11.2023 14:11, Stefano Garzarella wrote: >> On Wed, Nov 08, 2023 at 10:20:04AM +0300, Arseniy Krasnov wrote: >>> This adds test which checks, that updating SO_RCVLOWAT value also sends >> >> You can avoid "This adds", and write just "Add test ...". >> >> See https://docs.kernel.org/process/submitting-patches.html#describe-your-changes >> >> Describe your changes in imperative mood, e.g. "make xyzzy do frotz" >> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy >> to do frotz", as if you are giving orders to the codebase to change >> its behaviour. >> >> Also in the other patch. >> >>> credit update message. Otherwise mutual hungup may happen when receiver >>> didn't send credit update and then calls 'poll()' with non default >>> SO_RCVLOWAT value (e.g. waiting enough bytes to read), while sender >>> waits for free space at receiver's side. >>> >>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>> --- >>> tools/testing/vsock/vsock_test.c | 131 +++++++++++++++++++++++++++++++ >>> 1 file changed, 131 insertions(+) >>> >>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >>> index c1f7bc9abd22..c71b3875fd16 100644 >>> --- a/tools/testing/vsock/vsock_test.c >>> +++ b/tools/testing/vsock/vsock_test.c >>> @@ -1180,6 +1180,132 @@ static void test_stream_shutrd_server(const struct test_opts *opts) >>> close(fd); >>> } >>> >>> +#define RCVLOWAT_CREDIT_UPD_BUF_SIZE (1024 * 128) >>> +#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64) >> >> What about adding a comment like the one in the cover letter about >> dependency with kernel values? >> >> Please add it also in the commit description. >> >> I'm thinking if we should move all the defines that depends on the >> kernel in some special header. > >IIUC it will be new header file in tools/testing/vsock, which includes such defines. At >this moment in will contain only VIRTIO_VSOCK_MAX_PKT_BUF_SIZE. Idea is that such defines So this only works on the virtio transport though, not the other transports, right? (but maybe the others don't have this problem, so it's fine). >are not supposed to use by user (so do not move it to uapi headers), but needed by tests >to check kernel behaviour. Please correct me if i'm wrong. Right! Maybe if it's just one, we can leave it there for now, but with a comment on top explaining where it comes. Thanks, Stefano
On 17.11.2023 11:30, Stefano Garzarella wrote: > On Fri, Nov 17, 2023 at 10:12:38AM +0300, Arseniy Krasnov wrote: >> >> >> On 15.11.2023 14:11, Stefano Garzarella wrote: >>> On Wed, Nov 08, 2023 at 10:20:04AM +0300, Arseniy Krasnov wrote: >>>> This adds test which checks, that updating SO_RCVLOWAT value also sends >>> >>> You can avoid "This adds", and write just "Add test ...". >>> >>> See https://docs.kernel.org/process/submitting-patches.html#describe-your-changes >>> >>> Describe your changes in imperative mood, e.g. "make xyzzy do frotz" >>> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy >>> to do frotz", as if you are giving orders to the codebase to change >>> its behaviour. >>> >>> Also in the other patch. >>> >>>> credit update message. Otherwise mutual hungup may happen when receiver >>>> didn't send credit update and then calls 'poll()' with non default >>>> SO_RCVLOWAT value (e.g. waiting enough bytes to read), while sender >>>> waits for free space at receiver's side. >>>> >>>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>>> --- >>>> tools/testing/vsock/vsock_test.c | 131 +++++++++++++++++++++++++++++++ >>>> 1 file changed, 131 insertions(+) >>>> >>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >>>> index c1f7bc9abd22..c71b3875fd16 100644 >>>> --- a/tools/testing/vsock/vsock_test.c >>>> +++ b/tools/testing/vsock/vsock_test.c >>>> @@ -1180,6 +1180,132 @@ static void test_stream_shutrd_server(const struct test_opts *opts) >>>> close(fd); >>>> } >>>> >>>> +#define RCVLOWAT_CREDIT_UPD_BUF_SIZE (1024 * 128) >>>> +#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64) >>> >>> What about adding a comment like the one in the cover letter about >>> dependency with kernel values? >>> >>> Please add it also in the commit description. >>> >>> I'm thinking if we should move all the defines that depends on the >>> kernel in some special header. >> >> IIUC it will be new header file in tools/testing/vsock, which includes such defines. At >> this moment in will contain only VIRTIO_VSOCK_MAX_PKT_BUF_SIZE. Idea is that such defines > > So this only works on the virtio transport though, not the other > transports, right? (but maybe the others don't have this problem, so > it's fine). Yes, this case is only actual in virtio as this logic exists in virtio only (the same situation as for skb merging sometimes ago). > >> are not supposed to use by user (so do not move it to uapi headers), but needed by tests >> to check kernel behaviour. Please correct me if i'm wrong. > > Right! > Maybe if it's just one, we can leave it there for now, but with a > comment on top explaining where it comes. Ok, got it, I'll add comment Thanks, Arseniy > > Thanks, > Stefano >
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c index c1f7bc9abd22..c71b3875fd16 100644 --- a/tools/testing/vsock/vsock_test.c +++ b/tools/testing/vsock/vsock_test.c @@ -1180,6 +1180,132 @@ static void test_stream_shutrd_server(const struct test_opts *opts) close(fd); } +#define RCVLOWAT_CREDIT_UPD_BUF_SIZE (1024 * 128) +#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64) + +static void test_stream_rcvlowat_def_cred_upd_client(const struct test_opts *opts) +{ + size_t buf_size; + void *buf; + int fd; + + fd = vsock_stream_connect(opts->peer_cid, 1234); + if (fd < 0) { + perror("connect"); + exit(EXIT_FAILURE); + } + + /* Send 1 byte more than peer's buffer size. */ + buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE + 1; + + buf = malloc(buf_size); + if (!buf) { + perror("malloc"); + exit(EXIT_FAILURE); + } + + /* Wait until peer sets needed buffer size. */ + control_expectln("SRVREADY"); + + if (send(fd, buf, buf_size, 0) != buf_size) { + perror("send failed"); + exit(EXIT_FAILURE); + } + + free(buf); + close(fd); +} + +static void test_stream_rcvlowat_def_cred_upd_server(const struct test_opts *opts) +{ + size_t recv_buf_size; + struct pollfd fds; + size_t buf_size; + void *buf; + int fd; + + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); + if (fd < 0) { + perror("accept"); + exit(EXIT_FAILURE); + } + + buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE; + + if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE, + &buf_size, sizeof(buf_size))) { + perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"); + exit(EXIT_FAILURE); + } + + buf = malloc(buf_size); + if (!buf) { + perror("malloc"); + exit(EXIT_FAILURE); + } + + control_writeln("SRVREADY"); + + /* Wait until there will be 128KB of data in rx queue. */ + while (1) { + ssize_t res; + + res = recv(fd, buf, buf_size, MSG_PEEK); + if (res == buf_size) + break; + + if (res <= 0) { + fprintf(stderr, "unexpected 'recv()' return: %zi\n", res); + exit(EXIT_FAILURE); + } + } + + /* There is 128KB of data in the socket's rx queue, + * dequeue first 64KB, credit update is not sent. + */ + recv_buf_size = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE; + recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size); + recv_buf_size++; + + /* Updating SO_RCVLOWAT will send credit update. */ + if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT, + &recv_buf_size, sizeof(recv_buf_size))) { + perror("setsockopt(SO_RCVLOWAT)"); + exit(EXIT_FAILURE); + } + + memset(&fds, 0, sizeof(fds)); + fds.fd = fd; + fds.events = POLLIN | POLLRDNORM | POLLERR | + POLLRDHUP | POLLHUP; + + /* This 'poll()' will return once we receive last byte + * sent by client. + */ + if (poll(&fds, 1, -1) < 0) { + perror("poll"); + exit(EXIT_FAILURE); + } + + if (fds.revents & POLLERR) { + fprintf(stderr, "'poll()' error\n"); + exit(EXIT_FAILURE); + } + + if (fds.revents & (POLLIN | POLLRDNORM)) { + recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size); + } else { + /* These flags must be set, as there is at + * least 64KB of data ready to read. + */ + fprintf(stderr, "POLLIN | POLLRDNORM expected\n"); + exit(EXIT_FAILURE); + } + + free(buf); + close(fd); +} + static struct test_case test_cases[] = { { .name = "SOCK_STREAM connection reset", @@ -1285,6 +1411,11 @@ static struct test_case test_cases[] = { .run_client = test_stream_msgzcopy_empty_errq_client, .run_server = test_stream_msgzcopy_empty_errq_server, }, + { + .name = "SOCK_STREAM virtio SO_RCVLOWAT + deferred cred update", + .run_client = test_stream_rcvlowat_def_cred_upd_client, + .run_server = test_stream_rcvlowat_def_cred_upd_server, + }, {}, };