mbox series

[net-next,v5,0/6] virtio/vsock: introduce MSG_EOR flag for SEQPACKET

Message ID 20210903123016.3272800-1-arseny.krasnov@kaspersky.com (mailing list archive)
Headers show
Series virtio/vsock: introduce MSG_EOR flag for SEQPACKET | expand

Message

Arseny Krasnov Sept. 3, 2021, 12:30 p.m. UTC
This patchset implements support of MSG_EOR bit for SEQPACKET
AF_VSOCK sockets over virtio transport.
	First we need to define 'messages' and 'records' like this:
Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
etc. It has fixed maximum length, and it bounds are visible using
return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
Current implementation based on message definition above.
	Record has unlimited length, it consists of multiple message,
and bounds of record are visible via MSG_EOR flag returned from
'recvmsg()' call. Sender passes MSG_EOR to sending system call and
receiver will see MSG_EOR when corresponding message will be processed.
	Idea of patchset comes from POSIX: it says that SEQPACKET
supports record boundaries which are visible for receiver using
MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
and we don't need to maintain boundaries of corresponding send -
receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
that all these calls operates with messages, e.g. 'sendXXX()' sends
message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
must read one entire message from socket, dropping all out of size
bytes. Thus, both message boundaries and MSG_EOR bit must be supported
to follow POSIX rules.
	To support MSG_EOR new bit was added along with existing
'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
is used to mark 'MSG_EOR' bit passed from userspace.
	This patchset includes simple test for MSG_EOR.

 Arseny Krasnov(6):
  virtio/vsock: rename 'EOR' to 'EOM' bit.
  virtio/vsock: add 'VIRTIO_VSOCK_SEQ_EOR' bit.
  vhost/vsock: support MSG_EOR bit processing
  virtio/vsock: support MSG_EOR bit processing
  af_vsock: rename variables in receive loop
  vsock_test: update message bounds test for MSG_EOR

 drivers/vhost/vsock.c                   | 28 +++++++++++++----------
 include/uapi/linux/virtio_vsock.h       |  3 ++-
 net/vmw_vsock/af_vsock.c                | 10 ++++----
 net/vmw_vsock/virtio_transport_common.c | 23 ++++++++++++-------
 tools/testing/vsock/vsock_test.c        |  8 ++++++-
 5 files changed, 45 insertions(+), 27 deletions(-)

 v4 -> v5:
 - Move bitwise and out of le32_to_cpu() in 0003.

 v3 -> v4:
 - 'sendXXX()' renamed to 'send*()' in 0002- commit msg.
 - Comment about bit restore updated in 0003-.
 - 'same' renamed to 'similar' in 0003- commit msg.
 - u32 used instead of uint32_t in 0003-.

 v2 -> v3:
 - 'virtio/vsock: rename 'EOR' to 'EOM' bit.' - commit message updated.
 - 'VIRTIO_VSOCK_SEQ_EOR' bit add moved to separate patch.
 - 'vhost/vsock: support MSG_EOR bit processing' - commit message
   updated.
 - 'vhost/vsock: support MSG_EOR bit processing' - removed unneeded
   'le32_to_cpu()', because input argument was already in CPU
   endianness.

 v1 -> v2:
 - 'VIRTIO_VSOCK_SEQ_EOR' is renamed to 'VIRTIO_VSOCK_SEQ_EOM', to
   support backward compatibility.
 - use bitmask of flags to restore in vhost.c, instead of separated
   bool variable for each flag.
 - test for EAGAIN removed, as logically it is not part of this
   patchset(will be sent separately).
 - cover letter updated(added part with POSIX description).

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>

Comments

Jakub Kicinski Sept. 3, 2021, 10:17 p.m. UTC | #1
On Fri, 3 Sep 2021 15:30:13 +0300 Arseny Krasnov wrote:
> 	This patchset implements support of MSG_EOR bit for SEQPACKET
> AF_VSOCK sockets over virtio transport.
> 	First we need to define 'messages' and 'records' like this:
> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
> etc. It has fixed maximum length, and it bounds are visible using
> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
> Current implementation based on message definition above.
> 	Record has unlimited length, it consists of multiple message,
> and bounds of record are visible via MSG_EOR flag returned from
> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
> receiver will see MSG_EOR when corresponding message will be processed.
> 	Idea of patchset comes from POSIX: it says that SEQPACKET
> supports record boundaries which are visible for receiver using
> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
> and we don't need to maintain boundaries of corresponding send -
> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
> that all these calls operates with messages, e.g. 'sendXXX()' sends
> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
> must read one entire message from socket, dropping all out of size
> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
> to follow POSIX rules.
> 	To support MSG_EOR new bit was added along with existing
> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
> is used to mark 'MSG_EOR' bit passed from userspace.
> 	This patchset includes simple test for MSG_EOR.

