nxmutex replace nxsem when used as a lock

[anjiahao1]

Signed-off-by: anjiahao [email protected]

Summary

pull5070 is the first step, this is step 2, then change sem default behavior

Impact

schedule

Testing

CI

[anjiahao1]

this error can't fix

[xiaoxiang781216]

@anjiahao1 please fix the warning: https://github.com/apache/incubator-nuttx/runs/8127486548?check_suite_focus=true and fix the conflict.

[xiaoxiang781216]

@pkarashchenko could you review this patch which is part of effort to fix the potential issue when we enable priority inheritance?

[pkarashchenko]

I will review code soon

[anjiahao1]

@xiaoxiang781216 i check it all again,is look good

[xiaoxiang781216]

@pkarashchenko the patch is ready for review again.

[xiaoxiang781216]

With each new push 100-200 new files are modified. I'm feeling that I can't pursuit that and getting far and far away from completing this review

Since your suggestion may suitable for other source files, I grep the whole code base and apply the suggestion to all possible places. That's why the touched file become larger in each iteration. The good news is that all common issues are fixed, the list shouldn't increase anymore:).

[xiaoxiang781216]

just few last changes

Done.

[xiaoxiang781216]

Done.

[masayuki2009]

@anjiahao1 This PR contains only one big commit. Please divide the commit into smaller ones (e.g. style changes, etc)

[xiaoxiang781216]

@masayuki2009 it's hard to split the style change from the patch now. could you have method to retrieve the old revision from github?

[masayuki2009]

@masayuki2009 it's hard to split the style change from the patch now. could you have method to retrieve the old revision from github?

I think you can always retrieve the old revision from the latest master.

Actually, there are many changes that do not relate to 'nxmutex replace nexsem' For example, besides the style changes, I can also see the changes such as (kmm_malloc to kmm_zalloc, i2c_givesem to nxsem_post, etc).

[xiaoxiang781216]

@masayuki2009 it's hard to split the style change from the patch now. could you have method to retrieve the old revision from github?

I think you can always retrieve the old revision from the latest master.

I mean the intermediate change in this PR, not the latest master. This patch contain 20000~ change, 10000~ change is about sem_t/mutex_t. It's very hard(or impossible) to separate sem_t/mutext_t change from the style one in the current size if I can't restore the intermediate version which doesn't contain any or big style change).

Actually, there are many changes that do not relate to 'nxmutex replace nexsem' For example, besides the style changes, I can also see the changes such as (kmm_malloc to kmm_zalloc,

This is part of nxmutex_t change since @pkarashchenko suggest we initialize all global mutex_t field through NXMUTEX_INITIALIZER, which mean we need ensure no place zero out the global variables again, otherwise the static initialization will be lost. How to ensure this doesn't happen? The safest approach I can come up is to reduce memset as much as possible, so I can check the rest memset more quickly and less error. That's why I change kmm_malloc+memset to kmm_zalloc.

i2c_givesem to nxsem_post, etc).

It's also part of sem_t and nxmutex_t replacement to avoid people consider the code use sem_t as lock(i2c_givesem/i2c_takesem) and remove the unneeded/simple one line wrapper. After the change, the rule is simple:

1. sem_t is always used as signal or resource couting(call nxsem_wait/nxsem_post, never takesem/givesem)
2. mutex_t is always used as lock(call nxmutex_lock/nxmutex_unlock)

[xiaoxiang781216]

@masayuki2009 what do you think?

[masayuki2009]

@xiaoxiang781216 As for style changes, I think you can create another PR, and after the PR is merged then rebase this PR. The result would be much simpler to review.

[xiaoxiang781216]

@xiaoxiang781216 As for style changes, I think you can create another PR, and after the PR is merged then rebase this PR. The result would be much simpler to review.

But, the initial version doesn't exist in my local git anymore. If I can't retrieve the initial/early version of this patch from github, it's very hard and error prone to split the stye change from this current patch. That's why I ask the question how to retrieve the old revision from github.

[masayuki2009]

sim:usrsocktest with CONFIG_DEBUG_ASSERTIONS=y failed.

