Re: [Xen-devel] [PATCH net v2] xen-netback: fix abuse of napi budget

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@xxxxxxxxxx]
> Sent: 10 December 2013 11:28
> To: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> Cc: Paul Durrant; Wei Liu; Ian Campbell; David Vrabel
> Subject: [PATCH net v2] xen-netback: fix abuse of napi budget
> 
> netback seemed to be somewhat confused about the napi budget
> parameter. The
> parameter is supposed to limit the number of skgs process in each poll, but
> netback had this confused with grant operations.
> 
> This patch fixes that, properly limiting the work done in each poll. Note
> that this limit makes sure we do not process any more data from the shared
> ring than we intend to pass back from the poll. This is important to
> prevent tx_queue potentially growing without bound.
> 
> This patch also changes the RING_FINAL_CHECK_FOR_REQUESTS in
> xenvif_build_tx_gops to a check for RING_HAS_UNCONSUMED_REQUESTS
> as the
> former call has the side effect of advancing the ring event pointer and
> therefore inviting another interrupt from the frontend before the napi
> poll has actually finished, thereby defeating the point of napi.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

Sorry, this patch is incomplete. I need to remove the decrement of the 
work_to_do variable in xenvif_tx_build_gops too.

  Paul

> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
> v2:
> - More elaborate description of the problem and the fix in the comment.
> - Modified the check in xenvif_build_tx_gops.
> 
>  drivers/net/xen-netback/netback.c |   17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> netback/netback.c
> index 43341b8..0c8ca76 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1351,14 +1351,15 @@ static bool tx_credit_exceeded(struct xenvif *vif,
> unsigned size)
>       return false;
>  }
> 
> -static unsigned xenvif_tx_build_gops(struct xenvif *vif)
> +static unsigned xenvif_tx_build_gops(struct xenvif *vif, int budget)
>  {
>       struct gnttab_copy *gop = vif->tx_copy_ops, *request_gop;
>       struct sk_buff *skb;
>       int ret;
> 
>       while ((nr_pending_reqs(vif) + XEN_NETBK_LEGACY_SLOTS_MAX
> -             < MAX_PENDING_REQS)) {
> +             < MAX_PENDING_REQS) &&
> +            (skb_queue_len(&vif->tx_queue) < budget)) {
>               struct xen_netif_tx_request txreq;
>               struct xen_netif_tx_request
> txfrags[XEN_NETBK_LEGACY_SLOTS_MAX];
>               struct page *page;
> @@ -1380,8 +1381,7 @@ static unsigned xenvif_tx_build_gops(struct xenvif
> *vif)
>                       continue;
>               }
> 
> -             RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx,
> work_to_do);
> -             if (!work_to_do)
> +             if (!RING_HAS_UNCONSUMED_REQUESTS(&vif->tx))
>                       break;
> 
>               idx = vif->tx.req_cons;
> @@ -1520,14 +1520,13 @@ static unsigned xenvif_tx_build_gops(struct
> xenvif *vif)
>  }
> 
> 
> -static int xenvif_tx_submit(struct xenvif *vif, int budget)
> +static int xenvif_tx_submit(struct xenvif *vif)
>  {
>       struct gnttab_copy *gop = vif->tx_copy_ops;
>       struct sk_buff *skb;
>       int work_done = 0;
> 
> -     while (work_done < budget &&
> -            (skb = __skb_dequeue(&vif->tx_queue)) != NULL) {
> +     while ((skb = __skb_dequeue(&vif->tx_queue)) != NULL) {
>               struct xen_netif_tx_request *txp;
>               u16 pending_idx;
>               unsigned data_len;
> @@ -1602,14 +1601,14 @@ int xenvif_tx_action(struct xenvif *vif, int
> budget)
>       if (unlikely(!tx_work_todo(vif)))
>               return 0;
> 
> -     nr_gops = xenvif_tx_build_gops(vif);
> +     nr_gops = xenvif_tx_build_gops(vif, budget);
> 
>       if (nr_gops == 0)
>               return 0;
> 
>       gnttab_batch_copy(vif->tx_copy_ops, nr_gops);
> 
> -     work_done = xenvif_tx_submit(vif, nr_gops);
> +     work_done = xenvif_tx_submit(vif);
> 
>       return work_done;
>  }
> --
> 1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel