xen/netback: correctly calculate required slots of skb.

Quoted 114 lines · Wei Liu

Quoted 33 lines · Annie Li
When counting required slots for skb, netback directly uses DIV_ROUND_UP to get slots required by header data. This is wrong when offset in the page of header data is not zero, and is also inconsistent with following calculation for required slot in netbk_gop_skb.
In netbk_gop_skb, required slots are calculated based on offset and len in page of header data. It is possible that required slots here is larger than the one calculated in earlier netbk_count_requests. This inconsistency directly results in rx_req_cons_peek and xen_netbk_rx_ring_full judgement are wrong.
Then it comes to situation the ring is actually full, but netback thinks it is not and continues to create responses. This results in response overlaps request in the ring, then grantcopy gets wrong grant reference and throws out error, for example "(XEN) grant_table.c:1763:d0 Bad grant reference 2949120", the grant reference is invalid value here. Netback returns XEN_NETIF_RSP_ERROR(-1) to netfront when grant copy status is error, then netfront gets rx->status (the status is -1, not really data size now), and throws out error, "kernel: net eth1: rx->offset: 0, size: 4294967295". This issue can be reproduced by doing gzip/gunzip in nfs share with mtu = 9000, the guest would panic after running such test for a while.
This patch is based on 3.10-rc7.
Signed-off-by Annie Li <[email protected]>
---
  drivers/net/xen-netback/netback.c |   97 +++++++++++++++++++++++++-----------
  1 files changed, 67 insertions(+), 30 deletions(-)
drivers/net/xen-netback/netback.c +0 -0
index 8c20935..7ff9333 100644
drivers/net/xen-netback/netback.c static bool start_new_rx_buffer(int offset, unsigned long size, int head) · line 361
Quoted 3 lines
drivers/net/xen-netback/netback.c +0 -0
@@ -359,51 +359,88 @@ static bool start_new_rx_buffer(int offset, unsigned long size, int head)
   * the guest. This function is essentially a dry run of
   * netbk_gop_frag_copy.
   */
Just to be clear, now the above comment applies to your new function, right?
drivers/net/xen-netback/netback.c static bool start_new_rx_buffer(int offset, unsigned long size, int head) · line 362
Quoted 2 lines
drivers/net/xen-netback/netback.c +1 -1
-unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
+static void netbk_get_slots(struct xenvif *vif, struct sk_buff *skb,
A better name would be netbk_count_slots because it doesn't actually acquire any slots.
And you can make it return the slot count and adds that to the total number of slots in the caller, as this function doesn't seem to rely on previous count so there is not much point passing in *count.
Quoted 65 lines
drivers/net/xen-netback/netback.c +30 ~~-23~~
+			    struct page *page, int *copy_off,
+			    unsigned long size, unsigned long offset,
+			    int *head, int *count)
  {
-	unsigned int count;
-	int i, copy_off;
+	unsigned long bytes;
 
-	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
+	/* Data must not cross a page boundary. */
+	BUG_ON(size + offset > PAGE_SIZE<<compound_order(page));
 
-	copy_off = skb_headlen(skb) % PAGE_SIZE;
+	/* Skip unused frames from start of page */
+	page += offset >> PAGE_SHIFT;
+	offset &= ~PAGE_MASK;
 
-	if (skb_shinfo(skb)->gso_size)
-		count++;
+	while (size > 0) {
+		BUG_ON(offset >= PAGE_SIZE);
+		BUG_ON(*copy_off > MAX_BUFFER_OFFSET);
 
-	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
-		unsigned long size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
-		unsigned long offset = skb_shinfo(skb)->frags[i].page_offset;
-		unsigned long bytes;
+		bytes = PAGE_SIZE - offset;
 
-		offset &= ~PAGE_MASK;
+		if (bytes > size)
+			bytes = size;
 
-		while (size > 0) {
-			BUG_ON(offset >= PAGE_SIZE);
-			BUG_ON(copy_off > MAX_BUFFER_OFFSET);
+		if (start_new_rx_buffer(*copy_off, bytes, *head)) {
+			*count = *count + 1;
+			*copy_off = 0;
+		}
 
-			bytes = PAGE_SIZE - offset;
+		if (*copy_off + bytes > MAX_BUFFER_OFFSET)
+			bytes = MAX_BUFFER_OFFSET - *copy_off;
 
-			if (bytes > size)
-				bytes = size;
+		*copy_off += bytes;
 
-			if (start_new_rx_buffer(copy_off, bytes, 0)) {
-				count++;
-				copy_off = 0;
-			}
+		offset += bytes;
+		size -= bytes;
 
-			if (copy_off + bytes > MAX_BUFFER_OFFSET)
-				bytes = MAX_BUFFER_OFFSET - copy_off;
+		/* Next frame */
+		if (offset == PAGE_SIZE && size) {
+			BUG_ON(!PageCompound(page));
+			page++;
+			offset = 0;
+		}
 

Quoted 6 lines

drivers/net/xen-netback/netback.c static bool start_new_rx_buffer(int offset, unsigned long size, int head) · line 405
Quoted 2 lines
drivers/net/xen-netback/netback.c +1 -1
-			copy_off += bytes;
+		if (*head)
This seems to differ from netbk_gop_frag_copy. In that function it's like
     if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)

I mixed netbk_gop_frag_copy and xen_netbk_count_skb_slots just now, so please ignore the comments here in my last email. In xen_netbk_count_skb_slots, we have following code already, so it is not necessary to add gso condition here.

        if (skb_shinfo(skb)->gso_size)
                count++;

Thanks
Annie

Quoted 56 lines

Wei.

Quoted 53 lines

drivers/net/xen-netback/netback.c +35 -5

+			*count = *count + 1;
+		*head = 0; /* There must be something in this buffer now. */
+	}
+}
+
+unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
+{
+	int i, copy_off = 0;
+	int nr_frags = skb_shinfo(skb)->nr_frags;
+	unsigned char *data;
+	int head = 1;
+	unsigned int count = 0;
 
-			offset += bytes;
-			size -= bytes;
+	if (skb_shinfo(skb)->gso_size)
+		count++;
 
-			if (offset == PAGE_SIZE)
-				offset = 0;
-		}
+	data = skb->data;
+	while (data < skb_tail_pointer(skb)) {
+		unsigned int offset = offset_in_page(data);
+		unsigned int len = PAGE_SIZE - offset;
+
+		if (data + len > skb_tail_pointer(skb))
+			len = skb_tail_pointer(skb) - data;
+
+		netbk_get_slots(vif, skb, virt_to_page(data), &copy_off,
+				len, offset, &head, &count);
+		data += len;
+	}
+
+	for (i = 0; i < nr_frags; i++) {
+		netbk_get_slots(vif, skb,
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				&copy_off,
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				&head, &count);
  	}
+
  	return count;
  }

Signature · 6 lines

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wei Liu

2013-07-10 07:48 UTC #4 lore raw

On Wed, Jul 10, 2013 at 10:31:49AM +0800, annie li wrote:
[...]

Quoted 5 lines

Quoted 4 lines
This seems to differ from netbk_gop_frag_copy. In that function it's like
    if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)

Quoted 8 lines

I mixed netbk_gop_frag_copy and xen_netbk_count_skb_slots just now, so please ignore the comments here in my last email.
In xen_netbk_count_skb_slots, we have following code already, so it is not necessary to add gso condition here.
        if (skb_shinfo(skb)->gso_size)
                count++;

Oh I see. But the condition still seems to be different from the original one -- you missed !vif->gso_prefix.

Wei.

annie li

author

2013-07-10 08:34 UTC #5 lore raw

Quoted 11 lines · Wei Liu

On Wed, Jul 10, 2013 at 10:31:49AM +0800, annie li wrote:
[...]
Quoted 9 lines
Quoted 4 lines
This seems to differ from netbk_gop_frag_copy. In that function it's like
     if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
I mixed netbk_gop_frag_copy and xen_netbk_count_skb_slots just now, so please ignore the comments here in my last email.
In xen_netbk_count_skb_slots, we have following code already, so it is not necessary to add gso condition here.

Quoted 5 lines

Quoted 3 lines
         if (skb_shinfo(skb)->gso_size)
                 count++;
Oh I see. But the condition still seems to be different from the original one -- you missed !vif->gso_prefix.

The original code in xen_netbk_count_skb_slots does not have !vif->gso_prefix. But I think you are right, it is better to keep consistent with that in netbk_gop_frag_copy.

Thanks
Annie

Matt Wilson

2013-07-09 22:14 UTC #6 lore raw

In reply to Annie Li

Quoted 10 lines · Annie Li

When counting required slots for skb, netback directly uses DIV_ROUND_UP to get slots required by header data. This is wrong when offset in the page of header data is not zero, and is also inconsistent with following calculation for required slot in netbk_gop_skb.
In netbk_gop_skb, required slots are calculated based on offset and len in page of header data. It is possible that required slots here is larger than the one calculated in earlier netbk_count_requests. This inconsistency directly results in rx_req_cons_peek and xen_netbk_rx_ring_full judgement are wrong.

Quoted 10 lines

Then it comes to situation the ring is actually full, but netback thinks it is not and continues to create responses. This results in response overlaps request in the ring, then grantcopy gets wrong grant reference and throws out error, for example "(XEN) grant_table.c:1763:d0 Bad grant reference 2949120", the grant reference is invalid value here. Netback returns XEN_NETIF_RSP_ERROR(-1) to netfront when grant copy status is error, then netfront gets rx->status (the status is -1, not really data size now), and throws out error, "kernel: net eth1: rx->offset: 0, size: 4294967295". This issue can be reproduced by doing gzip/gunzip in nfs share with mtu = 9000, the guest would panic after running such test for a while.

I have a similar (but smaller) patch in my queue that Wei looked over at the Dublin hackathon. I don't have time to rebase and test it right now, but let me post it for a second data point.

--msw

Quoted 130 lines

This patch is based on 3.10-rc7.

Signed-off-by Annie Li <[email protected]>

---
 drivers/net/xen-netback/netback.c |   97 +++++++++++++++++++++++++-----------
 1 files changed, 67 insertions(+), 30 deletions(-)

drivers/net/xen-netback/netback.c +67 ~~-30~~

index 8c20935..7ff9333 100644
@@ -359,51 +359,88 @@ static bool start_new_rx_buffer(int offset, unsigned long size, int head)
  * the guest. This function is essentially a dry run of
  * netbk_gop_frag_copy.
  */
-unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
+static void netbk_get_slots(struct xenvif *vif, struct sk_buff *skb,
+			    struct page *page, int *copy_off,
+			    unsigned long size, unsigned long offset,
+			    int *head, int *count)
 {
-	unsigned int count;
-	int i, copy_off;
+	unsigned long bytes;
 
-	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
+	/* Data must not cross a page boundary. */
+	BUG_ON(size + offset > PAGE_SIZE<<compound_order(page));
 
-	copy_off = skb_headlen(skb) % PAGE_SIZE;
+	/* Skip unused frames from start of page */
+	page += offset >> PAGE_SHIFT;
+	offset &= ~PAGE_MASK;
 
-	if (skb_shinfo(skb)->gso_size)
-		count++;
+	while (size > 0) {
+		BUG_ON(offset >= PAGE_SIZE);
+		BUG_ON(*copy_off > MAX_BUFFER_OFFSET);
 
-	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
-		unsigned long size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
-		unsigned long offset = skb_shinfo(skb)->frags[i].page_offset;
-		unsigned long bytes;
+		bytes = PAGE_SIZE - offset;
 
-		offset &= ~PAGE_MASK;
+		if (bytes > size)
+			bytes = size;
 
-		while (size > 0) {
-			BUG_ON(offset >= PAGE_SIZE);
-			BUG_ON(copy_off > MAX_BUFFER_OFFSET);
+		if (start_new_rx_buffer(*copy_off, bytes, *head)) {
+			*count = *count + 1;
+			*copy_off = 0;
+		}
 
-			bytes = PAGE_SIZE - offset;
+		if (*copy_off + bytes > MAX_BUFFER_OFFSET)
+			bytes = MAX_BUFFER_OFFSET - *copy_off;
 
-			if (bytes > size)
-				bytes = size;
+		*copy_off += bytes;
 
-			if (start_new_rx_buffer(copy_off, bytes, 0)) {
-				count++;
-				copy_off = 0;
-			}
+		offset += bytes;
+		size -= bytes;
 
-			if (copy_off + bytes > MAX_BUFFER_OFFSET)
-				bytes = MAX_BUFFER_OFFSET - copy_off;
+		/* Next frame */
+		if (offset == PAGE_SIZE && size) {
+			BUG_ON(!PageCompound(page));
+			page++;
+			offset = 0;
+		}
 
-			copy_off += bytes;
+		if (*head)
+			*count = *count + 1;
+		*head = 0; /* There must be something in this buffer now. */
+	}
+}
+
+unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
+{
+	int i, copy_off = 0;
+	int nr_frags = skb_shinfo(skb)->nr_frags;
+	unsigned char *data;
+	int head = 1;
+	unsigned int count = 0;
 
-			offset += bytes;
-			size -= bytes;
+	if (skb_shinfo(skb)->gso_size)
+		count++;
 
-			if (offset == PAGE_SIZE)
-				offset = 0;
-		}
+	data = skb->data;
+	while (data < skb_tail_pointer(skb)) {
+		unsigned int offset = offset_in_page(data);
+		unsigned int len = PAGE_SIZE - offset;
+
+		if (data + len > skb_tail_pointer(skb))
+			len = skb_tail_pointer(skb) - data;
+
+		netbk_get_slots(vif, skb, virt_to_page(data), &copy_off,
+				len, offset, &head, &count);
+		data += len;
+	}
+
+	for (i = 0; i < nr_frags; i++) {
+		netbk_get_slots(vif, skb,
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				&copy_off,
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				&head, &count);
 	}
+
 	return count;
 }

Matt Wilson

2013-07-09 22:41 UTC #7 lore raw

RFC PATCH 1/1 xen-netback: calculate the number of slots required for large MTU vifs

From: Xi Xiong <[email protected]>

[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]

Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:

(XEN) grant_table.c:1797:d0 Bad grant reference 264241157

and kernel messages within the guest such as:

[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295

BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.

This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.

Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.

Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.

Signed-off-by Xi Xiong <[email protected]>

[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]

Signed-off-by Matt Wilson <[email protected]>

Cc Annie Li <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Campbell <[email protected]>

---
 drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)

drivers/net/xen-netback/netback.c +31 ~~-20~~

index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
 	void *mapping;
 };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
 struct xen_netbk {
 	wait_queue_head_t wq;
 	struct task_struct *task;

@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
 	unsigned int count;
 	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
 	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);

@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 				offset = 0;
 		}
 	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
 	return count;
 }

@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
 }
 
 /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
  */
 static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 				struct netrx_pending_operations *npo,
 				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
 {
 	struct gnttab_copy *copy_gop;
 	struct netbk_rx_meta *meta;

@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		if (bytes > size)
 			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
 			/*
 			 * Netfront requires there to be some data in the head
 			 * buffer.
 			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
 			meta = get_next_rx_buffer(vif, npo);
 		}

@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		}
 
 		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
 			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
 	}
 }

@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
 	struct xen_netif_rx_request *req;
 	struct netbk_rx_meta *meta;
 	unsigned char *data;
-	int head = 1;
+	int first = 1;
 	int old_meta_prod;
 
 	old_meta_prod = npo->meta_prod;

@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
 			len = skb_tail_pointer(skb) - data;
 
 		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
 		data += len;
 	}
 
 	for (i = 0; i < nr_frags; i++) {
 		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
 	}
 
 	return npo->meta_prod - old_meta_prod;

@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
 	}
 }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
 static void xen_netbk_rx_action(struct xen_netbk *netbk)
 {
 	struct xenvif *vif = NULL, *tmp;

@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
 	count = 0;
 
 	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+ 
 		vif = netdev_priv(skb->dev);
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
 		sco = (struct skb_cb_overlay *)skb->cb;
 		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
 		__skb_queue_tail(&rxq, skb);
 
+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
 		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
 			break;
 	}

Signature · 1 line

1.7.4.5

Wei Liu

2013-07-10 08:13 UTC #8 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 5 lines · Matt Wilson

From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]

Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...

Quoted 24 lines

Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.

Quoted 8 lines

Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will

Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?

Quoted 157 lines

consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.

Signed-off-by Xi Xiong <[email protected]>

[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]

Signed-off-by Matt Wilson <[email protected]>

Cc Annie Li <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Campbell <[email protected]>

---
 drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)

drivers/net/xen-netback/netback.c +25 ~~-18~~

index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
 	void *mapping;
 };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
 struct xen_netbk {
 	wait_queue_head_t wq;
 	struct task_struct *task;

@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
 	unsigned int count;
 	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
 	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);

@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 				offset = 0;
 		}
 	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
 	return count;
 }

@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
 }
 
 /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
  */
 static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 				struct netrx_pending_operations *npo,
 				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
 {
 	struct gnttab_copy *copy_gop;
 	struct netbk_rx_meta *meta;

@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		if (bytes > size)
 			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
 			/*
 			 * Netfront requires there to be some data in the head
 			 * buffer.
 			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
 			meta = get_next_rx_buffer(vif, npo);
 		}

@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		}
 
 		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
 			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
 	}
 }

@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
 	struct xen_netif_rx_request *req;
 	struct netbk_rx_meta *meta;
 	unsigned char *data;
-	int head = 1;
+	int first = 1;
 	int old_meta_prod;
 
 	old_meta_prod = npo->meta_prod;

@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
 			len = skb_tail_pointer(skb) - data;
 
 		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
 		data += len;
 	}
 
 	for (i = 0; i < nr_frags; i++) {
 		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
 	}
 
 	return npo->meta_prod - old_meta_prod;

@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
 	}
 }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
 static void xen_netbk_rx_action(struct xen_netbk *netbk)
 {
 	struct xenvif *vif = NULL, *tmp;

@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
 	count = 0;
 
 	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+ 
 		vif = netdev_priv(skb->dev);
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
 		sco = (struct skb_cb_overlay *)skb->cb;
 		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
 		__skb_queue_tail(&rxq, skb);

drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717

Quoted 10 lines

drivers/net/xen-netback/netback.c +6 -2

+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
 		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
 			break;

Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?

Wei.

Quoted 4 lines

drivers/net/xen-netback/netback.c +0 -0
 	}
 
Signature · 1 line
1.7.4.5

Wei Liu

2013-07-10 19:37 UTC #9 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 203 lines · Wei Liu

Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
Quoted 24 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Quoted 8 lines
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Quoted 157 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
 drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +25 ~~-18~~
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
 	void *mapping;
 };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
 struct xen_netbk {
 	wait_queue_head_t wq;
 	struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
 	unsigned int count;
 	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
 	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 
@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 				offset = 0;
 		}
 	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
 	return count;
 }
 
@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
 }
 
 /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
  */
 static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 				struct netrx_pending_operations *npo,
 				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
 {
 	struct gnttab_copy *copy_gop;
 	struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		if (bytes > size)
 			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
 			/*
 			 * Netfront requires there to be some data in the head
 			 * buffer.
 			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
 			meta = get_next_rx_buffer(vif, npo);
 		}
@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		}
 
 		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
 			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
 	}
 }
@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
 	struct xen_netif_rx_request *req;
 	struct netbk_rx_meta *meta;
 	unsigned char *data;
-	int head = 1;
+	int first = 1;
 	int old_meta_prod;
 
 	old_meta_prod = npo->meta_prod;
@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
 			len = skb_tail_pointer(skb) - data;
 
 		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
 		data += len;
 	}
 
 	for (i = 0; i < nr_frags; i++) {
 		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
 	}
 
 	return npo->meta_prod - old_meta_prod;
@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
 	}
 }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
 static void xen_netbk_rx_action(struct xen_netbk *netbk)
 {
 	struct xenvif *vif = NULL, *tmp;
@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
 	count = 0;
 
 	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+ 
 		vif = netdev_priv(skb->dev);
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
 		sco = (struct skb_cb_overlay *)skb->cb;
 		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
 		__skb_queue_tail(&rxq, skb);
 

Quoted 15 lines

drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717
Quoted 10 lines
drivers/net/xen-netback/netback.c +6 -2
+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
 		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
 			break;
Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?

Ah, I must be off my head here. I mixed several patches 1) my RFC patch that removes MAX_SKB_FRAGS in RX path, 2) Annie's patch and 3) your patch.

The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.

After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.

So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.

Comments?

Wei.

Quoted 7 lines

Wei.
Quoted 4 lines
drivers/net/xen-netback/netback.c +0 -0
 	}
 
Signature · 1 line
1.7.4.5

annie li

author

2013-07-11 01:25 UTC #10 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 224 lines · Wei Liu

Quoted 201 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
Quoted 24 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Quoted 8 lines
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Quoted 157 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +25 ~~-18~~
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
  	void *mapping;
  };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
  struct xen_netbk {
  	wait_queue_head_t wq;
  	struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
  	unsigned int count;
  	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
  	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 
@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  				offset = 0;
  		}
  	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
  	return count;
  }
 
@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
  }
 
  /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
   */
  static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  				struct netrx_pending_operations *npo,
  				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
  {
  	struct gnttab_copy *copy_gop;
  	struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  		if (bytes > size)
  			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
  			/*
  			 * Netfront requires there to be some data in the head
  			 * buffer.
  			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
  			meta = get_next_rx_buffer(vif, npo);
  		}
@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  		}
 
  		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
  			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
  	}
  }
@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
  	struct xen_netif_rx_request *req;
  	struct netbk_rx_meta *meta;
  	unsigned char *data;
-	int head = 1;
+	int first = 1;
  	int old_meta_prod;
 
  	old_meta_prod = npo->meta_prod;
@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
  			len = skb_tail_pointer(skb) - data;
 
  		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
  		data += len;
  	}
 
  	for (i = 0; i < nr_frags; i++) {
  		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
  	}
 
  	return npo->meta_prod - old_meta_prod;
@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
  	}
  }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
  static void xen_netbk_rx_action(struct xen_netbk *netbk)
  {
  	struct xenvif *vif = NULL, *tmp;
@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
  	count = 0;
 
  	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+
  		vif = netdev_priv(skb->dev);
  		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
  		sco = (struct skb_cb_overlay *)skb->cb;
  		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
  		__skb_queue_tail(&rxq, skb);
 
Quoted 14 lines
drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717
Quoted 10 lines
drivers/net/xen-netback/netback.c +6 -2
+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
  		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
  			break;
Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?
Ah, I must be off my head here. I mixed several patches 1) my RFC patch that removes MAX_SKB_FRAGS in RX path, 2) Annie's patch and 3) your patch.
The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.

Quoted 8 lines

After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.
So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.
Comments?

I agree, two patches looks fine.
xen_netbk_skb_count_slots returns wrong value, this could be fixed by my patch. protection from overrunning the ring, this could be implemented by Wei's/Matt's patch.

Thanks
Annie

Quoted 14 lines

Wei.
Quoted 6 lines
Wei.
Quoted 4 lines
drivers/net/xen-netback/netback.c +0 -0
  	}
 
Signature · 1 line
1.7.4.5
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

annie li

author

2013-07-11 01:25 UTC #11 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 224 lines · Wei Liu

Quoted 201 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
Quoted 24 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Quoted 8 lines
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Quoted 157 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +25 ~~-18~~
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
  	void *mapping;
  };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
  struct xen_netbk {
  	wait_queue_head_t wq;
  	struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
  	unsigned int count;
  	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
  	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 
@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  				offset = 0;
  		}
  	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
  	return count;
  }
 
@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
  }
 
  /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
   */
  static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  				struct netrx_pending_operations *npo,
  				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
  {
  	struct gnttab_copy *copy_gop;
  	struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  		if (bytes > size)
  			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
  			/*
  			 * Netfront requires there to be some data in the head
  			 * buffer.
  			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
  			meta = get_next_rx_buffer(vif, npo);
  		}
@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  		}
 
  		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
  			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
  	}
  }
@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
  	struct xen_netif_rx_request *req;
  	struct netbk_rx_meta *meta;
  	unsigned char *data;
-	int head = 1;
+	int first = 1;
  	int old_meta_prod;
 
  	old_meta_prod = npo->meta_prod;
@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
  			len = skb_tail_pointer(skb) - data;
 
  		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
  		data += len;
  	}
 
  	for (i = 0; i < nr_frags; i++) {
  		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
  	}
 
  	return npo->meta_prod - old_meta_prod;
@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
  	}
  }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
  static void xen_netbk_rx_action(struct xen_netbk *netbk)
  {
  	struct xenvif *vif = NULL, *tmp;
@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
  	count = 0;
 
  	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+
  		vif = netdev_priv(skb->dev);
  		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
  		sco = (struct skb_cb_overlay *)skb->cb;
  		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
  		__skb_queue_tail(&rxq, skb);
 
Quoted 14 lines
drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717
Quoted 10 lines
drivers/net/xen-netback/netback.c +6 -2
+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
  		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
  			break;
Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?
Ah, I must be off my head here. I mixed several patches 1) my RFC patch that removes MAX_SKB_FRAGS in RX path, 2) Annie's patch and 3) your patch.
The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.

Quoted 8 lines

After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.
So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.
Comments?

Thanks
Annie

Quoted 14 lines

Wei.
Quoted 6 lines
Wei.
Quoted 4 lines
drivers/net/xen-netback/netback.c +0 -0
  	}
 
Signature · 1 line
1.7.4.5
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Matt Wilson

2013-07-11 05:15 UTC #12 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 10 lines · Wei Liu

Quoted 9 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...

*nod*

Quoted 28 lines

Quoted 28 lines
Quoted 28 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?

Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).

Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.

Quoted 173 lines

Quoted 157 lines

Signed-off-by Xi Xiong <[email protected]>

[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]

Signed-off-by Matt Wilson <[email protected]>

Cc Annie Li <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Campbell <[email protected]>

---
 drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)

drivers/net/xen-netback/netback.c +25 ~~-18~~

index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
 	void *mapping;
 };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
 struct xen_netbk {
 	wait_queue_head_t wq;
 	struct task_struct *task;

@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
 	unsigned int count;
 	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
 	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);

@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 				offset = 0;
 		}
 	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
 	return count;
 }

@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
 }
 
 /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
  */
 static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 				struct netrx_pending_operations *npo,
 				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
 {
 	struct gnttab_copy *copy_gop;
 	struct netbk_rx_meta *meta;

@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		if (bytes > size)
 			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
 			/*
 			 * Netfront requires there to be some data in the head
 			 * buffer.
 			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
 			meta = get_next_rx_buffer(vif, npo);
 		}

@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		}
 
 		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
 			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
 	}
 }

@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
 	struct xen_netif_rx_request *req;
 	struct netbk_rx_meta *meta;
 	unsigned char *data;
-	int head = 1;
+	int first = 1;
 	int old_meta_prod;
 
 	old_meta_prod = npo->meta_prod;

@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
 			len = skb_tail_pointer(skb) - data;
 
 		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
 		data += len;
 	}
 
 	for (i = 0; i < nr_frags; i++) {
 		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
 	}
 
 	return npo->meta_prod - old_meta_prod;

@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
 	}
 }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
 static void xen_netbk_rx_action(struct xen_netbk *netbk)
 {
 	struct xenvif *vif = NULL, *tmp;

@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
 	count = 0;
 
 	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+ 
 		vif = netdev_priv(skb->dev);
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
 		sco = (struct skb_cb_overlay *)skb->cb;
 		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
 		__skb_queue_tail(&rxq, skb);

Quoted 15 lines

drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717
Quoted 10 lines
drivers/net/xen-netback/netback.c +6 -2
+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
 		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
 			break;
Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?

Quoted 3 lines

Ah, I must be off my head here. I mixed several patches 1) my RFC patch that removes MAX_SKB_FRAGS in RX path, 2) Annie's patch and 3) your patch.

Yes, I wanted to get this out since we're talking about multiple changes in similar areas.

Quoted 3 lines

The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.

Yes and no. With this patch, the xen_netbk_count_skb_slots() is now correct. xen_netbk_count_skb_slots() was under-counting when slots were inefficiently consumed. With this patch they match.

Since we've had trouble with the counting in the past, I felt like using the "real" number of slots here was safer and future-proof.

Quoted 3 lines

After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.

Agreed.

Quoted 2 lines

So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.

Yes, I think so.

Quoted 1 line

Comments?

I think that this patch addresses the problem more completely. Annie?

See also the thread from last August:
http://lists.xen.org/archives/html/xen-devel/2012-12/msg00274.html

--msw

annie li

author

2013-07-11 06:01 UTC #13 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 125 lines · Matt Wilson

Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 73 lines
Quoted 73 lines
Quoted 73 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +11 -3
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
  	void *mapping;
  };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
  struct xen_netbk {
  	wait_queue_head_t wq;
  	struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
  	unsigned int count;
  	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
  	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 
@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  				offset = 0;
  		}
  	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
  	return count;
  }
 
@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
  }
 
  /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
   */
  static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  				struct netrx_pending_operations *npo,
  				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
  {
  	struct gnttab_copy *copy_gop;
  	struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  		if (bytes > size)
  			bytes = size;
 

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
drivers/net/xen-netback/netback.c static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb, · line 490
Quoted 2 lines
drivers/net/xen-netback/netback.c +1 -1
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {

head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?

Quoted 9 lines

drivers/net/xen-netback/netback.c +1 -1

  			/*
  			 * Netfront requires there to be some data in the head
  			 * buffer.
  			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
  			meta = get_next_rx_buffer(vif, npo);
  		}

snip...

Quoted 2 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Comments?
I think that this patch addresses the problem more completely. Annie?

See my comments above, I am curious whether the header data is processed correctly.:-)

Thanks
Annie

annie li

author

2013-07-11 13:52 UTC #14 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 123 lines · annie li

Quoted 121 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 69 lines
Quoted 69 lines
Quoted 69 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51 
++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +5 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
      void *mapping;
  };
  +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
  struct xen_netbk {
      wait_queue_head_t wq;
      struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct 
 xenvif *vif, struct sk_buff *skb)
  {
      unsigned int count;
      int i, copy_off;
+    struct skb_cb_overlay *sco;
        count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
  @@ -411,6 +417,9 @@ unsigned int 
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
                  offset = 0;
          }
      }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
      return count;
  }
  @@ -443,13 +452,12 @@ static struct netbk_rx_meta 
*get_next_rx_buffer(struct xenvif *vif,
}
    /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif, struct
sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
  {
      struct gnttab_copy *copy_gop;
      struct netbk_rx_meta *meta;

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/net/xen-netback/netback.c static void netbk_gop_frag_copy(struct · line 491
Quoted 6 lines
drivers/net/xen-netback/netback.c +1 -0
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct 
 xenvif *vif, struct sk_buff *skb,
          if (bytes > size)
              bytes = size;
  -        if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+        if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?

Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand your patch, please ignore my last comments. Your patch keeps the original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy to make slots same with xen_netbk_count_skb_slots. All Roads Lead to Rome!:-)

Thanks
Annie

Wei Liu

2013-07-12 08:49 UTC #15 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 145 lines · annie li

Quoted 134 lines · annie li
Quoted 132 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a
loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1.
This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large
linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 77 lines
Quoted 77 lines
Quoted 77 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
 drivers/net/xen-netback/netback.c |   51
++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +4 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
     void *mapping;
 };
 +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
 struct xen_netbk {
     wait_queue_head_t wq;
     struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
     unsigned int count;
     int i, copy_off;
+    struct skb_cb_overlay *sco;
       count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 @@ -411,6 +417,9 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct
sk_buff *skb)
                 offset = 0;
         }
     }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
     return count;
 }
 @@ -443,13 +452,12 @@ static struct netbk_rx_meta
*get_next_rx_buffer(struct xenvif *vif,
}
/*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif,
struct sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
 {
     struct gnttab_copy *copy_gop;
     struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void
netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
if (bytes > size)
bytes = size;
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+ if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?

Quoted 6 lines

Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand your patch, please ignore my last comments. Your patch keeps the original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy to make slots same with xen_netbk_count_skb_slots. All Roads Lead to Rome!:-)

Actually I'm now in favor of Matt's approach as it a) fix the bug, b) make efficient use of the ring.

Annie, Ian, what's your opinion?

Wei.

Quoted 2 lines

Thanks
Annie

annie li

author

2013-07-12 09:18 UTC #16 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 148 lines · Wei Liu

Quoted 141 lines · annie li
Quoted 132 lines · annie li
Quoted 131 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a
loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1.
This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large
linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 76 lines
Quoted 76 lines
Quoted 76 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51
++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +4 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
      void *mapping;
  };
  +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
  struct xen_netbk {
      wait_queue_head_t wq;
      struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
      unsigned int count;
      int i, copy_off;
+    struct skb_cb_overlay *sco;
        count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
  @@ -411,6 +417,9 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct
sk_buff *skb)
                  offset = 0;
          }
      }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
      return count;
  }
  @@ -443,13 +452,12 @@ static struct netbk_rx_meta
*get_next_rx_buffer(struct xenvif *vif,
}
    /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif,
struct sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
  {
      struct gnttab_copy *copy_gop;
      struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void
netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
if (bytes > size)
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
              bytes = size;
- if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+ if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?
Quoted 6 lines
Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand your patch, please ignore my last comments. Your patch keeps the original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy to make slots same with xen_netbk_count_skb_slots. All Roads Lead to Rome!:-)

Quoted 4 lines

Actually I'm now in favor of Matt's approach as it a) fix the bug, b) make efficient use of the ring.
Annie, Ian, what's your opinion?

Me too. Looks his approach not only fix this bug, but also saves the ring usage for header data, and this is good. Combining my patch with his is not a good idea, so I think we can use his patch.

Thanks
Annie

Quoted 6 lines

Wei.
Quoted 2 lines
Thanks
Annie

Ian Campbell

2013-07-18 11:47 UTC #17 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 152 lines · annie li

Quoted 151 lines · Wei Liu
Quoted 141 lines · annie li
Quoted 132 lines · annie li
Quoted 131 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a
loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1.
This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large
linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 76 lines
Quoted 76 lines
Quoted 76 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51
++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +4 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
      void *mapping;
  };
  +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
  struct xen_netbk {
      wait_queue_head_t wq;
      struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
      unsigned int count;
      int i, copy_off;
+    struct skb_cb_overlay *sco;
        count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
  @@ -411,6 +417,9 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct
sk_buff *skb)
                  offset = 0;
          }
      }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
      return count;
  }
  @@ -443,13 +452,12 @@ static struct netbk_rx_meta
*get_next_rx_buffer(struct xenvif *vif,
}
    /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif,
struct sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
  {
      struct gnttab_copy *copy_gop;
      struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void
netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
if (bytes > size)
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
              bytes = size;
- if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+ if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?
Quoted 6 lines
Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand your patch, please ignore my last comments. Your patch keeps the original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy to make slots same with xen_netbk_count_skb_slots. All Roads Lead to Rome!:-)
Actually I'm now in favor of Matt's approach as it a) fix the bug, b) make efficient use of the ring.

Quoted 6 lines

Quoted 1 line
Annie, Ian, what's your opinion?
Me too. Looks his approach not only fix this bug, but also saves the ring usage for header data, and this is good.
Combining my patch with his is not a good idea, so I think we can use his patch.

Sounds good to me. I didn't review the patch in <[email protected]> in much detail because I think we are expecting a respin with some of Wei & Annie's comment addressed? If I'm wrong and I've just missed that posting please cluebat me.

Ian.

Ian Campbell

2013-07-18 11:47 UTC #18 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 152 lines · annie li

Quoted 151 lines · Wei Liu
Quoted 141 lines · annie li
Quoted 132 lines · annie li
Quoted 131 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a
loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1.
This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large
linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 76 lines
Quoted 76 lines
Quoted 76 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51
++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +4 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
      void *mapping;
  };
  +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
  struct xen_netbk {
      wait_queue_head_t wq;
      struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
      unsigned int count;
      int i, copy_off;
+    struct skb_cb_overlay *sco;
        count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
  @@ -411,6 +417,9 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct
sk_buff *skb)
                  offset = 0;
          }
      }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
      return count;
  }
  @@ -443,13 +452,12 @@ static struct netbk_rx_meta
*get_next_rx_buffer(struct xenvif *vif,
}
    /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif,
struct sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
  {
      struct gnttab_copy *copy_gop;
      struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void
netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
if (bytes > size)
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
              bytes = size;
- if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+ if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?
Quoted 6 lines
Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand your patch, please ignore my last comments. Your patch keeps the original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy to make slots same with xen_netbk_count_skb_slots. All Roads Lead to Rome!:-)
Actually I'm now in favor of Matt's approach as it a) fix the bug, b) make efficient use of the ring.

Quoted 6 lines

Quoted 1 line
Annie, Ian, what's your opinion?
Me too. Looks his approach not only fix this bug, but also saves the ring usage for header data, and this is good.
Combining my patch with his is not a good idea, so I think we can use his patch.

Ian.

annie li

author

2013-07-12 09:18 UTC #19 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 148 lines · Wei Liu

Quoted 141 lines · annie li
Quoted 132 lines · annie li
Quoted 131 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a
loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1.
This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large
linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 76 lines
Quoted 76 lines
Quoted 76 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51
++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +4 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
      void *mapping;
  };
  +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
  struct xen_netbk {
      wait_queue_head_t wq;
      struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
      unsigned int count;
      int i, copy_off;
+    struct skb_cb_overlay *sco;
        count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
  @@ -411,6 +417,9 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct
sk_buff *skb)
                  offset = 0;
          }
      }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
      return count;
  }
  @@ -443,13 +452,12 @@ static struct netbk_rx_meta
*get_next_rx_buffer(struct xenvif *vif,
}
    /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif,
struct sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
  {
      struct gnttab_copy *copy_gop;
      struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void
netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
if (bytes > size)
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
              bytes = size;
- if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+ if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?
Quoted 6 lines
Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand your patch, please ignore my last comments. Your patch keeps the original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy to make slots same with xen_netbk_count_skb_slots. All Roads Lead to Rome!:-)

Quoted 4 lines

Actually I'm now in favor of Matt's approach as it a) fix the bug, b) make efficient use of the ring.
Annie, Ian, what's your opinion?

Me too. Looks his approach not only fix this bug, but also saves the ring usage for header data, and this is good. Combining my patch with his is not a good idea, so I think we can use his patch.

Thanks
Annie

Quoted 6 lines

Wei.
Quoted 2 lines
Thanks
Annie

Wei Liu

2013-07-12 08:49 UTC #20 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 145 lines · annie li

Quoted 134 lines · annie li
Quoted 132 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a
loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1.
This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large
linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 77 lines
Quoted 77 lines
Quoted 77 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
 drivers/net/xen-netback/netback.c |   51
++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +4 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
     void *mapping;
 };
 +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
 struct xen_netbk {
     wait_queue_head_t wq;
     struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
     unsigned int count;
     int i, copy_off;
+    struct skb_cb_overlay *sco;
       count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 @@ -411,6 +417,9 @@ unsigned int
xen_netbk_count_skb_slots(struct xenvif *vif, struct
sk_buff *skb)
                 offset = 0;
         }
     }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
     return count;
 }
 @@ -443,13 +452,12 @@ static struct netbk_rx_meta
*get_next_rx_buffer(struct xenvif *vif,
}
/*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif,
struct sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
 {
     struct gnttab_copy *copy_gop;
     struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void
netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
if (bytes > size)
bytes = size;
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+ if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?

Quoted 6 lines

Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand your patch, please ignore my last comments. Your patch keeps the original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy to make slots same with xen_netbk_count_skb_slots. All Roads Lead to Rome!:-)

Actually I'm now in favor of Matt's approach as it a) fix the bug, b) make efficient use of the ring.

Annie, Ian, what's your opinion?

Wei.

Quoted 2 lines

Thanks
Annie

annie li

author

2013-07-11 13:53 UTC #21 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 123 lines · annie li

Quoted 121 lines · Matt Wilson
Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 69 lines
Quoted 69 lines
Quoted 69 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51 
++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +5 -0
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
      void *mapping;
  };
  +struct skb_cb_overlay {
+    int meta_slots_used;
+    int peek_slots_count;
+};
+
  struct xen_netbk {
      wait_queue_head_t wq;
      struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct 
 xenvif *vif, struct sk_buff *skb)
  {
      unsigned int count;
      int i, copy_off;
+    struct skb_cb_overlay *sco;
        count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
  @@ -411,6 +417,9 @@ unsigned int 
xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
                  offset = 0;
          }
      }
drivers/net/xen-netback/netback.c +3 -0
+
+    sco = (struct skb_cb_overlay *) skb->cb;
+    sco->peek_slots_count = count;
      return count;
  }
  @@ -443,13 +452,12 @@ static struct netbk_rx_meta 
*get_next_rx_buffer(struct xenvif *vif,
}
    /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
*/
static void netbk_gop_frag_copy(struct xenvif *vif, struct
sk_buff *skb,
struct netrx_pending_operations *npo, struct page *page, unsigned long size,
drivers/net/xen-netback/netback.c +1 -1
-                unsigned long offset, int *head)
+                unsigned long offset, int head, int *first)
  {
      struct gnttab_copy *copy_gop;
      struct netbk_rx_meta *meta;

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/net/xen-netback/netback.c static void netbk_gop_frag_copy(struct · line 491
Quoted 6 lines
drivers/net/xen-netback/netback.c +1 -0
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct 
 xenvif *vif, struct sk_buff *skb,
          if (bytes > size)
              bytes = size;
  -        if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+        if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
head is always 1 here when processing the header data, so it is impossible to start a new buffer for header data with larger header size, and get_next_rx_buffer is never called. I assume this would not work well for skb with larger header data size. Have you run network test with this patch?

Thanks
Annie

annie li

author

2013-07-11 06:01 UTC #22 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 125 lines · Matt Wilson

Quoted 9 lines · Wei Liu
Quoted 8 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
*nod*
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).
Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.
Quoted 73 lines
Quoted 73 lines
Quoted 73 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
  drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
  1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +11 -3
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
  	void *mapping;
  };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
  struct xen_netbk {
  	wait_queue_head_t wq;
  	struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  {
  	unsigned int count;
  	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
  	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 
@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
  				offset = 0;
  		}
  	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
  	return count;
  }
 
@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
  }
 
  /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
   */
  static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  				struct netrx_pending_operations *npo,
  				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
  {
  	struct gnttab_copy *copy_gop;
  	struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
  		if (bytes > size)
  			bytes = size;
 

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
drivers/net/xen-netback/netback.c static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb, · line 490
Quoted 2 lines
drivers/net/xen-netback/netback.c +1 -1
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {

Quoted 9 lines

drivers/net/xen-netback/netback.c +1 -1

  			/*
  			 * Netfront requires there to be some data in the head
  			 * buffer.
  			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
  			meta = get_next_rx_buffer(vif, npo);
  		}

snip...

Quoted 2 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Comments?
I think that this patch addresses the problem more completely. Annie?

See my comments above, I am curious whether the header data is processed correctly.:-)

Thanks
Annie

Wei Liu

2013-07-11 08:46 UTC #23 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

On Wed, Jul 10, 2013 at 10:14:43PM -0700, Matt Wilson wrote:
[...]

Quoted 11 lines

Yes, I wanted to get this out since we're talking about multiple changes in similar areas.
Quoted 3 lines
The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.
Yes and no. With this patch, the xen_netbk_count_skb_slots() is now correct. xen_netbk_count_skb_slots() was under-counting when slots were inefficiently consumed. With this patch they match.

This is worth mentioning in the commit message IMHO.

Quoted 3 lines

Since we've had trouble with the counting in the past, I felt like using the "real" number of slots here was safer and future-proof.

Fair enough. Worth a comment in the code.

Quoted 9 lines

Quoted 3 lines
After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.
Agreed.
Quoted 2 lines
So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.

Quoted 9 lines

Yes, I think so.
Quoted 1 line
Comments?
I think that this patch addresses the problem more completely. Annie?
See also the thread from last August:
   http://lists.xen.org/archives/html/xen-devel/2012-12/msg00274.html

So now it seems that "turning head in start_new_rx_buffer to flag" part should also be in the patch as it's actually a fix to match counting in both functions. The side effect of this change is "improving efficiency of the ring". This would also worth mentioning in the commit message IMHO.

Wei.

Quoted 1 line

--msw

Wei Liu

2013-07-11 08:46 UTC #24 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

On Wed, Jul 10, 2013 at 10:14:43PM -0700, Matt Wilson wrote:
[...]

Quoted 11 lines

Yes, I wanted to get this out since we're talking about multiple changes in similar areas.
Quoted 3 lines
The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.
Yes and no. With this patch, the xen_netbk_count_skb_slots() is now correct. xen_netbk_count_skb_slots() was under-counting when slots were inefficiently consumed. With this patch they match.

This is worth mentioning in the commit message IMHO.

Quoted 3 lines

Since we've had trouble with the counting in the past, I felt like using the "real" number of slots here was safer and future-proof.

Fair enough. Worth a comment in the code.

Quoted 9 lines

Quoted 3 lines
After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.
Agreed.
Quoted 2 lines
So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.

Quoted 9 lines

Yes, I think so.
Quoted 1 line
Comments?
I think that this patch addresses the problem more completely. Annie?
See also the thread from last August:
   http://lists.xen.org/archives/html/xen-devel/2012-12/msg00274.html

Wei.

Quoted 1 line

--msw

annie li

author

2013-07-16 10:06 UTC #25 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 1 line · Matt Wilson

I think that this patch addresses the problem more completely. Annie?

Agree, it is better to take your patch. Would you like to go forward with it? This issue is obvious and easy to be hit with larger MTU size.

Thanks
Annie

Wei Liu

2013-07-16 10:27 UTC #26 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 8 lines · annie li

Quoted 1 line · Matt Wilson
I think that this patch addresses the problem more completely. Annie?
Agree, it is better to take your patch. Would you like to go forward with it? This issue is obvious and easy to be hit with larger MTU size.

I'm going to be on vacation tomorrow. Matt, feel free to add my Acked-by to your patch.

Wei.

Quoted 2 lines

Thanks
Annie

Matt Wilson

2013-07-16 17:41 UTC #27 lore raw

Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 5 lines · Wei Liu

Quoted 4 lines · annie li
Quoted 1 line · Matt Wilson
I think that this patch addresses the problem more completely. Annie?

Quoted 7 lines

Quoted 4 lines
Agree, it is better to take your patch. Would you like to go forward with it? This issue is obvious and easy to be hit with larger MTU size.
I'm going to be on vacation tomorrow. Matt, feel free to add my Acked-by to your patch.

Thanks, Wei. I'm on vacation now and I'll return to the office tomorrow. Please accept my apologies for the delay.

I should have time to clean up a few things with this patch (no functional changes) and post a final version this week.

--msw

Quoted 4 lines

Wei.
Quoted 2 lines
Thanks
Annie

Matt Wilson

2013-07-16 17:41 UTC #28 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 5 lines · Wei Liu

Quoted 4 lines · annie li
Quoted 1 line · Matt Wilson
I think that this patch addresses the problem more completely. Annie?

Quoted 7 lines

Quoted 4 lines
Agree, it is better to take your patch. Would you like to go forward with it? This issue is obvious and easy to be hit with larger MTU size.
I'm going to be on vacation tomorrow. Matt, feel free to add my Acked-by to your patch.

Thanks, Wei. I'm on vacation now and I'll return to the office tomorrow. Please accept my apologies for the delay.

I should have time to clean up a few things with this patch (no functional changes) and post a final version this week.

--msw

Quoted 4 lines

Wei.
Quoted 2 lines
Thanks
Annie

Wei Liu

2013-07-16 10:27 UTC #29 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 8 lines · annie li

Quoted 1 line · Matt Wilson
I think that this patch addresses the problem more completely. Annie?
Agree, it is better to take your patch. Would you like to go forward with it? This issue is obvious and easy to be hit with larger MTU size.

I'm going to be on vacation tomorrow. Matt, feel free to add my Acked-by to your patch.

Wei.

Quoted 2 lines

Thanks
Annie

annie li

author

2013-07-16 10:06 UTC #30 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 1 line · Matt Wilson

I think that this patch addresses the problem more completely. Annie?

Agree, it is better to take your patch. Would you like to go forward with it? This issue is obvious and easy to be hit with larger MTU size.

Thanks
Annie

Matt Wilson

2013-07-11 05:15 UTC #31 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 10 lines · Wei Liu

Quoted 9 lines · Wei Liu
Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...

*nod*

Quoted 28 lines

Quoted 28 lines
Quoted 28 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?

Argh, I knew it was in there somewhere (since you pointed it out in Dubin :-).

Maybe it could be a separate patch. I think the description is also a bit confusing. I'll work on rewording it.

Quoted 173 lines

Quoted 157 lines

Signed-off-by Xi Xiong <[email protected]>

[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]

Signed-off-by Matt Wilson <[email protected]>

Cc Annie Li <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Campbell <[email protected]>

---
 drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)

drivers/net/xen-netback/netback.c +25 ~~-18~~

index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
 	void *mapping;
 };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
 struct xen_netbk {
 	wait_queue_head_t wq;
 	struct task_struct *task;

@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
 	unsigned int count;
 	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
 	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);

@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 				offset = 0;
 		}
 	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
 	return count;
 }

@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
 }
 
 /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
  */
 static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 				struct netrx_pending_operations *npo,
 				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
 {
 	struct gnttab_copy *copy_gop;
 	struct netbk_rx_meta *meta;

@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		if (bytes > size)
 			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
 			/*
 			 * Netfront requires there to be some data in the head
 			 * buffer.
 			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
 			meta = get_next_rx_buffer(vif, npo);
 		}

@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		}
 
 		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
 			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
 	}
 }

@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
 	struct xen_netif_rx_request *req;
 	struct netbk_rx_meta *meta;
 	unsigned char *data;
-	int head = 1;
+	int first = 1;
 	int old_meta_prod;
 
 	old_meta_prod = npo->meta_prod;

@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
 			len = skb_tail_pointer(skb) - data;
 
 		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
 		data += len;
 	}
 
 	for (i = 0; i < nr_frags; i++) {
 		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
 	}
 
 	return npo->meta_prod - old_meta_prod;

@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
 	}
 }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
 static void xen_netbk_rx_action(struct xen_netbk *netbk)
 {
 	struct xenvif *vif = NULL, *tmp;

@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
 	count = 0;
 
 	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+ 
 		vif = netdev_priv(skb->dev);
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
 		sco = (struct skb_cb_overlay *)skb->cb;
 		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
 		__skb_queue_tail(&rxq, skb);

Quoted 15 lines

drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717
Quoted 10 lines
drivers/net/xen-netback/netback.c +6 -2
+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
 		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
 			break;
Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?

Quoted 3 lines

Ah, I must be off my head here. I mixed several patches 1) my RFC patch that removes MAX_SKB_FRAGS in RX path, 2) Annie's patch and 3) your patch.

Yes, I wanted to get this out since we're talking about multiple changes in similar areas.

Quoted 3 lines

The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.

Yes and no. With this patch, the xen_netbk_count_skb_slots() is now correct. xen_netbk_count_skb_slots() was under-counting when slots were inefficiently consumed. With this patch they match.

Since we've had trouble with the counting in the past, I felt like using the "real" number of slots here was safer and future-proof.

Quoted 3 lines

After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.

Agreed.

Quoted 2 lines

So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.

Yes, I think so.

Quoted 1 line

Comments?

I think that this patch addresses the problem more completely. Annie?

See also the thread from last August:
http://lists.xen.org/archives/html/xen-devel/2012-12/msg00274.html

--msw

Wei Liu

2013-07-10 19:37 UTC #32 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 203 lines · Wei Liu

Quoted 5 lines · Matt Wilson
From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]
Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...
Quoted 24 lines
Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.
Quoted 8 lines
Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will
Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?
Quoted 157 lines
consume three RX slots instead of two. This patch changes the "head" parameter to netbk_gop_frag_copy() to act as a flag. When set, start_new_rx_buffer() will always place as much data as possible into each RX slot.
Signed-off-by Xi Xiong <[email protected]>
Reviewed-by Matt Wilson <[email protected]>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by Matt Wilson <[email protected]>
Cc Annie Li <[email protected]>
Cc Wei Liu <[email protected]>
Cc Ian Campbell <[email protected]>
Cc [email protected]
Cc [email protected]
---
 drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)
drivers/net/xen-netback/netback.c +25 ~~-18~~
index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
 	void *mapping;
 };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
 struct xen_netbk {
 	wait_queue_head_t wq;
 	struct task_struct *task;
@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
 	unsigned int count;
 	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
 	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
 
@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 				offset = 0;
 		}
 	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
 	return count;
 }
 
@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
 }
 
 /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
  */
 static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 				struct netrx_pending_operations *npo,
 				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
 {
 	struct gnttab_copy *copy_gop;
 	struct netbk_rx_meta *meta;
@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		if (bytes > size)
 			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
 			/*
 			 * Netfront requires there to be some data in the head
 			 * buffer.
 			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
 			meta = get_next_rx_buffer(vif, npo);
 		}
@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		}
 
 		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
 			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
 	}
 }
@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
 	struct xen_netif_rx_request *req;
 	struct netbk_rx_meta *meta;
 	unsigned char *data;
-	int head = 1;
+	int first = 1;
 	int old_meta_prod;
 
 	old_meta_prod = npo->meta_prod;
@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
 			len = skb_tail_pointer(skb) - data;
 
 		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
 		data += len;
 	}
 
 	for (i = 0; i < nr_frags; i++) {
 		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
 	}
 
 	return npo->meta_prod - old_meta_prod;
@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
 	}
 }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
 static void xen_netbk_rx_action(struct xen_netbk *netbk)
 {
 	struct xenvif *vif = NULL, *tmp;
@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
 	count = 0;
 
 	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+ 
 		vif = netdev_priv(skb->dev);
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
 		sco = (struct skb_cb_overlay *)skb->cb;
 		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
 		__skb_queue_tail(&rxq, skb);
 

Quoted 15 lines

drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717
Quoted 10 lines
drivers/net/xen-netback/netback.c +6 -2
+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
 		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
 			break;
Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?

Ah, I must be off my head here. I mixed several patches 1) my RFC patch that removes MAX_SKB_FRAGS in RX path, 2) Annie's patch and 3) your patch.

The reason that your patches uses req_cons instead of pre-calculated value is, that value returned by xen_netbk_skb_count_slots is actually *wrong* -- that's what Annie tried to fix in her patch.

After fixing xen_netbk_skb_count_slots, we would need the above snippet (or the snippet I proposed in my RFC patch) to prevent overruning the ring.

So a proper patch to this issue (not couting RX slots correctly causing overrun of the ring) would be the above two aspects combined.

Comments?

Wei.

Quoted 7 lines

Wei.
Quoted 4 lines
drivers/net/xen-netback/netback.c +0 -0
 	}
 
Signature · 1 line
1.7.4.5

Wei Liu

2013-07-10 08:13 UTC #33 lore raw

Re: [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

Quoted 5 lines · Matt Wilson

From: Xi Xiong <[email protected]>
[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]

Should probably rebase it on net.git because it is a bug fix. Let's worry about that later...

Quoted 24 lines

Currently the number of RX slots required to transmit a SKB to xen-netfront can be miscalculated when an interface uses a MTU larger than PAGE_SIZE. If the slot calculation is wrong, xen-netback can pause the queue indefinitely or reuse slots. The former manifests as a loss of connectivity to the guest (which can be restored by lowering the MTU set on the interface). The latter manifests with "Bad grant reference" messages from Xen such as:
(XEN) grant_table.c:1797:d0 Bad grant reference 264241157
and kernel messages within the guest such as:
[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295
BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.
This patch changes xen_netbk_rx_action() to count the number of RX slots actually consumed by netbk_gop_skb() instead of using nr_frags + 1. This prevents under-counting the number of RX slots consumed when a SKB has a large linear buffer.

Quoted 8 lines

Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.
Finally, the logic in start_new_rx_buffer() can cause RX slots to be wasted when setting up copy grant table operations for SKBs with large linear buffers. For example, a SKB with skb_headlen() equal to 8157 bytes that starts 64 bytes 64 bytes from the start of the page will

Duplicated "64 bytes". And this change looks like an improvement not a bug fix. Probably submit a separate patch for this?

Quoted 157 lines

Signed-off-by Xi Xiong <[email protected]>

[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]

Signed-off-by Matt Wilson <[email protected]>

Cc Annie Li <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Campbell <[email protected]>

---
 drivers/net/xen-netback/netback.c |   51 ++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 20 deletions(-)

drivers/net/xen-netback/netback.c +25 ~~-18~~

index 64828de..82dd207 100644
@@ -110,6 +110,11 @@ union page_ext {
 	void *mapping;
 };
 
+struct skb_cb_overlay {
+	int meta_slots_used;
+	int peek_slots_count;
+};
+
 struct xen_netbk {
 	wait_queue_head_t wq;
 	struct task_struct *task;

@@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 {
 	unsigned int count;
 	int i, copy_off;
+	struct skb_cb_overlay *sco;
 
 	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);

@@ -411,6 +417,9 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
 				offset = 0;
 		}
 	}
+
+	sco = (struct skb_cb_overlay *) skb->cb;
+	sco->peek_slots_count = count;
 	return count;
 }

@@ -443,13 +452,12 @@ static struct netbk_rx_meta *get_next_rx_buffer(struct xenvif *vif,
 }
 
 /*
- * Set up the grant operations for this fragment. If it's a flipping
- * interface, we also set up the unmap request from here.
+ * Set up the grant operations for this fragment.
  */
 static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 				struct netrx_pending_operations *npo,
 				struct page *page, unsigned long size,
-				unsigned long offset, int *head)
+				unsigned long offset, int head, int *first)
 {
 	struct gnttab_copy *copy_gop;
 	struct netbk_rx_meta *meta;

@@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		if (bytes > size)
 			bytes = size;
 
-		if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
+		if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
 			/*
 			 * Netfront requires there to be some data in the head
 			 * buffer.
 			 */
-			BUG_ON(*head);
+			BUG_ON(*first);
 
 			meta = get_next_rx_buffer(vif, npo);
 		}

@@ -529,10 +537,10 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		}
 
 		/* Leave a gap for the GSO descriptor. */
-		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+		if (*first && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
 			vif->rx.req_cons++;
 
-		*head = 0; /* There must be something in this buffer now. */
+		*first = 0; /* There must be something in this buffer now. */
 
 	}
 }

@@ -558,7 +566,7 @@ static int netbk_gop_skb(struct sk_buff *skb,
 	struct xen_netif_rx_request *req;
 	struct netbk_rx_meta *meta;
 	unsigned char *data;
-	int head = 1;
+	int first = 1;
 	int old_meta_prod;
 
 	old_meta_prod = npo->meta_prod;

@@ -594,16 +602,16 @@ static int netbk_gop_skb(struct sk_buff *skb,
 			len = skb_tail_pointer(skb) - data;
 
 		netbk_gop_frag_copy(vif, skb, npo,
-				    virt_to_page(data), len, offset, &head);
+				virt_to_page(data), len, offset, 1, &first);
 		data += len;
 	}
 
 	for (i = 0; i < nr_frags; i++) {
 		netbk_gop_frag_copy(vif, skb, npo,
-				    skb_frag_page(&skb_shinfo(skb)->frags[i]),
-				    skb_frag_size(&skb_shinfo(skb)->frags[i]),
-				    skb_shinfo(skb)->frags[i].page_offset,
-				    &head);
+				skb_frag_page(&skb_shinfo(skb)->frags[i]),
+				skb_frag_size(&skb_shinfo(skb)->frags[i]),
+				skb_shinfo(skb)->frags[i].page_offset,
+				0, &first);
 	}
 
 	return npo->meta_prod - old_meta_prod;

@@ -661,10 +669,6 @@ static void netbk_add_frag_responses(struct xenvif *vif, int status,
 	}
 }
 
-struct skb_cb_overlay {
-	int meta_slots_used;
-};
-
 static void xen_netbk_rx_action(struct xen_netbk *netbk)
 {
 	struct xenvif *vif = NULL, *tmp;

@@ -690,19 +694,26 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
 	count = 0;
 
 	while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
+		RING_IDX old_rx_req_cons;
+ 
 		vif = netdev_priv(skb->dev);
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
+		old_rx_req_cons = vif->rx.req_cons;
 		sco = (struct skb_cb_overlay *)skb->cb;
 		sco->meta_slots_used = netbk_gop_skb(skb, &npo);
 
-		count += nr_frags + 1;
+		count += vif->rx.req_cons - old_rx_req_cons;
 
 		__skb_queue_tail(&rxq, skb);

drivers/net/xen-netback/netback.c static void xen_netbk_rx_action(struct xen_netbk *netbk) · line 717

Quoted 10 lines

drivers/net/xen-netback/netback.c +6 -2

+		skb = skb_peek(&netbk->rx_queue);
+		if (skb == NULL)
+			break;
+		sco = (struct skb_cb_overlay *) skb->cb;
+
 		/* Filled the batch queue? */
-		/* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + sco->peek_slots_count >= XEN_NETIF_RX_RING_SIZE)
 			break;

Using req_cons to count is OK, but since you've already store the number of slots in sco->peek_slots_count before actually queueing the packet, why don't you use that directly?

Wei.

Quoted 4 lines

drivers/net/xen-netback/netback.c +0 -0
 	}
 
Signature · 1 line
1.7.4.5

Matt Wilson

2013-07-09 22:41 UTC #34 lore raw

RFC PATCH 1/1 xen-netback: calculate the number of slots required for large MTU vifs

From: Xi Xiong <[email protected]>

[ note: I've just cherry picked this onto net-next, and only compile
tested. This a RFC only. -msw ]

(XEN) grant_table.c:1797:d0 Bad grant reference 264241157

and kernel messages within the guest such as:

[  180.419567] net eth0: Invalid extra type: 112
[  180.868620] net eth0: rx->offset: 0, size: 4294967295
[  180.868629] net eth0: rx->offset: 0, size: 4294967295

BUG_ON() assertions can also be hit if RX slots are exhausted while handling a SKB.

Additionally, we now store the estimated number of RX slots required to handle a SKB in the cb overlay. This value is used to determine if the next SKB in the queue can be processed.

Signed-off-by Xi Xiong <[email protected]>

[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]

Signed-off-by Matt Wilson <[email protected]>

Cc Annie Li <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Campbell <[email protected]>