[PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[David Vrabel]

From: Malcolm Crossley <malcolm.crossley@citrix.com>

Unconditionally pulling 128 bytes into the linear area is not required
for:

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

- performance: Netback has already grant copied up-to 128 bytes from
  the first slot of a packet into the linear area. 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: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Ian Campbell]

On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> From: Malcolm Crossley <malcolm.crossley@citrix.com>
> 
> Unconditionally pulling 128 bytes into the linear area is not required
> for:
> 
> - security: Every protocol demux starts with pskb_may_pull() to pull
>   frag data into the linear area, if necessary, before looking at
>   headers.
> 
> - performance: Netback has already grant copied up-to 128 bytes from
>   the first slot of a packet into the linear area. The first slot
>   normally contain all the IPv4/IPv6 and TCP/UDP headers.

Thanks for adding these.

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

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Ian Campbell]

Dropping netdev since this isn't relevant to them, adding Paul

On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> - performance: Netback has already grant copied up-to 128 bytes from
>   the first slot of a packet into the linear area. The first slot
>   normally contain all the IPv4/IPv6 and TCP/UDP headers.

Does "normally" here include guests other than Linux? I thought Windows
in particular was prone to splitting the headers into a frag per layer
or thereabouts?

Ian.

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Ian Campbell]

Dropping netdev since this isn't relevant to them, adding Paul

On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> - performance: Netback has already grant copied up-to 128 bytes from
>   the first slot of a packet into the linear area. The first slot
>   normally contain all the IPv4/IPv6 and TCP/UDP headers.

Does "normally" include guests other than Linux? I thought Windows in
particular was prone to splitting the headers into a frag per layer or
thereabouts?

Ian

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[David Vrabel]

On 05/11/14 10:58, Ian Campbell wrote:
> Dropping netdev since this isn't relevant to them, adding Paul
> 
> On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
>> - performance: Netback has already grant copied up-to 128 bytes from
>>   the first slot of a packet into the linear area. The first slot
>>   normally contain all the IPv4/IPv6 and TCP/UDP headers.
> 
> Does "normally" here include guests other than Linux? I thought Windows
> in particular was prone to splitting the headers into a frag per layer
> or thereabouts?

I'm not sure what guests Malcolm tested with.

David

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 05 November 2014 11:00
> To: David Vrabel
> Cc: xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley; Paul Durrant
> Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> __pskb_pull_tail() in guest Tx path
> 
> Dropping netdev since this isn't relevant to them, adding Paul
> 
> On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> > - performance: Netback has already grant copied up-to 128 bytes from
> >   the first slot of a packet into the linear area. The first slot
> >   normally contain all the IPv4/IPv6 and TCP/UDP headers.
> 
> Does "normally" include guests other than Linux? I thought Windows in
> particular was prone to splitting the headers into a frag per layer or
> thereabouts?
> 

Current upstream Windows PV drivers will put all parsed headers in the first frag and the rest of the packet in subsequent flags. The parser currently knows about TCP and UDP over IPv4 or v6, with and without SNAP encapsulation. It doesn't, for example, know about ARP so the backend will see only the ethernet header in the first frag.

  Paul

> Ian
> 

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Ian Campbell]

On Wed, 2014-11-05 at 11:17 +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 05 November 2014 11:00
> > To: David Vrabel
> > Cc: xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley; Paul Durrant
> > Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> > __pskb_pull_tail() in guest Tx path
> > 
> > Dropping netdev since this isn't relevant to them, adding Paul
> > 
> > On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> > > - performance: Netback has already grant copied up-to 128 bytes from
> > >   the first slot of a packet into the linear area. The first slot
> > >   normally contain all the IPv4/IPv6 and TCP/UDP headers.
> > 
> > Does "normally" include guests other than Linux? I thought Windows in
> > particular was prone to splitting the headers into a frag per layer or
> > thereabouts?
> > 
> 
> Current upstream Windows PV drivers will put all parsed headers in the
> first frag and the rest of the packet in subsequent flags. The parser
> currently knows about TCP and UDP over IPv4 or v6, with and without
> SNAP encapsulation. It doesn't, for example, know about ARP so the
> backend will see only the ethernet header in the first frag.

Sounds like that is sufficient to reach the "normally" qualification,
thanks.

(I wonder what sort of benefit parsing arp would bring...)

