| Message ID | 20141125224406.1867.97911.stgit@ahduyck-vm-fedora20 |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote: > Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing > their own implementations of the function. > > Also this cleans up two other spots where skb_pad was called but the length > and tail pointers were being manipulated directly instead of just having > the padding length added via __skb_put. > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> > --- > drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------ > drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++-------- > drivers/net/ethernet/intel/igb/igb_main.c | 11 +++-------- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++-------- > drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++-------- > 5 files changed, 14 insertions(+), 38 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c > index 24f3986..862d198 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, > * 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); > - } > + if (eth_skb_pad(skb)) > + return NETDEV_TX_OK; > Its a bit sad almost no driver increments some drop counter. This probably could be generically done in eth_skb_pad() atomic_long_inc(&skb->dev->tx_dropped) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/25/2014 03:14 PM, Eric Dumazet wrote: > On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote: >> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing >> their own implementations of the function. >> >> Also this cleans up two other spots where skb_pad was called but the length >> and tail pointers were being manipulated directly instead of just having >> the padding length added via __skb_put. >> >> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> >> --- >> drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------ >> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++-------- >> drivers/net/ethernet/intel/igb/igb_main.c | 11 +++-------- >> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++-------- >> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++-------- >> 5 files changed, 14 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c >> index 24f3986..862d198 100644 >> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, >> * 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); >> - } >> + if (eth_skb_pad(skb)) >> + return NETDEV_TX_OK; >> > Its a bit sad almost no driver increments some drop counter. > > This probably could be generically done in eth_skb_pad() > > atomic_long_inc(&skb->dev->tx_dropped) The only problem is eth_skb_pad is called in the Rx path of some drivers as well. I wonder if we couldn't make this some sort of netdevice attribute to indicate what the smallest frame we can handle is and then just pad the frame to that as a part of __dev_xmit_skb. Then we could do that outside of the locks and take care of it before we even hit the qdisc layer. - Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/11/14 16:44, Alexander Duyck wrote: > On 11/25/2014 03:14 PM, Eric Dumazet wrote: >> On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote: >>> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing >>> their own implementations of the function. >>> >>> Also this cleans up two other spots where skb_pad was called but the length >>> and tail pointers were being manipulated directly instead of just having >>> the padding length added via __skb_put. >>> >>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> >>> --- >>> drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------ >>> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++-------- >>> drivers/net/ethernet/intel/igb/igb_main.c | 11 +++-------- >>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++-------- >>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++-------- >>> 5 files changed, 14 insertions(+), 38 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c >>> index 24f3986..862d198 100644 >>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >>> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, >>> * 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); >>> - } >>> + if (eth_skb_pad(skb)) >>> + return NETDEV_TX_OK; >>> >> Its a bit sad almost no driver increments some drop counter. >> >> This probably could be generically done in eth_skb_pad() >> >> atomic_long_inc(&skb->dev->tx_dropped) > > The only problem is eth_skb_pad is called in the Rx path of some drivers > as well. > > I wonder if we couldn't make this some sort of netdevice attribute to > indicate what the smallest frame we can handle is and then just pad the > frame to that as a part of __dev_xmit_skb. Then we could do that > outside of the locks and take care of it before we even hit the qdisc layer. One potential problem could be that the padding size varies at runtime based on e.g: netdev features, connection to an Ethernet switch etc... we could probably just advertise whatever maximum padding we need once and for all and just assume that any skb we get called with in a driver's xmit() has the required padding, that is probably fine too. -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-11-25 at 16:44 -0800, Alexander Duyck wrote: > > I wonder if we couldn't make this some sort of netdevice attribute to > indicate what the smallest frame we can handle is and then just pad the > frame to that as a part of __dev_xmit_skb. Then we could do that > outside of the locks and take care of it before we even hit the qdisc layer. Well, many NIC do not have such restriction. Do we have some kind of counters of skb->head reallocations caused by this padding ? I believe I finally have an idea why we had various + 15 in skb allocations in TCP stack ! We probably should reinstate them, as ACK packets can be 54 bytes long. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 25 Nov 2014 17:43:05 -0800 > I believe I finally have an idea why we had various + 15 in skb > allocations in TCP stack ! It was so that you could do one level of tunneling with "for free". Or that is my recollection. Those + 15 existed way before any of these padto() calls even existed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-11-25 at 22:19 -0500, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 25 Nov 2014 17:43:05 -0800 > > > I believe I finally have an idea why we had various + 15 in skb > > allocations in TCP stack ! > > It was so that you could do one level of tunneling with "for > free". Or that is my recollection. > > Those + 15 existed way before any of these padto() calls even > existed. Well, tunneling is added in front of the packet. Thats why we use MAX_TCP_HEADER. The +15 is in fact because TCP stack wanted to make sure the eventual padding (needing tailroom, not headroom) was possible... Note that ack packets never used the +15, but other packets did. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 25 Nov 2014 20:01:13 -0800 [ I am still intrigued, CC:'ing Alexey ] > On Tue, 2014-11-25 at 22:19 -0500, David Miller wrote: >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Tue, 25 Nov 2014 17:43:05 -0800 >> >> > I believe I finally have an idea why we had various + 15 in skb >> > allocations in TCP stack ! >> >> It was so that you could do one level of tunneling with "for >> free". Or that is my recollection. >> >> Those + 15 existed way before any of these padto() calls even >> existed. > > Well, tunneling is added in front of the packet. Thats why we use > MAX_TCP_HEADER. > > The +15 is in fact because TCP stack wanted to make sure the eventual > padding (needing tailroom, not headroom) was possible... > > Note that ack packets never used the +15, but other packets did. Alexey, do you remember exact reason for that +15 everywhere in TCP packet allocation sizing? I thought it was for headroom, but as Eric shows that's illogical, it can only be for tailroom considerations. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c index 24f3986..862d198 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_main.c +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, * 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); - } + if (eth_skb_pad(skb)) + return NETDEV_TX_OK; mss = skb_shinfo(skb)->gso_size; /* The controller does a simple calculation to diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c index 73457ed..91516ae 100644 --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c @@ -578,14 +578,9 @@ static bool fm10k_cleanup_headers(struct fm10k_ring *rx_ring, if (skb_is_nonlinear(skb)) fm10k_pull_tail(rx_ring, rx_desc, skb); - /* if skb_pad returns an error the skb was freed */ - if (unlikely(skb->len < 60)) { - int pad_len = 60 - skb->len; - - if (skb_pad(skb, pad_len)) - return true; - __skb_put(skb, pad_len); - } + /* if eth_skb_pad returns an error the skb was freed */ + if (eth_skb_pad(skb)) + return true; return false; } diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index b0e12e7..3cb54d0 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -6843,14 +6843,9 @@ static bool igb_cleanup_headers(struct igb_ring *rx_ring, if (skb_is_nonlinear(skb)) igb_pull_tail(rx_ring, rx_desc, skb); - /* if skb_pad returns an error the skb was freed */ - if (unlikely(skb->len < 60)) { - int pad_len = 60 - skb->len; - - if (skb_pad(skb, pad_len)) - return true; - __skb_put(skb, pad_len); - } + /* if eth_skb_pad returns an error the skb was freed */ + if (eth_skb_pad(skb)) + return true; return false; } diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 932f779..7d0991a 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -1766,14 +1766,9 @@ static bool ixgbe_cleanup_headers(struct ixgbe_ring *rx_ring, return false; #endif - /* if skb_pad returns an error the skb was freed */ - if (unlikely(skb->len < 60)) { - int pad_len = 60 - skb->len; - - if (skb_pad(skb, pad_len)) - return true; - __skb_put(skb, pad_len); - } + /* if eth_skb_pad returns an error the skb was freed */ + if (eth_skb_pad(skb)) + return true; return false; } diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c index 755f71f..465d6a8 100644 --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c @@ -612,14 +612,9 @@ static bool ixgbevf_cleanup_headers(struct ixgbevf_ring *rx_ring, if (skb_is_nonlinear(skb)) ixgbevf_pull_tail(rx_ring, skb); - /* if skb_pad returns an error the skb was freed */ - if (unlikely(skb->len < 60)) { - int pad_len = 60 - skb->len; - - if (skb_pad(skb, pad_len)) - return true; - __skb_put(skb, pad_len); - } + /* if eth_skb_pad returns an error the skb was freed */ + if (eth_skb_pad(skb)) + return true; return false; }
Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing their own implementations of the function. Also this cleans up two other spots where skb_pad was called but the length and tail pointers were being manipulated directly instead of just having the padding length added via __skb_put. Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> --- drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------ drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++-------- drivers/net/ethernet/intel/igb/igb_main.c | 11 +++-------- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++-------- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++-------- 5 files changed, 14 insertions(+), 38 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html