Re: [RFC PATCH v1 2/2] vsock/test: SO_RCVLOWAT + deferred credit update test

[Stefano Garzarella]

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