* [PATCH 3/4] r8169: Use fast_rmb() and fast_wmb() for DescOwn checks
From: Alexander Duyck @ 2014-11-17 17:18 UTC (permalink / raw)
To: linux-arch, netdev, linux-kernel
Cc: mathieu.desnoyers, peterz, benh, heiko.carstens, mingo, mikey,
linux, donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher,
romieu, paulmck, nic_swsd, will.deacon, michael, tony.luck,
torvalds, oleg, schwidefsky, fweisbec, davem
In-Reply-To: <20141117171005.22333.96544.stgit@ahduyck-server>
The r8169 use a pair of wmb() calls when setting up the descriptor rings.
The first is to synchronize the descriptor data with the descriptor status,
and the second is to synchronize the descriptor status with the use of the
MMIO doorbell to notify the device that descriptors are ready. This can come
at a heavy price on some systems, and is not really necessary on systems such
as x86 as a simple barrier() would suffice to order store/store accesses.
As such we can replace the first memory barrier with fast_wmb() to reduce
the cost for these accesses.
In addition the r8169 uses a rmb() to prevent compiler optimization in the
cleanup paths, however by moving the barrier down a few lines and
replacing with with a fast_rmb() we should be able to use it to guarantee
descriptor accesses do not occur until the device has updated the DescOwn
bit from its end.
One last change I made is to move the update of cur_tx in the xmit path to
after the wmb. This way we can guarantee the device and all CPUs should
see the DescOwn update before they see the cur_tx value update.
Cc: Realtek linux nic maintainers <nic_swsd@realtek.com>
Cc: Francois Romieu <romieu@fr.zoreil.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
drivers/net/ethernet/realtek/r8169.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index cf154f7..be79447 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6601,6 +6601,9 @@ static inline void rtl8169_mark_to_asic(struct RxDesc *desc, u32 rx_buf_sz)
{
u32 eor = le32_to_cpu(desc->opts1) & RingEnd;
+ /* Force memory writes to complete before releasing descriptor */
+ fast_wmb();
+
desc->opts1 = cpu_to_le32(DescOwn | eor | rx_buf_sz);
}
@@ -6608,7 +6611,6 @@ static inline void rtl8169_map_to_asic(struct RxDesc *desc, dma_addr_t mapping,
u32 rx_buf_sz)
{
desc->addr = cpu_to_le64(mapping);
- wmb();
rtl8169_mark_to_asic(desc, rx_buf_sz);
}
@@ -7077,16 +7079,18 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
skb_tx_timestamp(skb);
- wmb();
+ /* Force memory writes to complete before releasing descriptor */
+ fast_wmb();
/* Anti gcc 2.95.3 bugware (sic) */
status = opts[0] | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
txd->opts1 = cpu_to_le32(status);
- tp->cur_tx += frags + 1;
-
+ /* Force all memory writes to complete before notifying device */
wmb();
+ tp->cur_tx += frags + 1;
+
RTL_W8(TxPoll, NPQ);
mmiowb();
@@ -7185,11 +7189,16 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
struct ring_info *tx_skb = tp->tx_skb + entry;
u32 status;
- rmb();
status = le32_to_cpu(tp->TxDescArray[entry].opts1);
if (status & DescOwn)
break;
+ /* This barrier is needed to keep us from reading
+ * any other fields out of the Tx descriptor until
+ * we know the status of DescOwn
+ */
+ fast_rmb();
+
rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
tp->TxDescArray + entry);
if (status & LastFrag) {
@@ -7284,11 +7293,16 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, u32 budget
struct RxDesc *desc = tp->RxDescArray + entry;
u32 status;
- rmb();
status = le32_to_cpu(desc->opts1) & tp->opts1_mask;
-
if (status & DescOwn)
break;
+
+ /* This barrier is needed to keep us from reading
+ * any other fields out of the Rx descriptor until
+ * we know the status of DescOwn
+ */
+ fast_rmb();
+
if (unlikely(status & RxRES)) {
netif_info(tp, rx_err, dev, "Rx ERROR. status = %08x\n",
status);
@@ -7350,7 +7364,6 @@ process_pkt:
}
release_descriptor:
desc->opts2 = 0;
- wmb();
rtl8169_mark_to_asic(desc, rx_buf_sz);
}
^ permalink raw reply related
* [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Alexander Duyck @ 2014-11-17 17:18 UTC (permalink / raw)
To: linux-arch, netdev, linux-kernel
Cc: mathieu.desnoyers, peterz, benh, heiko.carstens, mingo, mikey,
linux, donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher,
romieu, paulmck, nic_swsd, will.deacon, michael, tony.luck,
torvalds, oleg, schwidefsky, fweisbec, davem
In-Reply-To: <20141117171005.22333.96544.stgit@ahduyck-server>
There are a number of situations where the mandatory barriers rmb() and
wmb() are used to order memory/memory operations in the device drivers
and those barriers are much heavier than they actually need to be. For
example in the case of PowerPC wmb() calls the heavy-weight sync
instruction when for memory/memory operations all that is really needed is
an lsync or eieio instruction.
This commit adds a fast (and loose) version of the mandatory memory
barriers rmb() and wmb(). The prefix to the name is actually based on the
version of the functions that already exist in the mips and tile trees.
However I thought it applicable since it gets at what we are trying to
accomplish with these barriers and somethat implies their risky nature.
These new barriers are not as safe as the standard rmb() and wmb().
Specifically they do not guarantee ordering between cache-enabled and
cache-inhibited memories. The primary use case for these would be to
enforce ordering of memory reads/writes when accessing cache-enabled memory
that is shared between the CPU and a device.
It may also be noted that there is no fast_mb(). This is due to the fact
that most architectures didn't seem to have a good way to do a full memory
barrier quickly and so they usually resorted to an mb() for their smp_mb
call. As such there no point in adding a fast_mb() function if it is going
to map to mb() for all architectures anyway.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
Cc: Michael Ellerman <michael@ellerman.id.au>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
Documentation/memory-barriers.txt | 41 +++++++++++++++++++++++++++++++++++
arch/arm/include/asm/barrier.h | 4 +++
arch/arm64/include/asm/barrier.h | 3 +++
arch/ia64/include/asm/barrier.h | 3 +++
arch/metag/include/asm/barrier.h | 14 ++++++------
arch/powerpc/include/asm/barrier.h | 22 +++++++++++--------
arch/s390/include/asm/barrier.h | 2 ++
arch/sparc/include/asm/barrier_64.h | 3 +++
arch/x86/include/asm/barrier.h | 11 ++++++---
arch/x86/um/asm/barrier.h | 13 ++++++-----
include/asm-generic/barrier.h | 8 +++++++
11 files changed, 98 insertions(+), 26 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 22a969c..fe55dea 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1615,6 +1615,47 @@ There are some more advanced barrier functions:
operations" subsection for information on where to use these.
+ (*) fast_wmb();
+ (*) fast_rmb();
+
+ These are for use with memory based device I/O to guarantee the ordering
+ of cache-enabled writes or reads with respect to other writes or reads
+ to cache-enabled memory.
+
+ For example, consider a device driver that shares memory with a device
+ and uses a descriptor status value to indicate if the descriptor belongs
+ to the device or the CPU, and a doorbell to notify it when new
+ descriptors are available:
+
+ if (desc->status != DEVICE_OWN) {
+ /* do not read data until we own descriptor */
+ fast_rmb();
+
+ /* read/modify data */
+ read_data = desc->data;
+ desc->data = write_data;
+
+ /* flush modifications before status update */
+ fast_wmb();
+
+ /* assign ownership */
+ desc->status = DEVICE_OWN;
+
+ /* force memory to sync before notifying device via MMIO */
+ wmb();
+
+ /* notify device of new descriptors */
+ writel(DESC_NOTIFY, doorbell);
+ }
+
+ The fast_rmb() allows us guarantee the device has released ownership
+ before we read the data from the descriptor, and he fast_wmb() allows
+ us to guarantee the data is written to the descriptor before the device
+ can see it now has ownership. The wmb() is needed to guarantee that the
+ cache-enabled memory writes have completed before attempting a write to
+ the cache-inhibited MMIO region.
+
+
MMIO WRITE BARRIER
------------------
diff --git a/arch/arm/include/asm/barrier.h b/arch/arm/include/asm/barrier.h
index c6a3e73..c57903c 100644
--- a/arch/arm/include/asm/barrier.h
+++ b/arch/arm/include/asm/barrier.h
@@ -43,10 +43,14 @@
#define mb() do { dsb(); outer_sync(); } while (0)
#define rmb() dsb()
#define wmb() do { dsb(st); outer_sync(); } while (0)
+#define fast_rmb() dmb()
+#define fast_wmb() dmb(st)
#else
#define mb() barrier()
#define rmb() barrier()
#define wmb() barrier()
+#define fast_rmb() barrier()
+#define fast_wmb() barrier()
#endif
#ifndef CONFIG_SMP
diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
index 6389d60..3ca1a15 100644
--- a/arch/arm64/include/asm/barrier.h
+++ b/arch/arm64/include/asm/barrier.h
@@ -32,6 +32,9 @@
#define rmb() dsb(ld)
#define wmb() dsb(st)
+#define fast_rmb() dmb(ld)
+#define fast_wmb() dmb(st)
+
#ifndef CONFIG_SMP
#define smp_mb() barrier()
#define smp_rmb() barrier()
diff --git a/arch/ia64/include/asm/barrier.h b/arch/ia64/include/asm/barrier.h
index e8fffb0..997f5b0 100644
--- a/arch/ia64/include/asm/barrier.h
+++ b/arch/ia64/include/asm/barrier.h
@@ -39,6 +39,9 @@
#define rmb() mb()
#define wmb() mb()
+#define fast_rmb() mb()
+#define fast_wmb() mb()
+
#ifdef CONFIG_SMP
# define smp_mb() mb()
#else
diff --git a/arch/metag/include/asm/barrier.h b/arch/metag/include/asm/barrier.h
index 6d8b8c9..edddb6d 100644
--- a/arch/metag/include/asm/barrier.h
+++ b/arch/metag/include/asm/barrier.h
@@ -4,8 +4,6 @@
#include <asm/metag_mem.h>
#define nop() asm volatile ("NOP")
-#define mb() wmb()
-#define rmb() barrier()
#ifdef CONFIG_METAG_META21
@@ -41,11 +39,13 @@ static inline void wr_fence(void)
#endif /* !CONFIG_METAG_META21 */
-static inline void wmb(void)
-{
- /* flush writes through the write combiner */
- wr_fence();
-}
+/* flush writes through the write combiner */
+#define mb() wr_fence()
+#define rmb() barrier()
+#define wmb() mb()
+
+#define fast_rmb() rmb()
+#define fast_wmb() wmb()
#ifndef CONFIG_SMP
#define fence() do { } while (0)
diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index cb6d66c..f480097 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -36,22 +36,20 @@
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#ifdef CONFIG_SMP
-
#ifdef __SUBARCH_HAS_LWSYNC
# define SMPWMB LWSYNC
#else
# define SMPWMB eieio
#endif
-#define __lwsync() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
+#define fast_rmb() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
+#define fast_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : : :"memory")
+#ifdef CONFIG_SMP
#define smp_mb() mb()
-#define smp_rmb() __lwsync()
-#define smp_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : : :"memory")
+#define smp_rmb() fast_rmb()
+#define smp_wmb() fast_wmb()
#else
-#define __lwsync() barrier()
-
#define smp_mb() barrier()
#define smp_rmb() barrier()
#define smp_wmb() barrier()
@@ -69,10 +67,16 @@
#define data_barrier(x) \
asm volatile("twi 0,%0,0; isync" : : "r" (x) : "memory");
+/*
+ * The use of smp_rmb() in these functions are actually meant to map from
+ * smp_rmb()->fast_rmb()->LWSYNC. This way if smp is disabled then
+ * smp_rmb()->barrier(), or if the platform doesn't support lwsync it will
+ * map to the more heavy-weight sync.
+ */
#define smp_store_release(p, v) \
do { \
compiletime_assert_atomic_type(*p); \
- __lwsync(); \
+ smp_rmb(); \
ACCESS_ONCE(*p) = (v); \
} while (0)
@@ -80,7 +84,7 @@ do { \
({ \
typeof(*p) ___p1 = ACCESS_ONCE(*p); \
compiletime_assert_atomic_type(*p); \
- __lwsync(); \
+ smp_rmb(); \
___p1; \
})
diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
index 33d191d..9a31301 100644
--- a/arch/s390/include/asm/barrier.h
+++ b/arch/s390/include/asm/barrier.h
@@ -24,6 +24,8 @@
#define rmb() mb()
#define wmb() mb()
+#define fast_rmb() rmb()
+#define fast_wmb() wmb()
#define smp_mb() mb()
#define smp_rmb() rmb()
#define smp_wmb() wmb()
diff --git a/arch/sparc/include/asm/barrier_64.h b/arch/sparc/include/asm/barrier_64.h
index 6c974c0..eac2777 100644
--- a/arch/sparc/include/asm/barrier_64.h
+++ b/arch/sparc/include/asm/barrier_64.h
@@ -37,6 +37,9 @@ do { __asm__ __volatile__("ba,pt %%xcc, 1f\n\t" \
#define rmb() __asm__ __volatile__("":::"memory")
#define wmb() __asm__ __volatile__("":::"memory")
+#define fast_rmb() rmb()
+#define fast_wmb() wmb()
+
#define set_mb(__var, __value) \
do { __var = __value; membar_safe("#StoreLoad"); } while(0)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index 5238000..cf440e7 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -24,13 +24,16 @@
#define wmb() asm volatile("sfence" ::: "memory")
#endif
-#ifdef CONFIG_SMP
-#define smp_mb() mb()
#ifdef CONFIG_X86_PPRO_FENCE
-# define smp_rmb() rmb()
+#define fast_rmb() rmb()
#else
-# define smp_rmb() barrier()
+#define fast_rmb() barrier()
#endif
+#define fast_wmb() barrier()
+
+#ifdef CONFIG_SMP
+#define smp_mb() mb()
+#define smp_rmb() fast_rmb()
#define smp_wmb() barrier()
#define set_mb(var, value) do { (void)xchg(&var, value); } while (0)
#else /* !SMP */
diff --git a/arch/x86/um/asm/barrier.h b/arch/x86/um/asm/barrier.h
index d6511d9..2c0405d 100644
--- a/arch/x86/um/asm/barrier.h
+++ b/arch/x86/um/asm/barrier.h
@@ -29,17 +29,18 @@
#endif /* CONFIG_X86_32 */
-#ifdef CONFIG_SMP
-
-#define smp_mb() mb()
#ifdef CONFIG_X86_PPRO_FENCE
-#define smp_rmb() rmb()
+#define fast_rmb() rmb()
#else /* CONFIG_X86_PPRO_FENCE */
-#define smp_rmb() barrier()
+#define fast_rmb() barrier()
#endif /* CONFIG_X86_PPRO_FENCE */
+#define fast_wmb() barrier()
-#define smp_wmb() barrier()
+#ifdef CONFIG_SMP
+#define smp_mb() mb()
+#define smp_rmb() fast_rmb()
+#define smp_wmb() fast_wmb()
#define set_mb(var, value) do { (void)xchg(&var, value); } while (0)
#else /* CONFIG_SMP */
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index 1402fa8..c1d60b9 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -42,6 +42,14 @@
#define wmb() mb()
#endif
+#ifndef fast_rmb
+#define fast_rmb() rmb()
+#endif
+
+#ifndef fast_wmb
+#define fast_wmb() wmb()
+#endif
+
#ifndef read_barrier_depends
#define read_barrier_depends() do { } while (0)
#endif
^ permalink raw reply related
* [PATCH 1/4] arch: Cleanup read_barrier_depends() and comments
From: Alexander Duyck @ 2014-11-17 17:17 UTC (permalink / raw)
To: linux-arch, netdev, linux-kernel
Cc: mathieu.desnoyers, peterz, benh, heiko.carstens, mingo, mikey,
linux, donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher,
romieu, paulmck, nic_swsd, will.deacon, michael, tony.luck,
torvalds, oleg, schwidefsky, fweisbec, davem
In-Reply-To: <20141117171005.22333.96544.stgit@ahduyck-server>
This patch is meant to cleanup the handling of read_barrier_depends and
smp_read_barrier_depends. In multiple spots in the kernel headers
read_barrier_depends is defined as "do {} while (0)", however we then go
into the SMP vs non-SMP sections and have the SMP version reference
read_barrier_depends, and the non-SMP define it as yet another empty
do/while.
With this commit I went through and cleaned out the duplicate definitions
and reduced the number of definitions down to 2 per header. In addition I
moved the 50 line comments for the macro from the x86 and mips headers that
defined it as an empty do/while to those that were actually defining the
macro, alpha and blackfin.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
arch/alpha/include/asm/barrier.h | 51 ++++++++++++++++++++++++++++++
arch/blackfin/include/asm/barrier.h | 51 ++++++++++++++++++++++++++++++
arch/ia64/include/asm/barrier.h | 22 +++++--------
arch/metag/include/asm/barrier.h | 7 ++--
arch/mips/include/asm/barrier.h | 52 -------------------------------
arch/powerpc/include/asm/barrier.h | 6 ++--
arch/s390/include/asm/barrier.h | 5 ++-
arch/sparc/include/asm/barrier_64.h | 4 +-
arch/x86/include/asm/barrier.h | 59 ++---------------------------------
arch/x86/um/asm/barrier.h | 7 ++--
10 files changed, 129 insertions(+), 135 deletions(-)
diff --git a/arch/alpha/include/asm/barrier.h b/arch/alpha/include/asm/barrier.h
index 3832bdb..77516c8 100644
--- a/arch/alpha/include/asm/barrier.h
+++ b/arch/alpha/include/asm/barrier.h
@@ -7,6 +7,57 @@
#define rmb() __asm__ __volatile__("mb": : :"memory")
#define wmb() __asm__ __volatile__("wmb": : :"memory")
+/**
+ * read_barrier_depends - Flush all pending reads that subsequents reads
+ * depend on.
+ *
+ * No data-dependent reads from memory-like regions are ever reordered
+ * over this barrier. All reads preceding this primitive are guaranteed
+ * to access memory (but not necessarily other CPUs' caches) before any
+ * reads following this primitive that depend on the data return by
+ * any of the preceding reads. This primitive is much lighter weight than
+ * rmb() on most CPUs, and is never heavier weight than is
+ * rmb().
+ *
+ * These ordering constraints are respected by both the local CPU
+ * and the compiler.
+ *
+ * Ordering is not guaranteed by anything other than these primitives,
+ * not even by data dependencies. See the documentation for
+ * memory_barrier() for examples and URLs to more information.
+ *
+ * For example, the following code would force ordering (the initial
+ * value of "a" is zero, "b" is one, and "p" is "&a"):
+ *
+ * <programlisting>
+ * CPU 0 CPU 1
+ *
+ * b = 2;
+ * memory_barrier();
+ * p = &b; q = p;
+ * read_barrier_depends();
+ * d = *q;
+ * </programlisting>
+ *
+ * because the read of "*q" depends on the read of "p" and these
+ * two reads are separated by a read_barrier_depends(). However,
+ * the following code, with the same initial values for "a" and "b":
+ *
+ * <programlisting>
+ * CPU 0 CPU 1
+ *
+ * a = 2;
+ * memory_barrier();
+ * b = 3; y = b;
+ * read_barrier_depends();
+ * x = a;
+ * </programlisting>
+ *
+ * does not enforce ordering, since there is no data dependency between
+ * the read of "a" and the read of "b". Therefore, on some CPUs, such
+ * as Alpha, "y" could be set to 3 and "x" to 0. Use rmb()
+ * in cases like this where there are no data dependencies.
+ */
#define read_barrier_depends() __asm__ __volatile__("mb": : :"memory")
#ifdef CONFIG_SMP
diff --git a/arch/blackfin/include/asm/barrier.h b/arch/blackfin/include/asm/barrier.h
index 4200068..dfb66fe 100644
--- a/arch/blackfin/include/asm/barrier.h
+++ b/arch/blackfin/include/asm/barrier.h
@@ -22,6 +22,57 @@
# define mb() do { barrier(); smp_check_barrier(); smp_mark_barrier(); } while (0)
# define rmb() do { barrier(); smp_check_barrier(); } while (0)
# define wmb() do { barrier(); smp_mark_barrier(); } while (0)
+/*
+ * read_barrier_depends - Flush all pending reads that subsequents reads
+ * depend on.
+ *
+ * No data-dependent reads from memory-like regions are ever reordered
+ * over this barrier. All reads preceding this primitive are guaranteed
+ * to access memory (but not necessarily other CPUs' caches) before any
+ * reads following this primitive that depend on the data return by
+ * any of the preceding reads. This primitive is much lighter weight than
+ * rmb() on most CPUs, and is never heavier weight than is
+ * rmb().
+ *
+ * These ordering constraints are respected by both the local CPU
+ * and the compiler.
+ *
+ * Ordering is not guaranteed by anything other than these primitives,
+ * not even by data dependencies. See the documentation for
+ * memory_barrier() for examples and URLs to more information.
+ *
+ * For example, the following code would force ordering (the initial
+ * value of "a" is zero, "b" is one, and "p" is "&a"):
+ *
+ * <programlisting>
+ * CPU 0 CPU 1
+ *
+ * b = 2;
+ * memory_barrier();
+ * p = &b; q = p;
+ * read_barrier_depends();
+ * d = *q;
+ * </programlisting>
+ *
+ * because the read of "*q" depends on the read of "p" and these
+ * two reads are separated by a read_barrier_depends(). However,
+ * the following code, with the same initial values for "a" and "b":
+ *
+ * <programlisting>
+ * CPU 0 CPU 1
+ *
+ * a = 2;
+ * memory_barrier();
+ * b = 3; y = b;
+ * read_barrier_depends();
+ * x = a;
+ * </programlisting>
+ *
+ * does not enforce ordering, since there is no data dependency between
+ * the read of "a" and the read of "b". Therefore, on some CPUs, such
+ * as Alpha, "y" could be set to 3 and "x" to 0. Use rmb()
+ * in cases like this where there are no data dependencies.
+ */
# define read_barrier_depends() do { barrier(); smp_check_barrier(); } while (0)
#endif
diff --git a/arch/ia64/include/asm/barrier.h b/arch/ia64/include/asm/barrier.h
index a48957c..e8fffb0 100644
--- a/arch/ia64/include/asm/barrier.h
+++ b/arch/ia64/include/asm/barrier.h
@@ -35,26 +35,22 @@
* it's (presumably) much slower than mf and (b) mf.a is supported for
* sequential memory pages only.
*/
-#define mb() ia64_mf()
-#define rmb() mb()
-#define wmb() mb()
-#define read_barrier_depends() do { } while(0)
+#define mb() ia64_mf()
+#define rmb() mb()
+#define wmb() mb()
#ifdef CONFIG_SMP
# define smp_mb() mb()
-# define smp_rmb() rmb()
-# define smp_wmb() wmb()
-# define smp_read_barrier_depends() read_barrier_depends()
-
#else
-
# define smp_mb() barrier()
-# define smp_rmb() barrier()
-# define smp_wmb() barrier()
-# define smp_read_barrier_depends() do { } while(0)
-
#endif
+#define smp_rmb() smp_mb()
+#define smp_wmb() smp_mb()
+
+#define read_barrier_depends() do { } while (0)
+#define smp_read_barrier_depends() do { } while (0)
+
#define smp_mb__before_atomic() barrier()
#define smp_mb__after_atomic() barrier()
diff --git a/arch/metag/include/asm/barrier.h b/arch/metag/include/asm/barrier.h
index c7591e8..6d8b8c9 100644
--- a/arch/metag/include/asm/barrier.h
+++ b/arch/metag/include/asm/barrier.h
@@ -47,8 +47,6 @@ static inline void wmb(void)
wr_fence();
}
-#define read_barrier_depends() do { } while (0)
-
#ifndef CONFIG_SMP
#define fence() do { } while (0)
#define smp_mb() barrier()
@@ -82,7 +80,10 @@ static inline void fence(void)
#define smp_wmb() barrier()
#endif
#endif
-#define smp_read_barrier_depends() do { } while (0)
+
+#define read_barrier_depends() do { } while (0)
+#define smp_read_barrier_depends() do { } while (0)
+
#define set_mb(var, value) do { var = value; smp_mb(); } while (0)
#define smp_store_release(p, v) \
diff --git a/arch/mips/include/asm/barrier.h b/arch/mips/include/asm/barrier.h
index d0101dd..3d69aa8 100644
--- a/arch/mips/include/asm/barrier.h
+++ b/arch/mips/include/asm/barrier.h
@@ -10,58 +10,6 @@
#include <asm/addrspace.h>
-/*
- * read_barrier_depends - Flush all pending reads that subsequents reads
- * depend on.
- *
- * No data-dependent reads from memory-like regions are ever reordered
- * over this barrier. All reads preceding this primitive are guaranteed
- * to access memory (but not necessarily other CPUs' caches) before any
- * reads following this primitive that depend on the data return by
- * any of the preceding reads. This primitive is much lighter weight than
- * rmb() on most CPUs, and is never heavier weight than is
- * rmb().
- *
- * These ordering constraints are respected by both the local CPU
- * and the compiler.
- *
- * Ordering is not guaranteed by anything other than these primitives,
- * not even by data dependencies. See the documentation for
- * memory_barrier() for examples and URLs to more information.
- *
- * For example, the following code would force ordering (the initial
- * value of "a" is zero, "b" is one, and "p" is "&a"):
- *
- * <programlisting>
- * CPU 0 CPU 1
- *
- * b = 2;
- * memory_barrier();
- * p = &b; q = p;
- * read_barrier_depends();
- * d = *q;
- * </programlisting>
- *
- * because the read of "*q" depends on the read of "p" and these
- * two reads are separated by a read_barrier_depends(). However,
- * the following code, with the same initial values for "a" and "b":
- *
- * <programlisting>
- * CPU 0 CPU 1
- *
- * a = 2;
- * memory_barrier();
- * b = 3; y = b;
- * read_barrier_depends();
- * x = a;
- * </programlisting>
- *
- * does not enforce ordering, since there is no data dependency between
- * the read of "a" and the read of "b". Therefore, on some CPUs, such
- * as Alpha, "y" could be set to 3 and "x" to 0. Use rmb()
- * in cases like this where there are no data dependencies.
- */
-
#define read_barrier_depends() do { } while(0)
#define smp_read_barrier_depends() do { } while(0)
diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index bab79a1..cb6d66c 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -33,7 +33,6 @@
#define mb() __asm__ __volatile__ ("sync" : : : "memory")
#define rmb() __asm__ __volatile__ ("sync" : : : "memory")
#define wmb() __asm__ __volatile__ ("sync" : : : "memory")
-#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { var = value; mb(); } while (0)
@@ -50,16 +49,17 @@
#define smp_mb() mb()
#define smp_rmb() __lwsync()
#define smp_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : : :"memory")
-#define smp_read_barrier_depends() read_barrier_depends()
#else
#define __lwsync() barrier()
#define smp_mb() barrier()
#define smp_rmb() barrier()
#define smp_wmb() barrier()
-#define smp_read_barrier_depends() do { } while(0)
#endif /* CONFIG_SMP */
+#define read_barrier_depends() do { } while (0)
+#define smp_read_barrier_depends() do { } while (0)
+
/*
* This is a barrier which prevents following instructions from being
* started until the value of the argument x is known. For example, if
diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
index b5dce65..33d191d 100644
--- a/arch/s390/include/asm/barrier.h
+++ b/arch/s390/include/asm/barrier.h
@@ -24,11 +24,12 @@
#define rmb() mb()
#define wmb() mb()
-#define read_barrier_depends() do { } while(0)
#define smp_mb() mb()
#define smp_rmb() rmb()
#define smp_wmb() wmb()
-#define smp_read_barrier_depends() read_barrier_depends()
+
+#define read_barrier_depends() do { } while (0)
+#define smp_read_barrier_depends() do { } while (0)
#define smp_mb__before_atomic() smp_mb()
#define smp_mb__after_atomic() smp_mb()
diff --git a/arch/sparc/include/asm/barrier_64.h b/arch/sparc/include/asm/barrier_64.h
index 305dcc3..6c974c0 100644
--- a/arch/sparc/include/asm/barrier_64.h
+++ b/arch/sparc/include/asm/barrier_64.h
@@ -37,7 +37,6 @@ do { __asm__ __volatile__("ba,pt %%xcc, 1f\n\t" \
#define rmb() __asm__ __volatile__("":::"memory")
#define wmb() __asm__ __volatile__("":::"memory")
-#define read_barrier_depends() do { } while(0)
#define set_mb(__var, __value) \
do { __var = __value; membar_safe("#StoreLoad"); } while(0)
@@ -51,7 +50,8 @@ do { __asm__ __volatile__("ba,pt %%xcc, 1f\n\t" \
#define smp_wmb() __asm__ __volatile__("":::"memory")
#endif
-#define smp_read_barrier_depends() do { } while(0)
+#define read_barrier_depends() do { } while (0)
+#define smp_read_barrier_depends() do { } while (0)
#define smp_store_release(p, v) \
do { \
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index 0f4460b..5238000 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -24,60 +24,6 @@
#define wmb() asm volatile("sfence" ::: "memory")
#endif
-/**
- * read_barrier_depends - Flush all pending reads that subsequents reads
- * depend on.
- *
- * No data-dependent reads from memory-like regions are ever reordered
- * over this barrier. All reads preceding this primitive are guaranteed
- * to access memory (but not necessarily other CPUs' caches) before any
- * reads following this primitive that depend on the data return by
- * any of the preceding reads. This primitive is much lighter weight than
- * rmb() on most CPUs, and is never heavier weight than is
- * rmb().
- *
- * These ordering constraints are respected by both the local CPU
- * and the compiler.
- *
- * Ordering is not guaranteed by anything other than these primitives,
- * not even by data dependencies. See the documentation for
- * memory_barrier() for examples and URLs to more information.
- *
- * For example, the following code would force ordering (the initial
- * value of "a" is zero, "b" is one, and "p" is "&a"):
- *
- * <programlisting>
- * CPU 0 CPU 1
- *
- * b = 2;
- * memory_barrier();
- * p = &b; q = p;
- * read_barrier_depends();
- * d = *q;
- * </programlisting>
- *
- * because the read of "*q" depends on the read of "p" and these
- * two reads are separated by a read_barrier_depends(). However,
- * the following code, with the same initial values for "a" and "b":
- *
- * <programlisting>
- * CPU 0 CPU 1
- *
- * a = 2;
- * memory_barrier();
- * b = 3; y = b;
- * read_barrier_depends();
- * x = a;
- * </programlisting>
- *
- * does not enforce ordering, since there is no data dependency between
- * the read of "a" and the read of "b". Therefore, on some CPUs, such
- * as Alpha, "y" could be set to 3 and "x" to 0. Use rmb()
- * in cases like this where there are no data dependencies.
- **/
-
-#define read_barrier_depends() do { } while (0)
-
#ifdef CONFIG_SMP
#define smp_mb() mb()
#ifdef CONFIG_X86_PPRO_FENCE
@@ -86,16 +32,17 @@
# define smp_rmb() barrier()
#endif
#define smp_wmb() barrier()
-#define smp_read_barrier_depends() read_barrier_depends()
#define set_mb(var, value) do { (void)xchg(&var, value); } while (0)
#else /* !SMP */
#define smp_mb() barrier()
#define smp_rmb() barrier()
#define smp_wmb() barrier()
-#define smp_read_barrier_depends() do { } while (0)
#define set_mb(var, value) do { var = value; barrier(); } while (0)
#endif /* SMP */
+#define read_barrier_depends() do { } while (0)
+#define smp_read_barrier_depends() do { } while (0)
+
#if defined(CONFIG_X86_PPRO_FENCE)
/*
diff --git a/arch/x86/um/asm/barrier.h b/arch/x86/um/asm/barrier.h
index cc04e67..d6511d9 100644
--- a/arch/x86/um/asm/barrier.h
+++ b/arch/x86/um/asm/barrier.h
@@ -29,8 +29,6 @@
#endif /* CONFIG_X86_32 */
-#define read_barrier_depends() do { } while (0)
-
#ifdef CONFIG_SMP
#define smp_mb() mb()
@@ -42,7 +40,6 @@
#define smp_wmb() barrier()
-#define smp_read_barrier_depends() read_barrier_depends()
#define set_mb(var, value) do { (void)xchg(&var, value); } while (0)
#else /* CONFIG_SMP */
@@ -50,11 +47,13 @@
#define smp_mb() barrier()
#define smp_rmb() barrier()
#define smp_wmb() barrier()
-#define smp_read_barrier_depends() do { } while (0)
#define set_mb(var, value) do { var = value; barrier(); } while (0)
#endif /* CONFIG_SMP */
+#define read_barrier_depends() do { } while (0)
+#define smp_read_barrier_depends() do { } while (0)
+
/*
* Stop RDTSC speculation. This is needed when you need to use RDTSC
* (or get_cycles or vread that possibly accesses the TSC) in a defined
^ permalink raw reply related
* [PATCH 0/4] Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Alexander Duyck @ 2014-11-17 17:17 UTC (permalink / raw)
To: linux-arch, netdev, linux-kernel
Cc: mathieu.desnoyers, peterz, benh, heiko.carstens, mingo, mikey,
linux, donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher,
romieu, paulmck, nic_swsd, will.deacon, michael, tony.luck,
torvalds, oleg, schwidefsky, fweisbec, davem
These patches introduce two new primitives for synchronizing cache-enabled
memory writes and reads. These two new primitives are:
fast_rmb()
fast_wmb()
The first patch cleans up some unnecessary overhead related to the
definition of read_barrier_depends, smp_read_barrier_depends, and comments
related to read_barrier_depends.
The second patch adds the primitives for the applicable architectures and
asm-generic. The names for the new primitives are based on the names of
similar primitives that already exist in the mips and tile trees.
The third patch adds the barriers to r8169 which turns out to be a good
example of where the new barriers might be useful as they have full
rmb()/wmb() barriers ordering accesses to the descriptors and the DescOwn
bit.
The fourth patch adds support for fast_rmb() to the Intel fm10k, igb, and
ixgbe drivers. Testing with the ixgbe driver has shown a processing
time reduction of at least 7ns per 64B frame on a Core i7-4930K.
This patch series is essentially the v3 for:
arch: Introduce load_acquire() and store_release()
or
arch: Introduce read_acquire()
The key changes in this patch series versus the earlier patches are:
v3:
- Added cleanup of read_barrier_depends
- Focus on rmb()/wmb() instead of acquire()/store()
- Added update to documentation with code example
- Added change in r8169 to fix cur_tx/DescOwn ordering
- Simplified changes to just replacing/moving barriers in r8169
v2:
- Renamed read_acquire() to be consistent with smp_load_acquire()
- Changed barrier used to be consistent with smp_load_acquire()
- Updated PowerPC code to use __lwsync based on IBM article
- Added store_release() as this is a viable use case for drivers
- Added r8169 patch which is able to fully use primitives
- Added fm10k/igb/ixgbe patch which is able to test performance
---
Alexander Duyck (4):
arch: Cleanup read_barrier_depends() and comments
arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
r8169: Use fast_rmb() and fast_wmb() for DescOwn checks
fm10k/igb/ixgbe: Use fast_rmb on Rx descriptor reads
Documentation/memory-barriers.txt | 41 +++++++++++++++
arch/alpha/include/asm/barrier.h | 51 ++++++++++++++++++
arch/arm/include/asm/barrier.h | 4 +
arch/arm64/include/asm/barrier.h | 3 +
arch/blackfin/include/asm/barrier.h | 51 ++++++++++++++++++
arch/ia64/include/asm/barrier.h | 25 ++++-----
arch/metag/include/asm/barrier.h | 19 ++++---
arch/mips/include/asm/barrier.h | 52 -------------------
arch/powerpc/include/asm/barrier.h | 28 ++++++----
arch/s390/include/asm/barrier.h | 7 ++-
arch/sparc/include/asm/barrier_64.h | 7 ++-
arch/x86/include/asm/barrier.h | 70 ++++---------------------
arch/x86/um/asm/barrier.h | 20 ++++---
drivers/net/ethernet/intel/fm10k/fm10k_main.c | 6 +-
drivers/net/ethernet/intel/igb/igb_main.c | 6 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 9 +--
drivers/net/ethernet/realtek/r8169.c | 29 ++++++++--
include/asm-generic/barrier.h | 8 +++
18 files changed, 257 insertions(+), 179 deletions(-)
--
^ permalink raw reply
* Re: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: Eric Dumazet @ 2014-11-17 17:09 UTC (permalink / raw)
To: David Laight
Cc: 'Nelson, Shannon', Kirsher, Jeffrey T,
davem@davemloft.net, Kong, Serey, netdev@vger.kernel.org,
nhorman@redhat.com, sassmann@redhat.com, jogreene@redhat.com
In-Reply-To: <1416243484.5102.21.camel@edumazet-glaptop2.roam.corp.google.com>
On Mon, 2014-11-17 at 08:58 -0800, Eric Dumazet wrote:
> This is the section of the code sending a _non_ TSO packet.
Sorry for the double post, it was non intended.
^ permalink raw reply
* Re: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: Eric Dumazet @ 2014-11-17 16:58 UTC (permalink / raw)
To: David Laight
Cc: 'Nelson, Shannon', Kirsher, Jeffrey T,
davem@davemloft.net, Kong, Serey, netdev@vger.kernel.org,
nhorman@redhat.com, sassmann@redhat.com, jogreene@redhat.com
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9F1F9F@AcuExch.aculab.com>
On Mon, 2014-11-17 at 16:16 +0000, David Laight wrote:
> Except that a TSO packet is likely to be just under 64k and comprise of
> a small header and 16 other fragments - most of which will be a complete 4k page.
>
This is the section of the code sending a _non_ TSO packet.
^ permalink raw reply
* Re: Device Tree Binding for Marvell DSA Switch on imx28 board over Mdio Interface
From: Oliver Graute @ 2014-11-17 16:54 UTC (permalink / raw)
To: Fabio Estevam; +Cc: Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <CAOMZO5Crue+NFEEkhzG3YYVQfq-Yu6s-+gZUyRQh3LawhbD8iQ@mail.gmail.com>
Hello Fabio,
and what's the correct one for the imx28 CPU?
Best Regards,
Oliver
On Mon, Nov 17, 2014 at 5:45 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Thu, Nov 13, 2014 at 1:15 PM, Oliver Graute <oliver.graute@gmail.com> wrote:
>
>
>> mdio_bus: mdio@800f0040 {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> device_type = "mdio";
>> //compatible = "fsl,gianfar-mdio";
>> compatible = "fsl,mpc875-fec-mdio", "fsl,pq1-fec-mdio";
>
> These compatible strings are for PowerQuicc devices, not for mx28.
>
>> reg = <0x800f0040 0x188>;
>> status = "okay";
>>
>> ethphy0: ethernet-phy@0 {
>> compatible = "fsl,gianfar-mdio";
>
> Same here.
^ permalink raw reply
* Re: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: Eric Dumazet @ 2014-11-17 16:52 UTC (permalink / raw)
To: David Laight
Cc: 'Nelson, Shannon', Kirsher, Jeffrey T,
davem@davemloft.net, Kong, Serey, netdev@vger.kernel.org,
nhorman@redhat.com, sassmann@redhat.com, jogreene@redhat.com
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9F1F9F@AcuExch.aculab.com>
On Mon, 2014-11-17 at 16:16 +0000, David Laight wrote:
> Except that a TSO packet is likely to be just under 64k and comprise of
> a small header and 16 other fragments - most of which will be a complete 4k page.
This is the code dealing with non TSO packets.
^ permalink raw reply
* Re: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: Eric Dumazet @ 2014-11-17 16:45 UTC (permalink / raw)
To: Nelson, Shannon
Cc: Kirsher, Jeffrey T, davem@davemloft.net, Kong, Serey,
netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com
In-Reply-To: <FC41C24E35F18A40888AACA1A36F3E418AD8F543@fmsmsx115.amr.corp.intel.com>
On Mon, 2014-11-17 at 16:04 +0000, Nelson, Shannon wrote:
> This typically hits when there are many little TSO packets in a single
> MTU size frame. The hardware doesn't like dealing with more than 8
> separate buffers.
>
> If skb_linearize() fails and we try to send the skb as is, the
> hardware will throw an MDD event and disable that particular queue,
> possibly causing a Tx hang, which will lead to a PF reset and then we
> continue on. It's a bit of a hit to throughput, and most folks get a
> little annoyed when seeing the Tx hang stack trace in their log, but
> it generally isn't fatal.
>
> If we disable the MDD detection, the hardware ends up putting garbage
> on the wire.
>
> It looks to me that the way for skb_linearize() to fail is with
> -ENOMEM, in which case we probably have other issues.
>
This kind of events happens in real workloads, trust me.
> If there are some alternative suggestions for dealing with this, we'd
> be happy to hear about them.
Nelson, all you have to do is to check skb_linearize(skb) return code.
If its a failure, then drop this packet. How hard is this ?
This should hardly trigger in normal conditions.
If some customer complains, then you might implement something more
complex in the future.
Lets fix the bug first in a 100% safe way.
^ permalink raw reply
* Re: Device Tree Binding for Marvell DSA Switch on imx28 board over Mdio Interface
From: Fabio Estevam @ 2014-11-17 16:45 UTC (permalink / raw)
To: Oliver Graute; +Cc: Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <CA+KjHfbwtwPFXFEVDDijRNeTcHcSn0BZ4_yGuwcgyFfubNFCQA@mail.gmail.com>
On Thu, Nov 13, 2014 at 1:15 PM, Oliver Graute <oliver.graute@gmail.com> wrote:
> mdio_bus: mdio@800f0040 {
> #address-cells = <1>;
> #size-cells = <0>;
> device_type = "mdio";
> //compatible = "fsl,gianfar-mdio";
> compatible = "fsl,mpc875-fec-mdio", "fsl,pq1-fec-mdio";
These compatible strings are for PowerQuicc devices, not for mx28.
> reg = <0x800f0040 0x188>;
> status = "okay";
>
> ethphy0: ethernet-phy@0 {
> compatible = "fsl,gianfar-mdio";
Same here.
^ permalink raw reply
* RE: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: David Laight @ 2014-11-17 16:16 UTC (permalink / raw)
To: 'Nelson, Shannon', Eric Dumazet, Kirsher, Jeffrey T
Cc: davem@davemloft.net, Kong, Serey, netdev@vger.kernel.org,
nhorman@redhat.com, sassmann@redhat.com, jogreene@redhat.com
In-Reply-To: <FC41C24E35F18A40888AACA1A36F3E418AD8F543@fmsmsx115.amr.corp.intel.com>
From: Nelson, Shannon
> > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> > Sent: Saturday, November 15, 2014 10:22 AM
> >
> > On Fri, 2014-11-14 at 22:08 -0800, Jeff Kirsher wrote:
> > > From: Serey Kong <serey.kong@intel.com>
> > >
> > > This handles the case where a single packet with more than 8 data
> > > descriptors triggers a Malicious Driver Detect event in the device.
> > >
>
> [...]
>
> > > - if (tx_flags & (I40E_TX_FLAGS_TSO | I40E_TX_FLAGS_FSO))
> > > + if (tx_flags & (I40E_TX_FLAGS_TSO | I40E_TX_FLAGS_FSO)) {
> > > gso_segs = skb_shinfo(skb)->gso_segs;
> > > - else
> > > + } else {
> > > gso_segs = 1;
> > > + if (skb_shinfo(skb)->nr_frags >= I40E_MAX_BUFFER_TXD)
> > > + skb_linearize(skb);
> >
> > What exactly happens if skb_linearize() fails ?
> >
> > Is this "Malicious Driver Detect event" fatal or simply packet is
> > dropped without additional harm ?
>
> This typically hits when there are many little TSO packets in a single MTU size frame. The hardware
> doesn't like dealing with more than 8 separate buffers.
Except that a TSO packet is likely to be just under 64k and comprise of
a small header and 16 other fragments - most of which will be a complete 4k page.
If the hardware can't handle such packets then I suspect you can't actually
use TSO - which makes the device pretty useless.
If the problem is just too many fragment boundaries within a small number
of bytes then you need to detect that specific problem - probably merging
the adjacent small fragments into one that is large enough.
David
^ permalink raw reply
* Re: Device Tree Binding for Marvell DSA Switch on imx28 board over Mdio Interface
From: Andrew Lunn @ 2014-11-17 16:09 UTC (permalink / raw)
To: Oliver Graute; +Cc: netdev, andrew, Florian Fainelli, buytenh
In-Reply-To: <CA+KjHfY13ynH4Zr5Cerhswo3eucj3DMwq0tZ5CQquT3yuxKv-Q@mail.gmail.com>
> dsa@0 {
> compatible = "marvell,dsa";
> #address-cells = <2>;
> #size-cells = <0>;
>
> interrupts = <10>;
> //dsa,ethernet = <ðphy1>;
> //dsa,ethernet = <ð1>;
> dsa,ethernet = <&mac1>;
> //dsa,ethernet = <&mac0>;
> dsa,mii-bus = <&mdio_bus>;
>
> switch@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <5 0>; /* MDIO address 5, switch 0 in tree */
Hi Oliver
How do you have the strapping pins on the switch set? They determine
what address on the mdio bus the chip responds to.
Does your u-boot have commands to read arbitrary phy registers?
Generally, reading a register that does not exist gives 0xffff. So
try some reads at different addresses and see what you can find.
Andrew
^ permalink raw reply
* RE: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: Nelson, Shannon @ 2014-11-17 16:04 UTC (permalink / raw)
To: Eric Dumazet, Kirsher, Jeffrey T
Cc: davem@davemloft.net, Kong, Serey, netdev@vger.kernel.org,
nhorman@redhat.com, sassmann@redhat.com, jogreene@redhat.com
In-Reply-To: <1416075695.17262.86.camel@edumazet-glaptop2.roam.corp.google.com>
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Saturday, November 15, 2014 10:22 AM
>
> On Fri, 2014-11-14 at 22:08 -0800, Jeff Kirsher wrote:
> > From: Serey Kong <serey.kong@intel.com>
> >
> > This handles the case where a single packet with more than 8 data
> > descriptors triggers a Malicious Driver Detect event in the device.
> >
[...]
> > - if (tx_flags & (I40E_TX_FLAGS_TSO | I40E_TX_FLAGS_FSO))
> > + if (tx_flags & (I40E_TX_FLAGS_TSO | I40E_TX_FLAGS_FSO)) {
> > gso_segs = skb_shinfo(skb)->gso_segs;
> > - else
> > + } else {
> > gso_segs = 1;
> > + if (skb_shinfo(skb)->nr_frags >= I40E_MAX_BUFFER_TXD)
> > + skb_linearize(skb);
>
> What exactly happens if skb_linearize() fails ?
>
> Is this "Malicious Driver Detect event" fatal or simply packet is
> dropped without additional harm ?
This typically hits when there are many little TSO packets in a single MTU size frame. The hardware doesn't like dealing with more than 8 separate buffers.
If skb_linearize() fails and we try to send the skb as is, the hardware will throw an MDD event and disable that particular queue, possibly causing a Tx hang, which will lead to a PF reset and then we continue on. It's a bit of a hit to throughput, and most folks get a little annoyed when seeing the Tx hang stack trace in their log, but it generally isn't fatal.
If we disable the MDD detection, the hardware ends up putting garbage on the wire.
It looks to me that the way for skb_linearize() to fail is with -ENOMEM, in which case we probably have other issues.
If there are some alternative suggestions for dealing with this, we'd be happy to hear about them.
Cheers,
sln
^ permalink raw reply
* Re: phy/micrel: KSZ8031RNL RMII clock reconfiguration bug
From: Johan Hovold @ 2014-11-17 16:00 UTC (permalink / raw)
To: Bruno Thomsen
Cc: Johan Hovold, netdev@vger.kernel.org, f.fainelli@gmail.com,
s.hauer@pengutronix.de, bruno.thomsen@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <915054555B5659448ACF8A70E114824D01704BD71E@Exchange2010.kamstrup.dk>
On Mon, Nov 17, 2014 at 02:56:45PM +0000, Bruno Thomsen wrote:
> > Did you specify a led-mode as well, or was the Operation Mode Strap
> > Override (OMSO) write the first access after the soft reset?
>
> No led-mode was specified so OMSO was the first write.
Did you try setting a led-mode before changing the clock mode? Perhaps
that could also trigger the problem (i.e. when the clock-mode change
isn't the first write after reset).
> > Would you able to test my series on your setup, and possibly a
> > couple of diagnostic patches on top?
>
> Sure, I can try later this week.
Much appreciated. I'll resend the last part of the series (which is not
already in linux-next) and make sure to put you on CC.
Thanks,
Johan
^ permalink raw reply
* Re: Device Tree Binding for Marvell DSA Switch on imx28 board over Mdio Interface
From: Oliver Graute @ 2014-11-17 15:58 UTC (permalink / raw)
To: netdev; +Cc: andrew, Florian Fainelli, buytenh
In-Reply-To: <CA+KjHfa=Tqs=CWe6TT+rbmc9UaFZghqW1OtigPm9wOyXgN2AOQ@mail.gmail.com>
Hello,
I'am still failing to probe the DSA switch via the mii bus. I'am
unsure if i really probe it on the right way (mii bus and mii address)
The IMX28 CPU board has two connected phys. These are successfully
probed by fec_enet_mii_probe and appear as eth0 and eth1 Interface. So
some mii bus probing is working fine there.
fec 800f0000.ethernet eth0: Freescale FEC PHY driver [Micrel KSZ8041]
(mii_bus:phy_addr=800f0000.etherne:01, irq=-1)
fec 800f4000.ethernet eth1: Freescale FEC PHY driver [Micrel KSZ8041]
(mii_bus:phy_addr=800f0000.etherne:03, irq=-1)
The last phy1 is connected with phy5 of the the switch board . All
phys are connected on the same mii bus of IMX28.
In the DSA part of the Device Tree I use the compatible Flag:
"fsl,gianfar-mdio"; That load the "bus->name=Freescale PowerQUICC MII
Bus"
Is this the right mii bus choice for me here on a imx28 board? or is
that another non existing mii bus?
Then I adapted a bit the 88E6060 driver to a 88e6071 driver. But
currently i only try to readout the revison number.
Revsion Number:
88E6071 = 0x071
static char *mv88e6071_probe(struct mii_bus *bus, int sw_addr)
{
int ret;
ret = mdiobus_read(bus, sw_addr + REG_PORT(0), 0x03);
if (ret >= 0) {
ret &= 0xfff0;
if (ret == 0x0710)
return "Marvell 88E6071";
}
return NULL;
}
Part of my Device Tree
ahb@80080000 {
usb0: usb@80080000 {
vbus-supply = <®_usb0_vbus>;
status = "okay";
};
usb1: usb@80090000 {
vbus-supply = <®_usb1_vbus>;
status = "okay";
};
mac0: ethernet@800f0000 {
phy-mode = "rmii";
pinctrl-names = "default";
pinctrl-0 = <&mac0_pins_a>;
//phy-supply = <®_fec_3v3>;
phy-reset-gpios = <&gpio4 13 0>;
phy-reset-duration = <100>;
status = "okay";
ethernet0-port@0 {
phy-handle = <ðphy0>;
fixed-link {
speed = <1000>;
full-duplex;
};
};
};
mac1: ethernet@800f4000 {
phy-mode = "rmii";
pinctrl-names = "default";
pinctrl-0 = <&mac1_pins_a>;
status = "okay";
ethernet1-port@1 {
phy-handle = <ðphy1>;
fixed-link {
speed = <1000>;
full-duplex;
};
};
};
};
/*
* Marvell Distributed Switch Architecture Device Tree Bindings
*
* A DSA node can contain multiple switch chips which are therefore
child nodes of
* the parent DSA node. The maximum number of allowed child nodes is 4
*/
dsa@0 {
compatible = "marvell,dsa";
#address-cells = <2>;
#size-cells = <0>;
interrupts = <10>;
//dsa,ethernet = <ðphy1>;
//dsa,ethernet = <ð1>;
dsa,ethernet = <&mac1>;
//dsa,ethernet = <&mac0>;
dsa,mii-bus = <&mdio_bus>;
switch@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <5 0>; /* MDIO address 5, switch 0 in tree */
port@0 {
reg = <0>;
label = "lan1";
phy-handle = <ðphy1>;
};
port@1 {
reg = <1>;
label = "lan2";
};
port@2 {
reg = <2>;
label = "lan3";
};
port@3 {
reg = <3>;
label = "lan4";
};
port@4 {
reg = <4>;
label = "lan5";
};
port@5 {
reg = <5>;
label = "cpu";
};
};
};
eth3: eth3 {
status = "okay";
ethernet1-port@1 {
phy-handle = <ðphy1>;
fixed-link {
speed = <1000>;
full-duplex;
};
};
};
mdio_bus: mdio@800f0040 {
#address-cells = <1>;
#size-cells = <0>;
device_type = "mdio";
//reg = <24520 20>;
//reg = <0xd0072004 0x4>;
compatible = "fsl,gianfar-mdio";
//compatible = "fsl,mpc875-fec-mdio", "fsl,pq1-fec-mdio";
//reg = <0xe00 0x188>;
reg = <0x800f0040 0x188>;
status = "okay";
ethphy0: ethernet-phy@0 {
compatible = "fsl,gianfar-mdio";
device_type = "network";
model = "FEC";
reg = <0>;
};
ethphy1: ethernet-phy@1 {
compatible = "fsl,gianfar-mdio";
device_type = "network";
model = "FEC";
reg = <1>;
};
//reg = <0xff>; */ /* No PHY attached */
//speed = <1000>;
//duple = <1>;
};
dmesg Output
[ 19.759175] fep->mii_bus->name=fec_enet_mii_bus
[ 19.842656] fec 800f4000.ethernet eth1: Freescale FEC PHY driver
[Micrel KSZ8041] (mii_bus:phy_addr=800f0000.etherne:03, irq=-1)
[ 19.887013] IPv6: ADDRCONF(NETDEV_UP): eth1: link is not ready
[ 20.900994] libphy: 800f0000.etherne:01 - Link is Up - 100/Full
[ 20.907002] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>[ 21.198275] systemd-journald[161]: Received SIGUSR1
[ 23.066029] !!!!!enter dsa_init_module!!!!!
[ 23.134637] !!!!Enter dsa Probe!!!!!
[ 23.138256] Distributed Switch Architecture driver version 0.1
[ 23.214204] !!!!!Enter dsa_of_probe!!!!!
[ 23.218323] !!!!!mdio->name=mdio mdio->type=mdio
mdio->full_name=/mdio@800f0040 !!!!!
[ 23.330114] !!!!!np->name=dsa np->type=<NULL> np->full_name=/dsa@0 !!!!!
[ 23.336859] !!!!before of_mdio_find_bus!!!!!
[ 23.433145] !!!!!enter of_mdio_find_bus!!!!!
[ 23.437464] !!!!!enter of_mdio_bus_match!!!!!
[ 23.590106] !!!!!enter of_mdio_bus_match!!!!!
[ 23.594510] !!!!!enter of_mdio_bus_match!!!!!
[ 23.598881] !!!!Leave of_mdio_find_bus !!!!!
[ 23.855136] !!!!before of_parse_phandle dsa,ethernet!!!!!
[ 23.898976] !!!!before of find_device_by_node!!!!!
[ 23.948053] !!!!!ethernet->name=ethernet ethernet->type=<NULL>
ethernet->full_name=/ahb@80080000/ethernet@800f4000 !!!!!
[ 24.050100] !!!!! enter of_find_device_by_node !!!!!
[ 24.055150] !!!!! Leave of_find_device_by_node dev=c790fe10 !!!!!
[ 24.189562] !!!!! dev->init_name=(null) !!!!!
[ 24.230110]
[ 24.230110] before to_platform_device test->name=800f4000.ethernet
[ 24.237904] !!!!before of kzalloc!!!!!
[ 24.331061] !!!!before pd->netdev!!!!!
[ 24.420120] !!!!before dev_to_net_device!!!!!
[ 24.424536] !!!!dev_put(dev)!!!!!
[ 24.427864] !!!!kzalloc!!!!!
[ 24.515908] !!!!platform_set_drv_data!!!!!
[ 24.594206] !!!!!enter dev_to_mii_bus!!!!!
[ 24.598353] !!!!!enter dsa_switch_setup!!!!!
[ 24.667627] !!!!name=!!!!!
[ 24.710108] !!!!bus->name=Freescale PowerQUICC MII Bus!!!!!
[ 24.715729] !!!!pd->sw_addr=5!!!!!
[ 24.719138] !!!!Enter dsa_switch_probe!!!!!
[ 24.752377] libphy: 800f0000.etherne:03 - Link is Up - 100/Full
[ 24.758382] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
[ 24.920168] !!!!Leave dsa_switch_probe!!!!!
[ 24.924406] eth1[0]: could not detect attached switch
[ 24.929476] eth1[0]: couldn't create dsa switch instance (error -22)
[ 25.090113] !!!!Leave dsa Probe!!!!!
[ 25.120641] !!!!!leave dsa_init_module!!!!!
Best regards,
Oliver
^ permalink raw reply
* Re: /proc/net/sockstat invalid memory accounting or memory leak in latest kernels? (trying to debug)
From: Denys Fedoryshchenko @ 2014-11-17 15:33 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Neal Cardwell, Yuchung Cheng, netdev
In-Reply-To: <9056c6967a870d9fbee54776cf950e48@visp.net.lb>
On 2014-11-17 12:22, Denys Fedoryshchenko wrote:
>> Hmm.... I have an updated patch, sorry.
>>
...
>
> Installed patch, but will have to wait a while (usually at least
> 24hours), to be sure if it is stable.
>
> Thanks a lot!
Tried updated patch, it seems crashed same after while with it too, and
on second test i noticed same value overflow.
In debug, after i added alert if sk_forward_alloc > 1147483648 i noticed
that on some sockets it continuously increasing this value until it will
overflow.
I can provide logs if it is interesting.
I will try to sysctl fastopen to zero, to make sure if it changes
anything.
^ permalink raw reply
* RE: phy/micrel: KSZ8031RNL RMII clock reconfiguration bug
From: Bruno Thomsen @ 2014-11-17 14:56 UTC (permalink / raw)
To: Johan Hovold
Cc: netdev@vger.kernel.org, f.fainelli@gmail.com,
s.hauer@pengutronix.de, bruno.thomsen@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20141115141804.GA24633@localhost>
> Did you specify a led-mode as well, or was the Operation Mode Strap Override (OMSO) write the first access after the soft reset?
No led-mode was specified so OMSO was the first write.
> Did you try any other workarounds besides setting the clock mode before doing the OMSO write?
I spend around 2 weeks hunting down the bug.
During which I tried many things in both the Freescale FEC MAC driver and the Micrel PHY driver.
Changing the init and the probe flow as well as adding a lot of extra debug traces.
> And REF_CLK (pin 16) is not connected?
Yes, pin 16 is floating.
> Would you able to test my series on your setup, and possibly a couple of diagnostic patches on top?
Sure, I can try later this week.
/Bruno
^ permalink raw reply
* Re: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: Eric Dumazet @ 2014-11-17 14:55 UTC (permalink / raw)
To: David Laight
Cc: Jeff Kirsher, davem@davemloft.net, Serey Kong,
netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com, Shannon Nelson
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9F1E87@AcuExch.aculab.com>
On Mon, 2014-11-17 at 14:40 +0000, David Laight wrote:
> Not that one.
> IIRC is was done to reduce the number of fragments through xennet.
>
> You really want to try to keep some of the old fragments, just adding
> new ones to merge short sections.
>
> OTOH 8 fragments isn't enough if the MAC supports TSO (dunno if it does).
I40 is a 40Gb NIC, it supports TSO, obviously.
> The skb_linearize() is only likely to fail for TSO anyway.
> Or rather, if it can't allocate a 4k page you are already stuffed.
So far, nobody wrote the code for this.
Page frag are allocated from pages, so if you cannot allocate a 4k page,
this function has no guarantee to succeed no matter how hard you tried.
alloc_skb_with_frags() was already _much_ better than a single
allocation of order-5 page. But if you feel you can do better, why don't
you provide the code ?
^ permalink raw reply
* RE: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: David Laight @ 2014-11-17 14:40 UTC (permalink / raw)
To: 'Eric Dumazet'
Cc: Jeff Kirsher, davem@davemloft.net, Serey Kong,
netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com, Shannon Nelson
In-Reply-To: <1416234687.5102.2.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric
> On Mon, 2014-11-17 at 14:15 +0000, David Laight wrote:
>
> > Didn't someone write a function that significantly reduces the
> > number of fragments without actually requiring a single linear block?
>
> I presume you refer to alloc_skb_with_frags() : It does the allocation,
> but I was expecting some kind soul to continue my work and provide the
> companion code to do the copy.
Not that one.
IIRC is was done to reduce the number of fragments through xennet.
You really want to try to keep some of the old fragments, just adding
new ones to merge short sections.
OTOH 8 fragments isn't enough if the MAC supports TSO (dunno if it does).
The skb_linearize() is only likely to fail for TSO anyway.
Or rather, if it can't allocate a 4k page you are already stuffed.
David.
^ permalink raw reply
* pull-request: can-next 2014-11-17
From: Marc Kleine-Budde @ 2014-11-17 14:41 UTC (permalink / raw)
To: netdev; +Cc: David Miller, linux-can@vger.kernel.org, kernel@pengutronix.de
[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]
Hello David,
this is a pull request of 9 patches for net-next/master.
All 9 patches are by Roger Quadros and update the c_can platform
driver. First by improving the initialization sequence of the message
RAM, making use of syscon/regmap. In the later patches support for
various TI SoCs is added.
Marc
---
The following changes since commit 65622ed40eef5ce2732365077b22416593fec4c8:
Merge branch 'rss_key_fill' (2014-11-16 15:59:19 -0500)
are available in the git repository at:
git://gitorious.org/linux-can/linux-can-next.git tags/linux-can-next-for-3.19-20141117
for you to fetch changes up to f2bf2589834faec7af8c02c3949c90788d21b790:
net: can: c_can: Add support for TI am4372 DCAN (2014-11-17 15:32:10 +0100)
----------------------------------------------------------------
linux-can-next-for-3.19-20141117
----------------------------------------------------------------
Roger Quadros (9):
can: c_can: Add timeout to c_can_hw_raminit_ti()
can: c_can: Introduce c_can_driver_data structure
can: c_can: Add RAMINIT register information to driver data
can: c_can: Add syscon/regmap RAMINIT mechanism
can: c_can: Add support for START pulse in RAMINIT sequence
can: c_can: Disable pins when CAN interface is down
can: c_can: Add support for TI DRA7 DCAN
can: c_can: Add support for TI am3352 DCAN
net: can: c_can: Add support for TI am4372 DCAN
.../devicetree/bindings/net/can/c_can.txt | 5 +
drivers/net/can/c_can/c_can.c | 13 ++
drivers/net/can/c_can/c_can.h | 25 ++-
drivers/net/can/c_can/c_can_platform.c | 201 ++++++++++++++-------
4 files changed, 181 insertions(+), 63 deletions(-)
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: Eric Dumazet @ 2014-11-17 14:31 UTC (permalink / raw)
To: David Laight
Cc: Jeff Kirsher, davem@davemloft.net, Serey Kong,
netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com, Shannon Nelson
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9F1E54@AcuExch.aculab.com>
On Mon, 2014-11-17 at 14:15 +0000, David Laight wrote:
> Didn't someone write a function that significantly reduces the
> number of fragments without actually requiring a single linear block?
I presume you refer to alloc_skb_with_frags() : It does the allocation,
but I was expecting some kind soul to continue my work and provide the
companion code to do the copy.
http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=2e4e44107176d552f8bb1bb76053e850e3809841
So far nobody took care of it.
^ permalink raw reply
* RE: [net-next 03/12] i40e: Handle a single mss packet with more than 8 frags
From: David Laight @ 2014-11-17 14:15 UTC (permalink / raw)
To: 'Eric Dumazet', Jeff Kirsher
Cc: davem@davemloft.net, Serey Kong, netdev@vger.kernel.org,
nhorman@redhat.com, sassmann@redhat.com, jogreene@redhat.com,
Shannon Nelson
In-Reply-To: <1416075695.17262.86.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet
> > This handles the case where a single packet with more than 8 data
> > descriptors triggers a Malicious Driver Detect event in the device.
Whose bright idea was that?
> > @@ -2129,10 +2129,16 @@ static void i40e_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,
> > I40E_TX_FLAGS_VLAN_SHIFT;
> > }
> >
> > - if (tx_flags & (I40E_TX_FLAGS_TSO | I40E_TX_FLAGS_FSO))
> > + if (tx_flags & (I40E_TX_FLAGS_TSO | I40E_TX_FLAGS_FSO)) {
> > gso_segs = skb_shinfo(skb)->gso_segs;
> > - else
> > + } else {
> > gso_segs = 1;
> > + if (skb_shinfo(skb)->nr_frags >= I40E_MAX_BUFFER_TXD)
> > + skb_linearize(skb);
>
> What exactly happens if skb_linearize() fails ?
Didn't someone write a function that significantly reduces the
number of fragments without actually requiring a single linear block?
David
^ permalink raw reply
* Re: [PATCH 1/1 net-next] wireless: remove unnecessary sizeof(u8)
From: John W. Linville @ 2014-11-17 13:36 UTC (permalink / raw)
To: Fabian Frederick
Cc: Julian Calaby, Johannes Berg, linux-wireless, Larry Finger,
Chaoming Li, netdev, linux-kernel@vger.kernel.org,
Emmanuel Grumbach, b43-dev, Stefano Brivio, Intel Linux Wireless
In-Reply-To: <828430032.148603.1416203787175.open-xchange@webmail.nmp.skynet.be>
On Mon, Nov 17, 2014 at 06:56:27AM +0100, Fabian Frederick wrote:
>
>
> > On 16 November 2014 at 23:33 Julian Calaby <julian.calaby@gmail.com> wrote:
> >
> >
> > Hi Fabian,
> >
> > On Sat, Nov 15, 2014 at 7:55 AM, Fabian Frederick <fabf@skynet.be> wrote:
> > > sizeof(u8) is always 1.
> >
> > I thought that sizeof(*variable) was preferred over sizeof(type), so
> > shouldn't these be switched to that format instead?
> >
> > (I know that this is all no-op, but it should reduce the potential for
> > highly unlikely bugs in the future. Also, the extra processing is
> > compile-time not run-time.)
> >
> > Thanks,
>
> Hi Julian,
>
> Of course but char/u8/s8... allocations never use it and result would be the
> same:
> factor 1 multiplication.
>
> Those rare occurrences (+- 30 in the whole kernel) where we have
> sizeof(u8/s8) is ambiguous.
>
> Having a patch removing it gives a pointer...
> If the developer meant something else, he will be able to fix it.
>
> Regards,
> Fabian
sizeof(*variable) still seems safer. Are the compilers unable to
optimize-away a "multiply by one"?
John
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* Re: [PATCH] net: team: expose sysfs attributes for each team option
From: Jiri Pirko @ 2014-11-17 13:02 UTC (permalink / raw)
To: Hamad Kadmany; +Cc: netdev
In-Reply-To: <93856491220bda9aede41a9571fb1ac1.squirrel@www.codeaurora.org>
Mon, Nov 17, 2014 at 11:11:31AM CET, hkadmany@codeaurora.org wrote:
>Current code provides only netlink API for user space
>to read/write options. Exposing sysfs API is useful for
>systems that don't have the required netlink
>user space support.
>
>Upon registration of team option, corresponding
>sysfs attribute is created.
Nak.
I don't like this patch. The plan for team was keep things simple and to
have single userspace api using netlink, as that is the correct way to
deal with things. Sysfs for this use-case is not. Please, use netlink api.
>
>Signed-off-by: Hamad Kadmany <hkadmany@codeaurora.org>
>---
> drivers/net/team/team.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/if_team.h | 3 +
> 2 files changed, 278 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index 1222229..afd2f8f 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -110,8 +110,198 @@ struct team_option_inst { /* One for each option instance */
> struct team_option_inst_info info;
> bool changed;
> bool removed;
>+ bool dev_attr_file_exist;
>+ struct device_attribute dev_attr; /* corresponding sysfs attribute */
> };
>
>+static struct attribute *team_sysfs_attrs[] = {
>+ NULL,
>+};
>+
>+static struct attribute_group team_sysfs_attr_group = {
>+ .attrs = team_sysfs_attrs,
>+};
>+
>+/* Device attributes (sysfs) */
>+
>+static ssize_t show_attribute(struct device *dev,
>+ struct device_attribute *attr,
>+ char *buf)
>+{
>+ struct team *team = dev_get_drvdata(dev);
>+ struct team_option_inst *opt_inst;
>+ ssize_t ret;
>+ struct team_option *option;
>+ struct team_gsetter_ctx ctx;
>+
>+ if (mutex_lock_interruptible(&team->lock))
>+ return -ERESTARTSYS;
>+
>+ opt_inst = container_of(attr, struct team_option_inst, dev_attr);
>+ option = opt_inst->option;
>+ if (!option->getter) {
>+ ret = -EOPNOTSUPP;
>+ netdev_err(team->dev,
>+ "Option %s is write only\n", attr->attr.name);
>+ goto exit;
>+ }
>+
>+ ctx.info = &opt_inst->info;
>+ /* let the option getter do its job */
>+ ret = option->getter(team, &ctx);
>+ if (ret)
>+ goto exit;
>+
>+ /* translate option's output into sysfs output */
>+ switch (option->type) {
>+ case TEAM_OPTION_TYPE_U32:
>+ ret = scnprintf(buf, PAGE_SIZE, "%u\n", ctx.data.u32_val);
>+ break;
>+ case TEAM_OPTION_TYPE_STRING:
>+ ret = scnprintf(buf, PAGE_SIZE, "%s\n", ctx.data.str_val);
>+ break;
>+ case TEAM_OPTION_TYPE_BINARY:
>+ if (ctx.data.bin_val.len > PAGE_SIZE) {
>+ netdev_err(team->dev,
>+ "Option output is too long (%d)\n",
>+ ctx.data.bin_val.len);
>+ break;
>+ }
>+
>+ memcpy(buf, ctx.data.bin_val.ptr, ctx.data.bin_val.len);
>+ ret = ctx.data.bin_val.len;
>+ break;
>+ case TEAM_OPTION_TYPE_BOOL:
>+ ret = scnprintf(buf, PAGE_SIZE, "%d\n", ctx.data.bool_val);
>+ break;
>+ case TEAM_OPTION_TYPE_S32:
>+ ret = scnprintf(buf, PAGE_SIZE, "%d\n", ctx.data.s32_val);
>+ break;
>+ default:
>+ BUG();
>+ }
>+
>+exit:
>+ mutex_unlock(&team->lock);
>+
>+ return ret;
>+}
>+
>+static int team_nl_send_event_options_get(struct team *team,
>+ struct list_head *sel_opt_inst_list);
>+
>+static ssize_t set_attribute(struct device *dev,
>+ struct device_attribute *attr,
>+ const char *buf, size_t count)
>+{
>+ struct team_option_inst *opt_inst;
>+ struct team *team = dev_get_drvdata(dev);
>+ int err = 0;
>+ struct team_option *option = NULL;
>+ struct team_gsetter_ctx ctx;
>+
>+ LIST_HEAD(opt_inst_list);
>+
>+ if (mutex_lock_interruptible(&team->lock))
>+ return -ERESTARTSYS;
>+
>+ opt_inst = container_of(attr, struct team_option_inst, dev_attr);
>+ option = opt_inst->option;
>+ if (!option->setter) {
>+ netdev_err(team->dev,
>+ "Option %s is read only\n", attr->attr.name);
>+ err = -EOPNOTSUPP;
>+ goto exit;
>+ }
>+
>+ ctx.info = &opt_inst->info;
>+
>+ /* translate sysfs input into option's input */
>+ switch (option->type) {
>+ case TEAM_OPTION_TYPE_U32:
>+ err = kstrtoint(buf, 0, &ctx.data.u32_val);
>+ break;
>+ case TEAM_OPTION_TYPE_STRING:
>+ if (count > TEAM_STRING_MAX_LEN) {
>+ netdev_err(team->dev,
>+ "Input buffer too long (%zu)\n", count);
>+ err = -EINVAL;
>+ break;
>+ }
>+ ctx.data.str_val = buf;
>+ break;
>+ case TEAM_OPTION_TYPE_BINARY:
>+ ctx.data.bin_val.len = count;
>+ ctx.data.bin_val.ptr = buf;
>+ break;
>+ case TEAM_OPTION_TYPE_BOOL:
>+ err = strtobool(buf, &ctx.data.bool_val);
>+ break;
>+ case TEAM_OPTION_TYPE_S32:
>+ err = kstrtoint(buf, 0, &ctx.data.s32_val);
>+ break;
>+ default:
>+ BUG();
>+ }
>+
>+ if (err) {
>+ netdev_err(team->dev, "Failed to translate input buffer\n");
>+ goto exit;
>+ }
>+
>+ /* let the option setter do its job */
>+ err = option->setter(team, &ctx);
>+ if (err)
>+ goto exit;
>+
>+ /* propagate option changed event */
>+ opt_inst->changed = true;
>+ list_add(&opt_inst->tmp_list, &opt_inst_list);
>+ err = team_nl_send_event_options_get(team, &opt_inst_list);
>+ if (err == -ESRCH) /* no listeners, not a real error */
>+ err = 0;
>+
>+exit:
>+ mutex_unlock(&team->lock);
>+
>+ if (!err)
>+ err = count;
>+ return err;
>+}
>+
>+/* create sysfs attribute for each option that is being registered */
>+static int __team_option_add_sysfs_attr(struct team *team,
>+ struct team_option_inst *opt_inst,
>+ bool create_sysfs_file)
>+{
>+ int err = 0;
>+ struct device_attribute *new_dev_attr = &opt_inst->dev_attr;
>+
>+ new_dev_attr->attr.name = opt_inst->option->name;
>+ new_dev_attr->attr.mode = S_IRUGO | S_IWUSR;
>+ new_dev_attr->show = show_attribute;
>+ new_dev_attr->store = set_attribute;
>+
>+ if (create_sysfs_file) {
>+ err = sysfs_create_file(&team->dev->dev.kobj,
>+ &new_dev_attr->attr);
>+ if (err)
>+ netdev_err(team->dev,
>+ "Failed to create sysfs attribute %s\n",
>+ new_dev_attr->attr.name);
>+ }
>+
>+ return err;
>+}
>+
>+static void __team_option_del_sysfs_attr(struct team *team,
>+ struct team_option_inst *opt_inst)
>+{
>+ if (opt_inst->dev_attr_file_exist)
>+ sysfs_remove_file(&team->dev->dev.kobj,
>+ &opt_inst->dev_attr.attr);
>+}
>+
> static struct team_option *__team_find_option(struct team *team,
> const char *opt_name)
> {
>@@ -124,8 +314,10 @@ static struct team_option *__team_find_option(struct team *team,
> return NULL;
> }
>
>-static void __team_option_inst_del(struct team_option_inst *opt_inst)
>+static void __team_option_inst_del(struct team *team,
>+ struct team_option_inst *opt_inst)
> {
>+ __team_option_del_sysfs_attr(team, opt_inst);
> list_del(&opt_inst->list);
> kfree(opt_inst);
> }
>@@ -137,7 +329,7 @@ static void __team_option_inst_del_option(struct team *team,
>
> list_for_each_entry_safe(opt_inst, tmp, &team->option_inst_list, list) {
> if (opt_inst->option == option)
>- __team_option_inst_del(opt_inst);
>+ __team_option_inst_del(team, opt_inst);
> }
> }
>
>@@ -162,6 +354,7 @@ static int __team_option_inst_add(struct team *team, struct team_option *option,
> opt_inst->info.array_index = i;
> opt_inst->changed = true;
> opt_inst->removed = false;
>+ opt_inst->dev_attr_file_exist = false;
> list_add_tail(&opt_inst->list, &team->option_inst_list);
> if (option->init) {
> err = option->init(team, &opt_inst->info);
>@@ -170,6 +363,20 @@ static int __team_option_inst_add(struct team *team, struct team_option *option,
> }
>
> }
>+
>+ /* add sysfs attribute. per-port and array options are skipped */
>+ if (!option->per_port && !option->array_size) {
>+ /* create the sysfs file only if our state allows it */
>+ bool create_sysfs_file = device_is_registered(&team->dev->dev);
>+
>+ err = __team_option_add_sysfs_attr(team, opt_inst,
>+ create_sysfs_file);
>+ if (err)
>+ return err;
>+
>+ opt_inst->dev_attr_file_exist = true;
>+ }
>+
> return 0;
> }
>
>@@ -218,7 +425,7 @@ static void __team_option_inst_del_port(struct team *team,
> list_for_each_entry_safe(opt_inst, tmp, &team->option_inst_list, list) {
> if (opt_inst->option->per_port &&
> opt_inst->info.port == port)
>- __team_option_inst_del(opt_inst);
>+ __team_option_inst_del(team, opt_inst);
> }
> }
>
>@@ -337,6 +544,51 @@ static void __team_options_unregister(struct team *team,
>
> static void __team_options_change_check(struct team *team);
>
>+static void team_attr_grp_free(struct team *team)
>+{
>+ kfree(team->attr_grp.attrs);
>+}
>+
>+/* allocate attribute group for creating sysfs for team's own options */
>+static int team_attr_grp_alloc(struct team *team)
>+{
>+ struct attribute **attributes;
>+ struct team_option_inst *opt_inst;
>+ int num_attr = 0;
>+ struct team_option *option;
>+
>+ list_for_each_entry(opt_inst, &team->option_inst_list, list) {
>+ option = opt_inst->option;
>+ /* per-port and array options currently not supported as
>+ * sysfs attributes
>+ */
>+ if (option->per_port || option->array_size)
>+ continue;
>+
>+ num_attr++;
>+ }
>+
>+ /* +1 for having NULL as last item in the array */
>+ attributes = kzalloc((num_attr + 1) * sizeof(*attributes), GFP_KERNEL);
>+ if (!attributes)
>+ return -ENOMEM;
>+ team->attr_grp.attrs = attributes;
>+
>+ num_attr = 0;
>+ list_for_each_entry(opt_inst, &team->option_inst_list, list) {
>+ option = opt_inst->option;
>+ /* per-port and array options currently not supported as
>+ * sysfs attributes
>+ */
>+ if (option->per_port || option->array_size)
>+ continue;
>+
>+ attributes[num_attr++] = &opt_inst->dev_attr.attr;
>+ }
>+
>+ return 0;
>+}
>+
> int team_options_register(struct team *team,
> const struct team_option *option,
> size_t option_count)
>@@ -1380,15 +1632,28 @@ static int team_init(struct net_device *dev)
>
> INIT_LIST_HEAD(&team->option_list);
> INIT_LIST_HEAD(&team->option_inst_list);
>- err = team_options_register(team, team_options, ARRAY_SIZE(team_options));
>+
>+ err = team_options_register(team, team_options,
>+ ARRAY_SIZE(team_options));
> if (err)
> goto err_options_register;
> netif_carrier_off(dev);
>
> team_set_lockdep_class(dev);
>
>+ /* store team context, to be used by Device attributes getter/setter */
>+ dev_set_drvdata(&dev->dev, team);
>+
>+ /* allocate and register sysfs attributes for team's own options */
>+ err = team_attr_grp_alloc(team);
>+ if (err)
>+ goto err_grp_alloc;
>+ dev->sysfs_groups[0] = &team->attr_grp;
>+
> return 0;
>
>+err_grp_alloc:
>+ team_options_unregister(team, team_options, ARRAY_SIZE(team_options));
> err_options_register:
> team_queue_override_fini(team);
> err_team_queue_override_init:
>@@ -1407,9 +1672,15 @@ static void team_uninit(struct net_device *dev)
> list_for_each_entry_safe(port, tmp, &team->port_list, list)
> team_port_del(team, port->dev);
>
>+ sysfs_remove_group(&team->dev->dev.kobj, &team->attr_grp);
>+ team_attr_grp_free(team);
>+ /* set to dummy group to avoid double free by core */
>+ dev->sysfs_groups[0] = &team_sysfs_attr_group;
>+
> __team_change_mode(team, NULL); /* cleanup */
> __team_options_unregister(team, team_options, ARRAY_SIZE(team_options));
> team_queue_override_fini(team);
>+
> mutex_unlock(&team->lock);
> }
>
>@@ -2194,9 +2465,6 @@ static int team_nl_cmd_options_get(struct sk_buff *skb, struct genl_info *info)
> return err;
> }
>
>-static int team_nl_send_event_options_get(struct team *team,
>- struct list_head *sel_opt_inst_list);
>-
> static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
> {
> struct team *team;
>diff --git a/include/linux/if_team.h b/include/linux/if_team.h
>index 25b8b15..2e9fb2a 100644
>--- a/include/linux/if_team.h
>+++ b/include/linux/if_team.h
>@@ -188,6 +188,9 @@ struct team {
> struct list_head option_list;
> struct list_head option_inst_list; /* list of option instances */
>
>+ /* attribute group for registering team's own options at init */
>+ struct attribute_group attr_grp;
>+
> const struct team_mode *mode;
> struct team_mode_ops ops;
> bool user_carrier_enabled;
>--
>1.8.5.2
>--
>Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>
^ permalink raw reply
* [BNX2] A Netdev Watchdog with kernel stable 3.4
From: Rui Xiang @ 2014-11-17 12:42 UTC (permalink / raw)
To: Michael Chan; +Cc: netdev
Hi Michael,
On a system that was running stable 3.4.87, I got the below stack.
That was a NETDEV WATCHDOG. And we could also see watchdog timeouts with the
BNX2. (After the stack, an oops occurred while running ifconfig. I think it
would be related to this timeout.)
Otherwises, the bnx2_dump_state and bnx2_dump_mcp_state have printed the states.
Through these states info, can we got the real situation of NIC1.
Or can we see what resulted the WATCHDOG, a bnx2 device fault or other reasons.
Thanks.
*The stack*:
WARNING: at /usr/src/packages/BUILD/kernel-default-3.4.87/linux-3.4/net/sched/sch_generic.c:256 dev_watchdog+0x256/0x260()
NETDEV WATCHDOG: NIC1 (bnx2): transmit queue 3 timed out
Modules linked in: smb3_failover(O) smb2(O) smb(O) smb_manager(O) nfs(O) nfs_acl(O) nfsd(O) lockd(O) nal(O) auth_rpcgss(O)
scsi_dh_hp_sw scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh scsi_mod [last unloaded: ipmi_msghandler]
Pid: 0, comm: swapper/0 Tainted: P W O 3.4.87-default #1
Call Trace:
<IRQ> [<ffffffff8103fcea>] warn_slowpath_common+0x7a/0xb0
[<ffffffff8103fdc1>] warn_slowpath_fmt+0x41/0x50
[<ffffffff81047749>] ? raise_softirq_irqoff+0x9/0x30
[<ffffffff813ae0f6>] dev_watchdog+0x256/0x260
[<ffffffff813adea0>] ? dev_deactivate_queue.constprop.30+0x70/0x70
[<ffffffff8104edc7>] run_timer_softirq+0x147/0x340
[<ffffffff810470d8>] __do_softirq+0xc8/0x1e0
[<ffffffff8109250f>] ? tick_program_event+0x1f/0x30
[<ffffffff81460a6c>] call_softirq+0x1c/0x30
[<ffffffff8100417d>] do_softirq+0x9d/0xd0
[<ffffffff810474a5>] irq_exit+0xb5/0xc0
[<ffffffff81021b49>] smp_apic_timer_interrupt+0x69/0xa0
[<ffffffff8146006f>] apic_timer_interrupt+0x6f/0x80
<EOI> [<ffffffff81457bdd>] ? retint_restore_args+0x13/0x13
[<ffffffff81360149>] ? poll_idle+0x49/0x90
[<ffffffff8136011f>] ? poll_idle+0x1f/0x90
[<ffffffff8135fcc9>] cpuidle_enter+0x19/0x20
[<ffffffff813602f2>] cpuidle_idle_call+0xa2/0x250
[<ffffffff8100b08f>] cpu_idle+0x6f/0xe0
[<ffffffff81915960>] ? rawsock_init+0x12/0x12
[<ffffffff814331c9>] rest_init+0x6d/0x74
[<ffffffff818d3be5>] start_kernel+0x3a2/0x3af
[<ffffffff818d3642>] ? repair_env_string+0x5e/0x5e
[<ffffffff818d332a>] x86_64_start_reservations+0x131/0x135
[<ffffffff818d342e>] x86_64_start_kernel+0x100/0x10f
---[ end trace 497e24e681e0c02d ]---
bnx2 0000:05:00.1: NIC1: DEBUG: intr_sem[0] PCI_CMD[00100002]
bnx2 0000:05:00.1: NIC1: DEBUG: PCI_PM[19002008] PCI_MISC_CFG[92000088]
bnx2 0000:05:00.1: NIC1: DEBUG: EMAC_TX_STATUS[00000008] EMAC_RX_STATUS[00000000]
bnx2 0000:05:00.1: NIC1: DEBUG: RPM_MGMT_PKT_CTRL[40000088]
bnx2 0000:05:00.1: NIC1: DEBUG: HC_STATS_INTERRUPT_STATUS[01ff0000]
bnx2 0000:05:00.1: NIC1: DEBUG: PBA[00000000]
bnx2 0000:05:00.1: NIC1: <--- start MCP states dump --->
bnx2 0000:05:00.1: NIC1: DEBUG: MCP_STATE_P0[0003e10e] MCP_STATE_P1[0003e10e]
bnx2 0000:05:00.1: NIC1: DEBUG: MCP mode[0000b800] state[80008000] evt_mask[00000500]
bnx2 0000:05:00.1: NIC1: DEBUG: pc[08008f60] pc[0800d21c] instr[00051080]
bnx2 0000:05:00.1: NIC1: DEBUG: shmem states:
bnx2 0000:05:00.1: NIC1: DEBUG: drv_mb[01030003] fw_mb[00000003] link_status[0000006f] drv_pulse_mb[0000073d]
bnx2 0000:05:00.1: NIC1: DEBUG: dev_info_signature[44564907] reset_type[01005254] condition[0003e10e]
bnx2 0000:05:00.1: NIC1: DEBUG: 000003cc: 00000000 00000000 00000000 00000000
bnx2 0000:05:00.1: NIC1: DEBUG: 000003dc: 00000000 00000000 00000000 00000000
bnx2 0000:05:00.1: NIC1: DEBUG: 000003ec: 00000000 00000000 00000000 00000000
bnx2 0000:05:00.1: NIC1: DEBUG: 0x3fc[00000000]
bnx2 0000:05:00.1: NIC1: <--- end MCP states dump --->
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox