Re: [PATCH net 1/4] Revert "net/smc: don't wait for send buffer space when data was already sent"

[Karsten Graul <kgraul@linux.ibm.com>]

On 27/10/2021 17:08, Jakub Kicinski wrote:
> On Wed, 27 Oct 2021 12:21:32 +0200 Karsten Graul wrote:
>> On 27/10/2021 10:52, Tony Lu wrote:
>>> From: Tony Lu <tony.ly@linux.alibaba.com>
>>>
>>> This reverts commit 6889b36da78a21a312d8b462c1fa25a03c2ff192.
>>>
>>> When using SMC to replace TCP, some userspace applications like netperf
>>> don't check the return code of send syscall correctly, which means how
>>> many bytes are sent. If rc of send() is smaller than expected, it should
>>> try to send again, instead of exit directly. It is difficult to change
>>> the uncorrect behaviors of userspace applications, so choose to revert it.  
>>
>> Your change would restore the old behavior to handle all sockets like they 
>> are blocking sockets, trying forever to send the provided data bytes.
>> This is not how it should work.
> 
> Isn't the application supposed to make the socket non-blocking or
> pass MSG_DONTWAIT if it doesn't want to sleep? It's unclear why 
> the fix was needed in the first place.
  
You are right, all of this non-blocking handling is already done in smc_tx_wait().
So this fix was for blocking sockets. The commit message explains the original
intention:

    net/smc: don't wait for send buffer space when data was already sent

    When there is no more send buffer space and at least 1 byte was already
    sent then return to user space. The wait is only done when no data was
    sent by the sendmsg() call.
    This fixes smc_tx_sendmsg() which tried to always send all user data and
    started to wait for free send buffer space when needed. During this wait
    the user space program was blocked in the sendmsg() call and hence not
    able to receive incoming data. When both sides were in such a situation
    then the connection stalled forever.

What we found out was that applications called sendmsg() with large data
buffers using blocking sockets. This led to the described situation, were the
solution was to early return to user space even if not all data were sent yet.
Userspace applications should not have a problem with the fact that sendmsg()
returns a smaller byte count than requested.

Reverting this patch would bring back the stalled connection problem.

>> We encountered the same issue with netperf, but this is the only 'broken'
>> application that we know of so far which does not implement the socket API
>> correctly.
> 

-- 
Karsten