* [REGRESSION][PATCH V5 3/3] bpf jit: Let the powerpc jit handle negative offsets
From: Jan Seiffert @ 2012-04-30 5:02 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, matt, linux-kernel, linuxppc-dev, David Miller
In-Reply-To: <1335759088.20866.32.camel@pasglop>
Now the helper function from filter.c for negative offsets is exported,
it can be used it in the jit to handle negative offsets.
First modify the asm load helper functions to handle:
- know positive offsets
- know negative offsets
- any offset
then the compiler can be modified to explicitly use these helper
when appropriate.
This fixes the case of a negative X register and allows to lift
the restriction that bpf programs with negative offsets can't
be jited.
Tested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com>
---
arch/powerpc/net/bpf_jit.h | 8 +++-
arch/powerpc/net/bpf_jit_64.S | 108 ++++++++++++++++++++++++++++++++++-----
arch/powerpc/net/bpf_jit_comp.c | 26 +++------
3 files changed, 111 insertions(+), 31 deletions(-)
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index af1ab5e..5c3cf2d 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -48,7 +48,13 @@
/*
* Assembly helpers from arch/powerpc/net/bpf_jit.S:
*/
-extern u8 sk_load_word[], sk_load_half[], sk_load_byte[], sk_load_byte_msh[];
+#define DECLARE_LOAD_FUNC(func) \
+ extern u8 func[], func##_negative_offset[], func##_positive_offset[]
+
+DECLARE_LOAD_FUNC(sk_load_word);
+DECLARE_LOAD_FUNC(sk_load_half);
+DECLARE_LOAD_FUNC(sk_load_byte);
+DECLARE_LOAD_FUNC(sk_load_byte_msh);
#define FUNCTION_DESCR_SIZE 24
diff --git a/arch/powerpc/net/bpf_jit_64.S b/arch/powerpc/net/bpf_jit_64.S
index ff4506e..55ba385 100644
--- a/arch/powerpc/net/bpf_jit_64.S
+++ b/arch/powerpc/net/bpf_jit_64.S
@@ -31,14 +31,13 @@
* then branch directly to slow_path_XXX if required. (In fact, could
* load a spare GPR with the address of slow_path_generic and pass size
* as an argument, making the call site a mtlr, li and bllr.)
- *
- * Technically, the "is addr < 0" check is unnecessary & slowing down
- * the ABS path, as it's statically checked on generation.
*/
.globl sk_load_word
sk_load_word:
cmpdi r_addr, 0
- blt bpf_error
+ blt bpf_slow_path_word_neg
+ .globl sk_load_word_positive_offset
+sk_load_word_positive_offset:
/* Are we accessing past headlen? */
subi r_scratch1, r_HL, 4
cmpd r_scratch1, r_addr
@@ -51,7 +50,9 @@ sk_load_word:
.globl sk_load_half
sk_load_half:
cmpdi r_addr, 0
- blt bpf_error
+ blt bpf_slow_path_half_neg
+ .globl sk_load_half_positive_offset
+sk_load_half_positive_offset:
subi r_scratch1, r_HL, 2
cmpd r_scratch1, r_addr
blt bpf_slow_path_half
@@ -61,7 +62,9 @@ sk_load_half:
.globl sk_load_byte
sk_load_byte:
cmpdi r_addr, 0
- blt bpf_error
+ blt bpf_slow_path_byte_neg
+ .globl sk_load_byte_positive_offset
+sk_load_byte_positive_offset:
cmpd r_HL, r_addr
ble bpf_slow_path_byte
lbzx r_A, r_D, r_addr
@@ -69,22 +72,20 @@ sk_load_byte:
/*
* BPF_S_LDX_B_MSH: ldxb 4*([offset]&0xf)
- * r_addr is the offset value, already known positive
+ * r_addr is the offset value
*/
.globl sk_load_byte_msh
sk_load_byte_msh:
+ cmpdi r_addr, 0
+ blt bpf_slow_path_byte_msh_neg
+ .globl sk_load_byte_msh_positive_offset
+sk_load_byte_msh_positive_offset:
cmpd r_HL, r_addr
ble bpf_slow_path_byte_msh
lbzx r_X, r_D, r_addr
rlwinm r_X, r_X, 2, 32-4-2, 31-2
blr
-bpf_error:
- /* Entered with cr0 = lt */
- li r3, 0
- /* Generated code will 'blt epilogue', returning 0. */
- blr
-
/* Call out to skb_copy_bits:
* We'll need to back up our volatile regs first; we have
* local variable space at r1+(BPF_PPC_STACK_BASIC).
@@ -136,3 +137,84 @@ bpf_slow_path_byte_msh:
lbz r_X, BPF_PPC_STACK_BASIC+(2*8)(r1)
rlwinm r_X, r_X, 2, 32-4-2, 31-2
blr
+
+/* Call out to bpf_internal_load_pointer_neg_helper:
+ * We'll need to back up our volatile regs first; we have
+ * local variable space at r1+(BPF_PPC_STACK_BASIC).
+ * Allocate a new stack frame here to remain ABI-compliant in
+ * stashing LR.
+ */
+#define sk_negative_common(SIZE) \
+ mflr r0; \
+ std r0, 16(r1); \
+ /* R3 goes in parameter space of caller's frame */ \
+ std r_skb, (BPF_PPC_STACKFRAME+48)(r1); \
+ std r_A, (BPF_PPC_STACK_BASIC+(0*8))(r1); \
+ std r_X, (BPF_PPC_STACK_BASIC+(1*8))(r1); \
+ stdu r1, -BPF_PPC_SLOWPATH_FRAME(r1); \
+ /* R3 = r_skb, as passed */ \
+ mr r4, r_addr; \
+ li r5, SIZE; \
+ bl bpf_internal_load_pointer_neg_helper; \
+ /* R3 != 0 on success */ \
+ addi r1, r1, BPF_PPC_SLOWPATH_FRAME; \
+ ld r0, 16(r1); \
+ ld r_A, (BPF_PPC_STACK_BASIC+(0*8))(r1); \
+ ld r_X, (BPF_PPC_STACK_BASIC+(1*8))(r1); \
+ mtlr r0; \
+ cmpldi r3, 0; \
+ beq bpf_error_slow; /* cr0 = EQ */ \
+ mr r_addr, r3; \
+ ld r_skb, (BPF_PPC_STACKFRAME+48)(r1); \
+ /* Great success! */
+
+bpf_slow_path_word_neg:
+ lis r_scratch1,-32 /* SKF_LL_OFF */
+ cmpd r_addr, r_scratch1 /* addr < SKF_* */
+ blt bpf_error /* cr0 = LT */
+ .globl sk_load_word_negative_offset
+sk_load_word_negative_offset:
+ sk_negative_common(4)
+ lwz r_A, 0(r_addr)
+ blr
+
+bpf_slow_path_half_neg:
+ lis r_scratch1,-32 /* SKF_LL_OFF */
+ cmpd r_addr, r_scratch1 /* addr < SKF_* */
+ blt bpf_error /* cr0 = LT */
+ .globl sk_load_half_negative_offset
+sk_load_half_negative_offset:
+ sk_negative_common(2)
+ lhz r_A, 0(r_addr)
+ blr
+
+bpf_slow_path_byte_neg:
+ lis r_scratch1,-32 /* SKF_LL_OFF */
+ cmpd r_addr, r_scratch1 /* addr < SKF_* */
+ blt bpf_error /* cr0 = LT */
+ .globl sk_load_byte_negative_offset
+sk_load_byte_negative_offset:
+ sk_negative_common(1)
+ lbz r_A, 0(r_addr)
+ blr
+
+bpf_slow_path_byte_msh_neg:
+ lis r_scratch1,-32 /* SKF_LL_OFF */
+ cmpd r_addr, r_scratch1 /* addr < SKF_* */
+ blt bpf_error /* cr0 = LT */
+ .globl sk_load_byte_msh_negative_offset
+sk_load_byte_msh_negative_offset:
+ sk_negative_common(1)
+ lbz r_X, 0(r_addr)
+ rlwinm r_X, r_X, 2, 32-4-2, 31-2
+ blr
+
+bpf_error_slow:
+ /* fabricate a cr0 = lt */
+ li r_scratch1, -1
+ cmpdi r_scratch1, 0
+bpf_error:
+ /* Entered with cr0 = lt */
+ li r3, 0
+ /* Generated code will 'blt epilogue', returning 0. */
+ blr
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 73619d3..2dc8b14 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -127,6 +127,9 @@ static void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
PPC_BLR();
}
+#define CHOOSE_LOAD_FUNC(K, func) \
+ ((int)K < 0 ? ((int)K >= SKF_LL_OFF ? func##_negative_offset : func) : func##_positive_offset)
+
/* Assemble the body code between the prologue & epilogue. */
static int bpf_jit_build_body(struct sk_filter *fp, u32 *image,
struct codegen_context *ctx,
@@ -391,21 +394,16 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image,
/*** Absolute loads from packet header/data ***/
case BPF_S_LD_W_ABS:
- func = sk_load_word;
+ func = CHOOSE_LOAD_FUNC(K, sk_load_word);
goto common_load;
case BPF_S_LD_H_ABS:
- func = sk_load_half;
+ func = CHOOSE_LOAD_FUNC(K, sk_load_half);
goto common_load;
case BPF_S_LD_B_ABS:
- func = sk_load_byte;
+ func = CHOOSE_LOAD_FUNC(K, sk_load_byte);
common_load:
- /*
- * Load from [K]. Reference with the (negative)
- * SKF_NET_OFF/SKF_LL_OFF offsets is unsupported.
- */
+ /* Load from [K]. */
ctx->seen |= SEEN_DATAREF;
- if ((int)K < 0)
- return -ENOTSUPP;
PPC_LI64(r_scratch1, func);
PPC_MTLR(r_scratch1);
PPC_LI32(r_addr, K);
@@ -429,7 +427,7 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image,
common_load_ind:
/*
* Load from [X + K]. Negative offsets are tested for
- * in the helper functions, and result in a 'ret 0'.
+ * in the helper functions.
*/
ctx->seen |= SEEN_DATAREF | SEEN_XREG;
PPC_LI64(r_scratch1, func);
@@ -443,13 +441,7 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image,
break;
case BPF_S_LDX_B_MSH:
- /*
- * x86 version drops packet (RET 0) when K<0, whereas
- * interpreter does allow K<0 (__load_pointer, special
- * ancillary data). common_load returns ENOTSUPP if K<0,
- * so we fall back to interpreter & filter works.
- */
- func = sk_load_byte_msh;
+ func = CHOOSE_LOAD_FUNC(K, sk_load_byte_msh);
goto common_load;
break;
^ permalink raw reply related
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets
From: Benjamin Herrenschmidt @ 2012-04-30 5:26 UTC (permalink / raw)
To: kaffeemonster
Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev,
David Miller
In-Reply-To: <4F9E188E.80503@googlemail.com>
> No idea, i was going by the old saying:
> "Thou shall not include kernel header, or you will feel the wrath of angry
> kernel gurus."
Heh :-)
Well, googling around, it looks like there's a mix of both type of
programs out there. Those using a struct bpf_program and those using a
struct soft_fprog.
Only the latter will work on BE machines as far as I can tell.
David, what's the right way to fix that ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc: Support lower minimum entitlement for virtual processors
From: Benjamin Herrenschmidt @ 2012-04-30 5:30 UTC (permalink / raw)
To: Robert Jennings; +Cc: linuxppc-dev
In-Reply-To: <20120323212213.GB9456@linux.vnet.ibm.com>
On Fri, 2012-03-23 at 16:22 -0500, Robert Jennings wrote:
> This patch changes the architecture vector to advertise support for a
> lower minimum virtual processor entitled capacity. The default
> minimum without this patch is 10%, this patch specifies 5%. This will
> allow 20 LPARs per CPU rather than only 10.
Any reason why we don't just put 1% in there and thus don't have to
change again when pHyp decides to lower their minimum ? :-)
(Can you test that it works, ie, that pHyp doesn't barf if we put 1% and
properly uses 5% in that case ?)
Cheers,
Ben.
> Signed-off-by: Robert Jennings <rcj@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/prom_init.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index eca626e..3d882ea 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -689,6 +689,9 @@ static void __init early_cmdline_parse(void)
> #define OV3_VMX 0x40 /* VMX/Altivec */
> #define OV3_DFP 0x20 /* decimal FP */
>
> +/* Option vector 4: IBM PAPR implementation */
> +#define OV4_MIN_ENT_CAP 0x05 /* minimum VP entitled capacity */
> +
> /* Option vector 5: PAPR/OF options supported */
> #define OV5_LPAR 0x80 /* logical partitioning supported */
> #define OV5_SPLPAR 0x40 /* shared-processor LPAR supported */
> @@ -753,8 +756,9 @@ static unsigned char ibm_architecture_vec[] = {
> OV3_FP | OV3_VMX | OV3_DFP,
>
> /* option vector 4: IBM PAPR implementation */
> - 2 - 2, /* length */
> + 3 - 2, /* length */
> 0, /* don't halt */
> + OV4_MIN_ENT_CAP, /* minimum VP entitled capacity */
>
> /* option vector 5: PAPR/OF options */
> 13 - 2, /* length */
> @@ -771,7 +775,7 @@ static unsigned char ibm_architecture_vec[] = {
> * must match by the macro below. Update the definition if
> * the structure layout changes.
> */
> -#define IBM_ARCH_VEC_NRCORES_OFFSET 100
> +#define IBM_ARCH_VEC_NRCORES_OFFSET 101
> W(NR_CPUS), /* number of cores supported */
>
> /* option vector 6: IBM PAPR hints */
^ permalink raw reply
* Re: [PATCH 3/4] powerpc: Add 64-bit CPU targets for gcc
From: Benjamin Herrenschmidt @ 2012-04-30 5:57 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, paulus, Anton Blanchard
In-Reply-To: <7A78BE77-1D97-4E8D-B8F3-026E2B3B2EEA@kernel.crashing.org>
On Wed, 2012-04-18 at 09:33 -0500, Kumar Gala wrote:
> On Apr 17, 2012, at 11:45 PM, Anton Blanchard wrote:
>
> >
> > Add a menu to select various 64-bit CPU targets for gcc. We
> > default to -mtune=power7 and if gcc doesn't understand that we
> > fallback to -mtune=power4.
> >
> > Signed-off-by: Anton Blanchard <anton@samba.org>
> > ---
>
> Can you add a target for e5500 cpu.
I'm going to put Anton patch in, can you send an add-on for e5500 ?
Cheers,
Ben.
> - k
>
> >
> > Index: linux-build/arch/powerpc/Makefile
> > ===================================================================
> > --- linux-build.orig/arch/powerpc/Makefile 2012-04-18 14:31:31.614666419 +1000
> > +++ linux-build/arch/powerpc/Makefile 2012-04-18 14:37:08.680708678 +1000
> > @@ -69,6 +69,16 @@ LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-y)
> >
> > CFLAGS-$(CONFIG_PPC64) := -mminimal-toc -mtraceback=no -mcall-aixdesc
> > CFLAGS-$(CONFIG_PPC32) := -ffixed-r2 -mmultiple
> > +
> > +CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power7,-mtune=power4)
> > +CFLAGS-$(CONFIG_CELL_CPU) += $(call cc-option,-mcpu=cell)
> > +CFLAGS-$(CONFIG_POWER4_CPU) += $(call cc-option,-mcpu=power4)
> > +CFLAGS-$(CONFIG_POWER5_CPU) += $(call cc-option,-mcpu=power5)
> > +CFLAGS-$(CONFIG_POWER6_CPU) += $(call cc-option,-mcpu=power6)
> > +CFLAGS-$(CONFIG_POWER7_CPU) += $(call cc-option,-mcpu=power7)
> > +
> > +CFLAGS-$(CONFIG_TUNE_CELL) += $(call cc-option,-mtune=cell)
> > +
> > KBUILD_CPPFLAGS += -Iarch/$(ARCH)
> > KBUILD_AFLAGS += -Iarch/$(ARCH)
> > KBUILD_CFLAGS += -msoft-float -pipe -Iarch/$(ARCH) $(CFLAGS-y)
> > @@ -76,22 +86,11 @@ CPP = $(CC) -E $(KBUILD_CFLAGS)
> >
> > CHECKFLAGS += -m$(CONFIG_WORD_SIZE) -D__powerpc__ -D__powerpc$(CONFIG_WORD_SIZE)__
> >
> > -ifeq ($(CONFIG_PPC64),y)
> > -ifeq ($(CONFIG_POWER4_ONLY),y)
> > - KBUILD_CFLAGS += $(call cc-option,-mcpu=power4)
> > -else
> > - KBUILD_CFLAGS += $(call cc-option,-mtune=power4)
> > -endif
> > -endif
> > -
> > KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
> >
> > -ifeq ($(CONFIG_TUNE_CELL),y)
> > - KBUILD_CFLAGS += $(call cc-option,-mtune=cell)
> > -endif
> > -
> > -# No AltiVec instruction when building kernel
> > +# No AltiVec or VSX instructions when building kernel
> > KBUILD_CFLAGS += $(call cc-option,-mno-altivec)
> > +KBUILD_CFLAGS += $(call cc-option,-mno-vsx)
> >
> > # No SPE instruction when building kernel
> > # (We use all available options to help semi-broken compilers)
> > Index: linux-build/arch/powerpc/platforms/Kconfig.cputype
> > ===================================================================
> > --- linux-build.orig/arch/powerpc/platforms/Kconfig.cputype 2012-04-18 14:31:25.134549903 +1000
> > +++ linux-build/arch/powerpc/platforms/Kconfig.cputype 2012-04-18 14:36:40.576207829 +1000
> > @@ -78,6 +78,36 @@ config PPC_BOOK3E_64
> >
> > endchoice
> >
> > +choice
> > + prompt "CPU selection"
> > + depends on PPC64
> > + default GENERIC_CPU
> > + help
> > + This will create a kernel which is optimised for a particular CPU.
> > + The resulting kernel may not run on other CPUs, so use this with care.
> > +
> > + If unsure, select Generic.
> > +
> > +config GENERIC_CPU
> > + bool "Generic"
> > +
> > +config CELL_CPU
> > + bool "Cell Broadband Engine"
> > +
> > +config POWER4_CPU
> > + bool "POWER4"
> > +
> > +config POWER5_CPU
> > + bool "POWER5"
> > +
> > +config POWER6_CPU
> > + bool "POWER6"
> > +
> > +config POWER7_CPU
> > + bool "POWER7"
> > +
> > +endchoice
> > +
> > config PPC_BOOK3S
> > def_bool y
> > depends on PPC_BOOK3S_32 || PPC_BOOK3S_64
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-30 7:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9D4D55.9070705@redhat.com>
On Sun, Apr 29, 2012 at 11:16:53AM -0300, Mauro Carvalho Chehab wrote:
> > Hey, are you looking at compiled code or at source code? Because I'm
> > looking at source code, and it is a pretty safe bet the majority of the
> > people here do that too.
>
> What I said is that, from source code POV, a code where the loop variables are
> initialized just before the loop is easier to read it when the initialization
> of those vars are on another part of the code.
>
> That's basically why the "for" syntax starts with a var initialization clause.
>
> The tot_dimms & friends are loop vars: their value is calculated within the loop.
>
> At the object code, this won't bring any difference.
>
> >
> >> it, either by using registers for those vars or by moving the initialization
> >> to the top of the function.
> >>
> >> This function is too complex, so it is better to initialize those vars
> >> just before the loops that are calculating those totals.
> >
> > Simply initialize those variables at declaration time and that's it.
> > Initializing them before the loop doesn't make the function less complex
> > - splitting it and sanitizing it does.
>
> Initializing loop-calculated vars just before the loop makes the code easier
> to read, and may avoid issues that might happen during code lifecycle.
This is getting ridiculous: the variable declaration and initialization
are on the same screen as the loop (unless one uses a screen which can
only show less than 40ish lines).
So the argument about making the code easier to read is bogus.
This function is already cluttered with a lot of crap, and is very large
so adding more lines which can simply be stashed away at declaration
time is better readability.
Besides, every modern editor can jump to the declaration of a local
variable so that the user can see to what it is initialized to.
> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
> + unsigned n_layers,
> + struct edac_mc_layer *layers,
> + bool rev_order,
> + unsigned sz_pvt)
> {
> void *ptr = NULL;
> struct mem_ctl_info *mci;
> - struct csrow_info *csi, *csrow;
> + struct edac_mc_layer *layer;
> + struct csrow_info *csi, *csr;
> struct rank_info *chi, *chp, *chan;
> struct dimm_info *dimm;
> + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
> void *pvt;
> - unsigned size;
> - int row, chn;
> + unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
> + unsigned tot_csrows, tot_channels, tot_errcount = 0;
> + int i, j;
> int err;
> + int row, chn;
> + bool per_rank = false;
> +
> + BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
> + /*
> + * Calculate the total amount of dimms and csrows/cschannels while
> + * in the old API emulation mode
> + */
> + tot_dimms = 1;
> + tot_channels = 1;
> + tot_csrows = 1;
> + for (i = 0; i < n_layers; i++) {
> + tot_dimms *= layers[i].size;
> + if (layers[i].is_virt_csrow)
> + tot_csrows *= layers[i].size;
> + else
> + tot_channels *= layers[i].size;
> +
> + if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
> + per_rank = true;
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-30 8:15 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9D46F8.1020104@redhat.com>
On Sun, Apr 29, 2012 at 10:49:44AM -0300, Mauro Carvalho Chehab wrote:
> > [ 10.486440] EDAC MC: DCT0 chip selects:
> > [ 10.486443] EDAC amd64: MC: 0: 2048MB 1: 2048MB
> > [ 10.486445] EDAC amd64: MC: 2: 2048MB 3: 2048MB
> > [ 10.486448] EDAC amd64: MC: 4: 0MB 5: 0MB
> > [ 10.486450] EDAC amd64: MC: 6: 0MB 7: 0MB
> > [ 10.486453] EDAC DEBUG: amd64_debug_display_dimm_sizes: F2x180 (DRAM Bank Address Mapping): 0x00000088
> > [ 10.486455] EDAC MC: DCT1 chip selects:
> > [ 10.486458] EDAC amd64: MC: 0: 2048MB 1: 2048MB
> > [ 10.486460] EDAC amd64: MC: 2: 2048MB 3: 2048MB
> > [ 10.486463] EDAC amd64: MC: 4: 0MB 5: 0MB
> > [ 10.486465] EDAC amd64: MC: 6: 0MB 7: 0MB
> > [ 10.486467] EDAC amd64: using x8 syndromes.
> > [ 10.486469] EDAC DEBUG: amd64_dump_dramcfg_low: F2x190 (DRAM Cfg Low): 0x00083100
> > [ 10.486472] EDAC DEBUG: amd64_dump_dramcfg_low: DIMM type: buffered; all DIMMs support ECC: yes
> > [ 10.486475] EDAC DEBUG: amd64_dump_dramcfg_low: PAR/ERR parity: enabled
> > [ 10.486478] EDAC DEBUG: amd64_dump_dramcfg_low: DCT 128bit mode width: 64b
> > [ 10.486481] EDAC DEBUG: amd64_dump_dramcfg_low: x4 logical DIMMs present: L0: yes L1: yes L2: no L3: no
> > [ 10.486485] EDAC DEBUG: f1x_early_channel_count: Data width is not 128 bits - need more decoding
> > [ 10.486488] EDAC amd64: MCT channel count: 2
> > [ 10.486493] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc(): allocating 3692 bytes for mci data (16 ranks, 16 csrows/channels)
> > [ 10.486501] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: rank0 (0:0:0): row 0, chan 0
> > [ 10.486506] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 1: rank1 (0:1:0): row 0, chan 1
> > [ 10.486510] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 2: rank2 (1:0:0): row 1, chan 0
> > [ 10.486514] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 3: rank3 (1:1:0): row 1, chan 1
> > [ 10.486518] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 4: rank4 (2:0:0): row 2, chan 0
> > [ 10.486522] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 5: rank5 (2:1:0): row 2, chan 1
> > [ 10.486526] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 6: rank6 (3:0:0): row 3, chan 0
> > [ 10.486530] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 7: rank7 (3:1:0): row 3, chan 1
> > [ 10.486534] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 8: rank8 (4:0:0): row 4, chan 0
> > [ 10.486538] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 9: rank9 (4:1:0): row 4, chan 1
> > [ 10.486542] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 10: rank10 (5:0:0): row 5, chan 0
> > [ 10.486546] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 11: rank11 (5:1:0): row 5, chan 1
> > [ 10.486550] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 12: rank12 (6:0:0): row 6, chan 0
> > [ 10.486554] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 13: rank13 (6:1:0): row 6, chan 1
> > [ 10.486558] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 14: rank14 (7:0:0): row 7, chan 0
> > [ 10.486562] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 15: rank15 (7:1:0): row 7, chan 1
> >
> > DCT0 has 4 ranks + DCT1 also 4 ranks = 8 ranks total.
> >
> > Now your change is showing 16 ranks. Still b0rked.
> >
> No, DCT0+DCT1 have 16 ranks, 8 filled and 8 empty. So, it is OK.
>
> As I said before when you've pointed this bug (likel at v3 review), edac_mc_alloc
> doesn't know how many ranks are filled, as the driver logic first calls it to
> allocate for the max amount of ranks, and then fills the rank with their info
> (or let them untouched with 0 pages, if they're empty).
Basically you're saying you're generating dimm_info structs for all
_possible_ dimms and the loop where this debug message comes from goes
and marrily initializes them all although some of them are empty:
+ for (i = 0; i < tot_dimms; i++) {
+ chan = &csi[row].channels[chn];
+ dimm = EDAC_DIMM_PTR(lay, mci->dimms, n_layers,
+ pos[0], pos[1], pos[2]);
+ dimm->mci = mci;
+
+ debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
+ i, (dimm - mci->dimms),
+ pos[0], pos[1], pos[2], row, chn);
+
+ /* Copy DIMM location */
+ for (j = 0; j < n_layers; j++)
+ dimm->location[j] = pos[j];
...
definitely superfluous.
Oh well, looking at edac_mc_alloc, it used to allocate structs for all
csrows on the controller even though some of them were empty...
Ok, then please remove this debug call because it is misleading. Having
[ 10.486493] EDAC DEBUG: new_edac_mc_alloc: allocating 3692 bytes for mci data (16 ranks, 16 csrows/channels)
is enough.
You probably want to say how many channels/csrows there are, though:
[ 10.486493] EDAC DEBUG: new_edac_mc_alloc: allocating 3692 bytes for mci data (16 ranks, 8 csrows, 2 channels)
or something similar. Simply dump tot_dimms, tot_channels and tot_csrows
and that's it.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-30 10:58 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <20120430081513.GD8182@aftab.osrc.amd.com>
Em 30-04-2012 05:15, Borislav Petkov escreveu:
> On Sun, Apr 29, 2012 at 10:49:44AM -0300, Mauro Carvalho Chehab wrote:
>>> [ 10.486440] EDAC MC: DCT0 chip selects:
>>> [ 10.486443] EDAC amd64: MC: 0: 2048MB 1: 2048MB
>>> [ 10.486445] EDAC amd64: MC: 2: 2048MB 3: 2048MB
>>> [ 10.486448] EDAC amd64: MC: 4: 0MB 5: 0MB
>>> [ 10.486450] EDAC amd64: MC: 6: 0MB 7: 0MB
>>> [ 10.486453] EDAC DEBUG: amd64_debug_display_dimm_sizes: F2x180 (DRAM Bank Address Mapping): 0x00000088
>>> [ 10.486455] EDAC MC: DCT1 chip selects:
>>> [ 10.486458] EDAC amd64: MC: 0: 2048MB 1: 2048MB
>>> [ 10.486460] EDAC amd64: MC: 2: 2048MB 3: 2048MB
>>> [ 10.486463] EDAC amd64: MC: 4: 0MB 5: 0MB
>>> [ 10.486465] EDAC amd64: MC: 6: 0MB 7: 0MB
>>> [ 10.486467] EDAC amd64: using x8 syndromes.
>>> [ 10.486469] EDAC DEBUG: amd64_dump_dramcfg_low: F2x190 (DRAM Cfg Low): 0x00083100
>>> [ 10.486472] EDAC DEBUG: amd64_dump_dramcfg_low: DIMM type: buffered; all DIMMs support ECC: yes
>>> [ 10.486475] EDAC DEBUG: amd64_dump_dramcfg_low: PAR/ERR parity: enabled
>>> [ 10.486478] EDAC DEBUG: amd64_dump_dramcfg_low: DCT 128bit mode width: 64b
>>> [ 10.486481] EDAC DEBUG: amd64_dump_dramcfg_low: x4 logical DIMMs present: L0: yes L1: yes L2: no L3: no
>>> [ 10.486485] EDAC DEBUG: f1x_early_channel_count: Data width is not 128 bits - need more decoding
>>> [ 10.486488] EDAC amd64: MCT channel count: 2
>>> [ 10.486493] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc(): allocating 3692 bytes for mci data (16 ranks, 16 csrows/channels)
>>> [ 10.486501] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: rank0 (0:0:0): row 0, chan 0
>>> [ 10.486506] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 1: rank1 (0:1:0): row 0, chan 1
>>> [ 10.486510] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 2: rank2 (1:0:0): row 1, chan 0
>>> [ 10.486514] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 3: rank3 (1:1:0): row 1, chan 1
>>> [ 10.486518] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 4: rank4 (2:0:0): row 2, chan 0
>>> [ 10.486522] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 5: rank5 (2:1:0): row 2, chan 1
>>> [ 10.486526] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 6: rank6 (3:0:0): row 3, chan 0
>>> [ 10.486530] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 7: rank7 (3:1:0): row 3, chan 1
>>> [ 10.486534] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 8: rank8 (4:0:0): row 4, chan 0
>>> [ 10.486538] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 9: rank9 (4:1:0): row 4, chan 1
>>> [ 10.486542] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 10: rank10 (5:0:0): row 5, chan 0
>>> [ 10.486546] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 11: rank11 (5:1:0): row 5, chan 1
>>> [ 10.486550] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 12: rank12 (6:0:0): row 6, chan 0
>>> [ 10.486554] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 13: rank13 (6:1:0): row 6, chan 1
>>> [ 10.486558] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 14: rank14 (7:0:0): row 7, chan 0
>>> [ 10.486562] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 15: rank15 (7:1:0): row 7, chan 1
>>>
>>> DCT0 has 4 ranks + DCT1 also 4 ranks = 8 ranks total.
>>>
>>> Now your change is showing 16 ranks. Still b0rked.
>>>
>> No, DCT0+DCT1 have 16 ranks, 8 filled and 8 empty. So, it is OK.
>>
>> As I said before when you've pointed this bug (likel at v3 review), edac_mc_alloc
>> doesn't know how many ranks are filled, as the driver logic first calls it to
>> allocate for the max amount of ranks, and then fills the rank with their info
>> (or let them untouched with 0 pages, if they're empty).
>
> Basically you're saying you're generating dimm_info structs for all
> _possible_ dimms and the loop where this debug message comes from goes
> and marrily initializes them all although some of them are empty:
>
> + for (i = 0; i < tot_dimms; i++) {
> + chan = &csi[row].channels[chn];
> + dimm = EDAC_DIMM_PTR(lay, mci->dimms, n_layers,
> + pos[0], pos[1], pos[2]);
> + dimm->mci = mci;
> +
> + debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
> + i, (dimm - mci->dimms),
> + pos[0], pos[1], pos[2], row, chn);
> +
> + /* Copy DIMM location */
> + for (j = 0; j < n_layers; j++)
> + dimm->location[j] = pos[j];
> ...
>
> definitely superfluous.
>
> Oh well, looking at edac_mc_alloc, it used to allocate structs for all
> csrows on the controller even though some of them were empty...
>
> Ok, then please remove this debug call because it is misleading. Having
>
> [ 10.486493] EDAC DEBUG: new_edac_mc_alloc: allocating 3692 bytes for mci data (16 ranks, 16 csrows/channels)
>
> is enough.
>
> You probably want to say how many channels/csrows there are, though:
>
> [ 10.486493] EDAC DEBUG: new_edac_mc_alloc: allocating 3692 bytes for mci data (16 ranks, 8 csrows, 2 channels)
>
> or something similar. Simply dump tot_dimms, tot_channels and tot_csrows
> and that's it.
>
It seems you have a very short memory. We had a similar discussion about that a while ago:
https://lkml.org/lkml/2012/3/8/440
See my comments at:
https://lkml.org/lkml/2012/3/9/101
https://lkml.org/lkml/2012/3/9/267
As it was explained there, those debug messages provide a map between the legacy per-csrow
data, used by the old API and the dimm_info representation. For a per-csrow memory controller,
the map is trivial, as the memory location will match the csrow/channel information, but
for modern memory controllers, the map info is not trivial and it helps to check what it is
expected to be found when retrieving information via the legacy EDAC API.
For example, this is the mapping used by the second memory controller of the SB machine
I'm using on my tests:
[52803.640043] EDAC DEBUG: sbridge_probe: Registering MC#1 (2 of 2)
...
[52803.640062] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc(): allocating 7196 bytes for mci data (12 dimms, 12 csrows/channels)
[52803.640070] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: initializing 12 dimms
[52803.640072] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
[52803.640074] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 1: dimm1 (0:1:0): row 0, chan 1
[52803.640077] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 2: dimm2 (0:2:0): row 0, chan 2
[52803.640080] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 3: dimm3 (1:0:0): row 0, chan 3
[52803.640083] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 4: dimm4 (1:1:0): row 1, chan 0
[52803.640086] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 5: dimm5 (1:2:0): row 1, chan 1
[52803.640089] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 6: dimm6 (2:0:0): row 1, chan 2
[52803.640092] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 7: dimm7 (2:1:0): row 1, chan 3
[52803.640095] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 8: dimm8 (2:2:0): row 2, chan 0
[52803.640098] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 9: dimm9 (3:0:0): row 2, chan 1
[52803.640101] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 10: dimm10 (3:1:0): row 2, chan 2
[52803.640104] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 11: dimm11 (3:2:0): row 2, chan 3
With the above info, it is clear that the DIMM located at mc#1, channel#3 slot#2 is
called "dimm11" at the new API, and corresponds to "csrow 2, channel 3" for a legacy
EDAC API call.
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-30 11:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9E7059.5070804@redhat.com>
On Mon, Apr 30, 2012 at 07:58:33AM -0300, Mauro Carvalho Chehab wrote:
> It seems you have a very short memory.
Oh, puh-lease, let's don't start with the insults now. You're not a
saint yourself. And maybe the fact that I'm having hard time grasping
your code is maybe because it is a load of crap and you seem to generate
a lot of senseless drivel when explaining what it does. And don't get me
started on the patch bombs.
So, let's stay constructive here before I, as the last and only one
person reviewing this stinking pile stops messing with it (I got other
stuff to do, you know) and NACK it completely.
> For example, this is the mapping used by the second memory controller of the SB machine
> I'm using on my tests:
>
> [52803.640043] EDAC DEBUG: sbridge_probe: Registering MC#1 (2 of 2)
> ...
> [52803.640062] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc(): allocating 7196 bytes for mci data (12 dimms, 12 csrows/channels)
> [52803.640070] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: initializing 12 dimms
> [52803.640072] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
> [52803.640074] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 1: dimm1 (0:1:0): row 0, chan 1
> [52803.640077] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 2: dimm2 (0:2:0): row 0, chan 2
> [52803.640080] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 3: dimm3 (1:0:0): row 0, chan 3
> [52803.640083] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 4: dimm4 (1:1:0): row 1, chan 0
> [52803.640086] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 5: dimm5 (1:2:0): row 1, chan 1
> [52803.640089] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 6: dimm6 (2:0:0): row 1, chan 2
> [52803.640092] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 7: dimm7 (2:1:0): row 1, chan 3
> [52803.640095] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 8: dimm8 (2:2:0): row 2, chan 0
> [52803.640098] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 9: dimm9 (3:0:0): row 2, chan 1
> [52803.640101] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 10: dimm10 (3:1:0): row 2, chan 2
> [52803.640104] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 11: dimm11 (3:2:0): row 2, chan 3
>
> With the above info, it is clear that the DIMM located at mc#1, channel#3 slot#2 is
> called "dimm11" at the new API, and corresponds to "csrow 2, channel 3" for a legacy
> EDAC API call.
Are all those DIMM slots above populated? What happens if they're not,
are you issuing the same dimm0-dimm11 lines for slots which aren't even
populated?
I have a much better idea: Generally, this debug info should come from
the specific driver that allocates the dimm descriptors, not from the
EDAC core. This way, you know in the driver which slots are populated
and those which are not should be omitted.
This way it says "initializing 12 dimms" and the user thinks there are
12 DIMMs on his system where this might not be true.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-30 11:23 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <20120430075940.GC8182@aftab.osrc.amd.com>
Em 30-04-2012 04:59, Borislav Petkov escreveu:
> On Sun, Apr 29, 2012 at 11:16:53AM -0300, Mauro Carvalho Chehab wrote:
>>> Hey, are you looking at compiled code or at source code? Because I'm
>>> looking at source code, and it is a pretty safe bet the majority of the
>>> people here do that too.
>>
>> What I said is that, from source code POV, a code where the loop variables are
>> initialized just before the loop is easier to read it when the initialization
>> of those vars are on another part of the code.
>>
>> That's basically why the "for" syntax starts with a var initialization clause.
>>
>> The tot_dimms & friends are loop vars: their value is calculated within the loop.
>>
>> At the object code, this won't bring any difference.
>>
>>>
>>>> it, either by using registers for those vars or by moving the initialization
>>>> to the top of the function.
>>>>
>>>> This function is too complex, so it is better to initialize those vars
>>>> just before the loops that are calculating those totals.
>>>
>>> Simply initialize those variables at declaration time and that's it.
>>> Initializing them before the loop doesn't make the function less complex
>>> - splitting it and sanitizing it does.
>>
>> Initializing loop-calculated vars just before the loop makes the code easier
>> to read, and may avoid issues that might happen during code lifecycle.
>
> This is getting ridiculous:
With this I fully agree: you're nacking patches because it is not the way you
write your code, not because the code there is doing anything wrong.
If you point anything wrong on the way I wrote, then I'll fix. Otherwise, why
should I do a change that will obfuscate the code?
> the variable declaration and initialization
> are on the same screen as the loop (unless one uses a screen which can
> only show less than 40ish lines).
>
> So the argument about making the code easier to read is bogus.
>
> This function is already cluttered with a lot of crap, and is very large
> so adding more lines which can simply be stashed away at declaration
> time is better readability.
>
> Besides, every modern editor can jump to the declaration of a local
> variable so that the user can see to what it is initialized to.
The editor used by te developer is not relevant. This is not a reason
to obfuscate the code.
>> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
>> + unsigned n_layers,
>> + struct edac_mc_layer *layers,
>> + bool rev_order,
>> + unsigned sz_pvt)
>> {
>> void *ptr = NULL;
>> struct mem_ctl_info *mci;
>> - struct csrow_info *csi, *csrow;
>> + struct edac_mc_layer *layer;
>> + struct csrow_info *csi, *csr;
>> struct rank_info *chi, *chp, *chan;
>> struct dimm_info *dimm;
>> + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
>> void *pvt;
>> - unsigned size;
>> - int row, chn;
>> + unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
>> + unsigned tot_csrows, tot_channels, tot_errcount = 0;
>> + int i, j;
>> int err;
>> + int row, chn;
>> + bool per_rank = false;
>> +
>> + BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
>> + /*
>> + * Calculate the total amount of dimms and csrows/cschannels while
>> + * in the old API emulation mode
>> + */
>> + tot_dimms = 1;
>> + tot_channels = 1;
>> + tot_csrows = 1;
>> + for (i = 0; i < n_layers; i++) {
>> + tot_dimms *= layers[i].size;
>> + if (layers[i].is_virt_csrow)
>> + tot_csrows *= layers[i].size;
>> + else
>> + tot_channels *= layers[i].size;
>> +
>> + if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
>> + per_rank = true;
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-30 11:37 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <20120430081513.GD8182@aftab.osrc.amd.com>
Em 30-04-2012 05:15, Borislav Petkov escreveu:
> On Sun, Apr 29, 2012 at 10:49:44AM -0300, Mauro Carvalho Chehab wrote:
>>> [ 10.486440] EDAC MC: DCT0 chip selects:
>>> [ 10.486443] EDAC amd64: MC: 0: 2048MB 1: 2048MB
>>> [ 10.486445] EDAC amd64: MC: 2: 2048MB 3: 2048MB
>>> [ 10.486448] EDAC amd64: MC: 4: 0MB 5: 0MB
>>> [ 10.486450] EDAC amd64: MC: 6: 0MB 7: 0MB
>>> [ 10.486453] EDAC DEBUG: amd64_debug_display_dimm_sizes: F2x180 (DRAM Bank Address Mapping): 0x00000088
>>> [ 10.486455] EDAC MC: DCT1 chip selects:
>>> [ 10.486458] EDAC amd64: MC: 0: 2048MB 1: 2048MB
>>> [ 10.486460] EDAC amd64: MC: 2: 2048MB 3: 2048MB
>>> [ 10.486463] EDAC amd64: MC: 4: 0MB 5: 0MB
>>> [ 10.486465] EDAC amd64: MC: 6: 0MB 7: 0MB
>>> [ 10.486467] EDAC amd64: using x8 syndromes.
>>> [ 10.486469] EDAC DEBUG: amd64_dump_dramcfg_low: F2x190 (DRAM Cfg Low): 0x00083100
>>> [ 10.486472] EDAC DEBUG: amd64_dump_dramcfg_low: DIMM type: buffered; all DIMMs support ECC: yes
>>> [ 10.486475] EDAC DEBUG: amd64_dump_dramcfg_low: PAR/ERR parity: enabled
>>> [ 10.486478] EDAC DEBUG: amd64_dump_dramcfg_low: DCT 128bit mode width: 64b
>>> [ 10.486481] EDAC DEBUG: amd64_dump_dramcfg_low: x4 logical DIMMs present: L0: yes L1: yes L2: no L3: no
>>> [ 10.486485] EDAC DEBUG: f1x_early_channel_count: Data width is not 128 bits - need more decoding
>>> [ 10.486488] EDAC amd64: MCT channel count: 2
>>> [ 10.486493] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc(): allocating 3692 bytes for mci data (16 ranks, 16 csrows/channels)
>>> [ 10.486501] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: rank0 (0:0:0): row 0, chan 0
>>> [ 10.486506] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 1: rank1 (0:1:0): row 0, chan 1
>>> [ 10.486510] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 2: rank2 (1:0:0): row 1, chan 0
>>> [ 10.486514] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 3: rank3 (1:1:0): row 1, chan 1
>>> [ 10.486518] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 4: rank4 (2:0:0): row 2, chan 0
>>> [ 10.486522] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 5: rank5 (2:1:0): row 2, chan 1
>>> [ 10.486526] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 6: rank6 (3:0:0): row 3, chan 0
>>> [ 10.486530] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 7: rank7 (3:1:0): row 3, chan 1
>>> [ 10.486534] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 8: rank8 (4:0:0): row 4, chan 0
>>> [ 10.486538] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 9: rank9 (4:1:0): row 4, chan 1
>>> [ 10.486542] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 10: rank10 (5:0:0): row 5, chan 0
>>> [ 10.486546] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 11: rank11 (5:1:0): row 5, chan 1
>>> [ 10.486550] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 12: rank12 (6:0:0): row 6, chan 0
>>> [ 10.486554] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 13: rank13 (6:1:0): row 6, chan 1
>>> [ 10.486558] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 14: rank14 (7:0:0): row 7, chan 0
>>> [ 10.486562] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 15: rank15 (7:1:0): row 7, chan 1
>>>
>>> DCT0 has 4 ranks + DCT1 also 4 ranks = 8 ranks total.
>>>
>>> Now your change is showing 16 ranks. Still b0rked.
>>>
>> No, DCT0+DCT1 have 16 ranks, 8 filled and 8 empty. So, it is OK.
>>
>> As I said before when you've pointed this bug (likel at v3 review), edac_mc_alloc
>> doesn't know how many ranks are filled, as the driver logic first calls it to
>> allocate for the max amount of ranks, and then fills the rank with their info
>> (or let them untouched with 0 pages, if they're empty).
>
> Basically you're saying you're generating dimm_info structs for all
> _possible_ dimms and the loop where this debug message comes from goes
> and marrily initializes them all although some of them are empty:
>
> + for (i = 0; i < tot_dimms; i++) {
> + chan = &csi[row].channels[chn];
> + dimm = EDAC_DIMM_PTR(lay, mci->dimms, n_layers,
> + pos[0], pos[1], pos[2]);
> + dimm->mci = mci;
> +
> + debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
> + i, (dimm - mci->dimms),
> + pos[0], pos[1], pos[2], row, chn);
> +
> + /* Copy DIMM location */
> + for (j = 0; j < n_layers; j++)
> + dimm->location[j] = pos[j];
> ...
>
> definitely superfluous.
This is the way the EDAC core works: everything is allocated, on one shot, when this
function is called, and, on most drivers, before the code that probes how many DIMMS/ranks
got initialized. That happens because the edac_mc_alloc() arguments provide the total
amount of ranks/dimms, but doesn't say anything about what is used there.
Changing from this model to another model that would dynamically initialize the per-dimm/rank
data is possible, but that would require another set of patches that will touch on all
drivers, and to convert the edac_mc_alloc function into 3 or 4 function calls, with the
corresponding changes on all drivers. Also, the changes at the drivers won't likely be
trivial.
The patches that convert kobj into "struct device" does part of the job, as, after it,
each dimm/csrow will be allocated by a separate kmalloc.
After having this series fully applied, it would be possible to work on such solution.
I'll eventually do that, as this would simplify the code at i7core_edac and sb_edac.
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-30 11:45 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <20120430111126.GD9303@aftab.osrc.amd.com>
Em 30-04-2012 08:11, Borislav Petkov escreveu:
> On Mon, Apr 30, 2012 at 07:58:33AM -0300, Mauro Carvalho Chehab wrote:
>> For example, this is the mapping used by the second memory controller of the SB machine
>> I'm using on my tests:
>>
>> [52803.640043] EDAC DEBUG: sbridge_probe: Registering MC#1 (2 of 2)
>> ...
>> [52803.640062] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc(): allocating 7196 bytes for mci data (12 dimms, 12 csrows/channels)
>> [52803.640070] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: initializing 12 dimms
>> [52803.640072] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
>> [52803.640074] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 1: dimm1 (0:1:0): row 0, chan 1
>> [52803.640077] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 2: dimm2 (0:2:0): row 0, chan 2
>> [52803.640080] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 3: dimm3 (1:0:0): row 0, chan 3
>> [52803.640083] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 4: dimm4 (1:1:0): row 1, chan 0
>> [52803.640086] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 5: dimm5 (1:2:0): row 1, chan 1
>> [52803.640089] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 6: dimm6 (2:0:0): row 1, chan 2
>> [52803.640092] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 7: dimm7 (2:1:0): row 1, chan 3
>> [52803.640095] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 8: dimm8 (2:2:0): row 2, chan 0
>> [52803.640098] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 9: dimm9 (3:0:0): row 2, chan 1
>> [52803.640101] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 10: dimm10 (3:1:0): row 2, chan 2
>> [52803.640104] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 11: dimm11 (3:2:0): row 2, chan 3
>>
>> With the above info, it is clear that the DIMM located at mc#1, channel#3 slot#2 is
>> called "dimm11" at the new API, and corresponds to "csrow 2, channel 3" for a legacy
>> EDAC API call.
>
> Are all those DIMM slots above populated? What happens if they're not,
> are you issuing the same dimm0-dimm11 lines for slots which aren't even
> populated?
>
> I have a much better idea: Generally, this debug info should come from
> the specific driver that allocates the dimm descriptors, not from the
> EDAC core. This way, you know in the driver which slots are populated
> and those which are not should be omitted.
The drivers don't allocate the dimm descriptors. They're allocated by the
core.
> This way it says "initializing 12 dimms" and the user thinks there are
> 12 DIMMs on his system where this might not be true.
I'm OK to remove the "initializing 12 dimms" message. It doesn't add anything
new.
With regards do the other messages, if the debug messages are not clear,
then let's fix them, instead of removing. What if we print, instead,
on a message like:
"row 1, chan 1 will represent dimm5 (1:2:0) if not empty"
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-30 12:38 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9E7B45.8010908@redhat.com>
On Mon, Apr 30, 2012 at 08:45:09AM -0300, Mauro Carvalho Chehab wrote:
> Em 30-04-2012 08:11, Borislav Petkov escreveu:
> > On Mon, Apr 30, 2012 at 07:58:33AM -0300, Mauro Carvalho Chehab wrote:
>
> >> For example, this is the mapping used by the second memory controller of the SB machine
> >> I'm using on my tests:
> >>
> >> [52803.640043] EDAC DEBUG: sbridge_probe: Registering MC#1 (2 of 2)
> >> ...
> >> [52803.640062] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc(): allocating 7196 bytes for mci data (12 dimms, 12 csrows/channels)
> >> [52803.640070] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: initializing 12 dimms
> >> [52803.640072] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
> >> [52803.640074] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 1: dimm1 (0:1:0): row 0, chan 1
> >> [52803.640077] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 2: dimm2 (0:2:0): row 0, chan 2
> >> [52803.640080] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 3: dimm3 (1:0:0): row 0, chan 3
> >> [52803.640083] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 4: dimm4 (1:1:0): row 1, chan 0
> >> [52803.640086] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 5: dimm5 (1:2:0): row 1, chan 1
> >> [52803.640089] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 6: dimm6 (2:0:0): row 1, chan 2
> >> [52803.640092] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 7: dimm7 (2:1:0): row 1, chan 3
> >> [52803.640095] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 8: dimm8 (2:2:0): row 2, chan 0
> >> [52803.640098] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 9: dimm9 (3:0:0): row 2, chan 1
> >> [52803.640101] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 10: dimm10 (3:1:0): row 2, chan 2
> >> [52803.640104] EDAC DEBUG: edac_mc_alloc: edac_mc_alloc: 11: dimm11 (3:2:0): row 2, chan 3
> >>
> >> With the above info, it is clear that the DIMM located at mc#1, channel#3 slot#2 is
> >> called "dimm11" at the new API, and corresponds to "csrow 2, channel 3" for a legacy
> >> EDAC API call.
> >
> > Are all those DIMM slots above populated? What happens if they're not,
> > are you issuing the same dimm0-dimm11 lines for slots which aren't even
> > populated?
> >
> > I have a much better idea: Generally, this debug info should come from
> > the specific driver that allocates the dimm descriptors, not from the
> > EDAC core. This way, you know in the driver which slots are populated
> > and those which are not should be omitted.
>
> The drivers don't allocate the dimm descriptors. They're allocated by the
> core.
I know that. The drivers call into EDAC core using edac_mc_alloc, this
is what I meant above.
> > This way it says "initializing 12 dimms" and the user thinks there are
> > 12 DIMMs on his system where this might not be true.
>
>
> I'm OK to remove the "initializing 12 dimms" message. It doesn't add anything
> new.
>
> With regards do the other messages, if the debug messages are not clear,
> then let's fix them, instead of removing. What if we print, instead,
> on a message like:
>
> "row 1, chan 1 will represent dimm5 (1:2:0) if not empty"
How about the following instead: the specific driver calls
edac_mc_alloc(), it gets the allocated dimm array in mci->dimms
_without_ dumping each dimm%d line. Then, each driver figures out which
subset of that dimms array actually has populated slots and prints only
the populated rank/slot/...
This information is much more valuable than saying how many _possible_
slots the edac core has allocated.
Then, each driver can decide whether it makes sense to dump that info or
not.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-30 12:51 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9E763E.2050808@redhat.com>
On Mon, Apr 30, 2012 at 08:23:42AM -0300, Mauro Carvalho Chehab wrote:
> With this I fully agree: you're nacking patches because it is not the way you
Where? Have I written Nacked-by somewhere?
> write your code, not because the code there is doing anything wrong.
>
> If you point anything wrong on the way I wrote, then I'll fix. Otherwise, why
> should I do a change that will obfuscate the code?
What obfuscation are you talking about? Having the initialization of
variables along with their declaration is not it.
Now let's look what you're doing:
> >> + unsigned tot_csrows, tot_channels, tot_errcount = 0;
> >> + unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
just to reassign 1 to some of them
> >> + tot_dimms = 1;
> >> + tot_channels = 1;
> >> + tot_csrows = 1;
a couple of lines below.
Now this is misleading.
Now let's look at what I'm proposing
unsigned tot_dimms = 1;
unsigned tot_csrows = 1;
unsigned tot_channels = 1;
How is this an obfuscation? It is basic code layout practices.
> The editor used by te developer is not relevant. This is not a reason
> to obfuscate the code.
>
> >> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
> >> + unsigned n_layers,
> >> + struct edac_mc_layer *layers,
> >> + bool rev_order,
> >> + unsigned sz_pvt)
> >> {
> >> void *ptr = NULL;
> >> struct mem_ctl_info *mci;
> >> - struct csrow_info *csi, *csrow;
> >> + struct edac_mc_layer *layer;
> >> + struct csrow_info *csi, *csr;
> >> struct rank_info *chi, *chp, *chan;
> >> struct dimm_info *dimm;
> >> + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
> >> void *pvt;
> >> - unsigned size;
> >> - int row, chn;
> >> + unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
> >> + unsigned tot_csrows, tot_channels, tot_errcount = 0;
> >> + int i, j;
> >> int err;
> >> + int row, chn;
> >> + bool per_rank = false;
> >> +
> >> + BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
> >> + /*
> >> + * Calculate the total amount of dimms and csrows/cschannels while
> >> + * in the old API emulation mode
> >> + */
> >> + tot_dimms = 1;
> >> + tot_channels = 1;
> >> + tot_csrows = 1;
> >> + for (i = 0; i < n_layers; i++) {
> >> + tot_dimms *= layers[i].size;
> >> + if (layers[i].is_virt_csrow)
> >> + tot_csrows *= layers[i].size;
> >> + else
> >> + tot_channels *= layers[i].size;
> >> +
> >> + if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
> >> + per_rank = true;
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-30 13:00 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <20120430123819.GF9303@aftab.osrc.amd.com>
Em 30-04-2012 09:38, Borislav Petkov escreveu:
> On Mon, Apr 30, 2012 at 08:45:09AM -0300, Mauro Carvalho Chehab wrote:
>> Em 30-04-2012 08:11, Borislav Petkov escreveu:
>>> On Mon, Apr 30, 2012 at 07:58:33AM -0300, Mauro Carvalho Chehab wrote:
>>> This way it says "initializing 12 dimms" and the user thinks there are
>>> 12 DIMMs on his system where this might not be true.
>>
>>
>> I'm OK to remove the "initializing 12 dimms" message. It doesn't add anything
>> new.
>>
>> With regards do the other messages, if the debug messages are not clear,
>> then let's fix them, instead of removing. What if we print, instead,
>> on a message like:
>>
>> "row 1, chan 1 will represent dimm5 (1:2:0) if not empty"
>
> How about the following instead: the specific driver calls
> edac_mc_alloc(), it gets the allocated dimm array in mci->dimms
> _without_ dumping each dimm%d line. Then, each driver figures out which
> subset of that dimms array actually has populated slots and prints only
> the populated rank/slot/...
>
> This information is much more valuable than saying how many _possible_
> slots the edac core has allocated.
>
> Then, each driver can decide whether it makes sense to dump that info or
> not.
No, that would add extra complexity at the drivers level just due to debug
messages. I think that the better is to move this printk to the debug-specific
routine that is called only when the dimm is filled (edac_mc_dump_dimm).
With this cange, the message will be printed only for the filled dimms.
This is a cleanup patch, so I'll write it, together with the change that
will get rid of the loop that uses KERN_CONT. It will use a function added
by a latter patch at edac_mc_sysfs so it can't be merged on this patch
anyway.
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-30 13:53 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9E8CFE.1040801@redhat.com>
Em 30-04-2012 10:00, Mauro Carvalho Chehab escreveu:
> Em 30-04-2012 09:38, Borislav Petkov escreveu:
>> On Mon, Apr 30, 2012 at 08:45:09AM -0300, Mauro Carvalho Chehab wrote:
>>> Em 30-04-2012 08:11, Borislav Petkov escreveu:
>>>> On Mon, Apr 30, 2012 at 07:58:33AM -0300, Mauro Carvalho Chehab wrote:
>
>>>> This way it says "initializing 12 dimms" and the user thinks there are
>>>> 12 DIMMs on his system where this might not be true.
>>>
>>>
>>> I'm OK to remove the "initializing 12 dimms" message. It doesn't add anything
>>> new.
>>>
>>> With regards do the other messages, if the debug messages are not clear,
>>> then let's fix them, instead of removing. What if we print, instead,
>>> on a message like:
>>>
>>> "row 1, chan 1 will represent dimm5 (1:2:0) if not empty"
>>
>> How about the following instead: the specific driver calls
>> edac_mc_alloc(), it gets the allocated dimm array in mci->dimms
>> _without_ dumping each dimm%d line. Then, each driver figures out which
>> subset of that dimms array actually has populated slots and prints only
>> the populated rank/slot/...
>>
>> This information is much more valuable than saying how many _possible_
>> slots the edac core has allocated.
>>
>> Then, each driver can decide whether it makes sense to dump that info or
>> not.
>
> No, that would add extra complexity at the drivers level just due to debug
> messages. I think that the better is to move this printk to the debug-specific
> routine that is called only when the dimm is filled (edac_mc_dump_dimm).
>
> With this cange, the message will be printed only for the filled dimms.
>
> This is a cleanup patch, so I'll write it, together with the change that
> will get rid of the loop that uses KERN_CONT. It will use a function added
> by a latter patch at edac_mc_sysfs so it can't be merged on this patch
> anyway.
The following patch dos the debug cleanup. I'll add at the end of my tree.
Regards,
Mauro.
From: Mauro Carvalho Chehab <mchehab@redhat.com>
Date: Mon, 30 Apr 2012 10:24:43 -0300
Subject: [PATCH] edac_mc: Cleanup per-dimm_info debug messages
The edac_mc_alloc() routine allocates one dimm_info device for all
possible memories, including the non-filled ones. The debug messages
there are somewhat confusing. So, cleans them, by moving the code
that prints the memory location to edac_mc, and using it on both
edac_mc_sysfs and edac_mc.
After this patch, a dimm-based memory controller will print the debug
info as:
[ 728.430828] EDAC DEBUG: edac_mc_dump_dimm: dimm2: channel 0 slot 2 mapped as virtual row 0, chan 2
[ 728.430834] EDAC DEBUG: edac_mc_dump_dimm: dimm->label = 'mc#0channel#0slot#2'
[ 728.430839] EDAC DEBUG: edac_mc_dump_dimm: dimm->nr_pages = 0x0
[ 728.430846] EDAC DEBUG: edac_mc_dump_dimm: dimm->grain = 0
[ 728.430850] EDAC DEBUG: edac_mc_dump_dimm: dimm->nr_pages = 0x0
(a rank-based memory controller would print, instead, "rank2"
on the above debug info)
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index d8278b3..1bc2843 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -40,6 +40,25 @@
static DEFINE_MUTEX(mem_ctls_mutex);
static LIST_HEAD(mc_devices);
+unsigned edac_dimm_info_location(struct dimm_info *dimm, char *buf,
+ int len)
+{
+ struct mem_ctl_info *mci = dimm->mci;
+ int i, n, count = 0;
+ char *p = buf;
+
+ for (i = 0; i < mci->n_layers; i++) {
+ n = snprintf(p, len, "%s %d ",
+ edac_layer_name[mci->layers[i].type],
+ dimm->location[i]);
+ p += n;
+ len -= n;
+ count += n;
+ }
+
+ return count;
+}
+
#ifdef CONFIG_EDAC_DEBUG
static void edac_mc_dump_channel(struct rank_info *chan)
@@ -50,20 +69,18 @@ static void edac_mc_dump_channel(struct rank_info *chan)
debugf4("\tchannel->dimm = %p\n", chan->dimm);
}
-static void edac_mc_dump_dimm(struct dimm_info *dimm)
+static void edac_mc_dump_dimm(struct dimm_info *dimm, int number)
{
- int i;
+ char location[80];
+
+ edac_dimm_info_location(dimm, location, sizeof(location));
debugf4("\tdimm = %p\n", dimm);
+ debugf4("\t%s%i: %smapped as virtual row %d, chan %d\n",
+ dimm->mci->mem_is_per_rank ? "rank" : "dimm",
+ number, location, dimm->csrow, dimm->cschannel);
debugf4("\tdimm->label = '%s'\n", dimm->label);
debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
- debugf4("\tdimm location ");
- for (i = 0; i < dimm->mci->n_layers; i++) {
- printk(KERN_CONT "%d", dimm->location[i]);
- if (i < dimm->mci->n_layers - 1)
- printk(KERN_CONT ".");
- }
- printk(KERN_CONT "\n");
debugf4("\tdimm->grain = %d\n", dimm->grain);
debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
}
@@ -337,8 +354,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
memset(&pos, 0, sizeof(pos));
row = 0;
chn = 0;
- debugf4("initializing %d %s\n", tot_dimms,
- per_rank ? "ranks" : "dimms");
for (i = 0; i < tot_dimms; i++) {
chan = mci->csrows[row]->channels[chn];
off = EDAC_DIMM_OFF(layer, n_layers, pos[0], pos[1], pos[2]);
@@ -351,10 +366,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
mci->dimms[off] = dimm;
dimm->mci = mci;
- debugf2("%d: %s%i (%d:%d:%d): row %d, chan %d\n", i,
- per_rank ? "rank" : "dimm", off,
- pos[0], pos[1], pos[2], row, chn);
-
/*
* Copy DIMM location and initialize the memory location
*/
@@ -730,7 +741,7 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
edac_mc_dump_channel(mci->csrows[i]->channels[j]);
}
for (i = 0; i < mci->tot_dimms; i++)
- edac_mc_dump_dimm(mci->dimms[i]);
+ edac_mc_dump_dimm(mci->dimms[i], i);
}
#endif
mutex_lock(&mem_ctls_mutex);
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 8f96c49..e3e9e75 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -488,13 +488,7 @@ static ssize_t dimmdev_location_show(struct device *dev,
int i;
char *p = data;
- for (i = 0; i < mci->n_layers; i++) {
- p += sprintf(p, "%s %d ",
- edac_layer_name[mci->layers[i].type],
- dimm->location[i]);
- }
-
- return p - data;
+ return edac_dimm_info_location(dimm, data, PAGE_SIZE);
}
static ssize_t dimmdev_label_show(struct device *dev,
diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h
index 1af1367..de92756 100644
--- a/drivers/edac/edac_module.h
+++ b/drivers/edac/edac_module.h
@@ -34,6 +34,9 @@ extern int edac_mc_get_panic_on_ue(void);
extern int edac_get_poll_msec(void);
extern int edac_mc_get_poll_msec(void);
+unsigned edac_dimm_info_location(struct dimm_info *dimm, char *buf,
+ int len);
+
/* on edac_device.c */
extern int edac_device_register_sysfs_main_kobj(
struct edac_device_ctl_info *edac_dev);
^ permalink raw reply related
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets
From: David Miller @ 2012-04-30 17:41 UTC (permalink / raw)
To: benh; +Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel,
linuxppc-dev
In-Reply-To: <1335763568.20866.37.camel@pasglop>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Mon, 30 Apr 2012 15:26:08 +1000
> David, what's the right way to fix that ?
There is no doubt that sock_fprog is the correct datastructure to use.
^ permalink raw reply
* Re: [REGRESSION][PATCH V5 3/3] bpf jit: Let the powerpc jit handle negative offsets
From: David Miller @ 2012-04-30 17:41 UTC (permalink / raw)
To: kaffeemonster; +Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev
In-Reply-To: <4F9E1CDB.9020104@googlemail.com>
From: Jan Seiffert <kaffeemonster@googlemail.com>
Date: Mon, 30 Apr 2012 07:02:19 +0200
> Now the helper function from filter.c for negative offsets is exported,
> it can be used it in the jit to handle negative offsets.
>
> First modify the asm load helper functions to handle:
> - know positive offsets
> - know negative offsets
> - any offset
>
> then the compiler can be modified to explicitly use these helper
> when appropriate.
>
> This fixes the case of a negative X register and allows to lift
> the restriction that bpf programs with negative offsets can't
> be jited.
>
> Tested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com>
Applied, thanks.
^ permalink raw reply
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets
From: Benjamin Herrenschmidt @ 2012-04-30 21:55 UTC (permalink / raw)
To: David Miller
Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel,
linuxppc-dev
In-Reply-To: <20120430.134140.1738751315208907289.davem@davemloft.net>
On Mon, 2012-04-30 at 13:41 -0400, David Miller wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Mon, 30 Apr 2012 15:26:08 +1000
>
> > David, what's the right way to fix that ?
>
> There is no doubt that sock_fprog is the correct datastructure to use.
Ok, so the right fix is to email anybody who posted code using struct
bpf_program to fix their code ? :-)
My question was more along the lines of should we attempt changing one
of the two variants to make them match on BE (since they are in effect
compatible on LE), tho of course this could have the usual annoying
consequence of breaking the mangled c++ name of the symbol).
>From your reply I assume the answer is no... so that leaves us to chase
up users and fix them. Great....
Cheers,
Ben.
^ permalink raw reply
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets
From: Benjamin Herrenschmidt @ 2012-04-30 21:57 UTC (permalink / raw)
To: David Miller
Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel,
linuxppc-dev
In-Reply-To: <1335822926.20866.47.camel@pasglop>
On Tue, 2012-05-01 at 07:55 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2012-04-30 at 13:41 -0400, David Miller wrote:
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Date: Mon, 30 Apr 2012 15:26:08 +1000
> >
> > > David, what's the right way to fix that ?
> >
> > There is no doubt that sock_fprog is the correct datastructure to use.
>
> Ok, so the right fix is to email anybody who posted code using struct
> bpf_program to fix their code ? :-)
Actually, the right fix is for anybody using pcap-bpf.h to not
use SO_ATTACH_FILTER directly but to use pcap_setfilter() which
handles the compatibility.
I'll start spamming web sites who tell people to do the wrong thing.
Cheers,
Ben.
^ permalink raw reply
* linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-04-30 22:37 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: linuxppc-dev, linux-kernel
Hi Paul,
On 3.4.0-rc4-next-20120427 and preceding linux-nexts (I've not tried
rc5-next-20120430 but expect it's the same), on PowerPC G5 quad with
CONFIG_PREEMPT=y and CONFIG_DEBUG_ATOMIC_SLEEP=y, I'm getting spurious
"BUG: sleeping function called from invalid context" messages from
__might_sleep().
Just once I saw such a message during startup. Once I saw such a
message when rebuilding the machine's kernel. Usually I see them
when I'm running a swapping load of kernel builds under memory
pressure (but that's what I'm habitually running there): perhaps
after a few minutes a flurry comes, then goes away, comes back
again later, and after perhaps a couple of hours of that I see
"INFO: rcu_preempt detected stalls" messages too, and soon it
freezes (or perhaps it's still running, but I'm so flooded by
messages that I reboot anyway).
Rather like from before you fixed schedule_tail() for your per-cpu
RCU mods, but not so easy to reproduce. I did a bisection and indeed
it converged as expected on the RCU changes. No such problem seen on
x86: it looks as if there's some further tweak required on PowerPC.
Here are my RCU config options (I don't usually have the TORTURE_TEST
in, but tried that for half an hour this morning, in the hope that it
would generate the issue: but it did not).
# RCU Subsystem
CONFIG_TREE_PREEMPT_RCU=y
CONFIG_PREEMPT_RCU=y
CONFIG_RCU_FANOUT=64
# CONFIG_RCU_FANOUT_EXACT is not set
CONFIG_TREE_RCU_TRACE=y
# CONFIG_RCU_BOOST is not set
CONFIG_HAVE_RCU_TABLE_FREE=y
# CONFIG_SPARSE_RCU_POINTER is not set
CONFIG_RCU_TORTURE_TEST=m
CONFIG_RCU_CPU_STALL_TIMEOUT=60
# CONFIG_RCU_CPU_STALL_VERBOSE is not set
# CONFIG_RCU_CPU_STALL_INFO is not set
CONFIG_RCU_TRACE=y
Here's the message when I was rebuilding the G5's kernel:
BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
Call Trace:
[c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
[c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
[c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
[c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
[c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
[c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
I've plenty more examples, most of them from page faults or from kswapd;
but I don't think there's any more useful information in them.
Anything I can try later on?
Thanks!
Hugh
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-04-30 23:14 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Paul E. McKenney, linuxppc-dev, linux-kernel
In-Reply-To: <alpine.LSU.2.00.1204301451270.2986@eggly.anvils>
On Mon, Apr 30, 2012 at 03:37:10PM -0700, Hugh Dickins wrote:
> Hi Paul,
>
> On 3.4.0-rc4-next-20120427 and preceding linux-nexts (I've not tried
> rc5-next-20120430 but expect it's the same), on PowerPC G5 quad with
> CONFIG_PREEMPT=y and CONFIG_DEBUG_ATOMIC_SLEEP=y, I'm getting spurious
> "BUG: sleeping function called from invalid context" messages from
> __might_sleep().
>
> Just once I saw such a message during startup. Once I saw such a
> message when rebuilding the machine's kernel. Usually I see them
> when I'm running a swapping load of kernel builds under memory
> pressure (but that's what I'm habitually running there): perhaps
> after a few minutes a flurry comes, then goes away, comes back
> again later, and after perhaps a couple of hours of that I see
> "INFO: rcu_preempt detected stalls" messages too, and soon it
> freezes (or perhaps it's still running, but I'm so flooded by
> messages that I reboot anyway).
>
> Rather like from before you fixed schedule_tail() for your per-cpu
> RCU mods, but not so easy to reproduce. I did a bisection and indeed
> it converged as expected on the RCU changes. No such problem seen on
> x86: it looks as if there's some further tweak required on PowerPC.
>
> Here are my RCU config options (I don't usually have the TORTURE_TEST
> in, but tried that for half an hour this morning, in the hope that it
> would generate the issue: but it did not).
>
> # RCU Subsystem
> CONFIG_TREE_PREEMPT_RCU=y
> CONFIG_PREEMPT_RCU=y
> CONFIG_RCU_FANOUT=64
> # CONFIG_RCU_FANOUT_EXACT is not set
> CONFIG_TREE_RCU_TRACE=y
> # CONFIG_RCU_BOOST is not set
> CONFIG_HAVE_RCU_TABLE_FREE=y
> # CONFIG_SPARSE_RCU_POINTER is not set
> CONFIG_RCU_TORTURE_TEST=m
> CONFIG_RCU_CPU_STALL_TIMEOUT=60
> # CONFIG_RCU_CPU_STALL_VERBOSE is not set
> # CONFIG_RCU_CPU_STALL_INFO is not set
> CONFIG_RCU_TRACE=y
>
> Here's the message when I was rebuilding the G5's kernel:
>
> BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> Call Trace:
> [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
>
> I've plenty more examples, most of them from page faults or from kswapd;
> but I don't think there's any more useful information in them.
>
> Anything I can try later on?
Interesting... As you say, I saw this sort of thing before applying
the changes to schedule_tail(), and it is all too possible that there
is some other "sneak path" for context switches.
Have you tried running with CONFIG_PROVE_RCU? This enables some
additional debugging in rcu_switch_from() and rcu_switch_to() that
helped track down the schedule_tail() problem.
Thanx, Paul
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Benjamin Herrenschmidt @ 2012-05-01 0:33 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Paul E. McKenney, linuxppc-dev, linux-kernel
In-Reply-To: <alpine.LSU.2.00.1204301451270.2986@eggly.anvils>
On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
>
> BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
Hrm ... in_atomic and irqs_disabled are both 0 ... so yeah it smells
like a preempt count problem... odd.
Did you get a specific bisect target yet ?
Cheers,
Ben.
> Call Trace:
> [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
>
> I've plenty more examples, most of them from page faults or from kswapd;
> but I don't think there's any more useful information in them.
>
> Anything I can try later on?
^ permalink raw reply
* Re: [PATCH v3 03/17] powerpc: Add PFO support to the VIO bus
From: Benjamin Herrenschmidt @ 2012-05-01 3:06 UTC (permalink / raw)
To: Kent Yoder; +Cc: rcj, linuxppc-dev, linux-kernel, linux-crypto
In-Reply-To: <1334243302.18090.10.camel@key-ThinkPad-W510>
Hrm... I don't like that much:
> + if (op->timeout)
> + deadline = jiffies + msecs_to_jiffies(op->timeout);
> +
> + while (true) {
> + hret = plpar_hcall_norets(H_COP, op->flags,
> + vdev->resource_id,
> + op->in, op->inlen, op->out,
> + op->outlen, op->csbcpb);
> +
> + if (hret == H_SUCCESS ||
> + (hret != H_NOT_ENOUGH_RESOURCES &&
> + hret != H_BUSY && hret != H_RESOURCE) ||
> + (op->timeout && time_after(deadline, jiffies)))
> + break;
> +
> + dev_dbg(dev, "%s: hcall ret(%ld), retrying.\n", __func__, hret);
> + }
> +
Is this meant to be called in atomic context ? If not, maybe it should
at the very least do a cond_resched() ?
Else, what about ceding the processor ? Or at the very least reducing
the thread priority for a bit ?
Shouldn't we also enforce to always have a timeout ? IE. Something like
30s or so if nothing specified to avoid having the kernel just hard
lock...
In general I don't like that sort of synchronous code, I'd rather return
the busy status up the chain which gives a chance to the caller to take
more appropriate measures depending on what it's doing, but that really
depends what you use that synchronous call for. I suppose if it's for
configuration type operations, it's ok...
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v3 03/17] powerpc: Add PFO support to the VIO bus
From: Benjamin Herrenschmidt @ 2012-05-01 4:10 UTC (permalink / raw)
To: Kent Yoder; +Cc: rcj, linuxppc-dev, linux-kernel, linux-crypto
In-Reply-To: <1335841603.3621.8.camel@pasglop>
> Else, what about ceding the processor ? Or at the very least reducing
> the thread priority for a bit ?
>
> Shouldn't we also enforce to always have a timeout ? IE. Something like
> 30s or so if nothing specified to avoid having the kernel just hard
> lock...
>
> In general I don't like that sort of synchronous code, I'd rather return
> the busy status up the chain which gives a chance to the caller to take
> more appropriate measures depending on what it's doing, but that really
> depends what you use that synchronous call for. I suppose if it's for
> configuration type operations, it's ok...
In any case, don't resend the whole series, just that one patch.
Cheers,
Ben.
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-05-01 5:10 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul E. McKenney, linuxppc-dev, linux-kernel
In-Reply-To: <1335832418.20866.95.camel@pasglop>
On Tue, 1 May 2012, Benjamin Herrenschmidt wrote:
> On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> >
> > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
>
> Hrm ... in_atomic and irqs_disabled are both 0 ... so yeah it smells
> like a preempt count problem... odd.
>
> Did you get a specific bisect target yet ?
Oh, I went as far as we need, I think, but I didn't bother quite to
complete it because, once in that area, we know the schedule_tail()
omission would muddy the waters: the tail of my bisect log was
# bad: [e798cf3385d3aa7c84afa65677eb92e0c0876dfd] rcu: Add exports for per-CPU variables used for inlining
git bisect bad e798cf3385d3aa7c84afa65677eb92e0c0876dfd
# good: [90aec3b06194393c909e3e5a47b6ed99bb8caba5] rcu: Make exit_rcu() more precise and consolidate
git bisect good 90aec3b06194393c909e3e5a47b6ed99bb8caba5
from which I concluded that the patch responsible is
commit ab8fc41a8545d40a4b58d745876c125af72a8a5c
Author: Paul E. McKenney <paul.mckenney@linaro.org>
Date: Fri Apr 13 14:32:01 2012 -0700
rcu: Move __rcu_read_lock() and __rcu_read_unlock() to per-CPU variables
This commit is another step towards inlinable __rcu_read_lock() and
__rcu_read_unlock() functions for preemptible RCU. This keeps these two
functions out of line, but switches them to use the per-CPU variables
that are required to export their definitions without requiring that
all RCU users include sched.h. These per-CPU variables are saved and
restored at context-switch time.
>
> Cheers,
> Ben.
>
> > Call Trace:
> > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
> >
> > I've plenty more examples, most of them from page faults or from kswapd;
> > but I don't think there's any more useful information in them.
> >
> > Anything I can try later on?
I'd forgotten about CONFIG_PROVE_RCU (and hadn't been using PROVE_LOCKING
on that machine), but following Paul's suggestion have now turned them on.
But not much light shed, I'm afraid. Within minutes it showed a trace
exactly like the one above, but the only thing PROVE_LOCKING and PROVE_RCU
had to say was that we're holding mmap_sem at that point, which is no
surprise and not a problem, just something lockdep is right to note.
That was an isolated occurrence, it continued quietly for maybe 20 minutes,
then output lots to the console screen - but garbled in a way I've not
seen before - the 0s came out just right (or perhaps all the hex digits
were being shown as 0s), but most everything else was grayly unreadable.
Then after a few minutes, spontaneously rebooted.
Perhaps I should remind myself of netdump; but getting the trace above
without complaint from PROVE_RCU tells me that it is not helping.
Hugh
^ 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