[PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[David Vrabel]

From: Malcolm Crossley <malcolm.crossley@citrix.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.

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@citrix.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.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
+
+
 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);
-- 
1.7.10.4

Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[Ian Campbell]

On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
> From: Malcolm Crossley <malcolm.crossley@citrix.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@citrix.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.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);

Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[David Vrabel]

On 03/11/14 17:39, Ian Campbell wrote:
> On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
>> From: Malcolm Crossley <malcolm.crossley@citrix.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.

The network stack will always pull any headers it needs to inspect (the
frag may be a userspace page which has the same security issues as a
frag with a foreign page).

e.g., see skb_checksum_setup() called slightly later on in netback.

> 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 grant copy only copies from the first frag which may be less than
128 bytes in length.

David

Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[Ian Campbell]

On Mon, 2014-11-03 at 17:46 +0000, David Vrabel wrote:
> On 03/11/14 17:39, Ian Campbell wrote:
> > On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
> >> From: Malcolm Crossley <malcolm.crossley@citrix.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.
> 
> The network stack will always pull any headers it needs to inspect (the
> frag may be a userspace page which has the same security issues as a
> frag with a foreign page).

I don't believe it will, unless something changed since I last looked.
The kernel assumes that it has been sensible enough to put the headers
in the linear area, since it is the one which generates them in most
cases. In other cases its up to the relevant driver to make sure this is
true.

> e.g., see skb_checksum_setup() called slightly later on in netback.

This however is what will make things safe for us (note that this is
only used by xen-net* in practice), it is this which should be mentioned
in the commit message I think.

> > 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 grant copy only copies from the first frag which may be less than
> 128 bytes in length.
> 
> David

Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[Zoltan Kiss]

On 03/11/14 17:46, David Vrabel wrote:
> On 03/11/14 17:39, Ian Campbell wrote:
>> On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
>>> From: Malcolm Crossley <malcolm.crossley@citrix.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.
>
> The network stack will always pull any headers it needs to inspect (the
> frag may be a userspace page which has the same security issues as a
> frag with a foreign page).
I wouldn't bet my life on this, but indeed it should always happen.

>
> e.g., see skb_checksum_setup() called slightly later on in netback.
Fortunately the call to checksum_setup a bit later makes sure that at 
least the TCP/UDP headers are in the linear area, which is quite the 
same as blindly pulling 128 bytes. With any other protocol we rely on 
the network stack that it will enforce packet header in the linear buffer.

Regards,

Zoli

Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[David Miller]

From: Zoltan Kiss <zoltan.kiss@linaro.org>
Date: Mon, 03 Nov 2014 18:23:03 +0000

> 
> 
> On 03/11/14 17:46, David Vrabel wrote:
>> On 03/11/14 17:39, Ian Campbell wrote:
>>> On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
>>>> From: Malcolm Crossley <malcolm.crossley@citrix.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.
>>
>> The network stack will always pull any headers it needs to inspect
>> (the
>> frag may be a userspace page which has the same security issues as a
>> frag with a foreign page).
> I wouldn't bet my life on this, but indeed it should always happen.

I would bet my life on it.

Every protocol demux starts with pskb_may_pull() to pull frag data
into the linear area, if necessary, before looking at headers.

Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[Eric Dumazet]

On Tue, 2014-11-04 at 16:17 -0500, David Miller wrote:


> 
> Every protocol demux starts with pskb_may_pull() to pull frag data
> into the linear area, if necessary, before looking at headers.

eth_get_headlen() might be relevant as well, to perform a single copy of
exactly all headers.

This is known to help a bit.

Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[David Vrabel]

On 04/11/14 21:43, Eric Dumazet wrote:
> On Tue, 2014-11-04 at 16:17 -0500, David Miller wrote:
> 
> 
>>
>> Every protocol demux starts with pskb_may_pull() to pull frag data
>> into the linear area, if necessary, before looking at headers.
> 
> eth_get_headlen() might be relevant as well, to perform a single copy of
> exactly all headers.

In netback's case we need an estimate of the header length before
reading any of the packet, since peeking at any frag would prevent any
TLB flush avoidance.

