[PATCH] pch_gbe: Adding read memory barriers

[PATCH] pch_gbe: Adding read memory barriers

[Erwan Velu]

 From bb1e271db0fa1a29df19bede69faf8004389132d Mon Sep 17 00:00:00 2001
From: Erwan Velu <erwan.velu@zodiacaerospace.com>
Date: Mon, 7 May 2012 19:15:29 +0000
Subject: [PATCH 1/1] pch_gbe: Adding read memory barriers

Under a strong incoming packet stream and/or high cpu usage,
the pch_gbe driver reports "Receive CRC Error" and discards packet.

It occurred on an Intel ATOM E620T while running a 300mbit/sec multicast
network stream leading to a ~100% cpu usage.

Adding rmb() calls before considering the network card's status solve
this issue.

Getting it into stable would be perfect as it solves reliability issues.

Signed-off-by: Erwan Velu <erwan.velu@zodiacaerospace.com>
---
  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c 
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 8035e5f..ace3654 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -1413,6 +1413,7 @@ static irqreturn_t pch_gbe_intr(int irq, void *data)
  			pch_gbe_mac_set_pause_packet(hw);
  		}
  	}
+	smp_rmb(); /* prevent any other reads before*/

  	/* When request status is Receive interruption */
  	if ((int_st & (PCH_GBE_INT_RX_DMA_CMPLT | PCH_GBE_INT_TX_CMPLT)) ||
@@ -1582,6 +1584,7 @@ pch_gbe_clean_tx(struct pch_gbe_adapter *adapter,

  	i = tx_ring->next_to_clean;
  	tx_desc = PCH_GBE_TX_DESC(*tx_ring, i);
+	rmb();  /* prevent any other reads before*/
  	pr_debug("gbec_status:0x%04x  dma_status:0x%04x\n",
  		 tx_desc->gbec_status, tx_desc->dma_status);

@@ -1682,6 +1685,7 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
  	while (*work_done < work_to_do) {
  		/* Check Rx descriptor status */
  		rx_desc = PCH_GBE_RX_DESC(*rx_ring, i);
+		rmb(); /* prevent any other reads before*/
  		if (rx_desc->gbec_status == DSC_INIT16)
  			break;
  		cleaned = true;
-- 
1.7.3.4

Re: [PATCH] pch_gbe: Adding read memory barriers

[Erwan Velu]

Le 07/05/2012 21:30, Erwan Velu a écrit :
> From bb1e271db0fa1a29df19bede69faf8004389132d Mon Sep 17 00:00:00 2001
> From: Erwan Velu <erwan.velu@zodiacaerospace.com>
> Date: Mon, 7 May 2012 19:15:29 +0000
> Subject: [PATCH 1/1] pch_gbe: Adding read memory barriers


Hey Fellows,

Does my patch can be considered as acceptable or shall I rework 
something on it ?

Cheers,
Erwan

Re: [PATCH] pch_gbe: Adding read memory barriers

[David Miller]

From: Erwan Velu <erwanaliasr1@gmail.com>
Date: Tue, 08 May 2012 20:35:28 +0200

> Le 07/05/2012 21:30, Erwan Velu a écrit :
>> From bb1e271db0fa1a29df19bede69faf8004389132d Mon Sep 17 00:00:00 2001
>> From: Erwan Velu <erwan.velu@zodiacaerospace.com>
>> Date: Mon, 7 May 2012 19:15:29 +0000
>> Subject: [PATCH 1/1] pch_gbe: Adding read memory barriers
> 
> Does my patch can be considered as acceptable or shall I rework
> something on it ?

You never need to ask questions like this.

Your patch is queued up to be reviewed in patchwork:

http://patchwork.ozlabs.org/project/netdev/list/

Therefore you only make more work for maintainers and irritate them by
asking this, and therefore it will take even longer for them to get to
your patch.

Re: [PATCH] pch_gbe: Adding read memory barriers

[Erwan Velu]

Le 08/05/2012 20:39, David Miller a écrit :
> You never need to ask questions like this. Your patch is queued up to 
> be reviewed in patchwork: 
> http://patchwork.ozlabs.org/project/netdev/list/ Therefore you only 
> make more work for maintainers and irritate them by asking this, and 
> therefore it will take even longer for them to get to your patch. 

It wasn't my aim to irritate anyone. I'm just brand new into committing 
something here and surely lack of a good understanding on the complete 
process.

I do understand you are very solicited and newbies like me can sometimes 
irritate by asking questions & doing thing not properly.

Anyway, thanks for your help & patience.

Erwan

RE: [PATCH] pch_gbe: Adding read memory barriers

[David Laight]

 
> Under a strong incoming packet stream and/or high cpu usage,
> the pch_gbe driver reports "Receive CRC Error" and discards packet.
> 
> It occurred on an Intel ATOM E620T while running a 
> 300mbit/sec multicast
> network stream leading to a ~100% cpu usage.
> 
> Adding rmb() calls before considering the network card's status solve
> this issue.
> 
> Getting it into stable would be perfect as it solves 
> reliability issues.
> 
> Signed-off-by: Erwan Velu <erwan.velu@zodiacaerospace.com>
> ---
>   .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c 
> b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> index 8035e5f..ace3654 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> @@ -1413,6 +1413,7 @@ static irqreturn_t pch_gbe_intr(int 
> irq, void *data)
>   			pch_gbe_mac_set_pause_packet(hw);
>   		}
>   	}
> +	smp_rmb(); /* prevent any other reads before*/

Under the assumption that your memory references are uncached,
you only need to stop gcc reordering the object code,
Rather than actually adding one of the 'fence' instructions.

So you should only need: asm volatile(:::"memory")
NFI which define generates that, the defines in the copy of
sysdep.h I just looked at always include one of the fences.

	David

RE: [PATCH] pch_gbe: Adding read memory barriers

[Ben Hutchings]

On Wed, 2012-05-09 at 11:47 +0100, David Laight wrote:
> > Under a strong incoming packet stream and/or high cpu usage,
> > the pch_gbe driver reports "Receive CRC Error" and discards packet.
> > 
> > It occurred on an Intel ATOM E620T while running a 
> > 300mbit/sec multicast
> > network stream leading to a ~100% cpu usage.
> > 
> > Adding rmb() calls before considering the network card's status solve
> > this issue.
> > 
> > Getting it into stable would be perfect as it solves 
> > reliability issues.
> > 
> > Signed-off-by: Erwan Velu <erwan.velu@zodiacaerospace.com>
> > ---
> >   .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    3 +++
> >   1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c 
> > b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > index 8035e5f..ace3654 100644
> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > @@ -1413,6 +1413,7 @@ static irqreturn_t pch_gbe_intr(int 
> > irq, void *data)
> >   			pch_gbe_mac_set_pause_packet(hw);
> >   		}
> >   	}
> > +	smp_rmb(); /* prevent any other reads before*/
> 
> Under the assumption that your memory references are uncached,
> you only need to stop gcc reordering the object code,
> Rather than actually adding one of the 'fence' instructions.