Ian.

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 05 November 2014 11:20
> To: Paul Durrant
> Cc: David Vrabel; xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley
> Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> __pskb_pull_tail() in guest Tx path
> 
> On Wed, 2014-11-05 at 11:17 +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Ian Campbell
> > > Sent: 05 November 2014 11:00
> > > To: David Vrabel
> > > Cc: xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley; Paul
> Durrant
> > > Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> > > __pskb_pull_tail() in guest Tx path
> > >
> > > Dropping netdev since this isn't relevant to them, adding Paul
> > >
> > > On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> > > > - performance: Netback has already grant copied up-to 128 bytes from
> > > >   the first slot of a packet into the linear area. The first slot
> > > >   normally contain all the IPv4/IPv6 and TCP/UDP headers.
> > >
> > > Does "normally" include guests other than Linux? I thought Windows in
> > > particular was prone to splitting the headers into a frag per layer or
> > > thereabouts?
> > >
> >
> > Current upstream Windows PV drivers will put all parsed headers in the
> > first frag and the rest of the packet in subsequent flags. The parser
> > currently knows about TCP and UDP over IPv4 or v6, with and without
> > SNAP encapsulation. It doesn't, for example, know about ARP so the
> > backend will see only the ethernet header in the first frag.
> 
> Sounds like that is sufficient to reach the "normally" qualification,
> thanks.
> 
> (I wonder what sort of benefit parsing arp would bring...)
> 

Previous versions of the drivers used to parse ARPs so that copies of outgoing gratuitous ARPs could be cached for replay after migrate. Newer drivers acquire IP address bindings from the Windows IP helper (which is now available in kernel) and synthesize ARPs/IPv6 neighbour solicitations.

  Paul

> Ian.

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Konrad Rzeszutek Wilk]

On Wed, Nov 05, 2014 at 11:24:16AM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 05 November 2014 11:20
> > To: Paul Durrant
> > Cc: David Vrabel; xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley
> > Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> > __pskb_pull_tail() in guest Tx path
> > 
> > On Wed, 2014-11-05 at 11:17 +0000, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Ian Campbell
> > > > Sent: 05 November 2014 11:00
> > > > To: David Vrabel
> > > > Cc: xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley; Paul
> > Durrant
> > > > Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> > > > __pskb_pull_tail() in guest Tx path
> > > >
> > > > Dropping netdev since this isn't relevant to them, adding Paul
> > > >
> > > > On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> > > > > - performance: Netback has already grant copied up-to 128 bytes from
> > > > >   the first slot of a packet into the linear area. The first slot
> > > > >   normally contain all the IPv4/IPv6 and TCP/UDP headers.
> > > >
> > > > Does "normally" include guests other than Linux? I thought Windows in
> > > > particular was prone to splitting the headers into a frag per layer or
> > > > thereabouts?
> > > >
> > >
> > > Current upstream Windows PV drivers will put all parsed headers in the
> > > first frag and the rest of the packet in subsequent flags. The parser
> > > currently knows about TCP and UDP over IPv4 or v6, with and without
> > > SNAP encapsulation. It doesn't, for example, know about ARP so the
> > > backend will see only the ethernet header in the first frag.
> > 
> > Sounds like that is sufficient to reach the "normally" qualification,
> > thanks.
> > 
> > (I wonder what sort of benefit parsing arp would bring...)
> > 
> 
> Previous versions of the drivers used to parse ARPs so that copies of outgoing gratuitous ARPs could be cached for replay after migrate. Newer drivers acquire IP address bindings from the Windows IP helper (which is now available in kernel) and synthesize ARPs/IPv6 neighbour solicitations.
> 

CC-ing Annie and James - the other two Windows drivers authors..
>   Paul
> 
> > Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[annie li]