Assuming you want this merged to net-next:


# Form letter - net-next is closed

We have already sent the networking pull request for 5.15 
and therefore net-next is closed for new drivers, features, 
code refactoring and optimizations. We are currently accepting 
bug fixes only.

Please repost when net-next reopens after 5.15-rc1 is cut.

Look out for the announcement on the mailing list or check:
http://vger.kernel.org/~davem/net-next.html

RFC patches sent for review only are obviously welcome at any time.
Michael S. Tsirkin Sept. 5, 2021, 3:55 p.m. UTC | #2
On Fri, Sep 03, 2021 at 03:30:13PM +0300, Arseny Krasnov wrote:
> 	This patchset implements support of MSG_EOR bit for SEQPACKET
> AF_VSOCK sockets over virtio transport.
> 	First we need to define 'messages' and 'records' like this:
> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
> etc. It has fixed maximum length, and it bounds are visible using
> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
> Current implementation based on message definition above.
> 	Record has unlimited length, it consists of multiple message,
> and bounds of record are visible via MSG_EOR flag returned from
> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
> receiver will see MSG_EOR when corresponding message will be processed.
> 	Idea of patchset comes from POSIX: it says that SEQPACKET
> supports record boundaries which are visible for receiver using
> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
> and we don't need to maintain boundaries of corresponding send -
> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
> that all these calls operates with messages, e.g. 'sendXXX()' sends
> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
> must read one entire message from socket, dropping all out of size
> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
> to follow POSIX rules.
> 	To support MSG_EOR new bit was added along with existing
> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
> is used to mark 'MSG_EOR' bit passed from userspace.
> 	This patchset includes simple test for MSG_EOR.


I'm prepared to merge this for this window,
but I'm not sure who's supposed to ack the net/vmw_vsock/af_vsock.c
bits. It's a harmless variable renaming so maybe it does not matter.

The rest is virtio stuff so I guess my tree is ok.

Objections, anyone?



>  Arseny Krasnov(6):
>   virtio/vsock: rename 'EOR' to 'EOM' bit.
>   virtio/vsock: add 'VIRTIO_VSOCK_SEQ_EOR' bit.
>   vhost/vsock: support MSG_EOR bit processing
>   virtio/vsock: support MSG_EOR bit processing
>   af_vsock: rename variables in receive loop
>   vsock_test: update message bounds test for MSG_EOR
> 
>  drivers/vhost/vsock.c                   | 28 +++++++++++++----------
>  include/uapi/linux/virtio_vsock.h       |  3 ++-
>  net/vmw_vsock/af_vsock.c                | 10 ++++----
>  net/vmw_vsock/virtio_transport_common.c | 23 ++++++++++++-------
>  tools/testing/vsock/vsock_test.c        |  8 ++++++-
>  5 files changed, 45 insertions(+), 27 deletions(-)
> 
>  v4 -> v5:
>  - Move bitwise and out of le32_to_cpu() in 0003.
> 
>  v3 -> v4:
>  - 'sendXXX()' renamed to 'send*()' in 0002- commit msg.
>  - Comment about bit restore updated in 0003-.
>  - 'same' renamed to 'similar' in 0003- commit msg.
>  - u32 used instead of uint32_t in 0003-.
> 
>  v2 -> v3:
>  - 'virtio/vsock: rename 'EOR' to 'EOM' bit.' - commit message updated.
>  - 'VIRTIO_VSOCK_SEQ_EOR' bit add moved to separate patch.
>  - 'vhost/vsock: support MSG_EOR bit processing' - commit message
>    updated.
>  - 'vhost/vsock: support MSG_EOR bit processing' - removed unneeded
>    'le32_to_cpu()', because input argument was already in CPU
>    endianness.
> 
>  v1 -> v2:
>  - 'VIRTIO_VSOCK_SEQ_EOR' is renamed to 'VIRTIO_VSOCK_SEQ_EOM', to
>    support backward compatibility.
>  - use bitmask of flags to restore in vhost.c, instead of separated
>    bool variable for each flag.
>  - test for EAGAIN removed, as logically it is not part of this
>    patchset(will be sent separately).
>  - cover letter updated(added part with POSIX description).
> 
> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> -- 
> 2.25.1
Arseny Krasnov Sept. 5, 2021, 4:02 p.m. UTC | #3
On 05.09.2021 18:55, Michael S. Tsirkin wrote:
> On Fri, Sep 03, 2021 at 03:30:13PM +0300, Arseny Krasnov wrote:
>> 	This patchset implements support of MSG_EOR bit for SEQPACKET
>> AF_VSOCK sockets over virtio transport.
>> 	First we need to define 'messages' and 'records' like this:
>> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
>> etc. It has fixed maximum length, and it bounds are visible using
>> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
>> Current implementation based on message definition above.
>> 	Record has unlimited length, it consists of multiple message,
>> and bounds of record are visible via MSG_EOR flag returned from
>> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
>> receiver will see MSG_EOR when corresponding message will be processed.
>> 	Idea of patchset comes from POSIX: it says that SEQPACKET
>> supports record boundaries which are visible for receiver using
>> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
>> and we don't need to maintain boundaries of corresponding send -
>> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
>> that all these calls operates with messages, e.g. 'sendXXX()' sends
>> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
>> must read one entire message from socket, dropping all out of size
>> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
>> to follow POSIX rules.
>> 	To support MSG_EOR new bit was added along with existing
>> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
>> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
>> is used to mark 'MSG_EOR' bit passed from userspace.
>> 	This patchset includes simple test for MSG_EOR.
>
> I'm prepared to merge this for this window,
> but I'm not sure who's supposed to ack the net/vmw_vsock/af_vsock.c
> bits. It's a harmless variable renaming so maybe it does not matter.
>
> The rest is virtio stuff so I guess my tree is ok.
>
> Objections, anyone?

