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

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

[Arseniy Krasnov]

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(-)

-- 
2.25.1

[RFC PATCH v5 1/2] virtio/vsock: allocate multiple skbuffs on tx

[Arseniy Krasnov]

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.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/vmw_vsock/virtio_transport_common.c | 57 +++++++++++++++++++------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 6564192e7f20..9e87c7d4d7cf 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -196,7 +196,8 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
 	const struct virtio_transport *t_ops;
 	struct virtio_vsock_sock *vvs;
 	u32 pkt_len = info->pkt_len;
-	struct sk_buff *skb;
+	u32 rest_len;
+	int ret;
 
 	info->type = virtio_transport_get_type(sk_vsock(vsk));
 
@@ -216,10 +217,6 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
 
 	vvs = vsk->trans;
 
-	/* we can send less than pkt_len bytes */
-	if (pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
-		pkt_len = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE;
-
 	/* virtio_transport_get_credit might return less than pkt_len credit */
 	pkt_len = virtio_transport_get_credit(vvs, pkt_len);
 
@@ -227,17 +224,49 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
 	if (pkt_len == 0 && info->op == VIRTIO_VSOCK_OP_RW)
 		return pkt_len;
 
-	skb = virtio_transport_alloc_skb(info, pkt_len,
-					 src_cid, src_port,
-					 dst_cid, dst_port);
-	if (!skb) {
-		virtio_transport_put_credit(vvs, pkt_len);
-		return -ENOMEM;
-	}
+	rest_len = pkt_len;
+
+	do {
+		struct sk_buff *skb;
+		size_t skb_len;
+
+		skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE, rest_len);
+
+		skb = virtio_transport_alloc_skb(info, skb_len,
+						 src_cid, src_port,
+						 dst_cid, dst_port);
+		if (!skb) {
+			ret = -ENOMEM;
+			break;
+		}
 
-	virtio_transport_inc_tx_pkt(vvs, skb);
+		virtio_transport_inc_tx_pkt(vvs, skb);
 
-	return t_ops->send_pkt(skb);
+		ret = t_ops->send_pkt(skb);
+		if (ret < 0)
+			break;
+
+		/* Both virtio and vhost 'send_pkt()' returns 'skb_len',
+		 * but for reliability use 'ret' instead of 'skb_len'.
+		 * Also if partial send happens (e.g. 'ret' != 'skb_len')
+		 * somehow, we break this loop, but account such returned
+		 * value in 'virtio_transport_put_credit()'.
+		 */
+		rest_len -= ret;
+
+		if (WARN_ONCE(ret != skb_len,
+			      "'send_pkt()' returns %i, but %zu expected\n",
+			      ret, skb_len))
+			break;
+	} while (rest_len);
+
+	virtio_transport_put_credit(vvs, rest_len);
+
+	/* Return number of bytes, if any data has been sent. */
+	if (rest_len != pkt_len)
+		ret = pkt_len - rest_len;
+
+	return ret;
 }
 
 static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
-- 
2.25.1

[RFC PATCH v5 2/2] virtio/vsock: check argument to avoid no effect call

[Arseniy Krasnov]

Both of these functions have no effect when input argument is 0, so to
avoid useless spinlock access, check argument before it.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 net/vmw_vsock/virtio_transport_common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 9e87c7d4d7cf..312658c176bd 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -302,6 +302,9 @@ u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit)
 {
 	u32 ret;
 
+	if (!credit)
+		return 0;
+
 	spin_lock_bh(&vvs->tx_lock);
 	ret = vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt);
 	if (ret > credit)
@@ -315,6 +318,9 @@ EXPORT_SYMBOL_GPL(virtio_transport_get_credit);
 
 void virtio_transport_put_credit(struct virtio_vsock_sock *vvs, u32 credit)
 {
+	if (!credit)
+		return;
+
 	spin_lock_bh(&vvs->tx_lock);
 	vvs->tx_cnt -= credit;
 	spin_unlock_bh(&vvs->tx_lock);
-- 
2.25.1

Re: [RFC PATCH v5 2/2] virtio/vsock: check argument to avoid no effect call

[Stefano Garzarella]

On Wed, Mar 22, 2023 at 09:36:24PM +0300, Arseniy Krasnov wrote:
>Both of these functions have no effect when input argument is 0, so to
>avoid useless spinlock access, check argument before it.
>
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> net/vmw_vsock/virtio_transport_common.c | 6 ++++++
> 1 file changed, 6 insertions(+)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 9e87c7d4d7cf..312658c176bd 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -302,6 +302,9 @@ u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit)
> {
> 	u32 ret;
>
>+	if (!credit)
>+		return 0;
>+
> 	spin_lock_bh(&vvs->tx_lock);
> 	ret = vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt);
> 	if (ret > credit)
>@@ -315,6 +318,9 @@ EXPORT_SYMBOL_GPL(virtio_transport_get_credit);
>
> void virtio_transport_put_credit(struct virtio_vsock_sock *vvs, u32 credit)
> {
>+	if (!credit)
>+		return;
>+
> 	spin_lock_bh(&vvs->tx_lock);
> 	vvs->tx_cnt -= credit;
> 	spin_unlock_bh(&vvs->tx_lock);
>-- 
>2.25.1
>

Re: [RFC PATCH v5 0/2] allocate multiple skbuffs on tx

[Arseniy Krasnov]

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(-)
> 

Re: [RFC PATCH v5 0/2] allocate multiple skbuffs on tx

[Stefano Garzarella]

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

Re: [RFC PATCH v5 0/2] allocate multiple skbuffs on tx

[Arseniy Krasnov]

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
> 

Re: [RFC PATCH v5 0/2] allocate multiple skbuffs on tx

[Stefano Garzarella]

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

Re: [RFC PATCH v5 0/2] allocate multiple skbuffs on tx

[Arseniy Krasnov]

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
>