Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <be8b1d3e-7032-76dc-042c-9513a933a0f3@salutedevices.com>
Date:   Wed, 29 Nov 2023 12:16:54 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [RFC PATCH v3 3/3] vsock/test: SO_RCVLOWAT + deferred credit
 update test
Content-Language: en-US
To:     Stefano Garzarella <sgarzare@redhat.com>
CC:     Stefan Hajnoczi <stefanha@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>,
        <kvm@vger.kernel.org>, <virtualization@lists.linux-foundation.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel@sberdevices.ru>, <oxffffaa@gmail.com>
References: <20231122180510.2297075-1-avkrasnov@salutedevices.com>
 <20231122180510.2297075-4-avkrasnov@salutedevices.com>
 <mklk6i6frkms33qntatlejbyl2czf7sp4quorkuxy6lpwmmlcn@foknxen36olr>
From:   Arseniy Krasnov <avkrasnov@salutedevices.com>
In-Reply-To: <mklk6i6frkms33qntatlejbyl2czf7sp4quorkuxy6lpwmmlcn@foknxen36olr>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk


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
>>
>