diff mbox series

[RFC,v3,3/3] vsock/test: SO_RCVLOWAT + deferred credit update test

Message ID 20231122180510.2297075-4-avkrasnov@salutedevices.com (mailing list archive)
State Superseded
Headers show
Series send credit update during setting SO_RCVLOWAT | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 1 maintainers not CCed: virtualization@lists.linux.dev
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Arseniy Krasnov Nov. 22, 2023, 6:05 p.m. UTC
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. Important thing is that this test relies on
kernel's define for maximum packet size for virtio transport and this
value is not exported to user: VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (this
define is used to control moment when to send credit update message).
If this value or its usage will be changed in kernel - this test may
become useless/broken.

Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
 Changelog:
 v1 -> v2:
  * Update commit message by removing 'This patch adds XXX' manner.
  * Update commit message by adding details about dependency for this
    test from kernel internal define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE.
  * Add comment for this dependency in 'vsock_test.c' where this define
    is duplicated.
 v2 -> v3:
  * Replace synchronization based on control TCP socket with vsock
    data socket - this is needed to allow sender transmit data only
    when new buffer size of receiver is visible to sender. Otherwise
    there is race and test fails sometimes.

 tools/testing/vsock/vsock_test.c | 142 +++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

Comments

Stefano Garzarella Nov. 29, 2023, 9:16 a.m. UTC | #1
On Wed, Nov 22, 2023 at 09:05:10PM +0300, Arseniy Krasnov wrote:
>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. Important thing is that this test relies on
>kernel's define for maximum packet size for virtio transport and this
>value is not exported to user: VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (this
>define is used to control moment when to send credit update message).
>If this value or its usage will be changed in kernel - this test may
>become useless/broken.
>
>Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
>---
> Changelog:
> v1 -> v2:
>  * Update commit message by removing 'This patch adds XXX' manner.
>  * Update commit message by adding details about dependency for this
>    test from kernel internal define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE.
>  * Add comment for this dependency in 'vsock_test.c' where this define
>    is duplicated.
> v2 -> v3:
>  * Replace synchronization based on control TCP socket with vsock
>    data socket - this is needed to allow sender transmit data only
>    when new buffer size of receiver is visible to sender. Otherwise
>    there is race and test fails sometimes.
>
> tools/testing/vsock/vsock_test.c | 142 +++++++++++++++++++++++++++++++
> 1 file changed, 142 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 5b0e93f9996c..773a71260fba 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1225,6 +1225,143 @@ static void test_double_bind_connect_client(const struct test_opts *opts)
> 	}
> }
>
>+#define RCVLOWAT_CREDIT_UPD_BUF_SIZE	(1024 * 128)
>+/* This define is the same as in 'include/linux/virtio_vsock.h':
>+ * it is used to decide when to send credit update message during
>+ * reading from rx queue of a socket. Value and its usage in
>+ * kernel is important for this test.
>+ */
>+#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. */
>+	recv_byte(fd, 1, 0);
>+
>+	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);
>+	}
>+
>+	/* Send one dummy byte here, because 'setsockopt()' above also
>+	 * sends special packet which tells sender to update our buffer
>+	 * size. This 'send_byte()' will serialize such packet with data
>+	 * reads in a loop below. Sender starts transmission only when
>+	 * it receives this single byte.
>+	 */
>+	send_byte(fd, 1, 0);
>+
>+	buf = malloc(buf_size);
>+	if (!buf) {
>+		perror("malloc");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	/* 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);

Should we set the socket non-blocking?

Otherwise, here poll() might wake up even if there are not all the
expected bytes due to some bug and recv() block waiting for the
remaining bytes, so we might not notice the bug.

Stefano

>+	} 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",
>@@ -1335,6 +1472,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_double_bind_connect_client,
> 		.run_server = test_double_bind_connect_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
>
Arseniy Krasnov Nov. 29, 2023, 9:16 a.m. UTC | #2
On 29.11.2023 12:16, Stefano Garzarella wrote:
> On Wed, Nov 22, 2023 at 09:05:10PM +0300, Arseniy Krasnov wrote:
>> 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. Important thing is that this test relies on
>> kernel's define for maximum packet size for virtio transport and this
>> value is not exported to user: VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (this
>> define is used to control moment when to send credit update message).
>> If this value or its usage will be changed in kernel - this test may
>> become useless/broken.
>>
>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
>> ---
>> Changelog:
>> v1 -> v2:
>>  * Update commit message by removing 'This patch adds XXX' manner.
>>  * Update commit message by adding details about dependency for this
>>    test from kernel internal define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE.
>>  * Add comment for this dependency in 'vsock_test.c' where this define
>>    is duplicated.
>> v2 -> v3:
>>  * Replace synchronization based on control TCP socket with vsock
>>    data socket - this is needed to allow sender transmit data only
>>    when new buffer size of receiver is visible to sender. Otherwise
>>    there is race and test fails sometimes.
>>
>> tools/testing/vsock/vsock_test.c | 142 +++++++++++++++++++++++++++++++
>> 1 file changed, 142 insertions(+)
>>
>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>> index 5b0e93f9996c..773a71260fba 100644
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -1225,6 +1225,143 @@ static void test_double_bind_connect_client(const struct test_opts *opts)
>>     }
>> }
>>
>> +#define RCVLOWAT_CREDIT_UPD_BUF_SIZE    (1024 * 128)
>> +/* This define is the same as in 'include/linux/virtio_vsock.h':
>> + * it is used to decide when to send credit update message during
>> + * reading from rx queue of a socket. Value and its usage in
>> + * kernel is important for this test.
>> + */
>> +#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. */
>> +    recv_byte(fd, 1, 0);
>> +
>> +    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);
>> +    }
>> +
>> +    /* Send one dummy byte here, because 'setsockopt()' above also
>> +     * sends special packet which tells sender to update our buffer
>> +     * size. This 'send_byte()' will serialize such packet with data
>> +     * reads in a loop below. Sender starts transmission only when
>> +     * it receives this single byte.
>> +     */
>> +    send_byte(fd, 1, 0);
>> +
>> +    buf = malloc(buf_size);
>> +    if (!buf) {
>> +        perror("malloc");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    /* 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);
> 
> Should we set the socket non-blocking?
> 
> Otherwise, here poll() might wake up even if there are not all the
> expected bytes due to some bug and recv() block waiting for the
> remaining bytes, so we might not notice the bug.

Good point! or just use MSG_DONTWAIT flag for only this 'recv()'.

Thanks, Arseniy

> 
> Stefano
> 
>> +    } 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",
>> @@ -1335,6 +1472,11 @@ static struct test_case test_cases[] = {
>>         .run_client = test_double_bind_connect_client,
>>         .run_server = test_double_bind_connect_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
>>
>
Stefano Garzarella Nov. 29, 2023, 9:36 a.m. UTC | #3
On Wed, Nov 29, 2023 at 12:16:54PM +0300, Arseniy Krasnov wrote:
>
>
>On 29.11.2023 12:16, Stefano Garzarella wrote:
>> On Wed, Nov 22, 2023 at 09:05:10PM +0300, Arseniy Krasnov wrote:
>>> 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. Important thing is that this test relies on
>>> kernel's define for maximum packet size for virtio transport and this
>>> value is not exported to user: VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (this
>>> define is used to control moment when to send credit update message).
>>> If this value or its usage will be changed in kernel - this test may
>>> become useless/broken.
>>>
>>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
>>> ---
>>> Changelog:
>>> v1 -> v2:
>>>  * Update commit message by removing 'This patch adds XXX' manner.
>>>  * Update commit message by adding details about dependency for this
>>>    test from kernel internal define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE.
>>>  * Add comment for this dependency in 'vsock_test.c' where this define
>>>    is duplicated.
>>> v2 -> v3:
>>>  * Replace synchronization based on control TCP socket with vsock
>>>    data socket - this is needed to allow sender transmit data only
>>>    when new buffer size of receiver is visible to sender. Otherwise
>>>    there is race and test fails sometimes.
>>>
>>> tools/testing/vsock/vsock_test.c | 142 +++++++++++++++++++++++++++++++
>>> 1 file changed, 142 insertions(+)
>>>
>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>> index 5b0e93f9996c..773a71260fba 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -1225,6 +1225,143 @@ static void test_double_bind_connect_client(const struct test_opts *opts)
>>>     }
>>> }
>>>
>>> +#define RCVLOWAT_CREDIT_UPD_BUF_SIZE    (1024 * 128)
>>> +/* This define is the same as in 'include/linux/virtio_vsock.h':
>>> + * it is used to decide when to send credit update message during
>>> + * reading from rx queue of a socket. Value and its usage in
>>> + * kernel is important for this test.
>>> + */
>>> +#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. */
>>> +    recv_byte(fd, 1, 0);
>>> +
>>> +    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);
>>> +    }
>>> +
>>> +    /* Send one dummy byte here, because 'setsockopt()' above also
>>> +     * sends special packet which tells sender to update our buffer
>>> +     * size. This 'send_byte()' will serialize such packet with data
>>> +     * reads in a loop below. Sender starts transmission only when
>>> +     * it receives this single byte.
>>> +     */
>>> +    send_byte(fd, 1, 0);
>>> +
>>> +    buf = malloc(buf_size);
>>> +    if (!buf) {
>>> +        perror("malloc");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    /* 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);
>>
>> Should we set the socket non-blocking?
>>
>> Otherwise, here poll() might wake up even if there are not all the
>> expected bytes due to some bug and recv() block waiting for the
>> remaining bytes, so we might not notice the bug.
>
>Good point! or just use MSG_DONTWAIT flag for only this 'recv()'.

Yep, right!

Stefano
diff mbox series

Patch

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 5b0e93f9996c..773a71260fba 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1225,6 +1225,143 @@  static void test_double_bind_connect_client(const struct test_opts *opts)
 	}
 }
 
+#define RCVLOWAT_CREDIT_UPD_BUF_SIZE	(1024 * 128)
+/* This define is the same as in 'include/linux/virtio_vsock.h':
+ * it is used to decide when to send credit update message during
+ * reading from rx queue of a socket. Value and its usage in
+ * kernel is important for this test.
+ */
+#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. */
+	recv_byte(fd, 1, 0);
+
+	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);
+	}
+
+	/* Send one dummy byte here, because 'setsockopt()' above also
+	 * sends special packet which tells sender to update our buffer
+	 * size. This 'send_byte()' will serialize such packet with data
+	 * reads in a loop below. Sender starts transmission only when
+	 * it receives this single byte.
+	 */
+	send_byte(fd, 1, 0);
+
+	buf = malloc(buf_size);
+	if (!buf) {
+		perror("malloc");
+		exit(EXIT_FAILURE);
+	}
+
+	/* 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",
@@ -1335,6 +1472,11 @@  static struct test_case test_cases[] = {
 		.run_client = test_double_bind_connect_client,
 		.run_server = test_double_bind_connect_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,
+	},
 	{},
 };