Mem barrier v2 #3

amergnat · 2022-02-07T18:12:54Z

Here is de CI result
zephyrproject-rtos#42549

For this proposal, ISB, DSB and DMB are, at least, defined for all arch. This allows to do a generic barrier API with. They are also definable in the toolchain.

The ARCH_HAS_MEMORY_BARRIER is more specific now. It seems the architecture have it own implementation of the barrier API.

include/arch/arc/barrier.h

carlocaione · 2022-02-07T18:22:04Z

include/device.h

@@ -30,6 +30,7 @@
 #include <linker/sections.h>
 #include <sys/device_mmio.h>
 #include <sys/util.h>
+#include <errno.h>


Had issue for some board here:

zephyr/include/device.h

Line 790 in 4f284b2

return z_device_is_ready(dev) ? 0 : -ENODEV;

Now CMSIS fallback changed, but I think that make sense to keep it because the only reason that don't fail for other board is because they include errno.h before device.h include.

To resume: -ENODEV is used in this file, then include errno.h made sense.

NVM, I pass the CI without now and it's out of context, I removed it

carlocaione · 2022-02-07T18:29:25Z

include/arch/riscv/barrier.h

+#if defined(CONFIG_ARCH_HAS_MEMORY_BARRIER)
+
+#define arch_isb()		__ISB()
+#define arch_mb()		__DSB()
+#define arch_rmb()		__asm__ volatile ("fence ir, ir" : : : "memory")
+#define arch_wmb()		__asm__ volatile ("fence ow, ow" : : : "memory")
+#define arch_smp_mb()	__DMB()
+#define arch_smp_rmb()	__asm__ volatile ("fence r, r" : : : "memory")
+#define arch_smp_wmb()	__asm__ volatile ("fence w, w" : : : "memory")
+
+#endif /* !CONFIG_ARCH_HAS_MEMORY_BARRIER */


Ok, I understand what you did here but I don't like it honestly. I have to think how to approach this better.

carlocaione · 2022-02-08T10:33:51Z

arch/Kconfig

 	imply XIP
+	select ARCH_HAS_MEMORY_BARRIER


swap these two lines

carlocaione · 2022-02-08T10:40:37Z

arch/Kconfig

+	bool "Enable architecture specific memory barrier support"
+	help
+	  This option enable architecture specific function for
+	  the memory barrier API support. These functions can be
+	  implemented or builtin toolchain.


The ARCH_HAS_* symbols are usually hidden with no prompt.

Suggested change

bool "Enable architecture specific memory barrier support"

help

This option enable architecture specific function for

the memory barrier API support. These functions can be

implemented or builtin toolchain.

bool

help

This option signifies that the architecture does support and implement the

memory barrier functions.

carlocaione · 2022-02-08T10:41:10Z

include/arch/arc/barrier.h

+
+#ifndef _ASMLANGUAGE
+
+/* Provide a default implementation for un-supported compilers. */


no need for this comment

carlocaione · 2022-02-08T10:41:30Z

include/arch/arc/barrier.h

@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2022 Carlo Caione <ccaione@baylibre.com>


remember to change the copyright (if you are sending the PR your name must be here).

carlocaione · 2022-02-08T10:42:01Z

include/arch/arc/barrier.h

+
+#if defined(CONFIG_ARCH_HAS_MEMORY_BARRIER)
+/* Not implemented yet */
+#error ARC arch has not memory barrier implementation


Suggested change

#error ARC arch has not memory barrier implementation

#error ARC arch does not have a memory barrier implementation

carlocaione · 2022-02-08T10:42:49Z

include/arch/arm/aarch32/barrier.h

+ */
+#include <cmsis_compiler.h>
+
+/* Provide a default implementation for un-supported compilers. */


Suggested change

/* Provide a default implementation for un-supported compilers. */

/*

* Provide a default implementation for un-supported compilers.

*/

carlocaione · 2022-02-08T10:43:46Z

