[PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[David Vrabel]

Using the MTU or GSO size to determine the number of required guest Rx
requests for an skb was subtly broken since these value may change at
runtime.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/netback.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 1049c34..61b97c3 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -149,20 +149,19 @@ static inline pending_ring_idx_t pending_index(unsigned i)
 	return i & (MAX_PENDING_REQS-1);
 }
 
-static int xenvif_rx_ring_slots_needed(struct xenvif *vif)
-{
-	if (vif->gso_mask)
-		return DIV_ROUND_UP(vif->dev->gso_max_size, XEN_PAGE_SIZE) + 1;
-	else
-		return DIV_ROUND_UP(vif->dev->mtu, XEN_PAGE_SIZE);
-}
-
 static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue)
 {
 	RING_IDX prod, cons;
+	struct sk_buff *skb;
 	int needed;
 
-	needed = xenvif_rx_ring_slots_needed(queue->vif);
+	skb = skb_peek(&queue->rx_queue);
+	if (!skb)
+		return false;
+
+	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
+	if (skb_is_gso(skb))
+		needed++;
 
 	do {
 		prod = queue->rx.sring->req_prod;
@@ -2005,8 +2004,7 @@ static bool xenvif_rx_queue_ready(struct xenvif_queue *queue)
 
 static bool xenvif_have_rx_work(struct xenvif_queue *queue)
 {
-	return (!skb_queue_empty(&queue->rx_queue)
-		&& xenvif_rx_ring_slots_available(queue))
+	return xenvif_rx_ring_slots_available(queue)
 		|| (queue->vif->stall_timeout &&
 		    (xenvif_rx_queue_stalled(queue)
 		     || xenvif_rx_queue_ready(queue)))
-- 
2.1.4

Re: [PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Thu, 14 Jan 2016 15:18:30 +0000

> -	needed = xenvif_rx_ring_slots_needed(queue->vif);
> +	skb = skb_peek(&queue->rx_queue);
> +	if (!skb)
> +		return false;
> +
> +	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
> +	if (skb_is_gso(skb))
> +		needed++;

If I am not mistaken, we moved away from this kind of test exactly because
it is inaccurate and may under-estimate the needs.

It is possible for an N byte SKB to require N segments.  Therefore, the:

	DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);

calculation doesn't cut it.

Re: [PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Thu, 14 Jan 2016 15:18:30 +0000

> -	needed = xenvif_rx_ring_slots_needed(queue->vif);
> +	skb = skb_peek(&queue->rx_queue);
> +	if (!skb)
> +		return false;
> +
> +	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
> +	if (skb_is_gso(skb))
> +		needed++;

If I am not mistaken, we moved away from this kind of test exactly because
it is inaccurate and may under-estimate the needs.

It is possible for an N byte SKB to require N segments.  Therefore, the:

	DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);

calculation doesn't cut it.

Re: [Xen-devel] [PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[David Vrabel]

On 14/01/16 21:54, David Miller wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> Date: Thu, 14 Jan 2016 15:18:30 +0000
> 
>> -	needed = xenvif_rx_ring_slots_needed(queue->vif);
>> +	skb = skb_peek(&queue->rx_queue);
>> +	if (!skb)
>> +		return false;
>> +
>> +	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
>> +	if (skb_is_gso(skb))
>> +		needed++;
> 
> If I am not mistaken, we moved away from this kind of test exactly because
> it is inaccurate and may under-estimate the needs.
> 
> It is possible for an N byte SKB to require N segments.  Therefore, the:
> 
> 	DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
> 
> calculation doesn't cut it.

After 1650d5455bd2dc6b5ee134bd6fc1a3236c266b5b (xen-netback: always
fully coalesce guest Rx packets) we always fully pack a packet into its
guest Rx slots.  Each slot has space for XEN_PAGE_SIZE bytes so this
calculation for the number of slots is correct.

Shall I resend with a more description changelog?

David

Re: [PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[David Vrabel]

On 14/01/16 21:54, David Miller wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> Date: Thu, 14 Jan 2016 15:18:30 +0000
> 
>> -	needed = xenvif_rx_ring_slots_needed(queue->vif);
>> +	skb = skb_peek(&queue->rx_queue);
>> +	if (!skb)
>> +		return false;
>> +
>> +	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
>> +	if (skb_is_gso(skb))
>> +		needed++;
> 
> If I am not mistaken, we moved away from this kind of test exactly because
> it is inaccurate and may under-estimate the needs.
> 
> It is possible for an N byte SKB to require N segments.  Therefore, the:
> 
> 	DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
> 
> calculation doesn't cut it.

After 1650d5455bd2dc6b5ee134bd6fc1a3236c266b5b (xen-netback: always
fully coalesce guest Rx packets) we always fully pack a packet into its
guest Rx slots.  Each slot has space for XEN_PAGE_SIZE bytes so this
calculation for the number of slots is correct.

Shall I resend with a more description changelog?

David

Re: [Xen-devel] [PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Fri, 15 Jan 2016 10:31:57 +0000

> On 14/01/16 21:54, David Miller wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>> Date: Thu, 14 Jan 2016 15:18:30 +0000
>> 
>>> -	needed = xenvif_rx_ring_slots_needed(queue->vif);
>>> +	skb = skb_peek(&queue->rx_queue);
>>> +	if (!skb)
>>> +		return false;
>>> +
>>> +	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
>>> +	if (skb_is_gso(skb))
>>> +		needed++;
>> 
>> If I am not mistaken, we moved away from this kind of test exactly because
>> it is inaccurate and may under-estimate the needs.
>> 
>> It is possible for an N byte SKB to require N segments.  Therefore, the:
>> 
>> 	DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
>> 
>> calculation doesn't cut it.
> 
> After 1650d5455bd2dc6b5ee134bd6fc1a3236c266b5b (xen-netback: always
> fully coalesce guest Rx packets) we always fully pack a packet into its
> guest Rx slots.  Each slot has space for XEN_PAGE_SIZE bytes so this
> calculation for the number of slots is correct.
> 
> Shall I resend with a more description changelog?

Yeah that would help a lot.

Re: [PATCHv1 net] xen-netback: use skb to determine number of required guest Rx requests

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Fri, 15 Jan 2016 10:31:57 +0000

> On 14/01/16 21:54, David Miller wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>> Date: Thu, 14 Jan 2016 15:18:30 +0000
>> 
>>> -	needed = xenvif_rx_ring_slots_needed(queue->vif);
>>> +	skb = skb_peek(&queue->rx_queue);
>>> +	if (!skb)
>>> +		return false;
>>> +
>>> +	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
>>> +	if (skb_is_gso(skb))
>>> +		needed++;
>> 
>> If I am not mistaken, we moved away from this kind of test exactly because
>> it is inaccurate and may under-estimate the needs.
>> 
>> It is possible for an N byte SKB to require N segments.  Therefore, the:
>> 
>> 	DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
>> 
>> calculation doesn't cut it.
> 
> After 1650d5455bd2dc6b5ee134bd6fc1a3236c266b5b (xen-netback: always
> fully coalesce guest Rx packets) we always fully pack a packet into its
> guest Rx slots.  Each slot has space for XEN_PAGE_SIZE bytes so this
> calculation for the number of slots is correct.
> 
> Shall I resend with a more description changelog?

Yeah that would help a lot.