Virtualization - Jul 2021 - [MASSMAIL KLMS]Re: [RFC PATCH v2 0/6] Improve SOCK_SEQPACKET receive logic

On Mon, Jul 05, 2021 at 01:48:28PM +0300, Arseny Krasnov
wrote:
>
>On 04.07.2021 12:54, Michael S. Tsirkin wrote:
>> On Sun, Jul 04, 2021 at 12:23:03PM +0300, Arseny Krasnov wrote:
>>> On 04.07.2021 11:30, Michael S. Tsirkin wrote:
>>>> On Sun, Jul 04, 2021 at 11:08:13AM +0300, Arseny Krasnov wrote:
>>>>> 	This patchset modifies receive logic for SOCK_SEQPACKET.
>>>>> Difference between current implementation and this version
is that
>>>>> now reader is woken up when there is at least one RW packet
in rx
>>>>> queue of socket and data is copied to user's buffer,
while merged
>>>>> approach wake up user only when whole message is received
and kept
>>>>> in queue. New implementation has several advantages:
>>>>>  1) There is no limit for message length. Merged approach
requires
>>>>>     that length must be smaller than
'peer_buf_alloc', otherwise
>>>>>     transmission will stuck.
>>>>>  2) There is no need to keep whole message in queue, thus
no
>>>>>     'kmalloc()' memory will be wasted until EOR is
received.
>>>>>
>>>>>     Also new approach has some feature: as fragments of
message
>>>>> are copied until EOR is received, it is possible that part
of
>>>>> message will be already in user's buffer, while rest of
message
>>>>> still not received. And if user will be interrupted by
signal or
>>>>> timeout with part of message in buffer, it will exit
receive loop,
>>>>> leaving rest of message in queue. To solve this problem
special
>>>>> callback was added to transport: it is called when user was
forced
>>>>> to leave exit loop and tells transport to drop any packet
until
>>>>> EOR met.
>>>> Sorry about commenting late in the game.  I'm a bit lost
>>>>
>>>>
>>>> SOCK_SEQPACKET
>>>> Provides sequenced, reliable, bidirectional, connection-mode
transmission paths for records. A record can be sent using one or more output
operations and received using one or more input operations, but a single
operation never transfers part of more than one record. Record boundaries are
visible to the receiver via the MSG_EOR flag.
>>>>
>>>> it's supposed to be reliable - how is it legal to drop
packets?
>>> Sorry, seems i need to rephrase description. "Packet"
here means fragment of record(message) at transport
>>>
>>> layer. As this is SEQPACKET mode, receiver could get only whole
message or error, so if only several fragments
>>>
>>> of message was copied (if signal received for example) we can't
return it to user - it breaks SEQPACKET sense. I think,
>>>
>>> in this case we can drop rest of record's fragments legally.
>>>
>>>
>>> Thank You
>> Would not that violate the reliable property? IIUC it's only ok to
>> return an error if socket gets closed. Just like e.g. TCP ...
>>
>Sorry for late answer, yes You're right, seems this is unwanted drop...
>
>Lets wait for Stefano Garzarella feedback
It was the same concern I had with the series that introduced SEQPACKET 
for vsock, which is why I suggested to wait until the message is 
complete, before copying it to the user's buffer.

IIUC, with the current upstream implementation, we don't have this 
problem, right?

I'm not sure how to fix this, other than by keeping all the fragments 
queued until we've successfully copied them to user space, which is what 
we should do without this series applied IIUC.

Stefano