[PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Claudiu Manoil]

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

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Eric Dumazet]

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.

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Claudiu Manoil]

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.
>
>
>
>
>

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Eric Dumazet]

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

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Eric Dumazet]

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[]

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Eric Dumazet]

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);
 

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Eric Dumazet]

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;

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Claudiu Manoil]

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

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Eric Dumazet]

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.

[PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL

[Claudiu Manoil]

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

Re: [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL

[Florian Fainelli]

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

Re: [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL

[Florian Fainelli]

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

Re: [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL

[David Miller]

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.

Re: [PATCH][net-next] gianfar: Fix reported sent bytes to BQL for Tx timestamping

[Eric Dumazet]

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.