include/arch/riscv/barrier.h

+#ifndef __ISB
+#define __ISB()			__asm__ volatile ("fence.i")
+#endif
+
+#ifndef __DSB
+#define __DSB()			__asm__ volatile ("fence iorw, iorw" : : : "memory")
+#endif
+
+#ifndef __DMB
+#define __DMB()			__asm__ volatile ("fence rw, rw" : : : "memory")
+#endif


Stuck on one style for indentation (compare riscv indentation to other archs)

carlocaione

Before this is ready please write a simple test build-only where you try to compile all the functions introduced for all the architectures.

amergnat · 2022-02-08T11:38:26Z

Before this is ready please write a simple test build-only where you try to compile all the functions introduced for all the architectures.

diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index 6c5c8a27dc..c1c2cadd67 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -10,4 +10,10 @@
 void main(void)
 {
        printk("Hello World! %s\n", CONFIG_BOARD);
+       __ISB();
+       __DSB();
+       arch_isb();
+       arch_mb();
+       arch_rmb();
+       arch_wmb();
 }

./scripts/twister -b -v --testcase-root samples/hello_world
ZEPHYR_BASE unset, using "/home/alex/project/zephyr_bugfix/zephyrproject/zephyr"
Renaming output directory to /home/alex/project/zephyr_bugfix/zephyrproject/zephyr/twister-out.7
INFO    - Zephyr version: v2.7.99-3406-g0f08b06ccb0a
INFO    - JOBS: 32
INFO    - Using 'zephyr' toolchain.
INFO    - Selecting default platforms per test case
INFO    - Building initial testcase list...
INFO    - 1 test scenarios (37 configurations) selected, 14 configurations discarded due to filters.
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    -  1/23 native_posix              samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  2/23 qemu_leon3                samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  3/23 m2gl025_miv               samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  4/23 qemu_nios2                samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  5/23 qemu_riscv64              samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  6/23 qemu_cortex_a53           samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  7/23 qemu_x86                  samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  8/23 qemu_xtensa               samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    -  9/23 qemu_riscv32              samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 10/23 qemu_cortex_a9            samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 11/23 qemu_x86_64               samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 12/23 qemu_cortex_a53_smp       samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 13/23 qemu_arc_hs               samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 14/23 qemu_arc_hs6x             samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 15/23 nsim_hs_smp               samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 16/23 qemu_arc_em               samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 17/23 nsim_sem                  samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 18/23 qemu_cortex_r5            samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 19/23 nsim_hs_mpuv6             samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 20/23 mps2_an385                samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 21/23 qemu_cortex_m0            samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 22/23 mps2_an521                samples/hello_world/sample.basic.helloworld        PASSED (build)
INFO    - 23/23 frdm_k64f                 samples/hello_world/sample.basic.helloworld        PASSED (build)

INFO    - 23 of 23 test configurations passed (100.00%), 0 failed, 14 skipped with 0 warnings in 11.46 seconds
INFO    - 0 test configurations executed on platforms, 23 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing xunit report /home/alex/project/zephyr_bugfix/zephyrproject/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/alex/project/zephyr_bugfix/zephyrproject/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

carlocaione · 2022-02-08T11:42:20Z

diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c

Can you make this a test instead of a sample?

carlocaione · 2022-02-08T14:34:54Z

include/arch/arc/barrier.h

+#endif
+
+#if defined(CONFIG_ARCH_HAS_MEMORY_BARRIER)
+/* Not implemented yet */


Remove these comments, the error is enough

carlocaione · 2022-02-08T14:35:09Z

include/arch/arm/aarch32/barrier.h

+ */
+
+#ifndef __ISB
+#define __ISB()	__asm__ volatile ("isb 0xF":::"memory")


spacing volatile ("isb 0xF" ::: "memory")

carlocaione · 2022-02-08T14:35:45Z