Also, the usual MMIO functions already include compiler barriers.

> So you should only need: asm volatile(:::"memory")
> NFI which define generates that, the defines in the copy of
> sysdep.h I just looked at always include one of the fences.

This is barrier().  But I think this must be intended to control
ordering of fields that are written elsewhere.

Really, this needs a much more specific comment to explain the intent.
Then reviewers can work out whether it actually achieves the intent!

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [PATCH] pch_gbe: Adding read memory barriers

[David Laight]

 
> So I've been reading how does others drivers are working regarding
this type of message.
> While reading many drivers, I found that many does rmb() calls at this
points.
> As I'm not a linux kernel expert, I did mimic them to the result
> and it was good since the errors disappeared.

> So do you mean the patch is wrong or shall I rework only the comments
?

The problem is that barriers are not understood at all well
by most driver writers - so a lot of code is sub-optimal.

It isn't helped by the readability of the cpu documentation,
never mind some old docs that described something that an
engineer thought they might want to allow - rather than
describing what any cpus actually did/do.

For x86 cpus (amd and intel at least) reads and writes to
uncached memory and io are guaranteed to happen in instruction
order. For other cpus (sparc and ppc) such reads can
overtake writes - requiring something to force the write to
complete (eg a synchronising instruction, or maybe a readeback).

