Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <923a6149-3bd5-c5b4-766d-8301f9e7484a@salutedevices.com>
Date:   Fri, 17 Nov 2023 10:12:38 +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 v1 2/2] 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: <20231108072004.1045669-1-avkrasnov@salutedevices.com>
 <20231108072004.1045669-3-avkrasnov@salutedevices.com>
 <zukasb6k7ogta33c2wik6cgadg2rkacestat7pkexd45u53swh@ovso3hafta77>
From:   Arseniy Krasnov <avkrasnov@salutedevices.com>
In-Reply-To: <zukasb6k7ogta33c2wik6cgadg2rkacestat7pkexd45u53swh@ovso3hafta77>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk


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