https://lkml.org/lkml/2021/9/3/76 this is v4. It is same as v5 in af_vsock.c changes.

It has Reviewed by from Stefano Garzarella.

>
>
>>  Arseny Krasnov(6):
>>   virtio/vsock: rename 'EOR' to 'EOM' bit.
>>   virtio/vsock: add 'VIRTIO_VSOCK_SEQ_EOR' bit.
>>   vhost/vsock: support MSG_EOR bit processing
>>   virtio/vsock: support MSG_EOR bit processing
>>   af_vsock: rename variables in receive loop
>>   vsock_test: update message bounds test for MSG_EOR
>>
>>  drivers/vhost/vsock.c                   | 28 +++++++++++++----------
>>  include/uapi/linux/virtio_vsock.h       |  3 ++-
>>  net/vmw_vsock/af_vsock.c                | 10 ++++----
>>  net/vmw_vsock/virtio_transport_common.c | 23 ++++++++++++-------
>>  tools/testing/vsock/vsock_test.c        |  8 ++++++-
>>  5 files changed, 45 insertions(+), 27 deletions(-)
>>
>>  v4 -> v5:
>>  - Move bitwise and out of le32_to_cpu() in 0003.
>>
>>  v3 -> v4:
>>  - 'sendXXX()' renamed to 'send*()' in 0002- commit msg.
>>  - Comment about bit restore updated in 0003-.
>>  - 'same' renamed to 'similar' in 0003- commit msg.
>>  - u32 used instead of uint32_t in 0003-.
>>
>>  v2 -> v3:
>>  - 'virtio/vsock: rename 'EOR' to 'EOM' bit.' - commit message updated.
>>  - 'VIRTIO_VSOCK_SEQ_EOR' bit add moved to separate patch.
>>  - 'vhost/vsock: support MSG_EOR bit processing' - commit message
>>    updated.
>>  - 'vhost/vsock: support MSG_EOR bit processing' - removed unneeded
>>    'le32_to_cpu()', because input argument was already in CPU
>>    endianness.
>>
>>  v1 -> v2:
>>  - 'VIRTIO_VSOCK_SEQ_EOR' is renamed to 'VIRTIO_VSOCK_SEQ_EOM', to
>>    support backward compatibility.
>>  - use bitmask of flags to restore in vhost.c, instead of separated
>>    bool variable for each flag.
>>  - test for EAGAIN removed, as logically it is not part of this
>>    patchset(will be sent separately).
>>  - cover letter updated(added part with POSIX description).
>>
>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>> -- 
>> 2.25.1
>
Michael S. Tsirkin Sept. 5, 2021, 4:19 p.m. UTC | #4
On Sun, Sep 05, 2021 at 07:02:44PM +0300, Arseny Krasnov wrote:
> 
> On 05.09.2021 18:55, Michael S. Tsirkin wrote:
> > On Fri, Sep 03, 2021 at 03:30:13PM +0300, Arseny Krasnov wrote:
> >> 	This patchset implements support of MSG_EOR bit for SEQPACKET
> >> AF_VSOCK sockets over virtio transport.
> >> 	First we need to define 'messages' and 'records' like this:
> >> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
> >> etc. It has fixed maximum length, and it bounds are visible using
> >> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
> >> Current implementation based on message definition above.
> >> 	Record has unlimited length, it consists of multiple message,
> >> and bounds of record are visible via MSG_EOR flag returned from
> >> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
> >> receiver will see MSG_EOR when corresponding message will be processed.
> >> 	Idea of patchset comes from POSIX: it says that SEQPACKET
> >> supports record boundaries which are visible for receiver using
> >> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
> >> and we don't need to maintain boundaries of corresponding send -
> >> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
> >> that all these calls operates with messages, e.g. 'sendXXX()' sends
> >> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
> >> must read one entire message from socket, dropping all out of size
> >> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
> >> to follow POSIX rules.
> >> 	To support MSG_EOR new bit was added along with existing
> >> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
> >> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
> >> is used to mark 'MSG_EOR' bit passed from userspace.
> >> 	This patchset includes simple test for MSG_EOR.
> >
> > I'm prepared to merge this for this window,
> > but I'm not sure who's supposed to ack the net/vmw_vsock/af_vsock.c
> > bits. It's a harmless variable renaming so maybe it does not matter.
> >
> > The rest is virtio stuff so I guess my tree is ok.
> >
> > Objections, anyone?
> 
> https://lkml.org/lkml/2021/9/3/76 this is v4. It is same as v5 in af_vsock.c changes.
> 
> It has Reviewed by from Stefano Garzarella.

