[RFC,v5,0/2] allocate multiple skbuffs on tx

  This adds small optimization for tx path: instead of allocating single
skbuff on every call to transport, allocate multiple skbuff's until
credit space allows, thus trying to send as much as possible data without
return to af_vsock.c.

Also this patchset includes second patch which adds check and return from
'virtio_transport_get_credit()' and 'virtio_transport_put_credit()' when
these functions are called with 0 argument. This is needed, because zero
argument makes both functions to behave as no-effect, but both of them
always tries to acquire spinlock. Moreover, first patch always calls
function 'virtio_transport_put_credit()' with zero argument in case of
successful packet transmission.

 Link to v1:
 https://lore.kernel.org/netdev/2c52aa26-8181-d37a-bccd-a86bd3cbc6e1@sberdevices.ru/
 Link to v2:
 https://lore.kernel.org/netdev/ea5725eb-6cb5-cf15-2938-34e335a442fa@sberdevices.ru/
 Link to v3:
 https://lore.kernel.org/netdev/f33ef593-982e-2b3f-0986-6d537a3aaf08@sberdevices.ru/
 Link to v4:
 https://lore.kernel.org/netdev/0e0c1421-7cdc-2582-b120-cad6f42824bb@sberdevices.ru/

 Changelog:
 v1 -> v2:
 - If sent something, return number of bytes sent (even in
   case of error). Return error only if failed to sent first
   skbuff.

 v2 -> v3:
 - Handle case when transport callback returns unexpected value which
   is not equal to 'skb->len'. Break loop.
 - Don't check for zero value of 'rest_len' before calling
   'virtio_transport_put_credit()'. Decided to add this check directly
   to 'virtio_transport_put_credit()' in separate patch.

 v3 -> v4:
 - Use WARN_ONCE() to handle case when transport callback returns
   unexpected value.
 - Remove useless 'ret = -EFAULT;' assignment for case above.

 v4 -> v5:
 - Remove extra 'ret' initialization.
 - Remove empty extra line before 'if (ret < 0)'.
 - Add R-b tag for the first patch.
 - Add second patch, thus creating patchset of 2 patches.

Arseniy Krasnov (2):
  virtio/vsock: allocate multiple skbuffs on tx
  virtio/vsock: check argument to avoid no effect call

 net/vmw_vsock/virtio_transport_common.c | 63 +++++++++++++++++++------
 1 file changed, 49 insertions(+), 14 deletions(-)
  

Hello Stefano,

thanks for review!

Since both patches are R-b, i can wait for a few days, then send this
as 'net-next'?

Thanks, Arseniy

On 22.03.2023 21:34, Arseniy Krasnov wrote:
> This adds small optimization for tx path: instead of allocating single
> skbuff on every call to transport, allocate multiple skbuff's until
> credit space allows, thus trying to send as much as possible data without
> return to af_vsock.c.
> 
> Also this patchset includes second patch which adds check and return from
> 'virtio_transport_get_credit()' and 'virtio_transport_put_credit()' when
> these functions are called with 0 argument. This is needed, because zero
> argument makes both functions to behave as no-effect, but both of them
> always tries to acquire spinlock. Moreover, first patch always calls
> function 'virtio_transport_put_credit()' with zero argument in case of
> successful packet transmission.
> 
>  Link to v1:
>  https://lore.kernel.org/netdev/2c52aa26-8181-d37a-bccd-a86bd3cbc6e1@sberdevices.ru/
>  Link to v2:
>  https://lore.kernel.org/netdev/ea5725eb-6cb5-cf15-2938-34e335a442fa@sberdevices.ru/
>  Link to v3:
>  https://lore.kernel.org/netdev/f33ef593-982e-2b3f-0986-6d537a3aaf08@sberdevices.ru/
>  Link to v4:
>  https://lore.kernel.org/netdev/0e0c1421-7cdc-2582-b120-cad6f42824bb@sberdevices.ru/
> 
>  Changelog:
>  v1 -> v2:
>  - If sent something, return number of bytes sent (even in
>    case of error). Return error only if failed to sent first
>    skbuff.
> 
>  v2 -> v3:
>  - Handle case when transport callback returns unexpected value which
>    is not equal to 'skb->len'. Break loop.
>  - Don't check for zero value of 'rest_len' before calling
>    'virtio_transport_put_credit()'. Decided to add this check directly
>    to 'virtio_transport_put_credit()' in separate patch.
> 
>  v3 -> v4:
>  - Use WARN_ONCE() to handle case when transport callback returns
>    unexpected value.
>  - Remove useless 'ret = -EFAULT;' assignment for case above.
> 
>  v4 -> v5:
>  - Remove extra 'ret' initialization.
>  - Remove empty extra line before 'if (ret < 0)'.
>  - Add R-b tag for the first patch.
>  - Add second patch, thus creating patchset of 2 patches.
> 
> Arseniy Krasnov (2):
>   virtio/vsock: allocate multiple skbuffs on tx
>   virtio/vsock: check argument to avoid no effect call
> 
>  net/vmw_vsock/virtio_transport_common.c | 63 +++++++++++++++++++------
>  1 file changed, 49 insertions(+), 14 deletions(-)
>
  

