From: Jeff Kirsher <hidden> Date: 2012-09-15 20:17:09
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
packets may get corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,returnNETDEV_TX_OK;}+/* On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,+*packetsmaygetcorruptedduringpaddingbyHW.+*ToWAthisissue,padallsmallpacketsmanually.+*/+if(skb->len<ETH_ZLEN){+if(skb_pad(skb,ETH_ZLEN-skb->len))+returnNETDEV_TX_OK;+skb->len=ETH_ZLEN;+skb_set_tail_pointer(skb,ETH_ZLEN);+}+mss=skb_shinfo(skb)->gso_size;/* The controller does a simple calculation to*makesurethereisenoughroomintheFIFObefore
From: Michał Mirosław <hidden> Date: 2012-09-15 20:45:19
2012/9/15 Jeff Kirsher [off-list ref]:
quoted hunk
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
packets may get corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,returnNETDEV_TX_OK;}+/* On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,+*packetsmaygetcorruptedduringpaddingbyHW.+*ToWAthisissue,padallsmallpacketsmanually.+*/+if(skb->len<ETH_ZLEN){+if(skb_pad(skb,ETH_ZLEN-skb->len))+returnNETDEV_TX_OK;+skb->len=ETH_ZLEN;+skb_set_tail_pointer(skb,ETH_ZLEN);+}+
Isn't there a skb_padto() that does just this?
Best Regards,
Michał Mirosław
From: Dave, Tushar N <hidden> Date: 2012-09-16 01:25:47
-----Original Message-----
From: Michał Mirosław [mailto:mirqus@gmail.com]
Sent: Saturday, September 15, 2012 1:45 PM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Dave, Tushar N; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net] e1000: Small packets may get corrupted during padding
by HW
2012/9/15 Jeff Kirsher [off-list ref]:
quoted
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN, packets may get
corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
return NETDEV_TX_OK;
}
+ /* On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
+ * packets may get corrupted during padding by HW.
+ * To WA this issue, pad all small packets manually.
+ */
+ if (skb->len < ETH_ZLEN) {
+ if (skb_pad(skb, ETH_ZLEN - skb->len))
+ return NETDEV_TX_OK;
+ skb->len = ETH_ZLEN;
+ skb_set_tail_pointer(skb, ETH_ZLEN);
+ }
+
Isn't there a skb_padto() that does just this?
Skb_padto calls skb_pad(). Calling skb_pad directly saves some cycles.
From: John Fastabend <hidden> Date: 2012-09-16 01:48:35
On 9/15/2012 6:25 PM, Dave, Tushar N wrote:
quoted
-----Original Message-----
From: Michał Mirosław [mailto:mirqus@gmail.com]
Sent: Saturday, September 15, 2012 1:45 PM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Dave, Tushar N; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net] e1000: Small packets may get corrupted during padding
by HW
2012/9/15 Jeff Kirsher [off-list ref]:
quoted
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN, packets may get
corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
return NETDEV_TX_OK;
}
+ /* On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
+ * packets may get corrupted during padding by HW.
+ * To WA this issue, pad all small packets manually.
+ */
+ if (skb->len < ETH_ZLEN) {
+ if (skb_pad(skb, ETH_ZLEN - skb->len))
+ return NETDEV_TX_OK;
+ skb->len = ETH_ZLEN;
+ skb_set_tail_pointer(skb, ETH_ZLEN);
+ }
+
Isn't there a skb_padto() that does just this?
Skb_padto calls skb_pad(). Calling skb_pad directly saves some cycles.
How/where?
static inline int skb_padto(struct sk_buff *skb, unsigned int len)
{
unsigned int size = skb->len;
if (likely(size >= len))
return 0;
return skb_pad(skb, len - size);
}
Also wouldn't you want an unlikely() in your patch?
.John
From: John Fastabend <hidden> Date: 2012-09-16 02:30:53
On 9/15/2012 6:48 PM, John Fastabend wrote:
On 9/15/2012 6:25 PM, Dave, Tushar N wrote:
quoted
quoted
-----Original Message-----
From: Michał Mirosław [mailto:mirqus@gmail.com]
Sent: Saturday, September 15, 2012 1:45 PM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Dave, Tushar N; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net] e1000: Small packets may get corrupted during padding
by HW
2012/9/15 Jeff Kirsher [off-list ref]:
quoted
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN, packets may get
corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
return NETDEV_TX_OK;
}
+ /* On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
+ * packets may get corrupted during padding by HW.
+ * To WA this issue, pad all small packets manually.
+ */
+ if (skb->len < ETH_ZLEN) {
+ if (skb_pad(skb, ETH_ZLEN - skb->len))
+ return NETDEV_TX_OK;
+ skb->len = ETH_ZLEN;
+ skb_set_tail_pointer(skb, ETH_ZLEN);
+ }
+
Isn't there a skb_padto() that does just this?
Skb_padto calls skb_pad(). Calling skb_pad directly saves some cycles.
From: Dave, Tushar N <hidden> Date: 2012-09-17 07:33:14
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of John Fastabend
Sent: Saturday, September 15, 2012 6:49 PM
To: Dave, Tushar N
Cc: Michal Miroslaw; Kirsher, Jeffrey T; davem@davemloft.net;
netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net] e1000: Small packets may get corrupted during padding
by HW
On 9/15/2012 6:25 PM, Dave, Tushar N wrote:
quoted
quoted
-----Original Message-----
From: Michał Mirosław [mailto:mirqus@gmail.com]
Sent: Saturday, September 15, 2012 1:45 PM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Dave, Tushar N; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net] e1000: Small packets may get corrupted during
padding by HW
2012/9/15 Jeff Kirsher [off-list ref]:
quoted
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN, packets may
get corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
return NETDEV_TX_OK;
}
+ /* On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
+ * packets may get corrupted during padding by HW.
+ * To WA this issue, pad all small packets manually.
+ */
+ if (skb->len < ETH_ZLEN) {
+ if (skb_pad(skb, ETH_ZLEN - skb->len))
+ return NETDEV_TX_OK;
+ skb->len = ETH_ZLEN;
+ skb_set_tail_pointer(skb, ETH_ZLEN);
+ }
+
Isn't there a skb_padto() that does just this?
Skb_padto calls skb_pad(). Calling skb_pad directly saves some cycles.
How/where?
static inline int skb_padto(struct sk_buff *skb, unsigned int len) {
unsigned int size = skb->len;
if (likely(size >= len))
return 0;
return skb_pad(skb, len - size); }
Also wouldn't you want an unlikely() in your patch?
No because it is quite normal to have packet < ETH_ZLEN. e.g. ARP packets.
From: Eric Dumazet <hidden> Date: 2012-09-17 07:58:28
On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote:
quoted
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of John Fastabend
quoted
Also wouldn't you want an unlikely() in your patch?
No because it is quite normal to have packet < ETH_ZLEN. e.g. ARP packets.
ARP packets ? Hardly a performance problem.
Or make sure all these packets have enough tailroom, or else you are
going to hit the cost of reallocating packets.
I would better point TCP pure ACK packets, since their size can be 54
bytes.
@@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk)/* We are not putting this on the write queue, so*tcp_transmit_skb()willsettheownershiptothis*sock.+*Add64bytesoftailroomsothatsomedriverscanuseskb_pad()*/-buff=alloc_skb(MAX_TCP_HEADER,sk_gfp_atomic(sk,GFP_ATOMIC));+buff=alloc_skb(MAX_TCP_HEADER+64,sk_gfp_atomic(sk,GFP_ATOMIC));if(buff==NULL){inet_csk_schedule_ack(sk);inet_csk(sk)->icsk_ack.ato=TCP_ATO_MIN;
From: Dave, Tushar N <hidden> Date: 2012-09-17 16:39:29
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of David Miller
Sent: Monday, September 17, 2012 9:31 AM
To: Dave, Tushar N
Cc: Fastabend, John R; mirqus@gmail.com; Kirsher, Jeffrey T;
netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net] e1000: Small packets may get corrupted during padding
by HW
From: "Dave, Tushar N" <redacted>
Date: Mon, 17 Sep 2012 07:33:12 +0000
quoted
No because it is quite normal to have packet < ETH_ZLEN. e.g. ARP
packets.
You're optimizing for ARP packets? You're kidding right?
ARP packet was just an example. I should have thought of better example.
From: Alexander Duyck <hidden> Date: 2012-09-17 19:41:19
On 09/15/2012 01:44 PM, Michał Mirosław wrote:
2012/9/15 Jeff Kirsher [off-list ref]:
quoted
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
packets may get corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,returnNETDEV_TX_OK;}+/* On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,+*packetsmaygetcorruptedduringpaddingbyHW.+*ToWAthisissue,padallsmallpacketsmanually.+*/+if(skb->len<ETH_ZLEN){+if(skb_pad(skb,ETH_ZLEN-skb->len))+returnNETDEV_TX_OK;+skb->len=ETH_ZLEN;+skb_set_tail_pointer(skb,ETH_ZLEN);+}+
Isn't there a skb_padto() that does just this?
Best Regards,
Michał Mirosław
The problem is skb_padto() doesn't update the packet length, it just
adds the padding but doesn't do anything to account for it.
Thanks,
Alex
From: Alexander Duyck <hidden> Date: 2012-09-17 20:53:57
On 09/17/2012 12:58 AM, Eric Dumazet wrote:
quoted hunk
On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote:
quoted
quoted
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of John Fastabend
Also wouldn't you want an unlikely() in your patch?
No because it is quite normal to have packet < ETH_ZLEN. e.g. ARP packets.
ARP packets ? Hardly a performance problem.
Or make sure all these packets have enough tailroom, or else you are
going to hit the cost of reallocating packets.
I would better point TCP pure ACK packets, since their size can be 54
bytes.
@@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk)/* We are not putting this on the write queue, so*tcp_transmit_skb()willsettheownershiptothis*sock.+*Add64bytesoftailroomsothatsomedriverscanuseskb_pad()*/-buff=alloc_skb(MAX_TCP_HEADER,sk_gfp_atomic(sk,GFP_ATOMIC));+buff=alloc_skb(MAX_TCP_HEADER+64,sk_gfp_atomic(sk,GFP_ATOMIC));if(buff==NULL){inet_csk_schedule_ack(sk);inet_csk(sk)->icsk_ack.ato=TCP_ATO_MIN;
For most systems that extra padding should already be added since
alloc_skb will cache line align the buffer anyway.
A more general fix might be to make it so that alloc_skb cannot allocate
less than 60 byte buffers on systems with a cache line size smaller than
64 bytes.
Thanks,
Alex
From: Eric Dumazet <hidden> Date: 2012-09-17 21:02:09
On Mon, 2012-09-17 at 13:53 -0700, Alexander Duyck wrote:
On 09/17/2012 12:58 AM, Eric Dumazet wrote:
quoted
On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote:
quoted
quoted
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of John Fastabend
Also wouldn't you want an unlikely() in your patch?
No because it is quite normal to have packet < ETH_ZLEN. e.g. ARP packets.
ARP packets ? Hardly a performance problem.
Or make sure all these packets have enough tailroom, or else you are
going to hit the cost of reallocating packets.
I would better point TCP pure ACK packets, since their size can be 54
bytes.
@@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk)/* We are not putting this on the write queue, so*tcp_transmit_skb()willsettheownershiptothis*sock.+*Add64bytesoftailroomsothatsomedriverscanuseskb_pad()*/-buff=alloc_skb(MAX_TCP_HEADER,sk_gfp_atomic(sk,GFP_ATOMIC));+buff=alloc_skb(MAX_TCP_HEADER+64,sk_gfp_atomic(sk,GFP_ATOMIC));if(buff==NULL){inet_csk_schedule_ack(sk);inet_csk(sk)->icsk_ack.ato=TCP_ATO_MIN;
For most systems that extra padding should already be added since
alloc_skb will cache line align the buffer anyway.
Please define 'most systems' ?
A more general fix might be to make it so that alloc_skb cannot allocate
less than 60 byte buffers on systems with a cache line size smaller than
64 bytes.
Nope, because we do a skb_reserve(skb, MAX_TCP_HEADER)
So we might have no bytes available at all after this MAX_TCP_HEADER
area.
Relying on extra padding in alloc_skb() is hacky anyway, as it
depends on external factors (external to TCP stack)
From: Alexander Duyck <hidden> Date: 2012-09-18 03:01:16
On 9/17/2012 2:02 PM, Eric Dumazet wrote:
On Mon, 2012-09-17 at 13:53 -0700, Alexander Duyck wrote:
quoted
On 09/17/2012 12:58 AM, Eric Dumazet wrote:
quoted
On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote:
quoted
quoted
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of John Fastabend
Also wouldn't you want an unlikely() in your patch?
No because it is quite normal to have packet < ETH_ZLEN. e.g. ARP packets.
ARP packets ? Hardly a performance problem.
Or make sure all these packets have enough tailroom, or else you are
going to hit the cost of reallocating packets.
I would better point TCP pure ACK packets, since their size can be 54
bytes.
@@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk)/* We are not putting this on the write queue, so*tcp_transmit_skb()willsettheownershiptothis*sock.+*Add64bytesoftailroomsothatsomedriverscanuseskb_pad()*/-buff=alloc_skb(MAX_TCP_HEADER,sk_gfp_atomic(sk,GFP_ATOMIC));+buff=alloc_skb(MAX_TCP_HEADER+64,sk_gfp_atomic(sk,GFP_ATOMIC));if(buff==NULL){inet_csk_schedule_ack(sk);inet_csk(sk)->icsk_ack.ato=TCP_ATO_MIN;
For most systems that extra padding should already be added since
alloc_skb will cache line align the buffer anyway.
Please define 'most systems' ?
Sorry I misspoke. What I meant to say is that the allocation will be
aligned to a slab size. If you take a look at alloc_skb it looks like
it is still using __alloc_skb so it is going to add skb_shared_info to
the size so at least in the case of most 64 bit systems the total
allocation size is going to be larger than 512 and as a result skb->head
will be allocated from a 1K slab cache leaving plenty of room for
padding to be added later. On 32 bit systems the total size will likely
be a little over 256 and get rounded up to 512.
The only real thing that bugged me about this is that you were adding 64
when the most you should ever need is 10. That was the only real reason
I felt like commenting on it.
quoted
A more general fix might be to make it so that alloc_skb cannot allocate
less than 60 byte buffers on systems with a cache line size smaller than
64 bytes.
Nope, because we do a skb_reserve(skb, MAX_TCP_HEADER)
So we might have no bytes available at all after this MAX_TCP_HEADER
area.
Relying on extra padding in alloc_skb() is hacky anyway, as it
depends on external factors (external to TCP stack)
That is true, but the fact is there is probably a fair amount of that
going on without people even realizing it. As I recall the smallest skb
head you can allocate on a 64 bit system currently is something like
128 bytes which comes from the 512 byte slab, the next step up after
that is a 640 byte head. Since MAX_TCP_HEADER starts at 160 the
likelihood of it not getting at least 16 bytes of padding is pretty low.
Thanks,
Alex
Since MAX_TCP_HEADER starts at 160 the likelihood of it not getting
at least 16 bytes of padding is pretty low.
I know it's not on many people's radar, but with SLOB it will happen
a lot probably.
That is true. I hadn't thought about anything other than SLAB/SLUB.
It also just occurred to me that there might be some benefit in cache
aligning the max header size. It seems like doing something like that
should reduce the overall memory footprint and would probably improve
performance.
Thanks,
Alex
From: Eric Dumazet <hidden> Date: 2012-09-18 05:45:26
On Mon, 2012-09-17 at 20:27 -0700, Alexander Duyck wrote:
It also just occurred to me that there might be some benefit in cache
aligning the max header size. It seems like doing something like that
should reduce the overall memory footprint and would probably improve
performance.
Given that most ACK packets are 66 bytes (14 ethernet + 20 IP + 32 TCP),
I am not sure we need to make any tweak on alignment ?
From: Alexander Duyck <hidden> Date: 2012-09-18 05:55:54
On 9/17/2012 10:45 PM, Eric Dumazet wrote:
On Mon, 2012-09-17 at 20:27 -0700, Alexander Duyck wrote:
quoted
It also just occurred to me that there might be some benefit in cache
aligning the max header size. It seems like doing something like that
should reduce the overall memory footprint and would probably improve
performance.
Given that most ACK packets are 66 bytes (14 ethernet + 20 IP + 32 TCP),
I am not sure we need to make any tweak on alignment ?
I'm honestly not sure myself. I will probably spend a few hours
tomorrow tweaking a few things to test and see if there is any gain to
be had there. The only reason why it occurred to me is that it really
isn't too far off from what we did back on the Rx side, except for there
we were aligning at the start of the buffer and working our way up.
Thanks,
Alex
From: Tushar Dave <redacted>
On PCI/PCI-X HW, if packet size is less than ETH_ZLEN,
packets may get corrupted during padding by HW.
To WA this issue, pad all small packets manually.
Signed-off-by: Tushar Dave <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
There has been a lot of bike shedding on this patch, but the fix is
correct and thus I'm going to apply it to 'net'.
Thanks.