Is Stefano the maintainer for af_vsock then?
I wasn't sure.

> >
> >
> >>  Arseny Krasnov(6):
> >>   virtio/vsock: rename 'EOR' to 'EOM' bit.
> >>   virtio/vsock: add 'VIRTIO_VSOCK_SEQ_EOR' bit.
> >>   vhost/vsock: support MSG_EOR bit processing
> >>   virtio/vsock: support MSG_EOR bit processing
> >>   af_vsock: rename variables in receive loop
> >>   vsock_test: update message bounds test for MSG_EOR
> >>
> >>  drivers/vhost/vsock.c                   | 28 +++++++++++++----------
> >>  include/uapi/linux/virtio_vsock.h       |  3 ++-
> >>  net/vmw_vsock/af_vsock.c                | 10 ++++----
> >>  net/vmw_vsock/virtio_transport_common.c | 23 ++++++++++++-------
> >>  tools/testing/vsock/vsock_test.c        |  8 ++++++-
> >>  5 files changed, 45 insertions(+), 27 deletions(-)
> >>
> >>  v4 -> v5:
> >>  - Move bitwise and out of le32_to_cpu() in 0003.
> >>
> >>  v3 -> v4:
> >>  - 'sendXXX()' renamed to 'send*()' in 0002- commit msg.
> >>  - Comment about bit restore updated in 0003-.
> >>  - 'same' renamed to 'similar' in 0003- commit msg.
> >>  - u32 used instead of uint32_t in 0003-.
> >>
> >>  v2 -> v3:
> >>  - 'virtio/vsock: rename 'EOR' to 'EOM' bit.' - commit message updated.
> >>  - 'VIRTIO_VSOCK_SEQ_EOR' bit add moved to separate patch.
> >>  - 'vhost/vsock: support MSG_EOR bit processing' - commit message
> >>    updated.
> >>  - 'vhost/vsock: support MSG_EOR bit processing' - removed unneeded
> >>    'le32_to_cpu()', because input argument was already in CPU
> >>    endianness.
> >>
> >>  v1 -> v2:
> >>  - 'VIRTIO_VSOCK_SEQ_EOR' is renamed to 'VIRTIO_VSOCK_SEQ_EOM', to
> >>    support backward compatibility.
> >>  - use bitmask of flags to restore in vhost.c, instead of separated
> >>    bool variable for each flag.
> >>  - test for EAGAIN removed, as logically it is not part of this
> >>    patchset(will be sent separately).
> >>  - cover letter updated(added part with POSIX description).
> >>
> >> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> >> -- 
> >> 2.25.1
> >
Arseny Krasnov Sept. 5, 2021, 4:21 p.m. UTC | #5
On 05.09.2021 19:19, Michael S. Tsirkin wrote:
> On Sun, Sep 05, 2021 at 07:02:44PM +0300, Arseny Krasnov wrote:
>> On 05.09.2021 18:55, Michael S. Tsirkin wrote:
>>> On Fri, Sep 03, 2021 at 03:30:13PM +0300, Arseny Krasnov wrote:
>>>> 	This patchset implements support of MSG_EOR bit for SEQPACKET
>>>> AF_VSOCK sockets over virtio transport.
>>>> 	First we need to define 'messages' and 'records' like this:
>>>> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
>>>> etc. It has fixed maximum length, and it bounds are visible using
>>>> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
>>>> Current implementation based on message definition above.
>>>> 	Record has unlimited length, it consists of multiple message,
>>>> and bounds of record are visible via MSG_EOR flag returned from
>>>> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
>>>> receiver will see MSG_EOR when corresponding message will be processed.
>>>> 	Idea of patchset comes from POSIX: it says that SEQPACKET
>>>> supports record boundaries which are visible for receiver using
>>>> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
>>>> and we don't need to maintain boundaries of corresponding send -
>>>> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
>>>> that all these calls operates with messages, e.g. 'sendXXX()' sends
>>>> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
>>>> must read one entire message from socket, dropping all out of size
>>>> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
>>>> to follow POSIX rules.
>>>> 	To support MSG_EOR new bit was added along with existing
>>>> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
>>>> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
>>>> is used to mark 'MSG_EOR' bit passed from userspace.
>>>> 	This patchset includes simple test for MSG_EOR.
>>> I'm prepared to merge this for this window,
>>> but I'm not sure who's supposed to ack the net/vmw_vsock/af_vsock.c
>>> bits. It's a harmless variable renaming so maybe it does not matter.
>>>
>>> The rest is virtio stuff so I guess my tree is ok.
>>>
>>> Objections, anyone?
>> https://lkml.org/lkml/2021/9/3/76 this is v4. It is same as v5 in af_vsock.c changes.
>>
>> It has Reviewed by from Stefano Garzarella.
> Is Stefano the maintainer for af_vsock then?
> I wasn't sure.
Ack, let's wait for maintainer's comment
>
>>>
>>>>  Arseny Krasnov(6):
>>>>   virtio/vsock: rename 'EOR' to 'EOM' bit.
>>>>   virtio/vsock: add 'VIRTIO_VSOCK_SEQ_EOR' bit.
>>>>   vhost/vsock: support MSG_EOR bit processing
>>>>   virtio/vsock: support MSG_EOR bit processing
>>>>   af_vsock: rename variables in receive loop
>>>>   vsock_test: update message bounds test for MSG_EOR
>>>>
>>>>  drivers/vhost/vsock.c                   | 28 +++++++++++++----------
>>>>  include/uapi/linux/virtio_vsock.h       |  3 ++-
>>>>  net/vmw_vsock/af_vsock.c                | 10 ++++----
>>>>  net/vmw_vsock/virtio_transport_common.c | 23 ++++++++++++-------
>>>>  tools/testing/vsock/vsock_test.c        |  8 ++++++-
>>>>  5 files changed, 45 insertions(+), 27 deletions(-)
>>>>
>>>>  v4 -> v5:
>>>>  - Move bitwise and out of le32_to_cpu() in 0003.
>>>>
>>>>  v3 -> v4:
>>>>  - 'sendXXX()' renamed to 'send*()' in 0002- commit msg.
>>>>  - Comment about bit restore updated in 0003-.
>>>>  - 'same' renamed to 'similar' in 0003- commit msg.
>>>>  - u32 used instead of uint32_t in 0003-.
>>>>
>>>>  v2 -> v3:
>>>>  - 'virtio/vsock: rename 'EOR' to 'EOM' bit.' - commit message updated.
>>>>  - 'VIRTIO_VSOCK_SEQ_EOR' bit add moved to separate patch.
>>>>  - 'vhost/vsock: support MSG_EOR bit processing' - commit message
>>>>    updated.
>>>>  - 'vhost/vsock: support MSG_EOR bit processing' - removed unneeded
>>>>    'le32_to_cpu()', because input argument was already in CPU
>>>>    endianness.
>>>>
>>>>  v1 -> v2:
>>>>  - 'VIRTIO_VSOCK_SEQ_EOR' is renamed to 'VIRTIO_VSOCK_SEQ_EOM', to
>>>>    support backward compatibility.
>>>>  - use bitmask of flags to restore in vhost.c, instead of separated
>>>>    bool variable for each flag.
>>>>  - test for EAGAIN removed, as logically it is not part of this
>>>>    patchset(will be sent separately).
>>>>  - cover letter updated(added part with POSIX description).
>>>>
>>>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>>>> -- 
>>>> 2.25.1
>
Michael S. Tsirkin Sept. 5, 2021, 8:18 p.m. UTC | #6
On Sun, Sep 05, 2021 at 07:21:10PM +0300, Arseny Krasnov wrote:
> 
> On 05.09.2021 19:19, Michael S. Tsirkin wrote:
> > On Sun, Sep 05, 2021 at 07:02:44PM +0300, Arseny Krasnov wrote:
> >> On 05.09.2021 18:55, Michael S. Tsirkin wrote:
> >>> On Fri, Sep 03, 2021 at 03:30:13PM +0300, Arseny Krasnov wrote:
> >>>> 	This patchset implements support of MSG_EOR bit for SEQPACKET
> >>>> AF_VSOCK sockets over virtio transport.
> >>>> 	First we need to define 'messages' and 'records' like this:
> >>>> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
> >>>> etc. It has fixed maximum length, and it bounds are visible using
> >>>> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
> >>>> Current implementation based on message definition above.
> >>>> 	Record has unlimited length, it consists of multiple message,
> >>>> and bounds of record are visible via MSG_EOR flag returned from
> >>>> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
> >>>> receiver will see MSG_EOR when corresponding message will be processed.
> >>>> 	Idea of patchset comes from POSIX: it says that SEQPACKET
> >>>> supports record boundaries which are visible for receiver using
> >>>> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
> >>>> and we don't need to maintain boundaries of corresponding send -
> >>>> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
> >>>> that all these calls operates with messages, e.g. 'sendXXX()' sends
> >>>> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
> >>>> must read one entire message from socket, dropping all out of size
> >>>> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
> >>>> to follow POSIX rules.
> >>>> 	To support MSG_EOR new bit was added along with existing
> >>>> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
> >>>> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
> >>>> is used to mark 'MSG_EOR' bit passed from userspace.
> >>>> 	This patchset includes simple test for MSG_EOR.
> >>> I'm prepared to merge this for this window,
> >>> but I'm not sure who's supposed to ack the net/vmw_vsock/af_vsock.c
> >>> bits. It's a harmless variable renaming so maybe it does not matter.
> >>>
> >>> The rest is virtio stuff so I guess my tree is ok.
> >>>
> >>> Objections, anyone?
> >> https://lkml.org/lkml/2021/9/3/76 this is v4. It is same as v5 in af_vsock.c changes.
> >>
> >> It has Reviewed by from Stefano Garzarella.
> > Is Stefano the maintainer for af_vsock then?
> > I wasn't sure.
> Ack, let's wait for maintainer's comment