It might be useful to use eth_get_headlen() to adjust the estimate at
runtime, but for now the fixed amount of 128 bytes is simple and seems
good enough.

David

Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[Ian Campbell]

On Wed, 2014-11-05 at 10:46 +0000, David Vrabel wrote:
> On 04/11/14 21:43, Eric Dumazet wrote:
> > On Tue, 2014-11-04 at 16:17 -0500, David Miller wrote:
> > 
> > 
> >>
> >> Every protocol demux starts with pskb_may_pull() to pull frag data
> >> into the linear area, if necessary, before looking at headers.
> > 
> > eth_get_headlen() might be relevant as well, to perform a single copy of
> > exactly all headers.
> 
> In netback's case we need an estimate of the header length before
> reading any of the packet, since peeking at any frag would prevent any
> TLB flush avoidance.
> 
> It might be useful to use eth_get_headlen() to adjust the estimate at
> runtime, but for now the fixed amount of 128 bytes is simple and seems
> good enough.

I think what Eric meant was that having done the 128 copy you could call
eth_get_headlen which in the common case should be a nop but would
ensure you always had the headers in the linear area for the uncommon
case.

It looks like the difference compared with skb_checksum_setup is that
eth_get_headlen deals with L4 too whereas skb_checksum_setup only goes
to L3 (and then only for some subset of protocols with checksums).

Ian.

Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[Ian Campbell]

On Tue, 2014-11-04 at 16:17 -0500, David Miller wrote:
> From: Zoltan Kiss <zoltan.kiss@linaro.org>
> Date: Mon, 03 Nov 2014 18:23:03 +0000
> 
> > 
> > 
> > On 03/11/14 17:46, David Vrabel wrote:
> >> On 03/11/14 17:39, Ian Campbell wrote:
> >>> On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
> >>>> From: Malcolm Crossley <malcolm.crossley@citrix.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.
> >>
> >> The network stack will always pull any headers it needs to inspect
> >> (the
> >> frag may be a userspace page which has the same security issues as a
> >> frag with a foreign page).
> > I wouldn't bet my life on this, but indeed it should always happen.
> 
> I would bet my life on it.
> 
> Every protocol demux starts with pskb_may_pull() to pull frag data
> into the linear area, if necessary, before looking at headers.

Then I stand corrected, I was sure this wasn't the case (but my
information could well be a decade out of date...).

Is this also true for things which hit the iptables paths? I suppose
they must necessarily have already been through the protocol demux stage
before iptables would even be able to interpret them as e.g. an IP
packet.

Ian.

Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Wed, 5 Nov 2014 09:51:20 +0000

> Is this also true for things which hit the iptables paths? I suppose
> they must necessarily have already been through the protocol demux stage
> before iptables would even be able to interpret them as e.g. an IP
> packet.

Netfilter often takes a different approach, by using
skb_header_pointer() which returns a direct pointer if the linear area
contains the requested range already, or alternatively copies from the
frags into a user supplied on-stack header buffer if not.

Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Mon, 3 Nov 2014 17:23:51 +0000

> From: Malcolm Crossley <malcolm.crossley@citrix.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.
> 
> 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@citrix.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Now that this has been discussed a bit, it is possible to get an ack or two?

Thanks.

Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[Ian Campbell]

On Tue, 2014-11-04 at 16:41 -0500, David Miller wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> Date: Mon, 3 Nov 2014 17:23:51 +0000
> 
> > From: Malcolm Crossley <malcolm.crossley@citrix.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.
> > 
> > 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@citrix.com>
> > Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Now that this has been discussed a bit, it is possible to get an ack or two?

I'd like to see the commit message expanded to explain why this isn't
introducing a (security) bug by not pulling enough stuff into the header
(IOW the conclusion of the discussion).

Ian.

Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Wed, 5 Nov 2014 09:53:05 +0000

> I'd like to see the commit message expanded to explain why this isn't
> introducing a (security) bug by not pulling enough stuff into the header
> (IOW the conclusion of the discussion).

Just because a fundamental aspect of how we handle packets isn't clear
to some people, doesn't mean that every commit that depends upon that
invariant has to explain it in the commit message. :-)