One problem is that the barrier instructions are generally
slow - so you don't want to use them where unnecessary,
however whether you need them is somewhat hardware specific
and the typical OS lists of barriers doesn't cover all the
cases - even for things that are actually compile time knowable.
Eg: what you need between a write to 'dma coherent' memory
and a write to a control register depends on whether 'dma
coherency' is implemented by disabling the cache.

For SMP there may be additional synchronisation, especially
for cached data, but for most drivers that is handled by
mutex/lock operations.

The other, and separate, issue is making sure that the compiler
itself doesn't reorder of optimise away memory cycles.
By far the best way is to mark the data (not the pointer to the
data) 'volatile', this forces the compiler to generate object
code for every variable reference in the source, and in the right
order.
Sometimes that is inappropiate, gcc's asm volatile (:::"memory")
tells the compiler that this asm statement might modify all
of memory (even though there are actually no instructions!).
This forces it to avoid having anything in a register that is
a copy of something in memory - thus enforcing the order of
memory accesses.
It can also be used to reduce register pressure within the
compiler.

In your case I suspect that the compiler has re-ordered the
reads - adding almost anything between them will change the
order.

	David

Re: [PATCH] pch_gbe: Adding read memory barriers

[Ben Hutchings]

On Wed, 2012-05-09 at 17:20 +0200, Erwan Velu wrote:
> Thanks for this very clear description.

It might be clear, but it's not very relevant: you need to understand
the Linux kernel barrier APIs (which cover all architectures).  See
Documentation/memory-barriers.txt.

> As I'm pretty new to this kind of integration in the kernel, do you
> request me to test this volatile option ?
> Does the patch tends to be rejected for this issue ?
> What should be the next step for my patch ?
[...]

Explain what kind of reordering you are trying to avoid.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH] pch_gbe: Adding read memory barriers

[Erwan Velu]

 From 3b65802e4c5a8827a84022066f10dec4d61c1f22 Mon Sep 17 00:00:00 2001
From: Erwan Velu <erwan.velu@zodiacaerospace.com>
Date: Mon, 7 May 2012 14:53:17 +0200
Subject: [PATCH 1/1] pch_gbe: Adding read memory barriers

Under a strong incoming packet stream and/or high cpu usage,
the pch_gbe driver reports "Receive CRC Error" and discards packet.

It occurred on an Intel ATOM E620T while running a 300mbit/sec multicast 
network stream leading to a ~100% cpu usage.

Adding rmb() calls before considering the network card's status solve
this issues.

This patch got validated on the 3.2.16 kernel but also apply to the 3.x 
family.

Getting it into stable would be perfect as it solves reliability issues.


Signed-off-by: Erwan Velu <erwan.velu@zodiacaerospace.com>
---
  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    8 ++++++++
  1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c 
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 48406ca..7746ca3 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -1222,6 +1222,8 @@ static irqreturn_t pch_gbe_intr(int irq, void *data)
  		}
  	}