The specific patch is a trivial variable renaming so
I parked this in my tree for now, will merge unless I
hear any objections in the next couple of days.

> >>>
> >>>>  Arseny Krasnov(6):
> >>>>   virtio/vsock: rename 'EOR' to 'EOM' bit.
> >>>>   virtio/vsock: add 'VIRTIO_VSOCK_SEQ_EOR' bit.
> >>>>   vhost/vsock: support MSG_EOR bit processing
> >>>>   virtio/vsock: support MSG_EOR bit processing
> >>>>   af_vsock: rename variables in receive loop
> >>>>   vsock_test: update message bounds test for MSG_EOR
> >>>>
> >>>>  drivers/vhost/vsock.c                   | 28 +++++++++++++----------
> >>>>  include/uapi/linux/virtio_vsock.h       |  3 ++-
> >>>>  net/vmw_vsock/af_vsock.c                | 10 ++++----
> >>>>  net/vmw_vsock/virtio_transport_common.c | 23 ++++++++++++-------
> >>>>  tools/testing/vsock/vsock_test.c        |  8 ++++++-
> >>>>  5 files changed, 45 insertions(+), 27 deletions(-)
> >>>>
> >>>>  v4 -> v5:
> >>>>  - Move bitwise and out of le32_to_cpu() in 0003.
> >>>>
> >>>>  v3 -> v4:
> >>>>  - 'sendXXX()' renamed to 'send*()' in 0002- commit msg.
> >>>>  - Comment about bit restore updated in 0003-.
> >>>>  - 'same' renamed to 'similar' in 0003- commit msg.
> >>>>  - u32 used instead of uint32_t in 0003-.
> >>>>
> >>>>  v2 -> v3:
> >>>>  - 'virtio/vsock: rename 'EOR' to 'EOM' bit.' - commit message updated.
> >>>>  - 'VIRTIO_VSOCK_SEQ_EOR' bit add moved to separate patch.
> >>>>  - 'vhost/vsock: support MSG_EOR bit processing' - commit message
> >>>>    updated.
> >>>>  - 'vhost/vsock: support MSG_EOR bit processing' - removed unneeded
> >>>>    'le32_to_cpu()', because input argument was already in CPU
> >>>>    endianness.
> >>>>
> >>>>  v1 -> v2:
> >>>>  - 'VIRTIO_VSOCK_SEQ_EOR' is renamed to 'VIRTIO_VSOCK_SEQ_EOM', to
> >>>>    support backward compatibility.
> >>>>  - use bitmask of flags to restore in vhost.c, instead of separated
> >>>>    bool variable for each flag.
> >>>>  - test for EAGAIN removed, as logically it is not part of this
> >>>>    patchset(will be sent separately).
> >>>>  - cover letter updated(added part with POSIX description).
> >>>>
> >>>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> >>>> -- 
> >>>> 2.25.1
> >
Stefano Garzarella Sept. 6, 2021, 7:33 a.m. UTC | #7
On Sun, Sep 05, 2021 at 04:18:52PM -0400, Michael S. Tsirkin wrote:
>On Sun, Sep 05, 2021 at 07:21:10PM +0300, Arseny Krasnov wrote:
>>
>> On 05.09.2021 19:19, Michael S. Tsirkin wrote:
>> > On Sun, Sep 05, 2021 at 07:02:44PM +0300, Arseny Krasnov wrote:
>> >> On 05.09.2021 18:55, Michael S. Tsirkin wrote:
>> >>> On Fri, Sep 03, 2021 at 03:30:13PM +0300, Arseny Krasnov wrote:
>> >>>> 	This patchset implements support of MSG_EOR bit for SEQPACKET
>> >>>> AF_VSOCK sockets over virtio transport.
>> >>>> 	First we need to define 'messages' and 'records' like this:
>> >>>> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
>> >>>> etc. It has fixed maximum length, and it bounds are visible using
>> >>>> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
>> >>>> Current implementation based on message definition above.
>> >>>> 	Record has unlimited length, it consists of multiple message,
>> >>>> and bounds of record are visible via MSG_EOR flag returned from
>> >>>> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
>> >>>> receiver will see MSG_EOR when corresponding message will be processed.
>> >>>> 	Idea of patchset comes from POSIX: it says that SEQPACKET
>> >>>> supports record boundaries which are visible for receiver using
>> >>>> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
>> >>>> and we don't need to maintain boundaries of corresponding send -
>> >>>> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
>> >>>> that all these calls operates with messages, e.g. 'sendXXX()' sends
>> >>>> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
>> >>>> must read one entire message from socket, dropping all out of size
>> >>>> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
>> >>>> to follow POSIX rules.
>> >>>> 	To support MSG_EOR new bit was added along with existing
>> >>>> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
>> >>>> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
>> >>>> is used to mark 'MSG_EOR' bit passed from userspace.
>> >>>> 	This patchset includes simple test for MSG_EOR.
>> >>> I'm prepared to merge this for this window,
>> >>> but I'm not sure who's supposed to ack the net/vmw_vsock/af_vsock.c
>> >>> bits. It's a harmless variable renaming so maybe it does not matter.
>> >>>
>> >>> The rest is virtio stuff so I guess my tree is ok.
>> >>>
>> >>> Objections, anyone?
>> >> https://lkml.org/lkml/2021/9/3/76 this is v4. It is same as v5 in af_vsock.c changes.
>> >>
>> >> It has Reviewed by from Stefano Garzarella.
>> > Is Stefano the maintainer for af_vsock then?
>> > I wasn't sure.