up_assert: Assertion failed at file:/mnt/m2ssd/opensource/github_anjiahao1/nuttx/include/nuttx/mutex.h line: 489 task: nsh_main

[masayuki2009]

@xiaoxiang781216

But, the initial version doesn't exist in my local git anymore. If I can't retrieve the initial/early version of this patch from github, it's very hard and error prone to split the stye change from this current patch. That's why I ask the question how to retrieve the old revision from github.

I think you don't have to use the initial commit to this PR. You can check the current differences in this PR, then create a new PR based on the differences manually.

[masayuki2009]

sim:usrsocktest with CONFIG_DEBUG_ASSERTIONS=y failed.

Same as spresense:wifi_smp with CONFIG_DEBUG_ASSERTIONS=y

AEG
[    0.440000] [CPU1] up_assert: Assertion failed CPU1 at file:/mnt/m2ssd/opensource/github_anjiahao1/nuttx/include/nuttx/mutex.h line: 489 task: spresense_main
[    0.450000] [CPU1] arm_registerdump: R0: 00000000 R1: 2d05ef80 R2: 041ac000  R3: 00000000
[    0.460000] [CPU1] arm_registerdump: R4: 2d05ef80 R5: 2d06b710 R6: 2d05e3a4  FP: 2d06c5c8
[    0.470000] [CPU1] arm_registerdump: R8: 2d06c5c8 SB: 00000002 SL: 00000000 R11: 00000000
[    0.470000] [CPU1] arm_registerdump: IP: 00000004 SP: 2d06c5b0 LR: 0d00f7e1  PC: 0d00fa44
[    0.480000] [CPU1] arm_registerdump: xPSR: 61000200 BASEPRI: 000000e0 CONTROL: 00000004
[    0.490000] [CPU1] arm_registerdump: EXC_RETURN: ffffffe9
[    0.500000] [CPU1] arm_dump_stack: IRQ Stack:
[    0.500000] [CPU1] arm_dump_stack: sp:     2d06c5c8
[    0.500000] [CPU1] arm_dump_stack:   base: 2d05db00
[    0.510000] [CPU1] arm_dump_stack:   size: 00000800
[    0.510000] [CPU1] arm_dump_stack: ERROR: IRQ Stack pointer is not within the stack
[    0.520000] [CPU1] arm_dump_stack: User Stack:
[    0.530000] [CPU1] arm_dump_stack: sp:     2d06c5c8
[    0.530000] [CPU1] arm_dump_stack:   base: 2d06bb50
[    0.540000] [CPU1] arm_dump_stack:   size: 00000bd0

[pkarashchenko]

Let me take an effort and separate FAR and some style changes and submit as a separate PR. I will need a couple of days based on my current work load

[xiaoxiang781216]

At least, you need to test with CONFIG_DEBUG_ASSERTIONS=y

Ok, the problem is nxrmutex_breaklock forget to clear ret value: https://github.com/apache/incubator-nuttx/pull/6965/files#diff-8f94a783bf92ca637d929226ed048a878be0030b9f88720844f9b1e63e3f594bR529

[xiaoxiang781216]

Let me take an effort and separate FAR and some style changes and submit as a separate PR. I will need a couple of days based on my current work load

I find a method to retrieve the previous revision:

git reflog anjiahao/nxmutex
84f46b474a (anjiahao/nxmutex, nxmutex) refs/remotes/anjiahao/nxmutex@{0}: update by push
36c31b0de3 refs/remotes/anjiahao/nxmutex@{1}: update by push
6e1eacf541 refs/remotes/anjiahao/nxmutex@{2}: update by push
0e6fe2b14b refs/remotes/anjiahao/nxmutex@{3}: update by push
1d74f75b5b refs/remotes/anjiahao/nxmutex@{4}: update by push
a3b457edb2 refs/remotes/anjiahao/nxmutex@{5}: update by push
cbfdf155ab refs/remotes/anjiahao/nxmutex@{6}: update by push

let me try to separate.

[xiaoxiang781216]

@masayuki2009 @pkarashchenko the style change is revert, please review again.

[masayuki2009]

@masayuki2009 @pkarashchenko the style change is revert, please review again.

@xiaoxiang781216 Thanks for your efforts and updates.

It seems much easier to check the differences but I still have some comments. In my understanding, this PR is intended to replace binary semaphore usage with nxmutex. However, I can see some other changes like critical section with nxsem (e.g. cxd56_i2c.c) and nxsem to nxsem (i.e. cxd56_icc.c). I think that kind of changes should not be mixed in this PR.

[xiaoxiang781216]

@masayuki2009 @pkarashchenko the style change is revert, please review again.

@xiaoxiang781216 Thanks for your efforts and updates.

It seems much easier to check the differences but I still have some comments. In my understanding, this PR is intended to replace binary semaphore usage with nxmutex. However, I can see some other changes like critical section with nxsem (e.g. cxd56_i2c.c) and nxsem to nxsem (i.e. cxd56_icc.c). I think that kind of changes should not be mixed in this PR.

nxmutex_t is initialized directly at the variables(g_i2cxdev) definition, not in cxd56_i2cbus_initialize, and then the code can be protected by mutex_t self, so it's redundancy to use the critical section here. That's why this patch remove the critical section too.

[masayuki2009]

nxmutex_t is initialized directly at the variables(g_i2cxdev) definition, not in cxd56_i2cbus_initialize, and then the code can be protected by mutex_t self, so it's redundancy to use the critical section here. That's why this patch remove the critical section too.

@xiaoxiang781216 My point is that such optimization is nothing to do with the original intention. (again, the original intention is just to replace nxsem with nxmutex) and should be in another commit. Otherwise, we confuse the commit.

[xiaoxiang781216]

@masayuki2009 ok, the change split into two patch, please review again.

[masayuki2009]

@masayuki2009 ok, the change split into two patch, please review again.

@xiaoxiang781216 Thanks for the changes. As far as I can see, the first commit still contains several intentions.

For example, in cxd56_emmc.c you did the following changes which just replace emmc_takesem with nxsem_wait_uninterruptible and emmc_givesem with nxsem_post. However, these changes do NOT replace the nxsem with the nxmutex. Actually, I can see similar changes in cxd56_farapi.c and other files. So, these changes should be separated into another commit too.

 static void emmc_cmdstarted(void)
 {
   uint32_t val;
@@ -429,7 +420,7 @@ static void emmc_send(int datatype, uint32_t opcode, uint32_t arg,

   /* Wait for command or data transfer done */

-  ret = emmc_takesem(&g_waitsem);
+  ret = nxsem_wait_uninterruptible(&g_waitsem);
   if (ret < 0)
     {
       return;
@@ -592,8 +583,7 @@ static int emmc_interrupt(int irq, void *context, void *arg)
       ferr("End-bit error/write no CRC.\n");
     }

-  emmc_givesem(&g_waitsem);
-
+  nxsem_post(&g_waitsem);
   return OK;
 }

[xiaoxiang781216]

@masayuki2009 ok, the change split into two patch, please review again.

@xiaoxiang781216 Thanks for the changes. As far as I can see, the first commit still contains several intentions.

For example, in cxd56_emmc.c you did the following changes which just replace emmc_takesem with nxsem_wait_uninterruptible and emmc_givesem with nxsem_post. However, these changes do NOT replace the nxsem with the nxmutex. Actually, I can see similar changes in cxd56_farapi.c and other files. So, these changes should be separated into another commit too.

As I said before, this patch want to distinguish sem_t usage which contain two part:

1. If sem is used as lock, change the variable name to lock and call nxmutex_lock/nxmutex_unlock
2. If sem is used as event/count, keep the variable name and call nxsem_wait_uninterruptible/nxsem_post instead _takesem/_givesem

If without the second part, takesem/givesem give reader expression that sem is still used as lock.

[masayuki2009]

@xiaoxiang781216

As I said before, this patch want to distinguish sem_t usage which contain two part:

I think the two parts are independent and if the commit is split into two commits, it would be easier to read.