Fw:Re: I want to submit some code, but there are someproblems.

[王翔]

> On Tue, Jul 03, 2018 at 04:23:07PM +0800, 王翔 wrote:
> > I want submit some code to support temporarily block multiple-threads.
> > But this code can't pass check by `checkpatch`.
> What is the error that checkpatch prints out?
> If you can't see it, try running
> util/lint/checkpatch.pl -f src/path/to/your/file.h
> directly.
> Hope that helps,
> Jonathan Neuschäfer

Error print like this:

```

WARNING: Macros with flow control statements should be avoided
#43: FILE: src/arch/riscv/include/smp.h:43:
+#define SMP_PAUSE(active) do \
+{ \
+    __label__ l_entry, l_exit; \
+    static int _flag_; \
+    if (active == read_csr(mhartid)) \
+        goto l_entry; \
+    do {barrier(); } while (!_flag_); \
+    goto l_exit; \
+l_entry:

WARNING: please, no spaces at the start of a line
#45: FILE: src/arch/riscv/include/smp.h:45:
+    __label__ l_entry, l_exit; $

WARNING: please, no spaces at the start of a line
#46: FILE: src/arch/riscv/include/smp.h:46:
+    static int _flag_; $

WARNING: please, no spaces at the start of a line
#47: FILE: src/arch/riscv/include/smp.h:47:
+    if (active == read_csr(mhartid)) $

ERROR: code indent should use tabs where possible
#48: FILE: src/arch/riscv/include/smp.h:48:
+        goto l_entry; $

WARNING: please, no spaces at the start of a line
#48: FILE: src/arch/riscv/include/smp.h:48:
+        goto l_entry; $

WARNING: please, no spaces at the start of a line
#49: FILE: src/arch/riscv/include/smp.h:49:
+    do {barrier(); } while (!_flag_); $

WARNING: please, no spaces at the start of a line
#50: FILE: src/arch/riscv/include/smp.h:50:
+    goto l_exit; $

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#54: FILE: src/arch/riscv/include/smp.h:54:
+#define SMP_RESUME() \
+    _flag_ = 1; \
+l_exit: \
+    barrier(); \
+} while (0)

WARNING: please, no spaces at the start of a line
#55: FILE: src/arch/riscv/include/smp.h:55:
+    _flag_ = 1; $

WARNING: please, no spaces at the start of a line
#57: FILE: src/arch/riscv/include/smp.h:57:
+    barrier(); $

total: 2 errors, 9 warnings, 65 lines checked
```

[Jonathan Neuschäfer]

On Tue, Jul 03, 2018 at 05:11:57PM +0800, 王翔 wrote:
> > On Tue, Jul 03, 2018 at 04:23:07PM +0800, 王翔 wrote:

[...]
> Error print like this:
> WARNING: Macros with flow control statements should be avoided
> #43: FILE: src/arch/riscv/include/smp.h:43:
> +#define SMP_PAUSE(active) do \
> +{ \
> +    __label__ l_entry, l_exit; \
> +    static int _flag_; \
> +    if (active == read_csr(mhartid)) \
> +        goto l_entry; \
> +    do {barrier(); } while (!_flag_); \
> +    goto l_exit; \
> +l_entry:

checkpatch doesn't like goto (or return) in macros, because it can be
unexpected for the author of the calling code, that a macro causes a
jump or return.
> WARNING: please, no spaces at the start of a line
> #45: FILE: src/arch/riscv/include/smp.h:45:
> __label__ l_entry, l_exit; $

You should indent with tabs instead of spaces.
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #54: FILE: src/arch/riscv/include/smp.h:54:
> +#define SMP_RESUME() \
> _flag_ = 1; \
>
> +l_exit: \
> barrier(); \
>
> +} while (0)

Oh! SMP_PAUSE actually jumps here! I would *not* expect that, when I
read code like this:

SMP_PAUSE(active);
    foo();
    bar(42);
    SMP_RESUME();

Something like the following would be a lot clearer for me:

if (mhartid() == active) {
    	foo();
    	bar(42);
    }

Or maybe:

if (running_on_hart(active)) { ... }

But note that this lacks the barriers/locking that your code has.

Jonathan Neuschäfer

Fw:Re: I want to submit some code, but there aresomeproblems.

[王翔]

> Oh! SMP_PAUSE actually jumps here! I would *not* expect that, when I
> read code like this:

> SMP_PAUSE(active);
    > foo();
    > bar(42);
    > SMP_RESUME();
> Something like the following would be a lot clearer for me:

> if (mhartid() == active) {
    	> foo();
> bar(42);

> }
> Or maybe:

> if (running_on_hart(active)) { ... }
> But note that this lacks the barriers/locking that your code has.
>
> Jonathan Neuschäfer

```
if (running_on_hart(active)) { ... }
```
Yes, these codes are clearer.

/*
 * If your code needs to temporarily block multiple-threads, do this:
 *     SMP_PAUSE(active)    // `active` is hartid of working thread
 *     ... single-threaded work ...
 *     SMP_RESUME()
 *     ... multi-threaded work ...
 */
But sometimes multi-threaded work has to wait for the single-threaded work to complete.

etc:
    single-threaded work : init ddr controller
    multi-threaded work  : some operations related to memory

It is necessary to wait for the completion of single-threaded work.

Xiang Wang

Fw:Re: I want to submit some code, but there aresomeproblems.

[王翔]

> if (running_on_hart(active)) { ... }
>
> Yes, these codes are clearer.
>
> /*
> If your code needs to temporarily block multiple-threads, do this:
> SMP_PAUSE(active)    // `active` is hartid of working thread
> ... single-threaded work ...
> SMP_RESUME()
> ... multi-threaded work ...
>
> */
> But sometimes multi-threaded work has to wait for the single-threaded work to complete.
>
> etc:
>    single-threaded work : init ddr controller
>    multi-threaded work  : some operations related to memory
>
> It is necessary to wait for the completion of single-threaded work.
>
> Xiang Wang

There is other solution here.
```
#define HART_NUM 5

void smp_sync(unsigned long *cnt) 
{
    atomic_add(cnt,1);
    do {
        barrier();
    }while(*cnt < HART_MAX);
}

static unsigned long cnt;
if (running_on_hart(active)) { ... }
smp_sync(&cnt);
```
This solution must define HART_NUM.
I think this solution is not as flexible as SMP_PAUSE/SMP_RESUME.
Which solution do you want to use?

Xiang Wang