+	smp_rmb(); /* prevent any other reads before*/
+
  	/* When request status is Receive interruption */
  	if ((int_st & (PCH_GBE_INT_RX_DMA_CMPLT | PCH_GBE_INT_TX_CMPLT)) ||
  	    (adapter->rx_stop_flag == true)) {
@@ -1390,6 +1392,9 @@ pch_gbe_clean_tx(struct pch_gbe_adapter *adapter,

  	i = tx_ring->next_to_clean;
  	tx_desc = PCH_GBE_TX_DESC(*tx_ring, i);
+
+	rmb();  /* prevent any other reads before*/
+
  	pr_debug("gbec_status:0x%04x  dma_status:0x%04x\n",
  		 tx_desc->gbec_status, tx_desc->dma_status);

@@ -1490,6 +1495,9 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
  	while (*work_done < work_to_do) {
  		/* Check Rx descriptor status */
  		rx_desc = PCH_GBE_RX_DESC(*rx_ring, i);
+
+		rmb(); /* prevent any other reads before*/
+
  		if (rx_desc->gbec_status == DSC_INIT16)
  			break;
  		cleaned = true;
-- 
1.7.4.4

Re: [PATCH] pch_gbe: Adding read memory barriers

[David Miller]

Your patch doesn't apply to the net-next tree which is what you should
be basing all of your networking patches on:

[davem@bql net-next]$ git am --signoff pch_gbe-Adding-read-memory-barriers.patch 
Applying: pch_gbe: Adding read memory barriers
error: patch failed: drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:1222
error: drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c: patch does not apply
Patch failed at 0001 pch_gbe: Adding read memory barriers
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Stop rushing things and take your time learning the process.

Otherwise you're going to make more work for maintainers and they
end up grumpy as a result, which you don't want.

[PATCH] pch_gbe: Adding read memory barriers

[Erwan Velu]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

Dear Linux Kernel Developers,

Please find attached, a patch to solve "Received CRC" errors reported by 
the pch_gbe driver under heavy load. It occurred on an Intel ATOM E620T 
while running a 300mbit/sec multicast network stream leading to a ~100% 
cpu usage.

This patch got validated on the 3.2.16 kernel but also apply to the 3.x
family.

Getting it into stable would be perfect as it solves reliability issues.

Cheers,
Erwan Velu

[-- Attachment #2: 0001-pch_gbe-Adding-read-memory-barriers.patch --]
[-- Type: application/octet-stream, Size: 1805 bytes --]

From 3b65802e4c5a8827a84022066f10dec4d61c1f22 Mon Sep 17 00:00:00 2001
From: Erwan Velu <erwan.velu@zodiacaerospace.com>
Date: Mon, 7 May 2012 14:53:17 +0200
Subject: [PATCH 1/1] pch_gbe: Adding read memory barriers

Under a strong incoming packet stream and/or high cpu usage,
the pch_gbe driver reports "Receive CRC Error" and discards packet.

Adding rmb() calls before considering the network card's status solve
this issues.

Signed-off-by: Erwan Velu <erwan.velu@zodiacaerospace.com>
---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 48406ca..7746ca3 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -1222,6 +1222,8 @@ static irqreturn_t pch_gbe_intr(int irq, void *data)
 		}
 	}
 
+	smp_rmb(); /* prevent any other reads before*/
+
 	/* When request status is Receive interruption */
 	if ((int_st & (PCH_GBE_INT_RX_DMA_CMPLT | PCH_GBE_INT_TX_CMPLT)) ||
 	    (adapter->rx_stop_flag == true)) {
@@ -1390,6 +1392,9 @@ pch_gbe_clean_tx(struct pch_gbe_adapter *adapter,
 
 	i = tx_ring->next_to_clean;
 	tx_desc = PCH_GBE_TX_DESC(*tx_ring, i);
+
+	rmb();  /* prevent any other reads before*/
+
 	pr_debug("gbec_status:0x%04x  dma_status:0x%04x\n",
 		 tx_desc->gbec_status, tx_desc->dma_status);
 
@@ -1490,6 +1495,9 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
 	while (*work_done < work_to_do) {
 		/* Check Rx descriptor status */
 		rx_desc = PCH_GBE_RX_DESC(*rx_ring, i);
+
+		rmb(); /* prevent any other reads before*/
+
 		if (rx_desc->gbec_status == DSC_INIT16)
 			break;
 		cleaned = true;
-- 
1.7.4.4

Re: [PATCH] pch_gbe: Adding read memory barriers

[David Miller]

From: Erwan Velu <erwanaliasr1@gmail.com>
Date: Mon, 07 May 2012 19:39:47 +0200

> Please find attached

First of all, for a patch which is not accepted yet you do not
CC: stable.

Second of all, do not put text in the main body of your email which is
unrelated to the patch and should not end up in the commit message.

Instead, post your patch to the appropriate primary mailing lists,
and if it's accepted it can then be submitted to -stable at some
later time.

Your patch posting email should be composed purely of the commit
log message in the message body, followed by the actual patch.

Otherwise the maintainer that applies your patch has to edit out
all of this other flowery text that is unrelated to the commit
and that makes more work for them.