| Message ID | 1452784710-11923-1-git-send-email-david.vrabel@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: David Vrabel <david.vrabel@citrix.com> Date: Thu, 14 Jan 2016 15:18:30 +0000 > - needed = xenvif_rx_ring_slots_needed(queue->vif); > + skb = skb_peek(&queue->rx_queue); > + if (!skb) > + return false; > + > + needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE); > + if (skb_is_gso(skb)) > + needed++; If I am not mistaken, we moved away from this kind of test exactly because it is inaccurate and may under-estimate the needs. It is possible for an N byte SKB to require N segments. Therefore, the: DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE); calculation doesn't cut it.
On 14/01/16 21:54, David Miller wrote: > From: David Vrabel <david.vrabel@citrix.com> > Date: Thu, 14 Jan 2016 15:18:30 +0000 > >> - needed = xenvif_rx_ring_slots_needed(queue->vif); >> + skb = skb_peek(&queue->rx_queue); >> + if (!skb) >> + return false; >> + >> + needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE); >> + if (skb_is_gso(skb)) >> + needed++; > > If I am not mistaken, we moved away from this kind of test exactly because > it is inaccurate and may under-estimate the needs. > > It is possible for an N byte SKB to require N segments. Therefore, the: > > DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE); > > calculation doesn't cut it. After 1650d5455bd2dc6b5ee134bd6fc1a3236c266b5b (xen-netback: always fully coalesce guest Rx packets) we always fully pack a packet into its guest Rx slots. Each slot has space for XEN_PAGE_SIZE bytes so this calculation for the number of slots is correct. Shall I resend with a more description changelog? David
From: David Vrabel <david.vrabel@citrix.com> Date: Fri, 15 Jan 2016 10:31:57 +0000 > On 14/01/16 21:54, David Miller wrote: >> From: David Vrabel <david.vrabel@citrix.com> >> Date: Thu, 14 Jan 2016 15:18:30 +0000 >> >>> - needed = xenvif_rx_ring_slots_needed(queue->vif); >>> + skb = skb_peek(&queue->rx_queue); >>> + if (!skb) >>> + return false; >>> + >>> + needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE); >>> + if (skb_is_gso(skb)) >>> + needed++; >> >> If I am not mistaken, we moved away from this kind of test exactly because >> it is inaccurate and may under-estimate the needs. >> >> It is possible for an N byte SKB to require N segments. Therefore, the: >> >> DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE); >> >> calculation doesn't cut it. > > After 1650d5455bd2dc6b5ee134bd6fc1a3236c266b5b (xen-netback: always > fully coalesce guest Rx packets) we always fully pack a packet into its > guest Rx slots. Each slot has space for XEN_PAGE_SIZE bytes so this > calculation for the number of slots is correct. > > Shall I resend with a more description changelog? Yeah that would help a lot.
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index 1049c34..61b97c3 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -149,20 +149,19 @@ static inline pending_ring_idx_t pending_index(unsigned i) return i & (MAX_PENDING_REQS-1); } -static int xenvif_rx_ring_slots_needed(struct xenvif *vif) -{ - if (vif->gso_mask) - return DIV_ROUND_UP(vif->dev->gso_max_size, XEN_PAGE_SIZE) + 1; - else - return DIV_ROUND_UP(vif->dev->mtu, XEN_PAGE_SIZE); -} - static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue) { RING_IDX prod, cons; + struct sk_buff *skb; int needed; - needed = xenvif_rx_ring_slots_needed(queue->vif); + skb = skb_peek(&queue->rx_queue); + if (!skb) + return false; + + needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE); + if (skb_is_gso(skb)) + needed++; do { prod = queue->rx.sring->req_prod; @@ -2005,8 +2004,7 @@ static bool xenvif_rx_queue_ready(struct xenvif_queue *queue) static bool xenvif_have_rx_work(struct xenvif_queue *queue) { - return (!skb_queue_empty(&queue->rx_queue) - && xenvif_rx_ring_slots_available(queue)) + return xenvif_rx_ring_slots_available(queue) || (queue->vif->stall_timeout && (xenvif_rx_queue_stalled(queue) || xenvif_rx_queue_ready(queue)))
Using the MTU or GSO size to determine the number of required guest Rx requests for an skb was subtly broken since these value may change at runtime. Signed-off-by: David Vrabel <david.vrabel@citrix.com> --- drivers/net/xen-netback/netback.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)