* Re: [PATCH 0/1] mv643xx_eth: Disable TSO by default
2014-11-01 17:26 ` [PATCH 0/1] mv643xx_eth: Disable TSO " Eric Dumazet
@ 2014-11-01 17:33 ` Thomas Petazzoni
2014-11-01 18:01 ` Eric Dumazet
2014-11-01 17:37 ` Eric Dumazet
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Thomas Petazzoni @ 2014-11-01 17:33 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ezequiel Garcia, netdev, David Miller, Gregory Clement,
Tawfik Bayouk, Lior Amsalem, Nadav Haklai
Dear Eric Dumazet,
On Sat, 01 Nov 2014 10:26:06 -0700, Eric Dumazet wrote:
> On Sat, 2014-11-01 at 12:30 -0300, Ezequiel Garcia wrote:
> > Several users ([1], [2]) have been reporting data corruption with TSO on
> > Kirkwood platforms (i.e. using the mv643xx_eth driver).
> >
> > Until we manage to find what's causing this, this simple patch will make
> > the TSO path disabled by default. This patch should be queued for stable,
> > fixing the TSO feature introduced in v3.16.
> >
> > The corruption itself is very easy to reproduce: checking md5sum on a mounted
> > NFS directory gives a different result each time. Same tests using the mvneta
> > driver (Armada 370/38x/XP SoC) pass with no issues.
> >
> > Frankly, I'm a bit puzzled about this, and so any ideas or debugging hints
> > are well received.
>
> lack of barriers maybe ?
>
> It seems you might need to populate all TX descriptors but delay the
> first, like doing the populate in descending order.
>
> If you take a look at txq_submit_skb(), you'll see the final
> desc->cmd_sts = cmd_sts (line 959) is done _after_ frags were cooked by
> txq_submit_frag_skb()
>
> You should kick the nick only when all TX descriptors are ready and
> committed to memory.
As far as I know, ARMv5 does not do out-of-order execution, and so on
ARMv5, mb() == rmb() == wmb() == barrier(). But might be a missing
compiler barrier, indeed, as that's not specific to the architecture.
Thanks for the hint!
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/1] mv643xx_eth: Disable TSO by default
2014-11-01 17:33 ` Thomas Petazzoni
@ 2014-11-01 18:01 ` Eric Dumazet
0 siblings, 0 replies; 15+ messages in thread
From: Eric Dumazet @ 2014-11-01 18:01 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Ezequiel Garcia, netdev, David Miller, Gregory Clement,
Tawfik Bayouk, Lior Amsalem, Nadav Haklai
On Sat, 2014-11-01 at 18:33 +0100, Thomas Petazzoni wrote:
> Dear Eric Dumazet,
You can call me Eric ;)
> As far as I know, ARMv5 does not do out-of-order execution, and so on
> ARMv5, mb() == rmb() == wmb() == barrier(). But might be a missing
> compiler barrier, indeed, as that's not specific to the architecture.
>
> Thanks for the hint!
I think the wmb() is needed, but the more worrying problem is the NIC
can start transmitting a segment while the data payload is not yet
placed in TX descriptor.
Normally, the patch I cooked should avoid this.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/1] mv643xx_eth: Disable TSO by default
2014-11-01 17:26 ` [PATCH 0/1] mv643xx_eth: Disable TSO " Eric Dumazet
2014-11-01 17:33 ` Thomas Petazzoni
@ 2014-11-01 17:37 ` Eric Dumazet
2014-11-01 19:05 ` Ezequiel Garcia
2014-11-01 17:42 ` David Miller
2014-11-03 14:51 ` David Laight
3 siblings, 1 reply; 15+ messages in thread
From: Eric Dumazet @ 2014-11-01 17:37 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: netdev, David Miller, Thomas Petazzoni, Gregory Clement,
Tawfik Bayouk, Lior Amsalem, Nadav Haklai
On Sat, 2014-11-01 at 10:26 -0700, Eric Dumazet wrote:
> On Sat, 2014-11-01 at 12:30 -0300, Ezequiel Garcia wrote:
> > Several users ([1], [2]) have been reporting data corruption with TSO on
> > Kirkwood platforms (i.e. using the mv643xx_eth driver).
> >
> > Until we manage to find what's causing this, this simple patch will make
> > the TSO path disabled by default. This patch should be queued for stable,
> > fixing the TSO feature introduced in v3.16.
> >
> > The corruption itself is very easy to reproduce: checking md5sum on a mounted
> > NFS directory gives a different result each time. Same tests using the mvneta
> > driver (Armada 370/38x/XP SoC) pass with no issues.
> >
> > Frankly, I'm a bit puzzled about this, and so any ideas or debugging hints
> > are well received.
>
> lack of barriers maybe ?
>
> It seems you might need to populate all TX descriptors but delay the
> first, like doing the populate in descending order.
>
> If you take a look at txq_submit_skb(), you'll see the final
> desc->cmd_sts = cmd_sts (line 959) is done _after_ frags were cooked by
> txq_submit_frag_skb()
>
> You should kick the nick only when all TX descriptors are ready and
> committed to memory.
>
Untested patch would be :
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index b151a949f352a20ec8e74b4f3a7b6bb194ce841c..44789cc9a263992f91e46006d7e12703a2824cb4 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -773,7 +773,8 @@ txq_put_data_tso(struct net_device *dev, struct tx_queue *txq,
}
static inline void
-txq_put_hdr_tso(struct sk_buff *skb, struct tx_queue *txq, int length)
+txq_put_hdr_tso(struct sk_buff *skb, struct tx_queue *txq, int length,
+ struct tx_desc **pdesc, u32 *cmd)
{
struct mv643xx_eth_private *mp = txq_to_mp(txq);
int hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
@@ -797,9 +798,13 @@ txq_put_hdr_tso(struct sk_buff *skb, struct tx_queue *txq, int length)
desc->byte_cnt = hdr_len;
desc->buf_ptr = txq->tso_hdrs_dma +
txq->tx_curr_desc * TSO_HEADER_SIZE;
- desc->cmd_sts = cmd_csum | BUFFER_OWNED_BY_DMA | TX_FIRST_DESC |
- GEN_CRC;
-
+ cmd_csum |= BUFFER_OWNED_BY_DMA | TX_FIRST_DESC | GEN_CRC;
+ if (*pdesc == NULL) {
+ *pdesc = desc;
+ *cmd = cmd_csum;
+ } else {
+ desc->cmd_sts = cmd_csum;
+ }
txq->tx_curr_desc++;
if (txq->tx_curr_desc == txq->tx_ring_size)
txq->tx_curr_desc = 0;
@@ -813,6 +818,8 @@ static int txq_submit_tso(struct tx_queue *txq, struct sk_buff *skb,
int desc_count = 0;
struct tso_t tso;
int hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
+ struct tx_desc *desc = NULL;
+ u32 cmd_sts = 0;
/* Count needed descriptors */
if ((txq->tx_desc_count + tso_count_descs(skb)) >= txq->tx_ring_size) {
@@ -834,7 +841,7 @@ static int txq_submit_tso(struct tx_queue *txq, struct sk_buff *skb,
/* prepare packet headers: MAC + IP + TCP */
hdr = txq->tso_hdrs + txq->tx_curr_desc * TSO_HEADER_SIZE;
tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
- txq_put_hdr_tso(skb, txq, data_left);
+ txq_put_hdr_tso(skb, txq, data_left, &desc, &cmd_sts);
while (data_left > 0) {
int size;
@@ -854,6 +861,10 @@ static int txq_submit_tso(struct tx_queue *txq, struct sk_buff *skb,
__skb_queue_tail(&txq->tx_skb, skb);
skb_tx_timestamp(skb);
+ /* ensure all other descriptors are written before first cmd_sts */
+ wmb();
+ desc->cmd_sts = cmd_sts;
+
/* clear TX_END status */
mp->work_tx_end &= ~(1 << txq->index);
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 0/1] mv643xx_eth: Disable TSO by default
2014-11-01 17:37 ` Eric Dumazet
@ 2014-11-01 19:05 ` Ezequiel Garcia
0 siblings, 0 replies; 15+ messages in thread
From: Ezequiel Garcia @ 2014-11-01 19:05 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, David Miller, Thomas Petazzoni, Gregory Clement,
Tawfik Bayouk, Lior Amsalem, Nadav Haklai
[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]
On 11/01/2014 02:37 PM, Eric Dumazet wrote:
> On Sat, 2014-11-01 at 10:26 -0700, Eric Dumazet wrote:
>> On Sat, 2014-11-01 at 12:30 -0300, Ezequiel Garcia wrote:
>>> Several users ([1], [2]) have been reporting data corruption with TSO on
>>> Kirkwood platforms (i.e. using the mv643xx_eth driver).
>>>
>>> Until we manage to find what's causing this, this simple patch will make
>>> the TSO path disabled by default. This patch should be queued for stable,
>>> fixing the TSO feature introduced in v3.16.
>>>
>>> The corruption itself is very easy to reproduce: checking md5sum on a mounted
>>> NFS directory gives a different result each time. Same tests using the mvneta
>>> driver (Armada 370/38x/XP SoC) pass with no issues.
>>>
>>> Frankly, I'm a bit puzzled about this, and so any ideas or debugging hints
>>> are well received.
>>
>> lack of barriers maybe ?
>>
Yup, that was my initial thought as well...
>> It seems you might need to populate all TX descriptors but delay the
>> first, like doing the populate in descending order.
>>
>> If you take a look at txq_submit_skb(), you'll see the final
>> desc->cmd_sts = cmd_sts (line 959) is done _after_ frags were cooked by
>> txq_submit_frag_skb()
>>
>> You should kick the nick only when all TX descriptors are ready and
>> committed to memory.
>>
>
> Untested patch would be :
>
Yeah, it makes sense. I'm still seeing the corruption after applying
your patch.
However, maybe we are onto something. I'll see about taking a closer
look and give this some more thought.
Thanks for the hint!
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/1] mv643xx_eth: Disable TSO by default
2014-11-01 17:26 ` [PATCH 0/1] mv643xx_eth: Disable TSO " Eric Dumazet
2014-11-01 17:33 ` Thomas Petazzoni
2014-11-01 17:37 ` Eric Dumazet
@ 2014-11-01 17:42 ` David Miller
2014-11-03 14:51 ` David Laight
3 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2014-11-01 17:42 UTC (permalink / raw)
To: eric.dumazet
Cc: ezequiel.garcia, netdev, thomas.petazzoni, gregory.clement,
tawfik, alior, nadavh
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 01 Nov 2014 10:26:06 -0700
> On Sat, 2014-11-01 at 12:30 -0300, Ezequiel Garcia wrote:
>> Several users ([1], [2]) have been reporting data corruption with TSO on
>> Kirkwood platforms (i.e. using the mv643xx_eth driver).
>>
>> Until we manage to find what's causing this, this simple patch will make
>> the TSO path disabled by default. This patch should be queued for stable,
>> fixing the TSO feature introduced in v3.16.
>>
>> The corruption itself is very easy to reproduce: checking md5sum on a mounted
>> NFS directory gives a different result each time. Same tests using the mvneta
>> driver (Armada 370/38x/XP SoC) pass with no issues.
>>
>> Frankly, I'm a bit puzzled about this, and so any ideas or debugging hints
>> are well received.
>
> lack of barriers maybe ?
>
> It seems you might need to populate all TX descriptors but delay the
> first, like doing the populate in descending order.
>
> If you take a look at txq_submit_skb(), you'll see the final
> desc->cmd_sts = cmd_sts (line 959) is done _after_ frags were cooked by
> txq_submit_frag_skb()
>
> You should kick the nick only when all TX descriptors are ready and
> committed to memory.
Yes, please look into whether doing something like this fixes the
problem.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 0/1] mv643xx_eth: Disable TSO by default
2014-11-01 17:26 ` [PATCH 0/1] mv643xx_eth: Disable TSO " Eric Dumazet
` (2 preceding siblings ...)
2014-11-01 17:42 ` David Miller
@ 2014-11-03 14:51 ` David Laight
2014-11-03 19:04 ` Eric Dumazet
3 siblings, 1 reply; 15+ messages in thread
From: David Laight @ 2014-11-03 14:51 UTC (permalink / raw)
To: 'Eric Dumazet', Ezequiel Garcia
Cc: netdev@vger.kernel.org, David Miller, Thomas Petazzoni,
Gregory Clement, Tawfik Bayouk, Lior Amsalem, Nadav Haklai
From: Eric Dumazet
> On Sat, 2014-11-01 at 12:30 -0300, Ezequiel Garcia wrote:
> > Several users ([1], [2]) have been reporting data corruption with TSO on
> > Kirkwood platforms (i.e. using the mv643xx_eth driver).
> >
> > Until we manage to find what's causing this, this simple patch will make
> > the TSO path disabled by default. This patch should be queued for stable,
> > fixing the TSO feature introduced in v3.16.
> >
> > The corruption itself is very easy to reproduce: checking md5sum on a mounted
> > NFS directory gives a different result each time. Same tests using the mvneta
> > driver (Armada 370/38x/XP SoC) pass with no issues.
> >
> > Frankly, I'm a bit puzzled about this, and so any ideas or debugging hints
> > are well received.
>
> lack of barriers maybe ?
>
> It seems you might need to populate all TX descriptors but delay the
> first, like doing the populate in descending order.
>
> If you take a look at txq_submit_skb(), you'll see the final
> desc->cmd_sts = cmd_sts (line 959) is done _after_ frags were cooked by
> txq_submit_frag_skb()
>
> You should kick the nick only when all TX descriptors are ready and
> committed to memory.
Don't forget that the nick might process the first descriptor without
being given a 'kick' - it will read it when it finishes processing the
previous frame.
This also means that you have to be careful about the order of the writes
to the first descriptor.
David
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/1] mv643xx_eth: Disable TSO by default
2014-11-03 14:51 ` David Laight
@ 2014-11-03 19:04 ` Eric Dumazet
0 siblings, 0 replies; 15+ messages in thread
From: Eric Dumazet @ 2014-11-03 19:04 UTC (permalink / raw)
To: David Laight
Cc: Ezequiel Garcia, netdev@vger.kernel.org, David Miller,
Thomas Petazzoni, Gregory Clement, Tawfik Bayouk, Lior Amsalem,
Nadav Haklai
On Mon, 2014-11-03 at 14:51 +0000, David Laight wrote:
> From: Eric Dumazet
> > On Sat, 2014-11-01 at 12:30 -0300, Ezequiel Garcia wrote:
> > > Several users ([1], [2]) have been reporting data corruption with TSO on
> > > Kirkwood platforms (i.e. using the mv643xx_eth driver).
> > >
> > > Until we manage to find what's causing this, this simple patch will make
> > > the TSO path disabled by default. This patch should be queued for stable,
> > > fixing the TSO feature introduced in v3.16.
> > >
> > > The corruption itself is very easy to reproduce: checking md5sum on a mounted
> > > NFS directory gives a different result each time. Same tests using the mvneta
> > > driver (Armada 370/38x/XP SoC) pass with no issues.
> > >
> > > Frankly, I'm a bit puzzled about this, and so any ideas or debugging hints
> > > are well received.
> >
> > lack of barriers maybe ?
> >
> > It seems you might need to populate all TX descriptors but delay the
> > first, like doing the populate in descending order.
> >
> > If you take a look at txq_submit_skb(), you'll see the final
> > desc->cmd_sts = cmd_sts (line 959) is done _after_ frags were cooked by
> > txq_submit_frag_skb()
> >
> > You should kick the nick only when all TX descriptors are ready and
> > committed to memory.
>
> Don't forget that the nick might process the first descriptor without
> being given a 'kick' - it will read it when it finishes processing the
> previous frame.
> This also means that you have to be careful about the order of the writes
> to the first descriptor.
This is what I implied and implemented in the patch ;)
^ permalink raw reply [flat|nested] 15+ messages in thread