Messages in this thread | | | Date | Fri, 17 Nov 2023 09:30:19 +0100 | From | Stefano Garzarella <> | Subject | Re: [RFC PATCH v1 2/2] vsock/test: SO_RCVLOWAT + deferred credit update test |
| |
On Fri, Nov 17, 2023 at 10:12:38AM +0300, Arseniy Krasnov wrote: > > >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
So this only works on the virtio transport though, not the other transports, right? (but maybe the others don't have this problem, so it's fine).
>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.
Right! Maybe if it's just one, we can leave it there for now, but with a comment on top explaining where it comes.
Thanks, Stefano
| |