| Message ID | 45822644-8e37-1625-5944-63fd5fc20dd3@sberdevices.ru (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | vsock: updates for SO_RCVLOWAT handling | expand |
On Wed, Aug 03, 2022 at 01:51:05PM +0000, Arseniy Krasnov wrote: >This adds transport specific callback for SO_RCVLOWAT, because in some >transports it may be difficult to know current available number of bytes >ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it. > >Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >--- > include/net/af_vsock.h | 1 + > net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > >diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >index f742e50207fb..eae5874bae35 100644 >--- a/include/net/af_vsock.h >+++ b/include/net/af_vsock.h >@@ -134,6 +134,7 @@ struct vsock_transport { > u64 (*stream_rcvhiwat)(struct vsock_sock *); > bool (*stream_is_active)(struct vsock_sock *); > bool (*stream_allow)(u32 cid, u32 port); >+ int (*set_rcvlowat)(struct vsock_sock *, int); checkpatch suggests to add identifier names. For some we put them in, for others we didn't, but I suggest putting them in for the new ones because I think it's clearer too. WARNING: function definition argument 'struct vsock_sock *' should also have an identifier name #25: FILE: include/net/af_vsock.h:137: + int (*set_rcvlowat)(struct vsock_sock *, int); WARNING: function definition argument 'int' should also have an identifier name #25: FILE: include/net/af_vsock.h:137: + int (*set_rcvlowat)(struct vsock_sock *, int); total: 0 errors, 2 warnings, 0 checks, 44 lines checked > > /* SEQ_PACKET. */ > ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg, >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index f04abf662ec6..016ad5ff78b7 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > return err; > } > >+static int vsock_set_rcvlowat(struct sock *sk, int val) >+{ >+ const struct vsock_transport *transport; >+ struct vsock_sock *vsk; >+ int err = 0; >+ >+ vsk = vsock_sk(sk); >+ >+ if (val > vsk->buffer_size) >+ return -EINVAL; >+ >+ transport = vsk->transport; >+ >+ if (!transport) >+ return -EOPNOTSUPP; I don't know whether it is better in this case to write it in sk->sk_rcvlowat, maybe we can return EOPNOTSUPP only when the trasport is assigned and set_rcvlowat is not defined. This is because usually the options are set just after creation, when the transport is practically unassigned. I mean something like this: if (transport) { if (transport->set_rcvlowat) return transport->set_rcvlowat(vsk, val); else return -EOPNOTSUPP; } WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); return 0; >+ >+ if (transport->set_rcvlowat) >+ err = transport->set_rcvlowat(vsk, val); >+ else >+ WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >+ >+ return err; >+} >+ > static const struct proto_ops vsock_stream_ops = { > .family = PF_VSOCK, > .owner = THIS_MODULE, >@@ -2148,6 +2172,7 @@ static const struct proto_ops vsock_stream_ops = { > .recvmsg = vsock_connectible_recvmsg, > .mmap = sock_no_mmap, > .sendpage = sock_no_sendpage, >+ .set_rcvlowat = vsock_set_rcvlowat, > }; > > static const struct proto_ops vsock_seqpacket_ops = { >-- >2.25.1
On Mon, Aug 8, 2022 at 12:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Wed, Aug 03, 2022 at 01:51:05PM +0000, Arseniy Krasnov wrote: > >This adds transport specific callback for SO_RCVLOWAT, because in some > >transports it may be difficult to know current available number of bytes > >ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it. > > > >Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >--- > > include/net/af_vsock.h | 1 + > > net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++ > > 2 files changed, 26 insertions(+) > > > >diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h > >index f742e50207fb..eae5874bae35 100644 > >--- a/include/net/af_vsock.h > >+++ b/include/net/af_vsock.h > >@@ -134,6 +134,7 @@ struct vsock_transport { > > u64 (*stream_rcvhiwat)(struct vsock_sock *); > > bool (*stream_is_active)(struct vsock_sock *); > > bool (*stream_allow)(u32 cid, u32 port); > >+ int (*set_rcvlowat)(struct vsock_sock *, int); > > checkpatch suggests to add identifier names. For some we put them in, > for others we didn't, but I suggest putting them in for the new ones > because I think it's clearer too. > > WARNING: function definition argument 'struct vsock_sock *' should also > have an identifier name > #25: FILE: include/net/af_vsock.h:137: > + int (*set_rcvlowat)(struct vsock_sock *, int); > > WARNING: function definition argument 'int' should also have an identifier name > #25: FILE: include/net/af_vsock.h:137: > + int (*set_rcvlowat)(struct vsock_sock *, int); > > total: 0 errors, 2 warnings, 0 checks, 44 lines checked > > > > > /* SEQ_PACKET. */ > > ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg, > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > >index f04abf662ec6..016ad5ff78b7 100644 > >--- a/net/vmw_vsock/af_vsock.c > >+++ b/net/vmw_vsock/af_vsock.c > >@@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > > return err; > > } > > > >+static int vsock_set_rcvlowat(struct sock *sk, int val) > >+{ > >+ const struct vsock_transport *transport; > >+ struct vsock_sock *vsk; > >+ int err = 0; > >+ > >+ vsk = vsock_sk(sk); > >+ > >+ if (val > vsk->buffer_size) > >+ return -EINVAL; > >+ > >+ transport = vsk->transport; > >+ > >+ if (!transport) > >+ return -EOPNOTSUPP; > > I don't know whether it is better in this case to write it in > sk->sk_rcvlowat, maybe we can return EOPNOTSUPP only when the trasport > is assigned and set_rcvlowat is not defined. This is because usually the > options are set just after creation, when the transport is practically > unassigned. > > I mean something like this: > > if (transport) { > if (transport->set_rcvlowat) > return transport->set_rcvlowat(vsk, val); > else > return -EOPNOTSUPP; > } > > WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); > > return 0; Since hv_sock implements `set_rcvlowat` to return EOPNOTSUPP. maybe we can just do the following: if (transport && transport->set_rcvlowat) return transport->set_rcvlowat(vsk, val); WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); return 0; That is, the default behavior is to set sk->sk_rcvlowat, but for transports that want a different behavior, they need to define set_rcvlowat() (like hv_sock). Thanks, Stefano
On 08.08.2022 13:30, Stefano Garzarella wrote: > On Mon, Aug 8, 2022 at 12:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Wed, Aug 03, 2022 at 01:51:05PM +0000, Arseniy Krasnov wrote: >>> This adds transport specific callback for SO_RCVLOWAT, because in some >>> transports it may be difficult to know current available number of bytes >>> ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it. >>> >>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>> --- >>> include/net/af_vsock.h | 1 + >>> net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >>> index f742e50207fb..eae5874bae35 100644 >>> --- a/include/net/af_vsock.h >>> +++ b/include/net/af_vsock.h >>> @@ -134,6 +134,7 @@ struct vsock_transport { >>> u64 (*stream_rcvhiwat)(struct vsock_sock *); >>> bool (*stream_is_active)(struct vsock_sock *); >>> bool (*stream_allow)(u32 cid, u32 port); >>> + int (*set_rcvlowat)(struct vsock_sock *, int); >> >> checkpatch suggests to add identifier names. For some we put them in, >> for others we didn't, but I suggest putting them in for the new ones >> because I think it's clearer too. >> >> WARNING: function definition argument 'struct vsock_sock *' should also >> have an identifier name >> #25: FILE: include/net/af_vsock.h:137: >> + int (*set_rcvlowat)(struct vsock_sock *, int); >> >> WARNING: function definition argument 'int' should also have an identifier name >> #25: FILE: include/net/af_vsock.h:137: >> + int (*set_rcvlowat)(struct vsock_sock *, int); >> >> total: 0 errors, 2 warnings, 0 checks, 44 lines checked >> >>> >>> /* SEQ_PACKET. */ >>> ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg, >>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >>> index f04abf662ec6..016ad5ff78b7 100644 >>> --- a/net/vmw_vsock/af_vsock.c >>> +++ b/net/vmw_vsock/af_vsock.c >>> @@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, >>> return err; >>> } >>> >>> +static int vsock_set_rcvlowat(struct sock *sk, int val) >>> +{ >>> + const struct vsock_transport *transport; >>> + struct vsock_sock *vsk; >>> + int err = 0; >>> + >>> + vsk = vsock_sk(sk); >>> + >>> + if (val > vsk->buffer_size) >>> + return -EINVAL; >>> + >>> + transport = vsk->transport; >>> + >>> + if (!transport) >>> + return -EOPNOTSUPP; >> >> I don't know whether it is better in this case to write it in >> sk->sk_rcvlowat, maybe we can return EOPNOTSUPP only when the trasport >> is assigned and set_rcvlowat is not defined. This is because usually the >> options are set just after creation, when the transport is practically >> unassigned. >> >> I mean something like this: >> >> if (transport) { >> if (transport->set_rcvlowat) >> return transport->set_rcvlowat(vsk, val); >> else >> return -EOPNOTSUPP; >> } >> >> WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >> >> return 0; > > Since hv_sock implements `set_rcvlowat` to return EOPNOTSUPP. maybe we > can just do the following: > > if (transport && transport->set_rcvlowat) > return transport->set_rcvlowat(vsk, val); > > WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); > return 0; > > That is, the default behavior is to set sk->sk_rcvlowat, but for > transports that want a different behavior, they need to define > set_rcvlowat() (like hv_sock). Hm ok, i see. I've implemented logic when non-empty transport is required, because hyperv transport forbids to set SO_RCVLOWAT, so user needs to call this setsockopt AFTER transport is assigned(to check that transport allows it. Not after socket creation as You mentioned above). Otherwise there is no sense in such callback - it will be never used. Also in code above - for hyperv we will have different behavior depends on when set_rcvlowat is called: before or after transport assignment. Is it ok? > > Thanks, > Stefano >
On 09.08.2022 12:37, Arseniy Krasnov wrote: > On 08.08.2022 13:30, Stefano Garzarella wrote: >> On Mon, Aug 8, 2022 at 12:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >>> >>> On Wed, Aug 03, 2022 at 01:51:05PM +0000, Arseniy Krasnov wrote: >>>> This adds transport specific callback for SO_RCVLOWAT, because in some >>>> transports it may be difficult to know current available number of bytes >>>> ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it. >>>> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>> --- >>>> include/net/af_vsock.h | 1 + >>>> net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++ >>>> 2 files changed, 26 insertions(+) >>>> >>>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >>>> index f742e50207fb..eae5874bae35 100644 >>>> --- a/include/net/af_vsock.h >>>> +++ b/include/net/af_vsock.h >>>> @@ -134,6 +134,7 @@ struct vsock_transport { >>>> u64 (*stream_rcvhiwat)(struct vsock_sock *); >>>> bool (*stream_is_active)(struct vsock_sock *); >>>> bool (*stream_allow)(u32 cid, u32 port); >>>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>> >>> checkpatch suggests to add identifier names. For some we put them in, >>> for others we didn't, but I suggest putting them in for the new ones >>> because I think it's clearer too. >>> >>> WARNING: function definition argument 'struct vsock_sock *' should also >>> have an identifier name >>> #25: FILE: include/net/af_vsock.h:137: >>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>> >>> WARNING: function definition argument 'int' should also have an identifier name >>> #25: FILE: include/net/af_vsock.h:137: >>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>> >>> total: 0 errors, 2 warnings, 0 checks, 44 lines checked >>> >>>> >>>> /* SEQ_PACKET. */ >>>> ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg, >>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >>>> index f04abf662ec6..016ad5ff78b7 100644 >>>> --- a/net/vmw_vsock/af_vsock.c >>>> +++ b/net/vmw_vsock/af_vsock.c >>>> @@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, >>>> return err; >>>> } >>>> >>>> +static int vsock_set_rcvlowat(struct sock *sk, int val) >>>> +{ >>>> + const struct vsock_transport *transport; >>>> + struct vsock_sock *vsk; >>>> + int err = 0; >>>> + >>>> + vsk = vsock_sk(sk); >>>> + >>>> + if (val > vsk->buffer_size) >>>> + return -EINVAL; >>>> + >>>> + transport = vsk->transport; >>>> + >>>> + if (!transport) >>>> + return -EOPNOTSUPP; >>> >>> I don't know whether it is better in this case to write it in >>> sk->sk_rcvlowat, maybe we can return EOPNOTSUPP only when the trasport >>> is assigned and set_rcvlowat is not defined. This is because usually the >>> options are set just after creation, when the transport is practically >>> unassigned. >>> >>> I mean something like this: >>> >>> if (transport) { >>> if (transport->set_rcvlowat) >>> return transport->set_rcvlowat(vsk, val); >>> else >>> return -EOPNOTSUPP; >>> } >>> >>> WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >>> >>> return 0; >> >> Since hv_sock implements `set_rcvlowat` to return EOPNOTSUPP. maybe we >> can just do the following: >> >> if (transport && transport->set_rcvlowat) >> return transport->set_rcvlowat(vsk, val); >> >> WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >> return 0; >> >> That is, the default behavior is to set sk->sk_rcvlowat, but for >> transports that want a different behavior, they need to define >> set_rcvlowat() (like hv_sock). > Hm ok, i see. I've implemented logic when non-empty transport is required, because hyperv transport > forbids to set SO_RCVLOWAT, so user needs to call this setsockopt AFTER transport is assigned(to check > that transport allows it. Not after socket creation as You mentioned above). Otherwise there is no sense > in such callback - it will be never used. Also in code above - for hyperv we will have different behavior > depends on when set_rcvlowat is called: before or after transport assignment. Is it ok? sorry, i mean: for hyperv, if user sets sk_rcvlowat before transport is assigned, it sees 0 - success, but in fact hyperv transport forbids this option. >> >> Thanks, >> Stefano >> >
On Tue, Aug 09, 2022 at 09:45:47AM +0000, Arseniy Krasnov wrote: >On 09.08.2022 12:37, Arseniy Krasnov wrote: >> On 08.08.2022 13:30, Stefano Garzarella wrote: >>> On Mon, Aug 8, 2022 at 12:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >>>> >>>> On Wed, Aug 03, 2022 at 01:51:05PM +0000, Arseniy Krasnov wrote: >>>>> This adds transport specific callback for SO_RCVLOWAT, because in some >>>>> transports it may be difficult to know current available number of bytes >>>>> ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it. >>>>> >>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>> --- >>>>> include/net/af_vsock.h | 1 + >>>>> net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++ >>>>> 2 files changed, 26 insertions(+) >>>>> >>>>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >>>>> index f742e50207fb..eae5874bae35 100644 >>>>> --- a/include/net/af_vsock.h >>>>> +++ b/include/net/af_vsock.h >>>>> @@ -134,6 +134,7 @@ struct vsock_transport { >>>>> u64 (*stream_rcvhiwat)(struct vsock_sock *); >>>>> bool (*stream_is_active)(struct vsock_sock *); >>>>> bool (*stream_allow)(u32 cid, u32 port); >>>>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>>> >>>> checkpatch suggests to add identifier names. For some we put them in, >>>> for others we didn't, but I suggest putting them in for the new ones >>>> because I think it's clearer too. >>>> >>>> WARNING: function definition argument 'struct vsock_sock *' should also >>>> have an identifier name >>>> #25: FILE: include/net/af_vsock.h:137: >>>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>>> >>>> WARNING: function definition argument 'int' should also have an identifier name >>>> #25: FILE: include/net/af_vsock.h:137: >>>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>>> >>>> total: 0 errors, 2 warnings, 0 checks, 44 lines checked >>>> >>>>> >>>>> /* SEQ_PACKET. */ >>>>> ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg, >>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >>>>> index f04abf662ec6..016ad5ff78b7 100644 >>>>> --- a/net/vmw_vsock/af_vsock.c >>>>> +++ b/net/vmw_vsock/af_vsock.c >>>>> @@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, >>>>> return err; >>>>> } >>>>> >>>>> +static int vsock_set_rcvlowat(struct sock *sk, int val) >>>>> +{ >>>>> + const struct vsock_transport *transport; >>>>> + struct vsock_sock *vsk; >>>>> + int err = 0; >>>>> + >>>>> + vsk = vsock_sk(sk); >>>>> + >>>>> + if (val > vsk->buffer_size) >>>>> + return -EINVAL; >>>>> + >>>>> + transport = vsk->transport; >>>>> + >>>>> + if (!transport) >>>>> + return -EOPNOTSUPP; >>>> >>>> I don't know whether it is better in this case to write it in >>>> sk->sk_rcvlowat, maybe we can return EOPNOTSUPP only when the trasport >>>> is assigned and set_rcvlowat is not defined. This is because usually the >>>> options are set just after creation, when the transport is practically >>>> unassigned. >>>> >>>> I mean something like this: >>>> >>>> if (transport) { >>>> if (transport->set_rcvlowat) >>>> return transport->set_rcvlowat(vsk, val); >>>> else >>>> return -EOPNOTSUPP; >>>> } >>>> >>>> WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >>>> >>>> return 0; >>> >>> Since hv_sock implements `set_rcvlowat` to return EOPNOTSUPP. maybe we >>> can just do the following: >>> >>> if (transport && transport->set_rcvlowat) >>> return transport->set_rcvlowat(vsk, val); >>> >>> WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >>> return 0; >>> >>> That is, the default behavior is to set sk->sk_rcvlowat, but for >>> transports that want a different behavior, they need to define >>> set_rcvlowat() (like hv_sock). >> Hm ok, i see. I've implemented logic when non-empty transport is required, because hyperv transport >> forbids to set SO_RCVLOWAT, so user needs to call this setsockopt AFTER transport is assigned(to check >> that transport allows it. Not after socket creation as You mentioned above). Otherwise there is no sense >> in such callback - it will be never used. Also in code above - for hyperv we will have different behavior >> depends on when set_rcvlowat is called: before or after transport assignment. Is it ok? >sorry, i mean: for hyperv, if user sets sk_rcvlowat before transport is assigned, it sees 0 - success, but in fact >hyperv transport forbids this option. I see, but I think it's better to set it and not respect in hyperv (as we've practically done until now with all transports) than to prevent the setting until we assign a transport. At most when we use hyperv anyway we get notified per byte, so we should just get more notifications than we expect. Thanks, Stefano
On 09.08.2022 13:03, Stefano Garzarella wrote: > On Tue, Aug 09, 2022 at 09:45:47AM +0000, Arseniy Krasnov wrote: >> On 09.08.2022 12:37, Arseniy Krasnov wrote: >>> On 08.08.2022 13:30, Stefano Garzarella wrote: >>>> On Mon, Aug 8, 2022 at 12:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >>>>> >>>>> On Wed, Aug 03, 2022 at 01:51:05PM +0000, Arseniy Krasnov wrote: >>>>>> This adds transport specific callback for SO_RCVLOWAT, because in some >>>>>> transports it may be difficult to know current available number of bytes >>>>>> ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it. >>>>>> >>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>>> --- >>>>>> include/net/af_vsock.h | 1 + >>>>>> net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++ >>>>>> 2 files changed, 26 insertions(+) >>>>>> >>>>>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >>>>>> index f742e50207fb..eae5874bae35 100644 >>>>>> --- a/include/net/af_vsock.h >>>>>> +++ b/include/net/af_vsock.h >>>>>> @@ -134,6 +134,7 @@ struct vsock_transport { >>>>>> u64 (*stream_rcvhiwat)(struct vsock_sock *); >>>>>> bool (*stream_is_active)(struct vsock_sock *); >>>>>> bool (*stream_allow)(u32 cid, u32 port); >>>>>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>>>> >>>>> checkpatch suggests to add identifier names. For some we put them in, >>>>> for others we didn't, but I suggest putting them in for the new ones >>>>> because I think it's clearer too. >>>>> >>>>> WARNING: function definition argument 'struct vsock_sock *' should also >>>>> have an identifier name >>>>> #25: FILE: include/net/af_vsock.h:137: >>>>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>>>> >>>>> WARNING: function definition argument 'int' should also have an identifier name >>>>> #25: FILE: include/net/af_vsock.h:137: >>>>> + int (*set_rcvlowat)(struct vsock_sock *, int); >>>>> >>>>> total: 0 errors, 2 warnings, 0 checks, 44 lines checked >>>>> >>>>>> >>>>>> /* SEQ_PACKET. */ >>>>>> ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg, >>>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >>>>>> index f04abf662ec6..016ad5ff78b7 100644 >>>>>> --- a/net/vmw_vsock/af_vsock.c >>>>>> +++ b/net/vmw_vsock/af_vsock.c >>>>>> @@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, >>>>>> return err; >>>>>> } >>>>>> >>>>>> +static int vsock_set_rcvlowat(struct sock *sk, int val) >>>>>> +{ >>>>>> + const struct vsock_transport *transport; >>>>>> + struct vsock_sock *vsk; >>>>>> + int err = 0; >>>>>> + >>>>>> + vsk = vsock_sk(sk); >>>>>> + >>>>>> + if (val > vsk->buffer_size) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + transport = vsk->transport; >>>>>> + >>>>>> + if (!transport) >>>>>> + return -EOPNOTSUPP; >>>>> >>>>> I don't know whether it is better in this case to write it in >>>>> sk->sk_rcvlowat, maybe we can return EOPNOTSUPP only when the trasport >>>>> is assigned and set_rcvlowat is not defined. This is because usually the >>>>> options are set just after creation, when the transport is practically >>>>> unassigned. >>>>> >>>>> I mean something like this: >>>>> >>>>> if (transport) { >>>>> if (transport->set_rcvlowat) >>>>> return transport->set_rcvlowat(vsk, val); >>>>> else >>>>> return -EOPNOTSUPP; >>>>> } >>>>> >>>>> WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >>>>> >>>>> return 0; >>>> >>>> Since hv_sock implements `set_rcvlowat` to return EOPNOTSUPP. maybe we >>>> can just do the following: >>>> >>>> if (transport && transport->set_rcvlowat) >>>> return transport->set_rcvlowat(vsk, val); >>>> >>>> WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); >>>> return 0; >>>> >>>> That is, the default behavior is to set sk->sk_rcvlowat, but for >>>> transports that want a different behavior, they need to define >>>> set_rcvlowat() (like hv_sock). >>> Hm ok, i see. I've implemented logic when non-empty transport is required, because hyperv transport >>> forbids to set SO_RCVLOWAT, so user needs to call this setsockopt AFTER transport is assigned(to check >>> that transport allows it. Not after socket creation as You mentioned above). Otherwise there is no sense >>> in such callback - it will be never used. Also in code above - for hyperv we will have different behavior >>> depends on when set_rcvlowat is called: before or after transport assignment. Is it ok? >> sorry, i mean: for hyperv, if user sets sk_rcvlowat before transport is assigned, it sees 0 - success, but in fact >> hyperv transport forbids this option. > > I see, but I think it's better to set it and not respect in hyperv (as we've practically done until now with all transports) than to prevent the setting until we assign a transport. > > At most when we use hyperv anyway we get notified per byte, so we should just get more notifications than we expect. see it, ok, thanks > > Thanks, > Stefano >
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index f742e50207fb..eae5874bae35 100644 --- a/include/net/af_vsock.h +++ b/include/net/af_vsock.h @@ -134,6 +134,7 @@ struct vsock_transport { u64 (*stream_rcvhiwat)(struct vsock_sock *); bool (*stream_is_active)(struct vsock_sock *); bool (*stream_allow)(u32 cid, u32 port); + int (*set_rcvlowat)(struct vsock_sock *, int); /* SEQ_PACKET. */ ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg, diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index f04abf662ec6..016ad5ff78b7 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, return err; } +static int vsock_set_rcvlowat(struct sock *sk, int val) +{ + const struct vsock_transport *transport; + struct vsock_sock *vsk; + int err = 0; + + vsk = vsock_sk(sk); + + if (val > vsk->buffer_size) + return -EINVAL; + + transport = vsk->transport; + + if (!transport) + return -EOPNOTSUPP; + + if (transport->set_rcvlowat) + err = transport->set_rcvlowat(vsk, val); + else + WRITE_ONCE(sk->sk_rcvlowat, val ? : 1); + + return err; +} + static const struct proto_ops vsock_stream_ops = { .family = PF_VSOCK, .owner = THIS_MODULE, @@ -2148,6 +2172,7 @@ static const struct proto_ops vsock_stream_ops = { .recvmsg = vsock_connectible_recvmsg, .mmap = sock_no_mmap, .sendpage = sock_no_sendpage, + .set_rcvlowat = vsock_set_rcvlowat, }; static const struct proto_ops vsock_seqpacket_ops = {
This adds transport specific callback for SO_RCVLOWAT, because in some transports it may be difficult to know current available number of bytes ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it. Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- include/net/af_vsock.h | 1 + net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+)