* [net] e1000: Small packets may get corrupted during padding by HW @ 2012-09-15 20:16 Jeff Kirsher 2012-09-15 20:44 ` Michał Mirosław 2012-09-18 20:33 ` David Miller 0 siblings, 2 replies; 18+ messages in thread From: Jeff Kirsher @ 2012-09-15 20:16 UTC (permalink / raw) To: davem; +Cc: Tushar Dave, netdev, gospo, sassmann, Jeff Kirsher From: Tushar Dave <tushar.n.dave@intel.com> 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 <tushar.n.dave@intel.com> Tested-by: Aaron Brown <aaron.f.brown@intel.com> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> --- drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c index 3bfbb8d..bde337e 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_main.c +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c @@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, 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); + } + mss = skb_shinfo(skb)->gso_size; /* The controller does a simple calculation to * make sure there is enough room in the FIFO before -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-15 20:16 [net] e1000: Small packets may get corrupted during padding by HW Jeff Kirsher @ 2012-09-15 20:44 ` Michał Mirosław 2012-09-16 1:25 ` Dave, Tushar N 2012-09-17 19:40 ` Alexander Duyck 2012-09-18 20:33 ` David Miller 1 sibling, 2 replies; 18+ messages in thread From: Michał Mirosław @ 2012-09-15 20:44 UTC (permalink / raw) To: Jeff Kirsher; +Cc: davem, Tushar Dave, netdev, gospo, sassmann 2012/9/15 Jeff Kirsher <jeffrey.t.kirsher@intel.com>: > From: Tushar Dave <tushar.n.dave@intel.com> > > 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 <tushar.n.dave@intel.com> > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c > index 3bfbb8d..bde337e 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > @@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, > 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? Best Regards, Michał Mirosław ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-15 20:44 ` Michał Mirosław @ 2012-09-16 1:25 ` Dave, Tushar N 2012-09-16 1:48 ` John Fastabend 2012-09-17 19:40 ` Alexander Duyck 1 sibling, 1 reply; 18+ messages in thread From: Dave, Tushar N @ 2012-09-16 1:25 UTC (permalink / raw) To: Michal Miroslaw, Kirsher, Jeffrey T Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com >-----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 <jeffrey.t.kirsher@intel.com>: >> From: Tushar Dave <tushar.n.dave@intel.com> >> >> 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 <tushar.n.dave@intel.com> >> Tested-by: Aaron Brown <aaron.f.brown@intel.com> >> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> --- >> drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c >> b/drivers/net/ethernet/intel/e1000/e1000_main.c >> index 3bfbb8d..bde337e 100644 >> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >> @@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct >sk_buff *skb, >> 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-16 1:25 ` Dave, Tushar N @ 2012-09-16 1:48 ` John Fastabend 2012-09-16 2:30 ` John Fastabend 2012-09-17 7:33 ` Dave, Tushar N 0 siblings, 2 replies; 18+ messages in thread From: John Fastabend @ 2012-09-16 1:48 UTC (permalink / raw) To: Dave, Tushar N Cc: Michal Miroslaw, Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com On 9/15/2012 6:25 PM, Dave, Tushar N wrote: >> -----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 <jeffrey.t.kirsher@intel.com>: >>> From: Tushar Dave <tushar.n.dave@intel.com> >>> >>> 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 <tushar.n.dave@intel.com> >>> Tested-by: Aaron Brown <aaron.f.brown@intel.com> >>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >>> --- >>> drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c >>> b/drivers/net/ethernet/intel/e1000/e1000_main.c >>> index 3bfbb8d..bde337e 100644 >>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >>> @@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct >> sk_buff *skb, >>> 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-16 1:48 ` John Fastabend @ 2012-09-16 2:30 ` John Fastabend 2012-09-17 7:33 ` Dave, Tushar N 1 sibling, 0 replies; 18+ messages in thread From: John Fastabend @ 2012-09-16 2:30 UTC (permalink / raw) To: Dave, Tushar N Cc: Michal Miroslaw, Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com On 9/15/2012 6:48 PM, John Fastabend wrote: > On 9/15/2012 6:25 PM, Dave, Tushar N wrote: >>> -----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 <jeffrey.t.kirsher@intel.com>: >>>> From: Tushar Dave <tushar.n.dave@intel.com> >>>> >>>> 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 <tushar.n.dave@intel.com> >>>> Tested-by: Aaron Brown <aaron.f.brown@intel.com> >>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >>>> --- >>>> drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> b/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> index 3bfbb8d..bde337e 100644 >>>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> @@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct >>> sk_buff *skb, >>>> 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? > OK maybe you avoid an if case. ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-16 1:48 ` John Fastabend 2012-09-16 2:30 ` John Fastabend @ 2012-09-17 7:33 ` Dave, Tushar N 2012-09-17 7:58 ` Eric Dumazet 2012-09-17 16:31 ` David Miller 1 sibling, 2 replies; 18+ messages in thread From: Dave, Tushar N @ 2012-09-17 7:33 UTC (permalink / raw) To: Fastabend, John R Cc: Michal Miroslaw, Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com >-----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: >>> -----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 <jeffrey.t.kirsher@intel.com>: >>>> From: Tushar Dave <tushar.n.dave@intel.com> >>>> >>>> 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 <tushar.n.dave@intel.com> >>>> Tested-by: Aaron Brown <aaron.f.brown@intel.com> >>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >>>> --- >>>> drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> b/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> index 3bfbb8d..bde337e 100644 >>>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >>>> @@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct >>> sk_buff *skb, >>>> 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-17 7:33 ` Dave, Tushar N @ 2012-09-17 7:58 ` Eric Dumazet 2012-09-17 20:53 ` Alexander Duyck 2012-09-17 16:31 ` David Miller 1 sibling, 1 reply; 18+ messages in thread From: Eric Dumazet @ 2012-09-17 7:58 UTC (permalink / raw) To: Dave, Tushar N Cc: Fastabend, John R, Michal Miroslaw, Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote: > >-----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. diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index cfe6ffe..aefc681 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk) /* We are not putting this on the write queue, so * tcp_transmit_skb() will set the ownership to this * sock. + * Add 64 bytes of tailroom so that some drivers can use skb_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; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-17 7:58 ` Eric Dumazet @ 2012-09-17 20:53 ` Alexander Duyck 2012-09-17 21:02 ` Eric Dumazet 0 siblings, 1 reply; 18+ messages in thread From: Alexander Duyck @ 2012-09-17 20:53 UTC (permalink / raw) To: Eric Dumazet Cc: Dave, Tushar N, Fastabend, John R, Michal Miroslaw, Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com On 09/17/2012 12:58 AM, Eric Dumazet wrote: > On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote: >>> -----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. > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index cfe6ffe..aefc681 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk) > /* We are not putting this on the write queue, so > * tcp_transmit_skb() will set the ownership to this > * sock. > + * Add 64 bytes of tailroom so that some drivers can use skb_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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-17 20:53 ` Alexander Duyck @ 2012-09-17 21:02 ` Eric Dumazet 2012-09-18 3:01 ` Alexander Duyck 0 siblings, 1 reply; 18+ messages in thread From: Eric Dumazet @ 2012-09-17 21:02 UTC (permalink / raw) To: Alexander Duyck Cc: Dave, Tushar N, Fastabend, John R, Michal Miroslaw, Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com On Mon, 2012-09-17 at 13:53 -0700, Alexander Duyck wrote: > On 09/17/2012 12:58 AM, Eric Dumazet wrote: > > On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote: > >>> -----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. > > > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > > index cfe6ffe..aefc681 100644 > > --- a/net/ipv4/tcp_output.c > > +++ b/net/ipv4/tcp_output.c > > @@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk) > > /* We are not putting this on the write queue, so > > * tcp_transmit_skb() will set the ownership to this > > * sock. > > + * Add 64 bytes of tailroom so that some drivers can use skb_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) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-17 21:02 ` Eric Dumazet @ 2012-09-18 3:01 ` Alexander Duyck 2012-09-18 3:03 ` David Miller 0 siblings, 1 reply; 18+ messages in thread From: Alexander Duyck @ 2012-09-18 3:01 UTC (permalink / raw) To: Eric Dumazet Cc: Alexander Duyck, Dave, Tushar N, Fastabend, John R, Michal Miroslaw, Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com On 9/17/2012 2:02 PM, Eric Dumazet wrote: > On Mon, 2012-09-17 at 13:53 -0700, Alexander Duyck wrote: >> On 09/17/2012 12:58 AM, Eric Dumazet wrote: >>> On Mon, 2012-09-17 at 07:33 +0000, Dave, Tushar N wrote: >>>>> -----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. >>> >>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c >>> index cfe6ffe..aefc681 100644 >>> --- a/net/ipv4/tcp_output.c >>> +++ b/net/ipv4/tcp_output.c >>> @@ -3083,8 +3083,9 @@ void tcp_send_ack(struct sock *sk) >>> /* We are not putting this on the write queue, so >>> * tcp_transmit_skb() will set the ownership to this >>> * sock. >>> + * Add 64 bytes of tailroom so that some drivers can use skb_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. >> 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-18 3:01 ` Alexander Duyck @ 2012-09-18 3:03 ` David Miller 2012-09-18 3:27 ` Alexander Duyck 0 siblings, 1 reply; 18+ messages in thread From: David Miller @ 2012-09-18 3:03 UTC (permalink / raw) To: alexander.duyck Cc: eric.dumazet, alexander.h.duyck, tushar.n.dave, john.r.fastabend, mirqus, jeffrey.t.kirsher, netdev, gospo, sassmann From: Alexander Duyck <alexander.duyck@gmail.com> Date: Mon, 17 Sep 2012 20:01:06 -0700 > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-18 3:03 ` David Miller @ 2012-09-18 3:27 ` Alexander Duyck 2012-09-18 5:45 ` Eric Dumazet 0 siblings, 1 reply; 18+ messages in thread From: Alexander Duyck @ 2012-09-18 3:27 UTC (permalink / raw) To: David Miller Cc: eric.dumazet, alexander.h.duyck, tushar.n.dave, john.r.fastabend, mirqus, jeffrey.t.kirsher, netdev, gospo, sassmann On 9/17/2012 8:03 PM, David Miller wrote: > From: Alexander Duyck <alexander.duyck@gmail.com> > Date: Mon, 17 Sep 2012 20:01:06 -0700 > >> 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-18 3:27 ` Alexander Duyck @ 2012-09-18 5:45 ` Eric Dumazet 2012-09-18 5:55 ` Alexander Duyck 0 siblings, 1 reply; 18+ messages in thread From: Eric Dumazet @ 2012-09-18 5:45 UTC (permalink / raw) To: Alexander Duyck Cc: David Miller, alexander.h.duyck, tushar.n.dave, john.r.fastabend, mirqus, jeffrey.t.kirsher, netdev, gospo, sassmann 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 ? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-18 5:45 ` Eric Dumazet @ 2012-09-18 5:55 ` Alexander Duyck 0 siblings, 0 replies; 18+ messages in thread From: Alexander Duyck @ 2012-09-18 5:55 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, alexander.h.duyck, tushar.n.dave, john.r.fastabend, mirqus, jeffrey.t.kirsher, netdev, gospo, sassmann On 9/17/2012 10:45 PM, Eric Dumazet wrote: > 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 ? 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-17 7:33 ` Dave, Tushar N 2012-09-17 7:58 ` Eric Dumazet @ 2012-09-17 16:31 ` David Miller 2012-09-17 16:39 ` Dave, Tushar N 1 sibling, 1 reply; 18+ messages in thread From: David Miller @ 2012-09-17 16:31 UTC (permalink / raw) To: tushar.n.dave Cc: john.r.fastabend, mirqus, jeffrey.t.kirsher, netdev, gospo, sassmann From: "Dave, Tushar N" <tushar.n.dave@intel.com> Date: Mon, 17 Sep 2012 07:33:12 +0000 > 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-17 16:31 ` David Miller @ 2012-09-17 16:39 ` Dave, Tushar N 0 siblings, 0 replies; 18+ messages in thread From: Dave, Tushar N @ 2012-09-17 16:39 UTC (permalink / raw) To: David Miller Cc: Fastabend, John R, mirqus@gmail.com, Kirsher, Jeffrey T, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com >-----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" <tushar.n.dave@intel.com> >Date: Mon, 17 Sep 2012 07:33:12 +0000 > >> 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-15 20:44 ` Michał Mirosław 2012-09-16 1:25 ` Dave, Tushar N @ 2012-09-17 19:40 ` Alexander Duyck 1 sibling, 0 replies; 18+ messages in thread From: Alexander Duyck @ 2012-09-17 19:40 UTC (permalink / raw) To: Michał Mirosław Cc: Jeff Kirsher, davem, Tushar Dave, netdev, gospo, sassmann On 09/15/2012 01:44 PM, Michał Mirosław wrote: > 2012/9/15 Jeff Kirsher <jeffrey.t.kirsher@intel.com>: >> From: Tushar Dave <tushar.n.dave@intel.com> >> >> 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 <tushar.n.dave@intel.com> >> Tested-by: Aaron Brown <aaron.f.brown@intel.com> >> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> --- >> drivers/net/ethernet/intel/e1000/e1000_main.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c >> index 3bfbb8d..bde337e 100644 >> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >> @@ -3149,6 +3149,17 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, >> 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? > > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net] e1000: Small packets may get corrupted during padding by HW 2012-09-15 20:16 [net] e1000: Small packets may get corrupted during padding by HW Jeff Kirsher 2012-09-15 20:44 ` Michał Mirosław @ 2012-09-18 20:33 ` David Miller 1 sibling, 0 replies; 18+ messages in thread From: David Miller @ 2012-09-18 20:33 UTC (permalink / raw) To: jeffrey.t.kirsher; +Cc: tushar.n.dave, netdev, gospo, sassmann From: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Date: Sat, 15 Sep 2012 13:16:57 -0700 > From: Tushar Dave <tushar.n.dave@intel.com> > > 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 <tushar.n.dave@intel.com> > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2012-09-18 20:33 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-15 20:16 [net] e1000: Small packets may get corrupted during padding by HW Jeff Kirsher 2012-09-15 20:44 ` Michał Mirosław 2012-09-16 1:25 ` Dave, Tushar N 2012-09-16 1:48 ` John Fastabend 2012-09-16 2:30 ` John Fastabend 2012-09-17 7:33 ` Dave, Tushar N 2012-09-17 7:58 ` Eric Dumazet 2012-09-17 20:53 ` Alexander Duyck 2012-09-17 21:02 ` Eric Dumazet 2012-09-18 3:01 ` Alexander Duyck 2012-09-18 3:03 ` David Miller 2012-09-18 3:27 ` Alexander Duyck 2012-09-18 5:45 ` Eric Dumazet 2012-09-18 5:55 ` Alexander Duyck 2012-09-17 16:31 ` David Miller 2012-09-17 16:39 ` Dave, Tushar N 2012-09-17 19:40 ` Alexander Duyck 2012-09-18 20:33 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).