On 2014/11/5 6:24, Paul Durrant wrote:
>> -----Original Message-----
>> From: Ian Campbell
>> Sent: 05 November 2014 11:20
>> To: Paul Durrant
>> Cc: David Vrabel; xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley
>> Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
>> __pskb_pull_tail() in guest Tx path
>>
>> On Wed, 2014-11-05 at 11:17 +0000, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Ian Campbell
>>>> Sent: 05 November 2014 11:00
>>>> To: David Vrabel
>>>> Cc: xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley; Paul
>> Durrant
>>>> Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
>>>> __pskb_pull_tail() in guest Tx path
>>>>
>>>> Dropping netdev since this isn't relevant to them, adding Paul
>>>>
>>>> On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
>>>>> - performance: Netback has already grant copied up-to 128 bytes from
>>>>>    the first slot of a packet into the linear area. The first slot
>>>>>    normally contain all the IPv4/IPv6 and TCP/UDP headers.
>>>> Does "normally" include guests other than Linux? I thought Windows in
>>>> particular was prone to splitting the headers into a frag per layer or
>>>> thereabouts?
>>>>
>>> Current upstream Windows PV drivers will put all parsed headers in the
>>> first frag and the rest of the packet in subsequent flags. The parser
>>> currently knows about TCP and UDP over IPv4 or v6, with and without
>>> SNAP encapsulation. It doesn't, for example, know about ARP so the
>>> backend will see only the ethernet header in the first frag.
>> Sounds like that is sufficient to reach the "normally" qualification,
>> thanks.
>>
>> (I wonder what sort of benefit parsing arp would bring...)
>>
> Previous versions of the drivers used to parse ARPs so that copies of outgoing gratuitous ARPs could be cached for replay after migrate. Newer drivers acquire IP address bindings from the Windows IP helper (which is now available in kernel) and synthesize ARPs/IPv6 neighbour solicitations.
I send out fake ARP after migration to deal with it, guess you are 
mentioning here 
http://msdn.microsoft.com/en-us/library/windows/hardware/ff557015(v=vs.85).aspx 
? which is available for kernel later than Vista.

Thanks
Annie

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[Paul Durrant]

> -----Original Message-----
> From: annie li [mailto:annie.li@oracle.com]
> Sent: 05 November 2014 19:02
> To: Paul Durrant
> Cc: Ian Campbell; xen-devel@lists.xenproject.org; Malcolm Crossley; Wei Liu;
> David Vrabel
> Subject: Re: [Xen-devel] [PATCHv2 net-next] xen-netback: remove
> unconditional __pskb_pull_tail() in guest Tx path
> 
> 
> On 2014/11/5 6:24, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Ian Campbell
> >> Sent: 05 November 2014 11:20
> >> To: Paul Durrant
> >> Cc: David Vrabel; xen-devel@lists.xenproject.org; Wei Liu; Malcolm
> Crossley
> >> Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> >> __pskb_pull_tail() in guest Tx path
> >>
> >> On Wed, 2014-11-05 at 11:17 +0000, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Ian Campbell
> >>>> Sent: 05 November 2014 11:00
> >>>> To: David Vrabel
> >>>> Cc: xen-devel@lists.xenproject.org; Wei Liu; Malcolm Crossley; Paul
> >> Durrant
> >>>> Subject: Re: [PATCHv2 net-next] xen-netback: remove unconditional
> >>>> __pskb_pull_tail() in guest Tx path
> >>>>
> >>>> Dropping netdev since this isn't relevant to them, adding Paul
> >>>>
> >>>> On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> >>>>> - performance: Netback has already grant copied up-to 128 bytes
> from
> >>>>>    the first slot of a packet into the linear area. The first slot
> >>>>>    normally contain all the IPv4/IPv6 and TCP/UDP headers.
> >>>> Does "normally" include guests other than Linux? I thought Windows in
> >>>> particular was prone to splitting the headers into a frag per layer or
> >>>> thereabouts?
> >>>>
> >>> Current upstream Windows PV drivers will put all parsed headers in the
> >>> first frag and the rest of the packet in subsequent flags. The parser
> >>> currently knows about TCP and UDP over IPv4 or v6, with and without
> >>> SNAP encapsulation. It doesn't, for example, know about ARP so the
> >>> backend will see only the ethernet header in the first frag.
> >> Sounds like that is sufficient to reach the "normally" qualification,
> >> thanks.
> >>
> >> (I wonder what sort of benefit parsing arp would bring...)
> >>
> > Previous versions of the drivers used to parse ARPs so that copies of
> outgoing gratuitous ARPs could be cached for replay after migrate. Newer
> drivers acquire IP address bindings from the Windows IP helper (which is
> now available in kernel) and synthesize ARPs/IPv6 neighbour solicitations.
> I send out fake ARP after migration to deal with it, guess you are
> mentioning here
> http://msdn.microsoft.com/en-
> us/library/windows/hardware/ff557015(v=vs.85).aspx
> ? which is available for kernel later than Vista.
> 

Yes. The upstream PV drivers are Vista+ only.

  Paul

> Thanks
> Annie

Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Wed, 5 Nov 2014 10:50:22 +0000

> From: Malcolm Crossley <malcolm.crossley@citrix.com>
> 
> Unconditionally pulling 128 bytes into the linear area is not required
> for:
> 
> - security: Every protocol demux starts with pskb_may_pull() to pull
>   frag data into the linear area, if necessary, before looking at
>   headers.
> 
> - performance: Netback has already grant copied up-to 128 bytes from
>   the first slot of a packet into the linear area. 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>

Applied, thanks.