Stefano Garzarella
2022-Aug-08 10:23 UTC
[RFC PATCH v3 1/9] vsock: SO_RCVLOWAT transport set callback
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 at 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
Stefano Garzarella
2022-Aug-08 10:30 UTC
[RFC PATCH v3 1/9] vsock: SO_RCVLOWAT transport set callback
On Mon, Aug 8, 2022 at 12:23 PM Stefano Garzarella <sgarzare at 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 at 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