* [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping @ 2013-04-29 12:57 Claudiu Manoil 2013-04-29 13:53 ` Eric Dumazet 0 siblings, 1 reply; 14+ messages in thread From: Claudiu Manoil @ 2013-04-29 12:57 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller When Tx timestamping is enabled the number of sent bytes reported to BQL via tx_completed_queue() falls short by GMAC_FCB_LEN + GMAC_TXPAL_LEN than the number of bytes reported via tx_sent_queue() on xmit. This leads to BQL stopping transmission errorneously followed by tx timeout firing. This fixes the amount of sent bytes reported to BQL on clean_tx_ring to match the amount reported on xmit, when Tx timestamping enabled. Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 2375a01..0d26a8b 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -2539,6 +2539,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) skb_tstamp_tx(skb, &shhwtstamps); bdp->lstatus &= BD_LFLAG(TXBD_WRAP); bdp = next; + bytes_sent += GMAC_FCB_LEN + GMAC_TXPAL_LEN; } bdp->lstatus &= BD_LFLAG(TXBD_WRAP); -- 1.7.11.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 12:57 [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping Claudiu Manoil @ 2013-04-29 13:53 ` Eric Dumazet 2013-04-29 14:27 ` Claudiu Manoil 0 siblings, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2013-04-29 13:53 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, Paul Gortmaker, David S. Miller On Mon, 2013-04-29 at 15:57 +0300, Claudiu Manoil wrote: > When Tx timestamping is enabled the number of sent bytes reported > to BQL via tx_completed_queue() falls short by GMAC_FCB_LEN + > GMAC_TXPAL_LEN than the number of bytes reported via tx_sent_queue() > on xmit. This leads to BQL stopping transmission errorneously followed > by tx timeout firing. > > This fixes the amount of sent bytes reported to BQL on clean_tx_ring > to match the amount reported on xmit, when Tx timestamping enabled. > > Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> > --- > drivers/net/ethernet/freescale/gianfar.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > index 2375a01..0d26a8b 100644 > --- a/drivers/net/ethernet/freescale/gianfar.c > +++ b/drivers/net/ethernet/freescale/gianfar.c > @@ -2539,6 +2539,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) > skb_tstamp_tx(skb, &shhwtstamps); > bdp->lstatus &= BD_LFLAG(TXBD_WRAP); > bdp = next; > + bytes_sent += GMAC_FCB_LEN + GMAC_TXPAL_LEN; > } > > bdp->lstatus &= BD_LFLAG(TXBD_WRAP); Technically speaking these bytes are not sent to the wire. I would rather fix gfar_start_xmit() to give to netdev_tx_sent_queue() call the correct amount of bytes on wire, to be consistent with other drivers. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 13:53 ` Eric Dumazet @ 2013-04-29 14:27 ` Claudiu Manoil 2013-04-29 15:23 ` Eric Dumazet 0 siblings, 1 reply; 14+ messages in thread From: Claudiu Manoil @ 2013-04-29 14:27 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev, Paul Gortmaker, David S. Miller Hi, Unfortunately fixing this the other way around would imply changes in both xmit and clean_tx, and additional overhead in clean_tx. On xmit skb->len gets incremented with FCB_LEN and/or TXPAL_LEN, on a case by case basis. This would imply to add extra checks on clean_tx to identify whether only FCB_LEN has been added, or FCB+TXPAL or neither, all these just to report the bytes on wire for BQL. Does BQL really need to measure the bytes-on-wire or the bytes consumed for buffering? Thanks, Claudiu On 4/29/2013 4:53 PM, Eric Dumazet wrote: > On Mon, 2013-04-29 at 15:57 +0300, Claudiu Manoil wrote: >> When Tx timestamping is enabled the number of sent bytes reported >> to BQL via tx_completed_queue() falls short by GMAC_FCB_LEN + >> GMAC_TXPAL_LEN than the number of bytes reported via tx_sent_queue() >> on xmit. This leads to BQL stopping transmission errorneously followed >> by tx timeout firing. >> >> This fixes the amount of sent bytes reported to BQL on clean_tx_ring >> to match the amount reported on xmit, when Tx timestamping enabled. >> >> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> >> --- >> drivers/net/ethernet/freescale/gianfar.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c >> index 2375a01..0d26a8b 100644 >> --- a/drivers/net/ethernet/freescale/gianfar.c >> +++ b/drivers/net/ethernet/freescale/gianfar.c >> @@ -2539,6 +2539,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) >> skb_tstamp_tx(skb, &shhwtstamps); >> bdp->lstatus &= BD_LFLAG(TXBD_WRAP); >> bdp = next; >> + bytes_sent += GMAC_FCB_LEN + GMAC_TXPAL_LEN; >> } >> >> bdp->lstatus &= BD_LFLAG(TXBD_WRAP); > > Technically speaking these bytes are not sent to the wire. > > I would rather fix gfar_start_xmit() to give to netdev_tx_sent_queue() > call the correct amount of bytes on wire, to be consistent with other > drivers. > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 14:27 ` Claudiu Manoil @ 2013-04-29 15:23 ` Eric Dumazet 2013-04-29 15:28 ` Eric Dumazet 2013-04-29 15:33 ` [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping Eric Dumazet 0 siblings, 2 replies; 14+ messages in thread From: Eric Dumazet @ 2013-04-29 15:23 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, Paul Gortmaker, David S. Miller On Mon, 2013-04-29 at 17:27 +0300, Claudiu Manoil wrote: > Hi, > > Unfortunately fixing this the other way around would imply changes > in both xmit and clean_tx, and additional overhead in clean_tx. > On xmit skb->len gets incremented with FCB_LEN and/or TXPAL_LEN, > on a case by case basis. This would imply to add extra checks on > clean_tx to identify whether only FCB_LEN has been added, or > FCB+TXPAL or neither, all these just to report the bytes on wire > for BQL. > Does BQL really need to measure the bytes-on-wire or the bytes consumed > for buffering? Yes. What about : diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 2375a01..bb727e1 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -2065,6 +2065,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) u32 bufaddr; unsigned long flags; unsigned int nr_frags, nr_txbds, length, fcb_length = GMAC_FCB_LEN; + unsigned int len; /* TOE=1 frames larger than 2500 bytes may see excess delays * before start of transmission. @@ -2130,7 +2131,8 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) } /* Update transmit stats */ - tx_queue->stats.tx_bytes += skb->len; + len = skb->len; + tx_queue->stats.tx_bytes += len; tx_queue->stats.tx_packets++; txbdp = txbdp_start = tx_queue->cur_tx; @@ -2231,7 +2233,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb); } - netdev_tx_sent_queue(txq, skb->len); + netdev_tx_sent_queue(txq, len); /* We can work in parallel with gfar_clean_tx_ring(), except * when modifying num_txbdfree. Note that we didn't grab the lock ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 15:23 ` Eric Dumazet @ 2013-04-29 15:28 ` Eric Dumazet 2013-04-29 15:39 ` Eric Dumazet 2013-04-29 15:52 ` Claudiu Manoil 2013-04-29 15:33 ` [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping Eric Dumazet 1 sibling, 2 replies; 14+ messages in thread From: Eric Dumazet @ 2013-04-29 15:28 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, Paul Gortmaker, David S. Miller On Mon, 2013-04-29 at 08:23 -0700, Eric Dumazet wrote: > On Mon, 2013-04-29 at 17:27 +0300, Claudiu Manoil wrote: > > Hi, > > > > Unfortunately fixing this the other way around would imply changes > > in both xmit and clean_tx, and additional overhead in clean_tx. > > On xmit skb->len gets incremented with FCB_LEN and/or TXPAL_LEN, > > on a case by case basis. This would imply to add extra checks on > > clean_tx to identify whether only FCB_LEN has been added, or > > FCB+TXPAL or neither, all these just to report the bytes on wire > > for BQL. > > Does BQL really need to measure the bytes-on-wire or the bytes consumed > > for buffering? > > Yes. > > What about : > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > index 2375a01..bb727e1 100644 > --- a/drivers/net/ethernet/freescale/gianfar.c > +++ b/drivers/net/ethernet/freescale/gianfar.c > @@ -2065,6 +2065,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) > u32 bufaddr; > unsigned long flags; > unsigned int nr_frags, nr_txbds, length, fcb_length = GMAC_FCB_LEN; > + unsigned int len; > > /* TOE=1 frames larger than 2500 bytes may see excess delays > * before start of transmission. > @@ -2130,7 +2131,8 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) > } > > /* Update transmit stats */ > - tx_queue->stats.tx_bytes += skb->len; > + len = skb->len; Idea is to use this value in TX completion. If at tx completion skb->len might be different, then store the true skb->len in skb->cb[] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 15:28 ` Eric Dumazet @ 2013-04-29 15:39 ` Eric Dumazet 2013-04-29 15:42 ` Eric Dumazet 2013-04-29 15:52 ` Claudiu Manoil 1 sibling, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2013-04-29 15:39 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, Paul Gortmaker, David S. Miller On Mon, 2013-04-29 at 08:28 -0700, Eric Dumazet wrote: > If at tx completion skb->len might be different, then store the true > skb->len in skb->cb[] Something like : diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 2375a01..5cd306f 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -2065,6 +2065,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) u32 bufaddr; unsigned long flags; unsigned int nr_frags, nr_txbds, length, fcb_length = GMAC_FCB_LEN; + unsigned int len; /* TOE=1 frames larger than 2500 bytes may see excess delays * before start of transmission. @@ -2130,7 +2131,9 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) } /* Update transmit stats */ - tx_queue->stats.tx_bytes += skb->len; + len = skb->len; + *(unsigned int)skb->cb = len; + tx_queue->stats.tx_bytes += len; tx_queue->stats.tx_packets++; txbdp = txbdp_start = tx_queue->cur_tx; @@ -2231,7 +2234,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb); } - netdev_tx_sent_queue(txq, skb->len); + netdev_tx_sent_queue(txq, len); /* We can work in parallel with gfar_clean_tx_ring(), except * when modifying num_txbdfree. Note that we didn't grab the lock @@ -2551,7 +2554,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) bdp = next_txbd(bdp, base, tx_ring_size); } - bytes_sent += skb->len; + bytes_sent += *(unsigned int)skb->cb; dev_kfree_skb_any(skb); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 15:39 ` Eric Dumazet @ 2013-04-29 15:42 ` Eric Dumazet 0 siblings, 0 replies; 14+ messages in thread From: Eric Dumazet @ 2013-04-29 15:42 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, Paul Gortmaker, David S. Miller On Mon, 2013-04-29 at 08:39 -0700, Eric Dumazet wrote: > + *(unsigned int)skb->cb = len; Obviously not compiled (I am in a bus right now and this driver doesnt compile on x86_64) *(unsigned int *)skb->cb = len; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 15:28 ` Eric Dumazet 2013-04-29 15:39 ` Eric Dumazet @ 2013-04-29 15:52 ` Claudiu Manoil 2013-04-29 15:59 ` Eric Dumazet 1 sibling, 1 reply; 14+ messages in thread From: Claudiu Manoil @ 2013-04-29 15:52 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev, Paul Gortmaker, David S. Miller On 4/29/2013 6:28 PM, Eric Dumazet wrote: > On Mon, 2013-04-29 at 08:23 -0700, Eric Dumazet wrote: >> On Mon, 2013-04-29 at 17:27 +0300, Claudiu Manoil wrote: >>> Hi, >>> >>> Unfortunately fixing this the other way around would imply changes >>> in both xmit and clean_tx, and additional overhead in clean_tx. >>> On xmit skb->len gets incremented with FCB_LEN and/or TXPAL_LEN, >>> on a case by case basis. This would imply to add extra checks on >>> clean_tx to identify whether only FCB_LEN has been added, or >>> FCB+TXPAL or neither, all these just to report the bytes on wire >>> for BQL. >>> Does BQL really need to measure the bytes-on-wire or the bytes consumed >>> for buffering? >> >> Yes. >> >> What about : >> >> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c >> index 2375a01..bb727e1 100644 >> --- a/drivers/net/ethernet/freescale/gianfar.c >> +++ b/drivers/net/ethernet/freescale/gianfar.c >> @@ -2065,6 +2065,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) >> u32 bufaddr; >> unsigned long flags; >> unsigned int nr_frags, nr_txbds, length, fcb_length = GMAC_FCB_LEN; >> + unsigned int len; >> >> /* TOE=1 frames larger than 2500 bytes may see excess delays >> * before start of transmission. >> @@ -2130,7 +2131,8 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) >> } >> >> /* Update transmit stats */ >> - tx_queue->stats.tx_bytes += skb->len; >> + len = skb->len; Already tried this change in xmit, but it breaks the UDP/TCP cases (eg. ftp). Due to (skb->ip_summed == CHECKSUM_PARTIAL) condition FCB is added on xmit (skb->len increased with FCB_LEN only), and Tx completion does not make this check. > Idea is to use this value in TX completion. > > If at tx completion skb->len might be different, then store the true > skb->len in skb->cb[] > Interesting but non-trivial, I think. Is this allowed? I mean, doesn't the net stack overwrite this field? Thanks, Claudiu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 15:52 ` Claudiu Manoil @ 2013-04-29 15:59 ` Eric Dumazet 2013-08-30 12:01 ` [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL Claudiu Manoil 0 siblings, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2013-04-29 15:59 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, Paul Gortmaker, David S. Miller On Mon, 2013-04-29 at 18:52 +0300, Claudiu Manoil wrote: > Interesting but non-trivial, I think. Is this allowed? I mean, doesn't > the net stack overwrite this field? Each layer is allowed to overwrite skb->cb[] Some network drivers already do. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL 2013-04-29 15:59 ` Eric Dumazet @ 2013-08-30 12:01 ` Claudiu Manoil 2013-09-03 18:59 ` Florian Fainelli 2013-09-04 2:14 ` David Miller 0 siblings, 2 replies; 14+ messages in thread From: Claudiu Manoil @ 2013-08-30 12:01 UTC (permalink / raw) To: netdev; +Cc: David S. Miller, Eric Dumazet Fix the amount of sent bytes reported to BQL by reporting the number of bytes on wire in the xmit routine, and recording that value for each skb in order to be correctly confirmed on Tx confirmation cleanup. Reporting skb->len to BQL just before exiting xmit is not correct due to possible insertions of TOE block and alignment bytes in the skb->data, which are being stripped off by the controller before transmission on wire. This led to mismatch of (incorrectly) reported bytes to BQL b/w xmit and Tx confirmation, resulting in Tx timeout firing, for the h/w tx timestamping acceleration case. There's no easy way to obtain the number of bytes on wire in the Tx confirmation routine, so skb->cb is used to convey that information from xmit to Tx confirmation, for now (as proposed by Eric). Revived the currently unused GFAR_CB() construct for that purpose. Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> Cc: Eric Dumazet <eric.dumazet@gmail.com> --- drivers/net/ethernet/freescale/gianfar.c | 18 +++++++++++------- drivers/net/ethernet/freescale/gianfar.h | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index b2c91dc..c4eaade 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -2092,7 +2092,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) int do_tstamp, do_csum, do_vlan; u32 bufaddr; unsigned long flags; - unsigned int nr_frags, nr_txbds, length, fcb_len = 0; + unsigned int nr_frags, nr_txbds, bytes_sent, fcb_len = 0; rq = skb->queue_mapping; tx_queue = priv->tx_queue[rq]; @@ -2147,7 +2147,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) } /* Update transmit stats */ - tx_queue->stats.tx_bytes += skb->len; + bytes_sent = skb->len; + tx_queue->stats.tx_bytes += bytes_sent; + /* keep Tx bytes on wire for BQL accounting */ + GFAR_CB(skb)->bytes_sent = bytes_sent; tx_queue->stats.tx_packets++; txbdp = txbdp_start = tx_queue->cur_tx; @@ -2167,12 +2170,13 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) } else { /* Place the fragment addresses and lengths into the TxBDs */ for (i = 0; i < nr_frags; i++) { + unsigned int frag_len; /* Point at the next BD, wrapping as needed */ txbdp = next_txbd(txbdp, base, tx_queue->tx_ring_size); - length = skb_shinfo(skb)->frags[i].size; + frag_len = skb_shinfo(skb)->frags[i].size; - lstatus = txbdp->lstatus | length | + lstatus = txbdp->lstatus | frag_len | BD_LFLAG(TXBD_READY); /* Handle the last BD specially */ @@ -2182,7 +2186,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) bufaddr = skb_frag_dma_map(priv->dev, &skb_shinfo(skb)->frags[i], 0, - length, + frag_len, DMA_TO_DEVICE); /* set the TxBD length and buffer pointer */ @@ -2250,7 +2254,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb); } - netdev_tx_sent_queue(txq, skb->len); + netdev_tx_sent_queue(txq, bytes_sent); /* We can work in parallel with gfar_clean_tx_ring(), except * when modifying num_txbdfree. Note that we didn't grab the lock @@ -2570,7 +2574,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) bdp = next_txbd(bdp, base, tx_ring_size); } - bytes_sent += skb->len; + bytes_sent += GFAR_CB(skb)->bytes_sent; dev_kfree_skb_any(skb); diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index 46f56f3..04112b9 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -575,7 +575,7 @@ struct rxfcb { }; struct gianfar_skb_cb { - int alignamount; + unsigned int bytes_sent; /* bytes-on-wire (i.e. no FCB) */ }; #define GFAR_CB(skb) ((struct gianfar_skb_cb *)((skb)->cb)) -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL 2013-08-30 12:01 ` [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL Claudiu Manoil @ 2013-09-03 18:59 ` Florian Fainelli 2013-09-03 19:18 ` Florian Fainelli 2013-09-04 2:14 ` David Miller 1 sibling, 1 reply; 14+ messages in thread From: Florian Fainelli @ 2013-09-03 18:59 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, David S. Miller, Eric Dumazet Hello Claudiu, 2013/8/30 Claudiu Manoil <claudiu.manoil@freescale.com>: > Fix the amount of sent bytes reported to BQL by reporting the > number of bytes on wire in the xmit routine, and recording that > value for each skb in order to be correctly confirmed on Tx > confirmation cleanup. > > Reporting skb->len to BQL just before exiting xmit is not correct > due to possible insertions of TOE block and alignment bytes in the > skb->data, which are being stripped off by the controller before > transmission on wire. This led to mismatch of (incorrectly) > reported bytes to BQL b/w xmit and Tx confirmation, resulting in > Tx timeout firing, for the h/w tx timestamping acceleration case. > > There's no easy way to obtain the number of bytes on wire in the Tx > confirmation routine, so skb->cb is used to convey that information > from xmit to Tx confirmation, for now (as proposed by Eric). Revived > the currently unused GFAR_CB() construct for that purpose. I do not see much difference between what this patch does and what the current net-next drivers does. If you need to correctly account for this, it seems to me like you should move the stats/bytes_sent computation below this line: if (unlikely(do_tstamp)) { skb_push(skb, GMAC_TXPAL_LEN); memset(skb->data, 0, GMAC_TXPAL_LEN); } to account for the SKB length update? > > Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > --- > drivers/net/ethernet/freescale/gianfar.c | 18 +++++++++++------- > drivers/net/ethernet/freescale/gianfar.h | 2 +- > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > index b2c91dc..c4eaade 100644 > --- a/drivers/net/ethernet/freescale/gianfar.c > +++ b/drivers/net/ethernet/freescale/gianfar.c > @@ -2092,7 +2092,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) > int do_tstamp, do_csum, do_vlan; > u32 bufaddr; > unsigned long flags; > - unsigned int nr_frags, nr_txbds, length, fcb_len = 0; > + unsigned int nr_frags, nr_txbds, bytes_sent, fcb_len = 0; > > rq = skb->queue_mapping; > tx_queue = priv->tx_queue[rq]; > @@ -2147,7 +2147,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) > } > > /* Update transmit stats */ > - tx_queue->stats.tx_bytes += skb->len; > + bytes_sent = skb->len; > + tx_queue->stats.tx_bytes += bytes_sent; > + /* keep Tx bytes on wire for BQL accounting */ > + GFAR_CB(skb)->bytes_sent = bytes_sent; > tx_queue->stats.tx_packets++; > > txbdp = txbdp_start = tx_queue->cur_tx; > @@ -2167,12 +2170,13 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) > } else { > /* Place the fragment addresses and lengths into the TxBDs */ > for (i = 0; i < nr_frags; i++) { > + unsigned int frag_len; > /* Point at the next BD, wrapping as needed */ > txbdp = next_txbd(txbdp, base, tx_queue->tx_ring_size); > > - length = skb_shinfo(skb)->frags[i].size; > + frag_len = skb_shinfo(skb)->frags[i].size; > > - lstatus = txbdp->lstatus | length | > + lstatus = txbdp->lstatus | frag_len | > BD_LFLAG(TXBD_READY); > > /* Handle the last BD specially */ > @@ -2182,7 +2186,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) > bufaddr = skb_frag_dma_map(priv->dev, > &skb_shinfo(skb)->frags[i], > 0, > - length, > + frag_len, > DMA_TO_DEVICE); > > /* set the TxBD length and buffer pointer */ > @@ -2250,7 +2254,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) > lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb); > } > > - netdev_tx_sent_queue(txq, skb->len); > + netdev_tx_sent_queue(txq, bytes_sent); > > /* We can work in parallel with gfar_clean_tx_ring(), except > * when modifying num_txbdfree. Note that we didn't grab the lock > @@ -2570,7 +2574,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) > bdp = next_txbd(bdp, base, tx_ring_size); > } > > - bytes_sent += skb->len; > + bytes_sent += GFAR_CB(skb)->bytes_sent; > > dev_kfree_skb_any(skb); > > diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h > index 46f56f3..04112b9 100644 > --- a/drivers/net/ethernet/freescale/gianfar.h > +++ b/drivers/net/ethernet/freescale/gianfar.h > @@ -575,7 +575,7 @@ struct rxfcb { > }; > > struct gianfar_skb_cb { > - int alignamount; > + unsigned int bytes_sent; /* bytes-on-wire (i.e. no FCB) */ > }; > > #define GFAR_CB(skb) ((struct gianfar_skb_cb *)((skb)->cb)) > -- > 1.7.11.7 > > > -- > 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 -- Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL 2013-09-03 18:59 ` Florian Fainelli @ 2013-09-03 19:18 ` Florian Fainelli 0 siblings, 0 replies; 14+ messages in thread From: Florian Fainelli @ 2013-09-03 19:18 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, David S. Miller, Eric Dumazet Le mardi 3 septembre 2013 19:59:42 Florian Fainelli a écrit : > Hello Claudiu, > > 2013/8/30 Claudiu Manoil <claudiu.manoil@freescale.com>: > > Fix the amount of sent bytes reported to BQL by reporting the > > number of bytes on wire in the xmit routine, and recording that > > value for each skb in order to be correctly confirmed on Tx > > confirmation cleanup. > > > > Reporting skb->len to BQL just before exiting xmit is not correct > > due to possible insertions of TOE block and alignment bytes in the > > skb->data, which are being stripped off by the controller before > > transmission on wire. This led to mismatch of (incorrectly) > > reported bytes to BQL b/w xmit and Tx confirmation, resulting in > > Tx timeout firing, for the h/w tx timestamping acceleration case. > > > > There's no easy way to obtain the number of bytes on wire in the Tx > > confirmation routine, so skb->cb is used to convey that information > > from xmit to Tx confirmation, for now (as proposed by Eric). Revived > > the currently unused GFAR_CB() construct for that purpose. > > I do not see much difference between what this patch does and what the > current net-next drivers does. If you need to correctly account for > this, it seems to me like you should move the stats/bytes_sent > computation below this line: > > if (unlikely(do_tstamp)) { > skb_push(skb, GMAC_TXPAL_LEN); > memset(skb->data, 0, GMAC_TXPAL_LEN); > } > > to account for the SKB length update? Just realized that this is precisely what your patch is fixing, sorry for the noise. -- Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL 2013-08-30 12:01 ` [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL Claudiu Manoil 2013-09-03 18:59 ` Florian Fainelli @ 2013-09-04 2:14 ` David Miller 1 sibling, 0 replies; 14+ messages in thread From: David Miller @ 2013-09-04 2:14 UTC (permalink / raw) To: claudiu.manoil; +Cc: netdev, eric.dumazet From: Claudiu Manoil <claudiu.manoil@freescale.com> Date: Fri, 30 Aug 2013 15:01:15 +0300 > Fix the amount of sent bytes reported to BQL by reporting the > number of bytes on wire in the xmit routine, and recording that > value for each skb in order to be correctly confirmed on Tx > confirmation cleanup. > > Reporting skb->len to BQL just before exiting xmit is not correct > due to possible insertions of TOE block and alignment bytes in the > skb->data, which are being stripped off by the controller before > transmission on wire. This led to mismatch of (incorrectly) > reported bytes to BQL b/w xmit and Tx confirmation, resulting in > Tx timeout firing, for the h/w tx timestamping acceleration case. > > There's no easy way to obtain the number of bytes on wire in the Tx > confirmation routine, so skb->cb is used to convey that information > from xmit to Tx confirmation, for now (as proposed by Eric). Revived > the currently unused GFAR_CB() construct for that purpose. > > Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> Applied. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping 2013-04-29 15:23 ` Eric Dumazet 2013-04-29 15:28 ` Eric Dumazet @ 2013-04-29 15:33 ` Eric Dumazet 1 sibling, 0 replies; 14+ messages in thread From: Eric Dumazet @ 2013-04-29 15:33 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, Paul Gortmaker, David S. Miller On Mon, 2013-04-29 at 08:23 -0700, Eric Dumazet wrote: > > Does BQL really need to measure the bytes-on-wire or the bytes consumed > > for buffering? > > Yes. I meant, bytes on wire. If it was bytes consumed for buffering, we would use skb->truesize here. Anyway, I am not sure your fix is enough if VLAN is used for example. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-09-04 2:14 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-29 12:57 [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping Claudiu Manoil 2013-04-29 13:53 ` Eric Dumazet 2013-04-29 14:27 ` Claudiu Manoil 2013-04-29 15:23 ` Eric Dumazet 2013-04-29 15:28 ` Eric Dumazet 2013-04-29 15:39 ` Eric Dumazet 2013-04-29 15:42 ` Eric Dumazet 2013-04-29 15:52 ` Claudiu Manoil 2013-04-29 15:59 ` Eric Dumazet 2013-08-30 12:01 ` [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL Claudiu Manoil 2013-09-03 18:59 ` Florian Fainelli 2013-09-03 19:18 ` Florian Fainelli 2013-09-04 2:14 ` David Miller 2013-04-29 15:33 ` [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping Eric Dumazet
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).