| Message ID | 1395922988-37702-3-git-send-email-paul.durrant@citrix.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2014-03-27 at 12:23 +0000, Paul Durrant wrote: > The worse-case estimate for skb ring slot usage in xenvif_rx_action() > fails to take fragment page_offset into account. The page_offset does, > however, affect the number of times the fragmentation code calls > start_new_rx_buffer() (i.e. consume another slot) and the worse-case > should assume that will always return true. This patch adds the page_offset > into the DIV_ROUND_UP for each frag. At least for the copying mode wasn't the idea that you would copy to the start of the page, so the offset wasn't relevant? IOW is the real issue that start_new_rx_buffer is/was too aggressive? Now that we do mapping though I suspect the offset becomes relevant again here and there is a 1:1 mapping from slots to frags again. (I could have sworn David V got rid of all this precalculating stuff.) > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Sander Eikelenboom <linux@eikelenboom.it> > --- > drivers/net/xen-netback/netback.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c > index befc413..ac35489 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -492,8 +492,18 @@ static void xenvif_rx_action(struct xenvif *vif) > PAGE_SIZE); > for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > unsigned int size; > + unsigned int offset; > + > size = skb_frag_size(&skb_shinfo(skb)->frags[i]); > - max_slots_needed += DIV_ROUND_UP(size, PAGE_SIZE); > + offset = skb_shinfo(skb)->frags[i].page_offset; > + > + /* For a worse-case estimate we need to factor in > + * the fragment page offset as this will affect the > + * number of times xenvif_gop_frag_copy() will > + * call start_new_rx_buffer(). > + */ > + max_slots_needed += DIV_ROUND_UP(offset + size, > + PAGE_SIZE); > } > if (skb_is_gso(skb) && > (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4 || -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Ian Campbell > Sent: 27 March 2014 12:28 > To: Paul Durrant > Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Sander > Eikelenboom > Subject: Re: [PATCH net 2/3] xen-netback: worse-case estimate in > xenvif_rx_action is underestimating > > On Thu, 2014-03-27 at 12:23 +0000, Paul Durrant wrote: > > The worse-case estimate for skb ring slot usage in xenvif_rx_action() > > fails to take fragment page_offset into account. The page_offset does, > > however, affect the number of times the fragmentation code calls > > start_new_rx_buffer() (i.e. consume another slot) and the worse-case > > should assume that will always return true. This patch adds the page_offset > > into the DIV_ROUND_UP for each frag. > > At least for the copying mode wasn't the idea that you would copy to the > start of the page, so the offset wasn't relevant? IOW is the real issue > that start_new_rx_buffer is/was too aggressive? > > Now that we do mapping though I suspect the offset becomes relevant > again here and there is a 1:1 mapping from slots to frags again. > We're always in copying mode. This is guest receive side :-) > (I could have sworn David V got rid of all this precalculating stuff.) > He did modify it. I got rid of it in favour of the best-case and worse-case estimations. Paul > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Cc: Ian Campbell <ian.campbell@citrix.com> > > Cc: Wei Liu <wei.liu2@citrix.com> > > Cc: Sander Eikelenboom <linux@eikelenboom.it> > > --- > > drivers/net/xen-netback/netback.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen- > netback/netback.c > > index befc413..ac35489 100644 > > --- a/drivers/net/xen-netback/netback.c > > +++ b/drivers/net/xen-netback/netback.c > > @@ -492,8 +492,18 @@ static void xenvif_rx_action(struct xenvif *vif) > > PAGE_SIZE); > > for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > > unsigned int size; > > + unsigned int offset; > > + > > size = skb_frag_size(&skb_shinfo(skb)->frags[i]); > > - max_slots_needed += DIV_ROUND_UP(size, > PAGE_SIZE); > > + offset = skb_shinfo(skb)->frags[i].page_offset; > > + > > + /* For a worse-case estimate we need to factor in > > + * the fragment page offset as this will affect the > > + * number of times xenvif_gop_frag_copy() will > > + * call start_new_rx_buffer(). > > + */ > > + max_slots_needed += DIV_ROUND_UP(offset + size, > > + PAGE_SIZE); > > } > > if (skb_is_gso(skb) && > > (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4 || >
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index befc413..ac35489 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -492,8 +492,18 @@ static void xenvif_rx_action(struct xenvif *vif) PAGE_SIZE); for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { unsigned int size; + unsigned int offset; + size = skb_frag_size(&skb_shinfo(skb)->frags[i]); - max_slots_needed += DIV_ROUND_UP(size, PAGE_SIZE); + offset = skb_shinfo(skb)->frags[i].page_offset; + + /* For a worse-case estimate we need to factor in + * the fragment page offset as this will affect the + * number of times xenvif_gop_frag_copy() will + * call start_new_rx_buffer(). + */ + max_slots_needed += DIV_ROUND_UP(offset + size, + PAGE_SIZE); } if (skb_is_gso(skb) && (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4 ||
The worse-case estimate for skb ring slot usage in xenvif_rx_action() fails to take fragment page_offset into account. The page_offset does, however, affect the number of times the fragmentation code calls start_new_rx_buffer() (i.e. consume another slot) and the worse-case should assume that will always return true. This patch adds the page_offset into the DIV_ROUND_UP for each frag. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Sander Eikelenboom <linux@eikelenboom.it> --- drivers/net/xen-netback/netback.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)