thr3ads.net - Linux Virtualization - [PATCH net-next v3 10/12] test/vsock: MSG

If this information is useful, please help other people find it:
Share via:
Stefano Garzarella
2023-Oct-10 07:19 UTC
[PATCH net-next v3 10/12] test/vsock: MSG_ZEROCOPY flag tests

On Mon, Oct 09, 2023 at 11:24:18PM +0300, Arseniy Krasnov
wrote:>
>
>On 09.10.2023 18:17, Stefano Garzarella wrote:
>> On Sat, Oct 07, 2023 at 08:21:37PM +0300, Arseniy Krasnov wrote:
>>> This adds three tests for MSG_ZEROCOPY feature:
>>> 1) SOCK_STREAM tx with different buffers.
>>> 2) SOCK_SEQPACKET tx with different buffers.
>>> 3) SOCK_STREAM test to read empty error queue of the socket.
>>>
>>> Patch also works as preparation for the next patches for tools in
this
>>> patchset: vsock_perf and vsock_uring_test:
>>> 1) Adds several new functions to util.c - they will be also used by
>>> ? vsock_uring_test.
>>> 2) Adds two new functions for MSG_ZEROCOPY handling to a new header
>>> ? file - such header will be shared between vsock_test, vsock_perf
and
>>> ? vsock_uring_test, thus avoiding code copy-pasting.
>>>
>>> Signed-off-by: Arseniy Krasnov <avkrasnov at
salutedevices.com>
>>> ---
>>> Changelog:
>>> v1 -> v2:
>>> ?* Move 'SOL_VSOCK' and 'VSOCK_RECVERR' from
'util.c' to 'util.h'.
>>> v2 -> v3:
>>> ?* Patch was reworked. Now it is also preparation patch (see commit
>>> ?? message). Shared stuff for 'vsock_perf' and tests is
placed to a
>>> ?? new header file, while shared code between current test tool and
>>> ?? future uring test is placed to the 'util.c'. I think,
that making
>>> ?? this patch as preparation allows to reduce number of changes in
the
>>> ?? next patches in this patchset.
>>> ?* Make 'struct vsock_test_data' private by placing it to
the .c file.
>>> ?? Also add comments to this struct to clarify sense of its fields.
>>>
>>> tools/testing/vsock/Makefile????????????? |?? 2 +-
>>> tools/testing/vsock/msg_zerocopy_common.h |? 92 ++++++
>>> tools/testing/vsock/util.c??????????????? | 110 +++++++
>>> tools/testing/vsock/util.h??????????????? |?? 5 +
>>> tools/testing/vsock/vsock_test.c????????? |? 16 +
>>> tools/testing/vsock/vsock_test_zerocopy.c | 367
++++++++++++++++++++++
>>> tools/testing/vsock/vsock_test_zerocopy.h |? 15 +
>>> 7 files changed, 606 insertions(+), 1 deletion(-)
>>> create mode 100644 tools/testing/vsock/msg_zerocopy_common.h
>>> create mode 100644 tools/testing/vsock/vsock_test_zerocopy.c
>>> create mode 100644 tools/testing/vsock/vsock_test_zerocopy.h
>>>
>>> diff --git a/tools/testing/vsock/Makefile
b/tools/testing/vsock/Makefile
>>> index 21a98ba565ab..1a26f60a596c 100644
>>> --- a/tools/testing/vsock/Makefile
>>> +++ b/tools/testing/vsock/Makefile
>>> @@ -1,7 +1,7 @@
>>> # SPDX-License-Identifier: GPL-2.0-only
>>> all: test vsock_perf
>>> test: vsock_test vsock_diag_test
>>> -vsock_test: vsock_test.o timeout.o control.o util.o
>>> +vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o control.o
util.o
>>> vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
>>> vsock_perf: vsock_perf.o
>>>
>>> diff --git a/tools/testing/vsock/msg_zerocopy_common.h
b/tools/testing/vsock/msg_zerocopy_common.h
>>> new file mode 100644
>>> index 000000000000..ce89f1281584
>>> --- /dev/null
>>> +++ b/tools/testing/vsock/msg_zerocopy_common.h
>>> @@ -0,0 +1,92 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef MSG_ZEROCOPY_COMMON_H
>>> +#define MSG_ZEROCOPY_COMMON_H
>>> +
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <sys/types.h>
>>> +#include <sys/socket.h>
>>> +#include <linux/errqueue.h>
>>> +
>>> +#ifndef SOL_VSOCK
>>> +#define SOL_VSOCK??? 287
>>> +#endif
>>> +
>>> +#ifndef VSOCK_RECVERR
>>> +#define VSOCK_RECVERR??? 1
>>> +#endif
>>> +
>>> +static void enable_so_zerocopy(int fd)
>>> +{
>>> +??? int val = 1;
>>> +
>>> +??? if (setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &val,
sizeof(val))) {
>>> +??????? perror("setsockopt");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +}
>>> +
>>> +static void vsock_recv_completion(int fd, const bool *zerocopied)
__maybe_unused;
>>
>> To avoid this, maybe we can implement those functions in .c file and
>> link the object.
>>
>> WDYT?
>>
>> Ah, here (cc (GCC) 13.2.1 20230728 (Red Hat 13.2.1-1)) the build is
>> failing:
>>
>> In file included from vsock_perf.c:23:
>> msg_zerocopy_common.h: In function ?vsock_recv_completion?:
>> msg_zerocopy_common.h:29:67: error: expected declaration specifiers
before ?__maybe_unused?
>> ?? 29 | static void vsock_recv_completion(int fd, const bool
*zerocopied) __maybe_unused;
>> ?????
|??????????????????????????????????????????????????????????????????
^~~~~~~~~~~~~~
>> msg_zerocopy_common.h:31:1: error: expected ?=?, ?,?, ?;?, ?asm? or
?__attribute__? before ?{? token
>> ?? 31 | {
>> ????? | ^
>>
>>> +static void vsock_recv_completion(int fd, const bool *zerocopied)
>>> +{
>>> +??? struct sock_extended_err *serr;
>>> +??? struct msghdr msg = { 0 };
>>> +??? char cmsg_data[128];
>>> +??? struct cmsghdr *cm;
>>> +??? ssize_t res;
>>> +
>>> +??? msg.msg_control = cmsg_data;
>>> +??? msg.msg_controllen = sizeof(cmsg_data);
>>> +
>>> +??? res = recvmsg(fd, &msg, MSG_ERRQUEUE);
>>> +??? if (res) {
>>> +??????? fprintf(stderr, "failed to read error queue:
%zi\n", res);
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? cm = CMSG_FIRSTHDR(&msg);
>>> +??? if (!cm) {
>>> +??????? fprintf(stderr, "cmsg: no cmsg\n");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? if (cm->cmsg_level != SOL_VSOCK) {
>>> +??????? fprintf(stderr, "cmsg: unexpected
'cmsg_level'\n");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? if (cm->cmsg_type != VSOCK_RECVERR) {
>>> +??????? fprintf(stderr, "cmsg: unexpected
'cmsg_type'\n");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? serr = (void *)CMSG_DATA(cm);
>>> +??? if (serr->ee_origin != SO_EE_ORIGIN_ZEROCOPY) {
>>> +??????? fprintf(stderr, "serr: wrong origin: %u\n",
serr->ee_origin);
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? if (serr->ee_errno) {
>>> +??????? fprintf(stderr, "serr: wrong error code: %u\n",
serr->ee_errno);
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? /* This flag is used for tests, to check that transmission was
>>> +???? * performed as expected: zerocopy or fallback to copy. If
NULL
>>> +???? * - don't care.
>>> +???? */
>>> +??? if (!zerocopied)
>>> +??????? return;
>>> +
>>> +??? if (*zerocopied && (serr->ee_code &
SO_EE_CODE_ZEROCOPY_COPIED)) {
>>> +??????? fprintf(stderr, "serr: was copy instead of
zerocopy\n");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? if (!*zerocopied && !(serr->ee_code &
SO_EE_CODE_ZEROCOPY_COPIED)) {
>>> +??????? fprintf(stderr, "serr: was zerocopy instead of
copy\n");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +}
>>> +
>>> +#endif /* MSG_ZEROCOPY_COMMON_H */
>>> diff --git a/tools/testing/vsock/util.c
b/tools/testing/vsock/util.c
>>> index 6779d5008b27..b1770edd8cc1 100644
>>> --- a/tools/testing/vsock/util.c
>>> +++ b/tools/testing/vsock/util.c
>>> @@ -11,10 +11,12 @@
>>> #include <stdio.h>
>>> #include <stdint.h>
>>> #include <stdlib.h>
>>> +#include <string.h>
>>> #include <signal.h>
>>> #include <unistd.h>
>>> #include <assert.h>
>>> #include <sys/epoll.h>
>>> +#include <sys/mman.h>
>>>
>>> #include "timeout.h"
>>> #include "control.h"
>>> @@ -444,3 +446,111 @@ unsigned long hash_djb2(const void *data,
size_t len)
>>>
>>> ????return hash;
>>> }
>>> +
>>> +size_t iovec_bytes(const struct iovec *iov, size_t iovnum)
>>> +{
>>> +??? size_t bytes;
>>> +??? int i;
>>> +
>>> +??? for (bytes = 0, i = 0; i < iovnum; i++)
>>> +??????? bytes += iov[i].iov_len;
>>> +
>>> +??? return bytes;
>>> +}
>>> +
>>> +unsigned long iovec_hash_djb2(const struct iovec *iov, size_t
iovnum)
>>> +{
>>> +??? unsigned long hash;
>>> +??? size_t iov_bytes;
>>> +??? size_t offs;
>>> +??? void *tmp;
>>> +??? int i;
>>> +
>>> +??? iov_bytes = iovec_bytes(iov, iovnum);
>>> +
>>> +??? tmp = malloc(iov_bytes);
>>> +??? if (!tmp) {
>>> +??????? perror("malloc");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? for (offs = 0, i = 0; i < iovnum; i++) {
>>> +??????? memcpy(tmp + offs, iov[i].iov_base, iov[i].iov_len);
>>> +??????? offs += iov[i].iov_len;
>>> +??? }
>>> +
>>> +??? hash = hash_djb2(tmp, iov_bytes);
>>> +??? free(tmp);
>>> +
>>> +??? return hash;
>>> +}
>>> +
>>> +struct iovec *iovec_from_test_data(const struct iovec *test_iovec,
int iovnum)
>>
>> From the name this function seems related to vsock_test_data, so
I'd
>> suggest to move this and free_iovec_test_data() in
vsock_test_zerocopy.c
>>
>>> +{
>>> +??? struct iovec *iovec;
>>> +??? int i;
>>> +
>>> +??? iovec = malloc(sizeof(*iovec) * iovnum);
>>> +??? if (!iovec) {
>>> +??????? perror("malloc");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? for (i = 0; i < iovnum; i++) {
>>> +??????? iovec[i].iov_len = test_iovec[i].iov_len;
>>> +
>>> +??????? iovec[i].iov_base = mmap(NULL, iovec[i].iov_len,
>>> +???????????????????? PROT_READ | PROT_WRITE,
>>> +???????????????????? MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE,
>>> +???????????????????? -1, 0);
>>> +??????? if (iovec[i].iov_base == MAP_FAILED) {
>>> +??????????? perror("mmap");
>>> +??????????? exit(EXIT_FAILURE);
>>> +??????? }
>>> +
>>> +??????? if (test_iovec[i].iov_base != MAP_FAILED)
>>> +??????????? iovec[i].iov_base +=
(uintptr_t)test_iovec[i].iov_base;
>>> +??? }
>>> +
>>> +??? /* Unmap "invalid" elements. */
>>> +??? for (i = 0; i < iovnum; i++) {
>>> +??????? if (test_iovec[i].iov_base == MAP_FAILED) {
>>> +??????????? if (munmap(iovec[i].iov_base, iovec[i].iov_len)) {
>>> +??????????????? perror("munmap");
>>> +??????????????? exit(EXIT_FAILURE);
>>> +??????????? }
>>> +??????? }
>>> +??? }
>>> +
>>> +??? for (i = 0; i < iovnum; i++) {
>>> +??????? int j;
>>> +
>>> +??????? if (test_iovec[i].iov_base == MAP_FAILED)
>>> +??????????? continue;
>>> +
>>> +??????? for (j = 0; j < iovec[i].iov_len; j++)
>>> +??????????? ((uint8_t *)iovec[i].iov_base)[j] = rand() & 0xff;
>>> +??? }
>>> +
>>> +??? return iovec;
>>> +}
>>> +
>>> +void free_iovec_test_data(const struct iovec *test_iovec,
>>> +????????????? struct iovec *iovec, int iovnum)
>>> +{
>>> +??? int i;
>>> +
>>> +??? for (i = 0; i < iovnum; i++) {
>>> +??????? if (test_iovec[i].iov_base != MAP_FAILED) {
>>> +??????????? if (test_iovec[i].iov_base)
>>> +??????????????? iovec[i].iov_base -=
(uintptr_t)test_iovec[i].iov_base;
>>> +
>>> +??????????? if (munmap(iovec[i].iov_base, iovec[i].iov_len)) {
>>> +??????????????? perror("munmap");
>>> +??????????????? exit(EXIT_FAILURE);
>>> +??????????? }
>>> +??????? }
>>> +??? }
>>> +
>>> +??? free(iovec);
>>> +}
>>> diff --git a/tools/testing/vsock/util.h
b/tools/testing/vsock/util.h
>>> index e5407677ce05..4cacb8d804c1 100644
>>> --- a/tools/testing/vsock/util.h
>>> +++ b/tools/testing/vsock/util.h
>>> @@ -53,4 +53,9 @@ void list_tests(const struct test_case
*test_cases);
>>> void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>> ?????????? const char *test_id_str);
>>> unsigned long hash_djb2(const void *data, size_t len);
>>> +size_t iovec_bytes(const struct iovec *iov, size_t iovnum);
>>> +unsigned long iovec_hash_djb2(const struct iovec *iov, size_t
iovnum);
>>> +struct iovec *iovec_from_test_data(const struct iovec *test_iovec,
int iovnum);
>>> +void free_iovec_test_data(const struct iovec *test_iovec,
>>> +????????????? struct iovec *iovec, int iovnum);
>>> #endif /* UTIL_H */
>>> diff --git a/tools/testing/vsock/vsock_test.c
b/tools/testing/vsock/vsock_test.c
>>> index da4cb819a183..c1f7bc9abd22 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -21,6 +21,7 @@
>>> #include <poll.h>
>>> #include <signal.h>
>>>
>>> +#include "vsock_test_zerocopy.h"
>>> #include "timeout.h"
>>> #include "control.h"
>>> #include "util.h"
>>> @@ -1269,6 +1270,21 @@ static struct test_case test_cases[] = {
>>> ??????? .run_client = test_stream_shutrd_client,
>>> ??????? .run_server = test_stream_shutrd_server,
>>> ????},
>>> +??? {
>>> +??????? .name = "SOCK_STREAM MSG_ZEROCOPY",
>>> +??????? .run_client = test_stream_msgzcopy_client,
>>> +??????? .run_server = test_stream_msgzcopy_server,
>>> +??? },
>>> +??? {
>>> +??????? .name = "SOCK_SEQPACKET MSG_ZEROCOPY",
>>> +??????? .run_client = test_seqpacket_msgzcopy_client,
>>> +??????? .run_server = test_seqpacket_msgzcopy_server,
>>> +??? },
>>> +??? {
>>> +??????? .name = "SOCK_STREAM MSG_ZEROCOPY empty
MSG_ERRQUEUE",
>>> +??????? .run_client = test_stream_msgzcopy_empty_errq_client,
>>> +??????? .run_server = test_stream_msgzcopy_empty_errq_server,
>>> +??? },
>>> ????{},
>>> };
>>>
>>> diff --git a/tools/testing/vsock/vsock_test_zerocopy.c
b/tools/testing/vsock/vsock_test_zerocopy.c
>>> new file mode 100644
>>> index 000000000000..af14efdf334b
>>> --- /dev/null
>>> +++ b/tools/testing/vsock/vsock_test_zerocopy.c
>>> @@ -0,0 +1,367 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/* MSG_ZEROCOPY feature tests for vsock
>>> + *
>>> + * Copyright (C) 2023 SberDevices.
>>> + *
>>> + * Author: Arseniy Krasnov <avkrasnov at salutedevices.com>
>>> + */
>>> +
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <string.h>
>>> +#include <sys/mman.h>
>>> +#include <unistd.h>
>>> +#include <poll.h>
>>> +#include <linux/errqueue.h>
>>> +#include <linux/kernel.h>
>>> +#include <errno.h>
>>> +
>>> +#include "control.h"
>>> +#include "vsock_test_zerocopy.h"
>>> +#include "msg_zerocopy_common.h"
>>> +
>>> +#define PAGE_SIZE??????? 4096
>>
>> In some tests I saw `sysconf(_SC_PAGESIZE)` is used,
>> e.g. in selftests/ptrace/peeksiginfo.c:
>>
>> #ifndef PAGE_SIZE
>> #define PAGE_SIZE sysconf(_SC_PAGESIZE)
>> #endif
>>
>> WDYT?
>
>Only small problem with that - in this case I can't use PAGE_SIZE
>as array initializer. I think to add some reserved constant value
>to designate that iov element must be size of page, then use this
>value as initializer and handle it during test iov creating...
Okay I see. Maybe I'm overthinking!
It is just a test, let's do not complicate it.

Feel free to use the previous version, I'd just add the guards.

Thanks,
Stefano
Linux Virtualization - Oct 2023 - [PATCH net-next v3 10/12] test/vsock: MSG_ZEROCOPY flag tests

[PATCH net-next v3 10/12] test/vsock: MSG_ZEROCOPY flag tests