On Thu, Mar 23, 2023 at 01:01:40PM +0300, Arseniy Krasnov wrote:
>Hello Stefano,
>
>thanks for review!

You're welcome!

>
>Since both patches are R-b, i can wait for a few days, then send this
>as 'net-next'?

Yep, maybe even this series could have been directly without RFC ;-)

Thanks,
Stefano
  

On 23.03.2023 13:48, Stefano Garzarella wrote:
> On Thu, Mar 23, 2023 at 01:01:40PM +0300, Arseniy Krasnov wrote:
>> Hello Stefano,
>>
>> thanks for review!
> 
> You're welcome!
> 
>>
>> Since both patches are R-b, i can wait for a few days, then send this
>> as 'net-next'?
> 
> Yep, maybe even this series could have been directly without RFC ;-)

"directly", You mean 'net' tag? Of just without RFC, like [PATCH v5]. In this case 
it will be merged to 'net' right?

Thanks, Arseniy
> 
> Thanks,
> Stefano
>
  

On Thu, Mar 23, 2023 at 01:53:40PM +0300, Arseniy Krasnov wrote:
>
>
>On 23.03.2023 13:48, Stefano Garzarella wrote:
>> On Thu, Mar 23, 2023 at 01:01:40PM +0300, Arseniy Krasnov wrote:
>>> Hello Stefano,
>>>
>>> thanks for review!
>>
>> You're welcome!
>>
>>>
>>> Since both patches are R-b, i can wait for a few days, then send this
>>> as 'net-next'?
>>
>> Yep, maybe even this series could have been directly without RFC ;-)
>
>"directly", You mean 'net' tag? Of just without RFC, like [PATCH v5]. In this case
>it will be merged to 'net' right?

Sorry for the confusion. I meant without RFC but with net-next.

Being enhancements and not fixes this is definitely net-next material,
so even in RFCs you can already use the net-next tag, so the reviewer
knows which branch to apply them to. (It's not super important since
being RFCs it's expected that it's not complete, but it's definitely an
help for the reviewer).

Speaking of the RFC, we usually use it for patches that we don't think
are ready to be merged. But when they reach a good state (like this
series for example), we can start publishing them already without the
RFC tag.

Anyway, if you are not sure, use RFC and then when a maintainer has
reviewed them all, surely you can remove the RFC tag.

Hope this helps, at least that's what I usually do, so don't take that
as a strict rule ;-)

Thanks,
Stefano
  

On 23.03.2023 14:11, Stefano Garzarella wrote:
> On Thu, Mar 23, 2023 at 01:53:40PM +0300, Arseniy Krasnov wrote:
>>
>>
>> On 23.03.2023 13:48, Stefano Garzarella wrote:
>>> On Thu, Mar 23, 2023 at 01:01:40PM +0300, Arseniy Krasnov wrote:
>>>> Hello Stefano,
>>>>
>>>> thanks for review!
>>>
>>> You're welcome!
>>>
>>>>
>>>> Since both patches are R-b, i can wait for a few days, then send this
>>>> as 'net-next'?
>>>
>>> Yep, maybe even this series could have been directly without RFC ;-)
>>
>> "directly", You mean 'net' tag? Of just without RFC, like [PATCH v5]. In this case
>> it will be merged to 'net' right?
> 
> Sorry for the confusion. I meant without RFC but with net-next.
> 
> Being enhancements and not fixes this is definitely net-next material,
> so even in RFCs you can already use the net-next tag, so the reviewer
> knows which branch to apply them to. (It's not super important since
> being RFCs it's expected that it's not complete, but it's definitely an
> help for the reviewer).
> 
> Speaking of the RFC, we usually use it for patches that we don't think
> are ready to be merged. But when they reach a good state (like this
> series for example), we can start publishing them already without the
> RFC tag.
> 
> Anyway, if you are not sure, use RFC and then when a maintainer has
> reviewed them all, surely you can remove the RFC tag.
> 
> Hope this helps, at least that's what I usually do, so don't take that
> as a strict rule ;-)

Ah ok, I see now, thanks for details

Thanks, Arseniy

> 
> Thanks,
> Stefano
>