I'm maintaining virtio-vsock stuff, but I'm reviewing most of the 
af_vsock patches. We don't have an entry for it in MAINTAINERS, maybe we 
should.

>> Ack, let's wait for maintainer's comment
>
>
>The specific patch is a trivial variable renaming so
>I parked this in my tree for now, will merge unless I
>hear any objections in the next couple of days.

I agree, I think your tree is fine, since this series is mostly about 
virtio-vsock.

Thanks,
Stefano
Michael S. Tsirkin Sept. 6, 2021, 8:03 a.m. UTC | #8
On Mon, Sep 06, 2021 at 09:33:15AM +0200, Stefano Garzarella wrote:
> On Sun, Sep 05, 2021 at 04:18:52PM -0400, Michael S. Tsirkin wrote:
> > On Sun, Sep 05, 2021 at 07:21:10PM +0300, Arseny Krasnov wrote:
> > > 
> > > On 05.09.2021 19:19, Michael S. Tsirkin wrote:
> > > > On Sun, Sep 05, 2021 at 07:02:44PM +0300, Arseny Krasnov wrote:
> > > >> On 05.09.2021 18:55, Michael S. Tsirkin wrote:
> > > >>> On Fri, Sep 03, 2021 at 03:30:13PM +0300, Arseny Krasnov wrote:
> > > >>>> 	This patchset implements support of MSG_EOR bit for SEQPACKET
> > > >>>> AF_VSOCK sockets over virtio transport.
> > > >>>> 	First we need to define 'messages' and 'records' like this:
> > > >>>> Message is result of sending calls: 'write()', 'send()', 'sendmsg()'
> > > >>>> etc. It has fixed maximum length, and it bounds are visible using
> > > >>>> return from receive calls: 'read()', 'recv()', 'recvmsg()' etc.
> > > >>>> Current implementation based on message definition above.
> > > >>>> 	Record has unlimited length, it consists of multiple message,
> > > >>>> and bounds of record are visible via MSG_EOR flag returned from
> > > >>>> 'recvmsg()' call. Sender passes MSG_EOR to sending system call and
> > > >>>> receiver will see MSG_EOR when corresponding message will be processed.
> > > >>>> 	Idea of patchset comes from POSIX: it says that SEQPACKET
> > > >>>> supports record boundaries which are visible for receiver using
> > > >>>> MSG_EOR bit. So, it looks like MSG_EOR is enough thing for SEQPACKET
> > > >>>> and we don't need to maintain boundaries of corresponding send -
> > > >>>> receive system calls. But, for 'sendXXX()' and 'recXXX()' POSIX says,
> > > >>>> that all these calls operates with messages, e.g. 'sendXXX()' sends
> > > >>>> message, while 'recXXX()' reads messages and for SEQPACKET, 'recXXX()'
> > > >>>> must read one entire message from socket, dropping all out of size
> > > >>>> bytes. Thus, both message boundaries and MSG_EOR bit must be supported
> > > >>>> to follow POSIX rules.
> > > >>>> 	To support MSG_EOR new bit was added along with existing
> > > >>>> 'VIRTIO_VSOCK_SEQ_EOR': 'VIRTIO_VSOCK_SEQ_EOM'(end-of-message) - now it
> > > >>>> works in the same way as 'VIRTIO_VSOCK_SEQ_EOR'. But 'VIRTIO_VSOCK_SEQ_EOR'
> > > >>>> is used to mark 'MSG_EOR' bit passed from userspace.
> > > >>>> 	This patchset includes simple test for MSG_EOR.
> > > >>> I'm prepared to merge this for this window,
> > > >>> but I'm not sure who's supposed to ack the net/vmw_vsock/af_vsock.c
> > > >>> bits. It's a harmless variable renaming so maybe it does not matter.
> > > >>>
> > > >>> The rest is virtio stuff so I guess my tree is ok.
> > > >>>
> > > >>> Objections, anyone?
> > > >> https://lkml.org/lkml/2021/9/3/76 this is v4. It is same as v5 in af_vsock.c changes.
> > > >>
> > > >> It has Reviewed by from Stefano Garzarella.
> > > > Is Stefano the maintainer for af_vsock then?
> > > > I wasn't sure.
> 
> I'm maintaining virtio-vsock stuff, but I'm reviewing most of the af_vsock
> patches. We don't have an entry for it in MAINTAINERS, maybe we should.

Yea, please add that. And the test I guess?
It's now Dave and while he's great as we all know,
reducing the load on him is a good thing to do.

> > > Ack, let's wait for maintainer's comment
> > 
> > 
> > The specific patch is a trivial variable renaming so
> > I parked this in my tree for now, will merge unless I
> > hear any objections in the next couple of days.
> 
> I agree, I think your tree is fine, since this series is mostly about
> virtio-vsock.
> 
> Thanks,
> Stefano