| Message ID | 20180320154014.7346-2-ddstreet@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xenial,1/2] i40e/i40evf: Allow up to 12K bytes of data per Tx descriptor instead of 8K | expand |
sorry for the duplicates, i thought it was a mail server issue on my end at first but looks like they all finally made it thru. On Tue, Mar 20, 2018 at 11:40 AM, Dan Streetman <ddstreet@canonical.com> wrote: > From: Alexander Duyck <alexander.h.duyck@intel.com> > > BugLink: http://bugs.launchpad.net/bugs/1723127 > > The original code for __i40e_chk_linearize didn't take into account the > fact that if a fragment is 16K in size or larger it has to be split over 2 > descriptors and the smaller of those 2 descriptors will be on the trailing > edge of the transmit. As a result we can get into situations where we didn't > catch requests that could result in a Tx hang. > > This patch takes care of that by subtracting the length of all but the > trailing edge of the stale fragment before we test for sum. By doing this > we can guarantee that we have all cases covered, including the case of a > fragment that spans multiple descriptors. We don't need to worry about > checking the inner portions of this since 12K is the maximum aligned DMA > size and that is larger than any MSS will ever be since the MTU limit for > jumbos is something on the order of 9K. > > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > (cherry-picked from 248de22e638f10bd5bfc7624a357f940f66ba137) > Signed-off-by: Dan Streetman <ddstreet@canonical.com> > > --- > Note that this patch applies to Xenial and Artful, but the previous patch > in the series is only required in Xenial. > > drivers/net/ethernet/intel/i40e/i40e_txrx.c | 26 +++++++++++++++++++++++--- > drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 26 +++++++++++++++++++++++--- > 2 files changed, 46 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > index a18c91e33dea..2c83a6ce1b47 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > @@ -2640,10 +2640,30 @@ bool __i40e_chk_linearize(struct sk_buff *skb) > /* Walk through fragments adding latest fragment, testing it, and > * then removing stale fragments from the sum. > */ > - stale = &skb_shinfo(skb)->frags[0]; > - for (;;) { > + for (stale = &skb_shinfo(skb)->frags[0];; stale++) { > + int stale_size = skb_frag_size(stale); > + > sum += skb_frag_size(frag++); > > + /* The stale fragment may present us with a smaller > + * descriptor than the actual fragment size. To account > + * for that we need to remove all the data on the front and > + * figure out what the remainder would be in the last > + * descriptor associated with the fragment. > + */ > + if (stale_size > I40E_MAX_DATA_PER_TXD) { > + int align_pad = -(stale->page_offset) & > + (I40E_MAX_READ_REQ_SIZE - 1); > + > + sum -= align_pad; > + stale_size -= align_pad; > + > + do { > + sum -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + stale_size -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + } while (stale_size > I40E_MAX_DATA_PER_TXD); > + } > + > /* if sum is negative we failed to make sufficient progress */ > if (sum < 0) > return true; > @@ -2651,7 +2671,7 @@ bool __i40e_chk_linearize(struct sk_buff *skb) > if (!nr_frags--) > break; > > - sum -= skb_frag_size(stale++); > + sum -= stale_size; > } > > return false; > diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c > index f2163735528d..1f48bcdf1ea5 100644 > --- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c > +++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c > @@ -1843,10 +1843,30 @@ bool __i40evf_chk_linearize(struct sk_buff *skb) > /* Walk through fragments adding latest fragment, testing it, and > * then removing stale fragments from the sum. > */ > - stale = &skb_shinfo(skb)->frags[0]; > - for (;;) { > + for (stale = &skb_shinfo(skb)->frags[0];; stale++) { > + int stale_size = skb_frag_size(stale); > + > sum += skb_frag_size(frag++); > > + /* The stale fragment may present us with a smaller > + * descriptor than the actual fragment size. To account > + * for that we need to remove all the data on the front and > + * figure out what the remainder would be in the last > + * descriptor associated with the fragment. > + */ > + if (stale_size > I40E_MAX_DATA_PER_TXD) { > + int align_pad = -(stale->page_offset) & > + (I40E_MAX_READ_REQ_SIZE - 1); > + > + sum -= align_pad; > + stale_size -= align_pad; > + > + do { > + sum -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + stale_size -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + } while (stale_size > I40E_MAX_DATA_PER_TXD); > + } > + > /* if sum is negative we failed to make sufficient progress */ > if (sum < 0) > return true; > @@ -1854,7 +1874,7 @@ bool __i40evf_chk_linearize(struct sk_buff *skb) > if (!nr_frags--) > break; > > - sum -= skb_frag_size(stale++); > + sum -= stale_size; > } > > return false; > -- > 2.15.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 03/20/18 16:40, Dan Streetman wrote: > From: Alexander Duyck <alexander.h.duyck@intel.com> > > BugLink: http://bugs.launchpad.net/bugs/1723127 > > The original code for __i40e_chk_linearize didn't take into account the > fact that if a fragment is 16K in size or larger it has to be split over 2 > descriptors and the smaller of those 2 descriptors will be on the trailing > edge of the transmit. As a result we can get into situations where we didn't > catch requests that could result in a Tx hang. > > This patch takes care of that by subtracting the length of all but the > trailing edge of the stale fragment before we test for sum. By doing this > we can guarantee that we have all cases covered, including the case of a > fragment that spans multiple descriptors. We don't need to worry about > checking the inner portions of this since 12K is the maximum aligned DMA > size and that is larger than any MSS will ever be since the MTU limit for > jumbos is something on the order of 9K. > > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > (cherry-picked from 248de22e638f10bd5bfc7624a357f940f66ba137) > Signed-off-by: Dan Streetman <ddstreet@canonical.com> > > --- > Note that this patch applies to Xenial and Artful, but the previous patch > in the series is only required in Xenial. > > drivers/net/ethernet/intel/i40e/i40e_txrx.c | 26 +++++++++++++++++++++++--- > drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 26 +++++++++++++++++++++++--- > 2 files changed, 46 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > index a18c91e33dea..2c83a6ce1b47 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > @@ -2640,10 +2640,30 @@ bool __i40e_chk_linearize(struct sk_buff *skb) > /* Walk through fragments adding latest fragment, testing it, and > * then removing stale fragments from the sum. > */ > - stale = &skb_shinfo(skb)->frags[0]; > - for (;;) { > + for (stale = &skb_shinfo(skb)->frags[0];; stale++) { > + int stale_size = skb_frag_size(stale); > + > sum += skb_frag_size(frag++); > > + /* The stale fragment may present us with a smaller > + * descriptor than the actual fragment size. To account > + * for that we need to remove all the data on the front and > + * figure out what the remainder would be in the last > + * descriptor associated with the fragment. > + */ > + if (stale_size > I40E_MAX_DATA_PER_TXD) { > + int align_pad = -(stale->page_offset) & > + (I40E_MAX_READ_REQ_SIZE - 1); > + > + sum -= align_pad; > + stale_size -= align_pad; > + > + do { > + sum -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + stale_size -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + } while (stale_size > I40E_MAX_DATA_PER_TXD); > + } > + > /* if sum is negative we failed to make sufficient progress */ > if (sum < 0) > return true; > @@ -2651,7 +2671,7 @@ bool __i40e_chk_linearize(struct sk_buff *skb) > if (!nr_frags--) > break; > > - sum -= skb_frag_size(stale++); > + sum -= stale_size; > } > > return false; > diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c > index f2163735528d..1f48bcdf1ea5 100644 > --- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c > +++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c > @@ -1843,10 +1843,30 @@ bool __i40evf_chk_linearize(struct sk_buff *skb) > /* Walk through fragments adding latest fragment, testing it, and > * then removing stale fragments from the sum. > */ > - stale = &skb_shinfo(skb)->frags[0]; > - for (;;) { > + for (stale = &skb_shinfo(skb)->frags[0];; stale++) { > + int stale_size = skb_frag_size(stale); > + > sum += skb_frag_size(frag++); > > + /* The stale fragment may present us with a smaller > + * descriptor than the actual fragment size. To account > + * for that we need to remove all the data on the front and > + * figure out what the remainder would be in the last > + * descriptor associated with the fragment. > + */ > + if (stale_size > I40E_MAX_DATA_PER_TXD) { > + int align_pad = -(stale->page_offset) & > + (I40E_MAX_READ_REQ_SIZE - 1); > + > + sum -= align_pad; > + stale_size -= align_pad; > + > + do { > + sum -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + stale_size -= I40E_MAX_DATA_PER_TXD_ALIGNED; > + } while (stale_size > I40E_MAX_DATA_PER_TXD); > + } > + > /* if sum is negative we failed to make sufficient progress */ > if (sum < 0) > return true; > @@ -1854,7 +1874,7 @@ bool __i40evf_chk_linearize(struct sk_buff *skb) > if (!nr_frags--) > break; > > - sum -= skb_frag_size(stale++); > + sum -= stale_size; > } > > return false; > Applied to xenial/master-next and artful/master-next branches. Thanks, Kleber
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c index a18c91e33dea..2c83a6ce1b47 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c @@ -2640,10 +2640,30 @@ bool __i40e_chk_linearize(struct sk_buff *skb) /* Walk through fragments adding latest fragment, testing it, and * then removing stale fragments from the sum. */ - stale = &skb_shinfo(skb)->frags[0]; - for (;;) { + for (stale = &skb_shinfo(skb)->frags[0];; stale++) { + int stale_size = skb_frag_size(stale); + sum += skb_frag_size(frag++); + /* The stale fragment may present us with a smaller + * descriptor than the actual fragment size. To account + * for that we need to remove all the data on the front and + * figure out what the remainder would be in the last + * descriptor associated with the fragment. + */ + if (stale_size > I40E_MAX_DATA_PER_TXD) { + int align_pad = -(stale->page_offset) & + (I40E_MAX_READ_REQ_SIZE - 1); + + sum -= align_pad; + stale_size -= align_pad; + + do { + sum -= I40E_MAX_DATA_PER_TXD_ALIGNED; + stale_size -= I40E_MAX_DATA_PER_TXD_ALIGNED; + } while (stale_size > I40E_MAX_DATA_PER_TXD); + } + /* if sum is negative we failed to make sufficient progress */ if (sum < 0) return true; @@ -2651,7 +2671,7 @@ bool __i40e_chk_linearize(struct sk_buff *skb) if (!nr_frags--) break; - sum -= skb_frag_size(stale++); + sum -= stale_size; } return false; diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c index f2163735528d..1f48bcdf1ea5 100644 --- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c +++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c @@ -1843,10 +1843,30 @@ bool __i40evf_chk_linearize(struct sk_buff *skb) /* Walk through fragments adding latest fragment, testing it, and * then removing stale fragments from the sum. */ - stale = &skb_shinfo(skb)->frags[0]; - for (;;) { + for (stale = &skb_shinfo(skb)->frags[0];; stale++) { + int stale_size = skb_frag_size(stale); + sum += skb_frag_size(frag++); + /* The stale fragment may present us with a smaller + * descriptor than the actual fragment size. To account + * for that we need to remove all the data on the front and + * figure out what the remainder would be in the last + * descriptor associated with the fragment. + */ + if (stale_size > I40E_MAX_DATA_PER_TXD) { + int align_pad = -(stale->page_offset) & + (I40E_MAX_READ_REQ_SIZE - 1); + + sum -= align_pad; + stale_size -= align_pad; + + do { + sum -= I40E_MAX_DATA_PER_TXD_ALIGNED; + stale_size -= I40E_MAX_DATA_PER_TXD_ALIGNED; + } while (stale_size > I40E_MAX_DATA_PER_TXD); + } + /* if sum is negative we failed to make sufficient progress */ if (sum < 0) return true; @@ -1854,7 +1874,7 @@ bool __i40evf_chk_linearize(struct sk_buff *skb) if (!nr_frags--) break; - sum -= skb_frag_size(stale++); + sum -= stale_size; } return false;