| Message ID | df70a274-4e69-ca1f-acba-126eb517e532@sberdevices.ru (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | virtio/vsock: use SO_RCVLOWAT to set POLLIN/POLLRDNORM | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 3 of 3 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| 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: 0 this patch: 0 |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Jul 18, 2022 at 08:19:06AM +0000, Arseniy Krasnov wrote: >This adds test to check, that when poll() returns POLLIN and >POLLRDNORM bits, next read call won't block. > >Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >--- > tools/testing/vsock/vsock_test.c | 90 ++++++++++++++++++++++++++++++++ > 1 file changed, 90 insertions(+) > >diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >index dc577461afc2..8e394443eaf6 100644 >--- a/tools/testing/vsock/vsock_test.c >+++ b/tools/testing/vsock/vsock_test.c >@@ -18,6 +18,7 @@ > #include <sys/socket.h> > #include <time.h> > #include <sys/mman.h> >+#include <poll.h> > > #include "timeout.h" > #include "control.h" >@@ -596,6 +597,90 @@ static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opt > close(fd); > } > >+static void test_stream_poll_rcvlowat_server(const struct test_opts *opts) >+{ >+#define RCVLOWAT_BUF_SIZE 128 >+ int fd; >+ int i; >+ >+ fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >+ if (fd < 0) { >+ perror("accept"); >+ exit(EXIT_FAILURE); >+ } >+ >+ /* Send 1 byte. */ >+ send_byte(fd, 1, 0); >+ >+ control_writeln("SRVSENT"); >+ >+ /* Just empirically delay value. */ >+ sleep(4); Why we need this sleep()?
On 19.07.2022 15:52, Stefano Garzarella wrote: > On Mon, Jul 18, 2022 at 08:19:06AM +0000, Arseniy Krasnov wrote: >> This adds test to check, that when poll() returns POLLIN and >> POLLRDNORM bits, next read call won't block. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> tools/testing/vsock/vsock_test.c | 90 ++++++++++++++++++++++++++++++++ >> 1 file changed, 90 insertions(+) >> >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >> index dc577461afc2..8e394443eaf6 100644 >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -18,6 +18,7 @@ >> #include <sys/socket.h> >> #include <time.h> >> #include <sys/mman.h> >> +#include <poll.h> >> >> #include "timeout.h" >> #include "control.h" >> @@ -596,6 +597,90 @@ static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opt >> close(fd); >> } >> >> +static void test_stream_poll_rcvlowat_server(const struct test_opts *opts) >> +{ >> +#define RCVLOWAT_BUF_SIZE 128 >> + int fd; >> + int i; >> + >> + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >> + if (fd < 0) { >> + perror("accept"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Send 1 byte. */ >> + send_byte(fd, 1, 0); >> + >> + control_writeln("SRVSENT"); >> + >> + /* Just empirically delay value. */ >> + sleep(4); > > Why we need this sleep()? Purpose of sleep() is to move client in state, when it has 1 byte of rx data and poll() won't wake. For example: client: server: waits for "SRVSENT" send 1 byte send "SRVSENT" poll() sleep ... poll sleeps ... send rest of data poll wake up I think, without sleep there is chance, that client enters poll() when whole data from server is already received, thus test will be useless(it just tests poll()). May be i can remove "SRVSENT" as sleep is enough. >
On Wed, Jul 20, 2022 at 05:46:01AM +0000, Arseniy Krasnov wrote: >On 19.07.2022 15:52, Stefano Garzarella wrote: >> On Mon, Jul 18, 2022 at 08:19:06AM +0000, Arseniy Krasnov wrote: >>> This adds test to check, that when poll() returns POLLIN and >>> POLLRDNORM bits, next read call won't block. >>> >>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>> --- >>> tools/testing/vsock/vsock_test.c | 90 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 90 insertions(+) >>> >>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >>> index dc577461afc2..8e394443eaf6 100644 >>> --- a/tools/testing/vsock/vsock_test.c >>> +++ b/tools/testing/vsock/vsock_test.c >>> @@ -18,6 +18,7 @@ >>> #include <sys/socket.h> >>> #include <time.h> >>> #include <sys/mman.h> >>> +#include <poll.h> >>> >>> #include "timeout.h" >>> #include "control.h" >>> @@ -596,6 +597,90 @@ static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opt >>> close(fd); >>> } >>> >>> +static void test_stream_poll_rcvlowat_server(const struct test_opts *opts) >>> +{ >>> +#define RCVLOWAT_BUF_SIZE 128 >>> + int fd; >>> + int i; >>> + >>> + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >>> + if (fd < 0) { >>> + perror("accept"); >>> + exit(EXIT_FAILURE); >>> + } >>> + >>> + /* Send 1 byte. */ >>> + send_byte(fd, 1, 0); >>> + >>> + control_writeln("SRVSENT"); >>> + >>> + /* Just empirically delay value. */ >>> + sleep(4); >> >> Why we need this sleep()? >Purpose of sleep() is to move client in state, when it has 1 byte of rx data >and poll() won't wake. For example: >client: server: >waits for "SRVSENT" > send 1 byte > send "SRVSENT" >poll() > sleep >... >poll sleeps >... > send rest of data >poll wake up > >I think, without sleep there is chance, that client enters poll() when whole >data from server is already received, thus test will be useless(it just tests Right, I see (maybe add a comment in the test). >poll()). May be i can remove "SRVSENT" as sleep is enough. I think it's fine. An alternative could be to use the `timeout` of poll(): client: server: waits for "SRVSENT" send 1 byte send "SRVSENT" poll(, timeout = 1 * 1000) wait for "CLNSENT" poll should return 0 send "CLNSENT" poll(, timeout = 10 * 1000) ... poll sleeps ... send rest of data poll wake up I don't have a strong opinion, also your version seems fine, just an alternative ;-) Maybe in your version you can add a 10 sec timeout to poll, to avoid that the test stuck for some reason (failing if the timeout is reached). Thanks, Stefano
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c index dc577461afc2..8e394443eaf6 100644 --- a/tools/testing/vsock/vsock_test.c +++ b/tools/testing/vsock/vsock_test.c @@ -18,6 +18,7 @@ #include <sys/socket.h> #include <time.h> #include <sys/mman.h> +#include <poll.h> #include "timeout.h" #include "control.h" @@ -596,6 +597,90 @@ static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opt close(fd); } +static void test_stream_poll_rcvlowat_server(const struct test_opts *opts) +{ +#define RCVLOWAT_BUF_SIZE 128 + int fd; + int i; + + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); + if (fd < 0) { + perror("accept"); + exit(EXIT_FAILURE); + } + + /* Send 1 byte. */ + send_byte(fd, 1, 0); + + control_writeln("SRVSENT"); + + /* Just empirically delay value. */ + sleep(4); + + for (i = 0; i < RCVLOWAT_BUF_SIZE - 1; i++) + send_byte(fd, 1, 0); + + /* Keep socket in active state. */ + control_expectln("POLLDONE"); + + close(fd); +} + +static void test_stream_poll_rcvlowat_client(const struct test_opts *opts) +{ + unsigned long lowat_val = RCVLOWAT_BUF_SIZE; + char buf[RCVLOWAT_BUF_SIZE]; + struct pollfd fds; + ssize_t read_res; + short poll_flags; + int fd; + + fd = vsock_stream_connect(opts->peer_cid, 1234); + if (fd < 0) { + perror("connect"); + exit(EXIT_FAILURE); + } + + if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT, + &lowat_val, sizeof(lowat_val))) { + perror("setsockopt"); + exit(EXIT_FAILURE); + } + + control_expectln("SRVSENT"); + + /* At this point, server sent 1 byte. */ + fds.fd = fd; + poll_flags = POLLIN | POLLRDNORM; + fds.events = poll_flags; + + if (poll(&fds, 1, -1) < 0) { + perror("poll"); + exit(EXIT_FAILURE); + } + + /* Only these two bits are expected. */ + if (fds.revents != poll_flags) { + fprintf(stderr, "Unexpected poll result %hx\n", + fds.revents); + exit(EXIT_FAILURE); + } + + /* Use MSG_DONTWAIT, if call is going to wait, EAGAIN + * will be returned. + */ + read_res = recv(fd, buf, sizeof(buf), MSG_DONTWAIT); + if (read_res != RCVLOWAT_BUF_SIZE) { + fprintf(stderr, "Unexpected recv result %zi\n", + read_res); + exit(EXIT_FAILURE); + } + + control_writeln("POLLDONE"); + + close(fd); +} + static struct test_case test_cases[] = { { .name = "SOCK_STREAM connection reset", @@ -646,6 +731,11 @@ static struct test_case test_cases[] = { .run_client = test_seqpacket_invalid_rec_buffer_client, .run_server = test_seqpacket_invalid_rec_buffer_server, }, + { + .name = "SOCK_STREAM poll() + SO_RCVLOWAT", + .run_client = test_stream_poll_rcvlowat_client, + .run_server = test_stream_poll_rcvlowat_server, + }, {}, };
This adds test to check, that when poll() returns POLLIN and POLLRDNORM bits, next read call won't block. Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- tools/testing/vsock/vsock_test.c | 90 ++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)