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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1415036346.1411.3.camel@citrix.com>
Date:	Mon, 3 Nov 2014 17:39:06 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	David Vrabel <david.vrabel@...rix.com>
CC:	<netdev@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
	Wei Liu <wei.liu2@...rix.com>,
	Malcolm Crossley <malcolm.crossley@...rix.com>
Subject: Re: [PATCHv1 net-next] xen-netback: remove unconditional
 pull_skb_tail in guest Tx path

On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
> From: Malcolm Crossley <malcolm.crossley@...rix.com>
> 
> Unconditionally pulling 128 bytes into the linear buffer is not
> required. Netback has already grant copied up-to 128 bytes from the
> first slot of a packet into the linear buffer. The first slot normally
> contain all the IPv4/IPv6 and TCP/UDP headers.

What about when it doesn't? It sounds as if we now won't pull up, which
would be bad.

To avoid the pull up the code would need to grant copy up-to 128 bytes
from as many slots as needed, not only the first.

Also, if the grant copy has already placed 128 bytes in the linear area,
why is the pull up touching anything in the first place? Shouldn't it be
a nop in that case?

> The unconditional pull would often copy frag data unnecessarily.  This
> is a performance problem when running on a version of Xen where grant
> unmap avoids TLB flushes for pages which are not accessed.  TLB
> flushes can now be avoided for > 99% of unmaps (it was 0% before).
> 
> Grant unmap TLB flush avoidance will be available in a future version
> of Xen (probably 4.6).
> 
> Signed-off-by: Malcolm Crossley <malcolm.crossley@...rix.com>
> Signed-off-by: David Vrabel <david.vrabel@...rix.com>
> ---
>  drivers/net/xen-netback/netback.c |   26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 730252c..14e18bb 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -82,6 +82,16 @@ MODULE_PARM_DESC(max_queues,
>  static unsigned int fatal_skb_slots = FATAL_SKB_SLOTS_DEFAULT;
>  module_param(fatal_skb_slots, uint, 0444);
>  
> +/* The amount to copy out of the first guest Tx slot into the skb's
> + * linear area.  If the first slot has more data, it will be mapped
> + * and put into the first frag.
> + *
> + * This is sized to avoid pulling headers from the frags for most
> + * TCP/IP packets.
> + */
> +#define XEN_NETBACK_TX_COPY_LEN 128

Was it strictly necessary to both move and rename this? I can see why
the comment needed to change.

>  static void xenvif_idx_release(struct xenvif_queue *queue, u16 pending_idx,
>  			       u8 status);
>  
> @@ -125,13 +135,6 @@ static inline struct xenvif_queue *ubuf_to_queue(const struct ubuf_info *ubuf)
>  			    pending_tx_info[0]);
>  }
>  
> -/* This is a miniumum size for the linear area to avoid lots of
> - * calls to __pskb_pull_tail() as we set up checksum offsets. The
> - * value 128 was chosen as it covers all IPv4 and most likely
> - * IPv6 headers.
> - */
> -#define PKT_PROT_LEN 128
> -
>  static u16 frag_get_pending_idx(skb_frag_t *frag)
>  {
>  	return (u16)frag->page_offset;
> @@ -1446,9 +1449,9 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
>  		index = pending_index(queue->pending_cons);
>  		pending_idx = queue->pending_ring[index];
>  
> -		data_len = (txreq.size > PKT_PROT_LEN &&
> +		data_len = (txreq.size > XEN_NETBACK_TX_COPY_LEN &&
>  			    ret < XEN_NETBK_LEGACY_SLOTS_MAX) ?
> -			PKT_PROT_LEN : txreq.size;
> +			XEN_NETBACK_TX_COPY_LEN : txreq.size;
>  
>  		skb = xenvif_alloc_skb(data_len);
>  		if (unlikely(skb == NULL)) {
> @@ -1653,11 +1656,6 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
>  			}
>  		}
>  
> -		if (skb_is_nonlinear(skb) && skb_headlen(skb) < PKT_PROT_LEN) {
> -			int target = min_t(int, skb->len, PKT_PROT_LEN);
> -			__pskb_pull_tail(skb, target - skb_headlen(skb));
> -		}
> -
>  		skb->dev      = queue->vif->dev;
>  		skb->protocol = eth_type_trans(skb, skb->dev);
>  		skb_reset_network_header(skb);


--
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