include/arch/arm64/barrier.h

+#define arch_isb()	__ISB()
+#define arch_mb()	__DSB()
+#define arch_rmb()	__asm__ volatile ("dsb ld" : : : "memory")
+#define arch_wmb()	__asm__ volatile ("dsb st" : : : "memory")


keep a consistent spacing, you used one space for __ISB() & co and now you are using tab / multiple spaces.

carlocaione · 2022-02-08T14:36:26Z

tests/kernel/mem_barrier/src/main.c

+	__DMB();
+	arch_isb();


carlocaione · 2022-02-08T18:26:48Z

include/arch/arm/aarch32/barrier.h

+ */
+
+#ifndef __ISB
+#define __ISB() __asm__ volatile ("isb 0xF":::"memory")


let's decide between ":::" and ": : :" and stick to it.

carlocaione · 2022-02-08T18:27:43Z

include/arch/riscv/barrier.h

+#define arch_isb() __ISB()
+#define arch_mb() __DSB()
+#define arch_rmb() __asm__ volatile ("fence ir, ir" : : : "memory")
+#define arch_wmb() __asm__ volatile ("fence ow, ow" : : : "memory")


ok, this is ugly. Switch to a single tab for spacing.

z_device_usable_check and device_usable_check functions return -ENODEV if device is not usable. Then, errno.h must be include to avoid build issue. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

amergnat · 2022-02-08T19:05:25Z

I bring back the missing error define fix due to CI fail: https://github.com/zephyrproject-rtos/zephyr/runs/4870325901?check_suite_focus=true#step:11:659

carlocaione · 2022-02-08T19:30:27Z

include/barrier.h

+ * This is used to guarantee that any subsequent instructions are fetched, so
+ * that privilege and access are checked with the current MMU configuration.
+ */
+#define arch_isb()		__ISB()


Alex, again, pick one single indenting style for macros and use that everywhere (we have now 1 space for __ISB() & co, 1 tab for arch_* in the arch headers and multiple tabs (??) here)

we have now 1 space for __ISB() & co

It's tab but wide as 1 space.

All arch headers use 1 Tab everywhere,
But you're right about this file, there are 2 Tabs and I will fix that

- Introduce a new preprocessor based API for barriers. - Add HAS_ARCH_MEMORY_BARRIER option for memory barrier: The architecture must select it to use its own specific memory barrier functions. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Add a basic implementation for ARM, ARM64 and RISC-V. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

- This test checks that the memory barrier API is fully defined to avoid build regression. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

This patch reworks how fragments are handled in the net_buf infrastructure. In particular, it removes the union around the node and frags members in the main net_buf structure. This is done so that both can be used at the same time, at a cost of 4 bytes per net_buf instance. This implies that the layout of net_buf instances changes whenever being inserted into a queue (fifo or lifo) or a linked list (slist). Until now, this is what happened when enqueueing a net_buf with frags in a queue or linked list: 1.1 Before enqueueing: +--------+ +--------+ +--------+ |#1 node|\ |#2 node|\ |#3 node|\ | | \ | | \ | | \ | frags |------| frags |------| frags |------NULL +--------+ +--------+ +--------+ net_buf #1 has 2 fragments, net_bufs #2 and #3. Both the node and frags pointers (they are the same, since they are unioned) point to the next fragment. 1.2 After enqueueing: +--------+ +--------+ +--------+ +--------+ +--------+ |q/slist |------|#1 node|------|#2 node|------|#3 node|------|q/slist | |node | | *flag | / | *flag | / | | / |node | | | | frags |/ | frags |/ | frags |/ | | +--------+ +--------+ +--------+ +--------+ +--------+ When enqueing a net_buf (in this case #1) that contains fragments, the current net_buf implementation actually enqueues all the fragments (in this case #2 and #3) as actual queue/slist items, since node and frags are one and the same in memory. This makes the enqueuing operation expensive and it makes it impossible to atomically dequeue. The `*flag` notation here means that the `flags` member has been set to `NET_BUF_FRAGS` in order to be able to reconstruct the frags pointers when dequeuing. After this patch, the layout changes considerably: 2.1 Before enqueueing: +--------+ +--------+ +--------+ |#1 node|--NULL |#2 node|--NULL |#3 node|--NULL | | | | | | | frags |-------| frags |-------| frags |------NULL +--------+ +--------+ +--------+ This is very similar to 1.1, except that now node and frags are different pointers, so node is just set to NULL. 2.2 After enqueueing: +--------+ +--------+ +--------+ |q/slist |-------|#1 node|-------|q/slist | |node | | | |node | | | | frags | | | +--------+ +--------+ +--------+ | +--------+ +--------+ | |#2 node|--NULL |#3 node|--NULL | | | | | +------------| frags |-------| frags |------NULL +--------+ +--------+ When enqueuing net_buf #1, now we only enqueue that very item, instead of enqueing the frags as well, since now node and frags are separate pointers. This simplifies the operation and makes it atomic. Resolves zephyrproject-rtos#52718. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

amergnat requested a review from carlocaione as a code owner February 7, 2022 18:12

github-actions bot added area: API area: ARC area: ARM area: ARM64 area: native port area: NIOS2 area: RISCV area: X86 area: Xtensa labels Feb 7, 2022

carlocaione reviewed Feb 7, 2022

View reviewed changes

amergnat force-pushed the mem_barrier_carlo branch from 4f284b2 to 41e959b Compare February 8, 2022 10:29

carlocaione approved these changes Feb 8, 2022

View reviewed changes

carlocaione requested changes Feb 8, 2022

View reviewed changes

amergnat force-pushed the mem_barrier_carlo branch from 41e959b to a2f1281 Compare February 8, 2022 14:20

github-actions bot added area: Tests area: Kernel labels Feb 8, 2022

amergnat force-pushed the mem_barrier_carlo branch from a2f1281 to 3eecd53 Compare February 8, 2022 14:23

carlocaione reviewed Feb 8, 2022

View reviewed changes

amergnat force-pushed the mem_barrier_carlo branch from 3eecd53 to 96570a5 Compare February 8, 2022 15:09

carlocaione reviewed Feb 8, 2022

View reviewed changes

device: fix undeclared define

7c0a915

z_device_usable_check and device_usable_check functions return -ENODEV if device is not usable. Then, errno.h must be include to avoid build issue. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

amergnat force-pushed the mem_barrier_carlo branch from 96570a5 to f80acae Compare February 8, 2022 19:03

carlocaione reviewed Feb 8, 2022

View reviewed changes

amergnat added 3 commits February 8, 2022 21:45

barrier: Introduce barriers API

5c10a79

- Introduce a new preprocessor based API for barriers. - Add HAS_ARCH_MEMORY_BARRIER option for memory barrier: The architecture must select it to use its own specific memory barrier functions. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

barrier: Hook up the barrier API to the archs

ae1533f

Add a basic implementation for ARM, ARM64 and RISC-V. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

tests: mem_barrier: check api support

d29a618

- This test checks that the memory barrier API is fully defined to avoid build regression. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

amergnat force-pushed the mem_barrier_carlo branch from f80acae to d29a618 Compare February 8, 2022 20:45


		#ifndef _ASMLANGUAGE

		/* Provide a default implementation for un-supported compilers. */

		@@ -0,0 +1,35 @@
		/*
		* Copyright (c) 2022 Carlo Caione <ccaione@baylibre.com>

	#error ARC arch has not memory barrier implementation
	#error ARC arch does not have a memory barrier implementation

Mem barrier v2 #3

Are you sure you want to change the base?

Mem barrier v2 #3

Conversation

amergnat commented Feb 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocaione left a comment

Choose a reason for hiding this comment

amergnat commented Feb 8, 2022

carlocaione commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amergnat commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment