Wei Liu
2013-Sep-22 18:03 UTC
[PATCH net-next] xen-netback: improve ring effeciency for guest RX
There was a bug that netback routines netbk/xenvif_skb_count_slots and
netbk/xenvif_gop_frag_copy disagreed with each other, which caused
netback to push wrong number of responses to netfront, which caused
netfront to eventually crash. The bug was fixed in 6e43fc04a
("xen-netback: count number required slots for an skb more
carefully").
Commit 6e43fc04a focused on backport-ability. The drawback with the
existing packing scheme is that the ring is not used effeciently, as
stated in 6e43fc04a.
skb->data like:
| 1111|222222222222|3333 |
is arranged as:
|1111 |222222222222|3333 |
If we can do this:
|111122222222|22223333 |
That would save one ring slot, which improves ring effeciency.
This patch effectively reverts 6e43fc04a. That patch made count_slots
agree with gop_frag_copy, while this patch goes the other way around --
make gop_frag_copy agree with count_slots. The end result is that they
still agree with each other, and the ring is now arranged like:
|111122222222|22223333 |
The patch that improves packing was first posted by Xi Xong and Matt
Wilson. I only rebase it on top of net-next and rewrite commit message,
so I retain all their SoBs. For more infomation about the original bug
please refer to email listed below and commit message of 6e43fc04a.
Original patch:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg00760.html
Signed-off-by: Xi Xiong <xixiong@amazon.com>
Reviewed-by: Matt Wilson <msw@amazon.com>
[ msw: minor code cleanups, rewrote commit message, adjusted code
to count RX slots instead of meta structures ]
Signed-off-by: Matt Wilson <msw@amazon.com>
Cc: Annie Li <annie.li@oracle.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
[ liuw: rebased on top of net-next tree, rewrote commit message, coding
style cleanup. ]
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
drivers/net/xen-netback/netback.c | 144 ++++++++++++++++---------------------
1 file changed, 61 insertions(+), 83 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index f3e591c..d0b0feb 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -47,6 +47,14 @@
#include <asm/xen/hypercall.h>
#include <asm/xen/page.h>
+/* SKB control block overlay is used to store useful information when
+ * doing guest RX.
+ */
+struct skb_cb_overlay {
+ int meta_slots_used;
+ int peek_slots_count;
+};
+
/* Provide an option to disable split event channels at load time as
* event channels are limited resource. Split event channels are
* enabled by default.
@@ -212,49 +220,6 @@ static bool start_new_rx_buffer(int offset, unsigned long
size, int head)
return false;
}
-struct xenvif_count_slot_state {
- unsigned long copy_off;
- bool head;
-};
-
-unsigned int xenvif_count_frag_slots(struct xenvif *vif,
- unsigned long offset, unsigned long size,
- struct xenvif_count_slot_state *state)
-{
- unsigned count = 0;
-
- offset &= ~PAGE_MASK;
-
- while (size > 0) {
- unsigned long bytes;
-
- bytes = PAGE_SIZE - offset;
-
- if (bytes > size)
- bytes = size;
-
- if (start_new_rx_buffer(state->copy_off, bytes, state->head)) {
- count++;
- state->copy_off = 0;
- }
-
- if (state->copy_off + bytes > MAX_BUFFER_OFFSET)
- bytes = MAX_BUFFER_OFFSET - state->copy_off;
-
- state->copy_off += bytes;
-
- offset += bytes;
- size -= bytes;
-
- if (offset == PAGE_SIZE)
- offset = 0;
-
- state->head = false;
- }
-
- return count;
-}
-
/*
* Figure out how many ring slots we''re going to need to send @skb to
* the guest. This function is essentially a dry run of
@@ -262,40 +227,53 @@ unsigned int xenvif_count_frag_slots(struct xenvif *vif,
*/
unsigned int xenvif_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
{
- struct xenvif_count_slot_state state;
unsigned int count;
- unsigned char *data;
- unsigned i;
+ int i, copy_off;
+ struct skb_cb_overlay *sco;
- state.head = true;
- state.copy_off = 0;
+ count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
- /* Slot for the first (partial) page of data. */
- count = 1;
+ copy_off = skb_headlen(skb) % PAGE_SIZE;
- /* Need a slot for the GSO prefix for GSO extra data? */
if (skb_shinfo(skb)->gso_size)
count++;
- data = skb->data;
- while (data < skb_tail_pointer(skb)) {
- unsigned long offset = offset_in_page(data);
- unsigned long size = PAGE_SIZE - 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;
- if (data + size > skb_tail_pointer(skb))
- size = skb_tail_pointer(skb) - data;
+ offset &= ~PAGE_MASK;
- count += xenvif_count_frag_slots(vif, offset, size, &state);
+ while (size > 0) {
+ BUG_ON(offset >= PAGE_SIZE);
+ BUG_ON(copy_off > MAX_BUFFER_OFFSET);
- data += size;
- }
+ bytes = PAGE_SIZE - 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;
+ if (bytes > size)
+ bytes = size;
+
+ if (start_new_rx_buffer(copy_off, bytes, 0)) {
+ count++;
+ copy_off = 0;
+ }
- count += xenvif_count_frag_slots(vif, offset, size, &state);
+ if (copy_off + bytes > MAX_BUFFER_OFFSET)
+ bytes = MAX_BUFFER_OFFSET - copy_off;
+
+ copy_off += bytes;
+
+ offset += bytes;
+ size -= bytes;
+
+ if (offset == PAGE_SIZE)
+ offset = 0;
+ }
}
+
+ sco = (struct skb_cb_overlay *)skb->cb;
+ sco->peek_slots_count = count;
return count;
}
@@ -327,14 +305,11 @@ static struct xenvif_rx_meta *get_next_rx_buffer(struct
xenvif *vif,
return meta;
}
-/*
- * 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 xenvif_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 xenvif_rx_meta *meta;
@@ -358,12 +333,12 @@ static void xenvif_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);
}
@@ -397,10 +372,10 @@ static void xenvif_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. */
}
}
@@ -426,7 +401,7 @@ static int xenvif_gop_skb(struct sk_buff *skb,
struct xen_netif_rx_request *req;
struct xenvif_rx_meta *meta;
unsigned char *data;
- int head = 1;
+ int first = 1;
int old_meta_prod;
old_meta_prod = npo->meta_prod;
@@ -462,7 +437,7 @@ static int xenvif_gop_skb(struct sk_buff *skb,
len = skb_tail_pointer(skb) - data;
xenvif_gop_frag_copy(vif, skb, npo,
- virt_to_page(data), len, offset, &head);
+ virt_to_page(data), len, offset, 1, &first);
data += len;
}
@@ -471,7 +446,7 @@ static int xenvif_gop_skb(struct sk_buff *skb,
skb_frag_page(&skb_shinfo(skb)->frags[i]),
skb_frag_size(&skb_shinfo(skb)->frags[i]),
skb_shinfo(skb)->frags[i].page_offset,
- &head);
+ 0, &first);
}
return npo->meta_prod - old_meta_prod;
@@ -529,10 +504,6 @@ static void xenvif_add_frag_responses(struct xenvif *vif,
int status,
}
}
-struct skb_cb_overlay {
- int meta_slots_used;
-};
-
static void xenvif_kick_thread(struct xenvif *vif)
{
wake_up(&vif->wq);
@@ -563,19 +534,26 @@ void xenvif_rx_action(struct xenvif *vif)
count = 0;
while ((skb = skb_dequeue(&vif->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 = xenvif_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(&vif->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;
}
--
1.7.10.4
Ian Campbell
2013-Sep-24 09:41 UTC
Re: [PATCH net-next] xen-netback: improve ring effeciency for guest RX
On Sun, 2013-09-22 at 19:03 +0100, Wei Liu wrote:> There was a bug that netback routines netbk/xenvif_skb_count_slots and > netbk/xenvif_gop_frag_copy disagreed with each other, which caused > netback to push wrong number of responses to netfront, which caused > netfront to eventually crash. The bug was fixed in 6e43fc04a > ("xen-netback: count number required slots for an skb more carefully"). > > Commit 6e43fc04a focused on backport-ability. The drawback with the > existing packing scheme is that the ring is not used effeciently, as > stated in 6e43fc04a. > > skb->data like: > | 1111|222222222222|3333 | > > is arranged as: > |1111 |222222222222|3333 | > > If we can do this: > |111122222222|22223333 | > That would save one ring slot, which improves ring effeciency. > > This patch effectively reverts 6e43fc04a. That patch made count_slots > agree with gop_frag_copy, while this patch goes the other way around -- > make gop_frag_copy agree with count_slots. The end result is that they > still agree with each other, and the ring is now arranged like: > |111122222222|22223333 | > > The patch that improves packing was first posted by Xi Xong and Matt > Wilson. I only rebase it on top of net-next and rewrite commit message, > so I retain all their SoBs. For more infomation about the original bug > please refer to email listed below and commit message of 6e43fc04a. > > Original patch: > http://lists.xen.org/archives/html/xen-devel/2013-07/msg00760.html > > Signed-off-by: Xi Xiong <xixiong@amazon.com> > Reviewed-by: Matt Wilson <msw@amazon.com> > [ msw: minor code cleanups, rewrote commit message, adjusted code > to count RX slots instead of meta structures ] > Signed-off-by: Matt Wilson <msw@amazon.com> > Cc: Annie Li <annie.li@oracle.com> > Cc: Wei Liu <wei.liu2@citrix.com>Acked-by: Ian Campbell <Ian.Campbell@citrix.com>> [ liuw: rebased on top of net-next tree, rewrote commit message, coding > style cleanup. ] > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Cc: David Vrabel <david.vrabel@citrix.com> > --- > drivers/net/xen-netback/netback.c | 144 ++++++++++++++++--------------------- > 1 file changed, 61 insertions(+), 83 deletions(-) > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c > index f3e591c..d0b0feb 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -47,6 +47,14 @@ > #include <asm/xen/hypercall.h> > #include <asm/xen/page.h> > > +/* SKB control block overlay is used to store useful information when > + * doing guest RX. > + */ > +struct skb_cb_overlay { > + int meta_slots_used; > + int peek_slots_count; > +}; > + > /* Provide an option to disable split event channels at load time as > * event channels are limited resource. Split event channels are > * enabled by default. > @@ -212,49 +220,6 @@ static bool start_new_rx_buffer(int offset, unsigned long size, int head) > return false; > } > > -struct xenvif_count_slot_state { > - unsigned long copy_off; > - bool head; > -}; > - > -unsigned int xenvif_count_frag_slots(struct xenvif *vif, > - unsigned long offset, unsigned long size, > - struct xenvif_count_slot_state *state) > -{ > - unsigned count = 0; > - > - offset &= ~PAGE_MASK; > - > - while (size > 0) { > - unsigned long bytes; > - > - bytes = PAGE_SIZE - offset; > - > - if (bytes > size) > - bytes = size; > - > - if (start_new_rx_buffer(state->copy_off, bytes, state->head)) { > - count++; > - state->copy_off = 0; > - } > - > - if (state->copy_off + bytes > MAX_BUFFER_OFFSET) > - bytes = MAX_BUFFER_OFFSET - state->copy_off; > - > - state->copy_off += bytes; > - > - offset += bytes; > - size -= bytes; > - > - if (offset == PAGE_SIZE) > - offset = 0; > - > - state->head = false; > - } > - > - return count; > -} > - > /* > * Figure out how many ring slots we''re going to need to send @skb to > * the guest. This function is essentially a dry run of > @@ -262,40 +227,53 @@ unsigned int xenvif_count_frag_slots(struct xenvif *vif, > */ > unsigned int xenvif_count_skb_slots(struct xenvif *vif, struct sk_buff *skb) > { > - struct xenvif_count_slot_state state; > unsigned int count; > - unsigned char *data; > - unsigned i; > + int i, copy_off; > + struct skb_cb_overlay *sco; > > - state.head = true; > - state.copy_off = 0; > + count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE); > > - /* Slot for the first (partial) page of data. */ > - count = 1; > + copy_off = skb_headlen(skb) % PAGE_SIZE; > > - /* Need a slot for the GSO prefix for GSO extra data? */ > if (skb_shinfo(skb)->gso_size) > count++; > > - data = skb->data; > - while (data < skb_tail_pointer(skb)) { > - unsigned long offset = offset_in_page(data); > - unsigned long size = PAGE_SIZE - 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; > > - if (data + size > skb_tail_pointer(skb)) > - size = skb_tail_pointer(skb) - data; > + offset &= ~PAGE_MASK; > > - count += xenvif_count_frag_slots(vif, offset, size, &state); > + while (size > 0) { > + BUG_ON(offset >= PAGE_SIZE); > + BUG_ON(copy_off > MAX_BUFFER_OFFSET); > > - data += size; > - } > + bytes = PAGE_SIZE - 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; > + if (bytes > size) > + bytes = size; > + > + if (start_new_rx_buffer(copy_off, bytes, 0)) { > + count++; > + copy_off = 0; > + } > > - count += xenvif_count_frag_slots(vif, offset, size, &state); > + if (copy_off + bytes > MAX_BUFFER_OFFSET) > + bytes = MAX_BUFFER_OFFSET - copy_off; > + > + copy_off += bytes; > + > + offset += bytes; > + size -= bytes; > + > + if (offset == PAGE_SIZE) > + offset = 0; > + } > } > + > + sco = (struct skb_cb_overlay *)skb->cb; > + sco->peek_slots_count = count; > return count; > } > > @@ -327,14 +305,11 @@ static struct xenvif_rx_meta *get_next_rx_buffer(struct xenvif *vif, > return meta; > } > > -/* > - * 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 xenvif_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 xenvif_rx_meta *meta; > @@ -358,12 +333,12 @@ static void xenvif_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); > } > @@ -397,10 +372,10 @@ static void xenvif_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. */ > > } > } > @@ -426,7 +401,7 @@ static int xenvif_gop_skb(struct sk_buff *skb, > struct xen_netif_rx_request *req; > struct xenvif_rx_meta *meta; > unsigned char *data; > - int head = 1; > + int first = 1; > int old_meta_prod; > > old_meta_prod = npo->meta_prod; > @@ -462,7 +437,7 @@ static int xenvif_gop_skb(struct sk_buff *skb, > len = skb_tail_pointer(skb) - data; > > xenvif_gop_frag_copy(vif, skb, npo, > - virt_to_page(data), len, offset, &head); > + virt_to_page(data), len, offset, 1, &first); > data += len; > } > > @@ -471,7 +446,7 @@ static int xenvif_gop_skb(struct sk_buff *skb, > skb_frag_page(&skb_shinfo(skb)->frags[i]), > skb_frag_size(&skb_shinfo(skb)->frags[i]), > skb_shinfo(skb)->frags[i].page_offset, > - &head); > + 0, &first); > } > > return npo->meta_prod - old_meta_prod; > @@ -529,10 +504,6 @@ static void xenvif_add_frag_responses(struct xenvif *vif, int status, > } > } > > -struct skb_cb_overlay { > - int meta_slots_used; > -}; > - > static void xenvif_kick_thread(struct xenvif *vif) > { > wake_up(&vif->wq); > @@ -563,19 +534,26 @@ void xenvif_rx_action(struct xenvif *vif) > count = 0; > > while ((skb = skb_dequeue(&vif->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 = xenvif_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(&vif->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; > } >