pch_gbe: Adding read memory barriers

From bb1e271db0fa1a29df19bede69faf8004389132d Mon Sep 17 00:00:00 2001
From: Erwan Velu <[email protected]>
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.

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

index 8035e5f..ace3654 100644
@@ -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

Hey Fellows,

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

Cheers,
Erwan

From: Erwan Velu <[email protected]>
Date: Tue, 08 May 2012 20:35:28 +0200

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

Also, the usual MMIO functions already include compiler barriers.

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.

this type of message.

points.

?

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

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.

[...]

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.