netdev - Re: [PATCH net] xen-netback: fix abuse of napi budget

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1386671136.11925.46.camel@kazak.uk.xensource.com>
Date:	Tue, 10 Dec 2013 10:25:36 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	Paul Durrant <paul.durrant@...rix.com>
CC:	<xen-devel@...ts.xen.org>, <netdev@...r.kernel.org>,
	Wei Liu <wei.liu2@...rix.com>,
	David Vrabel <david.vrabel@...rix.com>
Subject: Re: [PATCH net] xen-netback: fix abuse of napi budget

On Tue, 2013-12-10 at 10:16 +0000, Paul Durrant wrote:
> netback seemed to be somewhat confused about the napi budget parameter and
> basically ignored it. This patch fixes that, properly limiting the work done
> in each poll.

What do you mean "ignored", xenvif_tx_submit seems to be tracking and
testing work_done against the budget.

I suspect this change is probably worthwhile but it would be good to get
an accurate description of why, which I presume is because the tx
process is xenvif_tx_build_gops followed by, gnttab_batch_copy then
xenvif_tx_submit and that it is better to do the budget enforcement
earlier on.

How does this change impact the batching in gnttab_batch_copy and
therefore performance? Do we need to tweak the the NAPI budget to ensure
we are getting good batching? I suspect that netback is a bit unusual
among NIC drivers in that the rx path contains a fair bit of actual work
to do, so perhaps the NAPI defaults are not necessarily going to be the
best for it.

> 
> Signed-off-by: Paul Durrant <paul.durrant@...rix.com>
> Cc: Wei Liu <wei.liu2@...rix.com>
> Cc: Ian Campbell <ian.campbell@...rix.com>
> Cc: David Vrabel <david.vrabel@...rix.com>
> ---
>  drivers/net/xen-netback/netback.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 43341b8..83b4e5b 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;
> @@ -1520,14 +1521,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 +1602,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;
>  }


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