You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/05/24 14:44:26 UTC
[GitHub] [incubator-nuttx] anjiahao1 opened a new pull request, #6320: use rmutex inside of all repeated implementation
anjiahao1 opened a new pull request, #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320
Signed-off-by: anjiahao <an...@xiaomi.com>
## Impact
maybe Slightly reduced szie and increased speed
## Testing
CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r880655857
##########
drivers/1wire/1wire.c:
##########
@@ -221,37 +219,7 @@ static int onewire_pm_prepare(FAR struct pm_callback_s *cb, int domain,
int onewire_sem_wait(FAR struct onewire_master_s *master)
Review Comment:
ditto
##########
drivers/1wire/1wire.c:
##########
@@ -124,9 +122,9 @@ static inline void onewire_sem_init(FAR struct onewire_sem_s *sem)
*
****************************************************************************/
-static inline void onewire_sem_destroy(FAR struct onewire_sem_s *sem)
+static inline void onewire_sem_destroy(FAR rmutex_t *lock)
Review Comment:
maybe can replace `onewire_sem_destroy` with `nxrmutex_destroy` in all the places? Or do a define
##########
drivers/1wire/1wire.c:
##########
@@ -110,11 +110,9 @@ static inline uint32_t onewire_leuint32(uint32_t x)
*
****************************************************************************/
-static inline void onewire_sem_init(FAR struct onewire_sem_s *sem)
+static inline void onewire_sem_init(FAR rmutex_t *lock)
Review Comment:
maybe can replace `onewire_sem_init` with `nxrmutex_init` in all the places? Or do a define
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882628608
##########
include/nuttx/mutex.h:
##########
@@ -214,7 +224,7 @@ static inline bool nxmutex_is_locked(FAR mutex_t *mutex)
static inline int nxmutex_unlock(FAR mutex_t *mutex)
{
- return nxsem_post(mutex);
+ return _SEM_POST(mutex);
Review Comment:
@pkarashchenko nxmutex is designed for kernel only, the userspace should use pthread_mutex_t instead, so we don't need call _SEM_POST/_SEM_WAIT in this file, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883842743
##########
include/nuttx/mutex.h:
##########
@@ -113,7 +120,14 @@ static inline int nxmutex_init(FAR mutex_t *mutex)
static inline int nxmutex_destroy(FAR mutex_t *mutex)
{
- return nxsem_destroy(mutex);
+ int ret = _SEM_DESTROY(mutex);
+
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
Yes. True. We have two of those:
```
# define _SEM_ERRNO(r) (-(r))
# define _SEM_ERRVAL(r) (r)
```
You are completely right, again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#issuecomment-1139898465
Sounds like a plan :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anjiahao1 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
anjiahao1 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882589012
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
you mean add `DEBUGASSERT` to `nxmutex_locl` ？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#issuecomment-1139897160
> @xiaoxiang781216 I see that we are attempting to solve the mutex issue with "small blood", but IMO we need to look deeper. We have priority inheritance in place that should be applied to mutex only, but is implemented at semaphore level (that I believe is totally wrong), so we have holder for mutex and holder for semaphore in case of priority inheritance that at the moment of time when mutex is held are the same process IDs. I truly believe that we need to implement nxmutex similar to nxsem as a separate kernel object and locate priority inheritance there, so we will "kill two rabbits with a single shot". Until then we will suffer from dozens of corner cases and complicating logic to cover it.
Yes, I agree that it isn't good to use semaphore both for the signal and lock. That's why we add nxmutex_t and nxrmutex_t:
1. Implement the recursive lock(nxmutex_t) and migrate all usage to it
2. Migrate all unrecursive sem lock/unlock to mutex_t
With the above change, we can get the clean code base that:
1. All code(except the implementation of [pthread_|nxr]mutex_t) use [nx]sem_t for the signal only
2. All code use [pthread_|nxr]mutex_t as the lock
The next steps are:
1. Change the default protocol from SEM_PRIO_INHERIT to SEM_PRIO_NONE
2. Remove sem_setprotocol(&sem, SEM_PRIO_NONE) from the code base
3. Add sem_setprotocol(&sem, SEM_PRIO_INHERIT) to the implementation of [pthread_|nxr]mutex_t)
After this, all priority inheritance should get fixed. Of course, we can move the implementation of priority inheritance from sem_t to mutex_t.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883807701
##########
include/nuttx/mutex.h:
##########
@@ -113,7 +120,14 @@ static inline int nxmutex_init(FAR mutex_t *mutex)
static inline int nxmutex_destroy(FAR mutex_t *mutex)
{
- return nxsem_destroy(mutex);
+ int ret = _SEM_DESTROY(mutex);
+
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
We need to decide what the nxmutex is. If it is an internal API, the all internal APIs use negative return codes and current code is ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882530936
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +174,7 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ return _SEM_TRYWAIT(mutex);
Review Comment:
```suggestion
int ret;
ret = _SEM_TRYWAIT(mutex);
if (ret < 0)
{
return _SEM_ERRNO(ret);
}
return ret;
```
##########
include/nuttx/mutex.h:
##########
@@ -138,7 +138,17 @@ static inline int nxmutex_destroy(FAR mutex_t *mutex)
static inline int nxmutex_lock(FAR mutex_t *mutex)
{
- return nxsem_wait_uninterruptible(mutex);
+ int ret;
+
+ do
+ {
+ /* Take the semaphore (perhaps waiting) */
+
+ ret = _SEM_WAIT(mutex);
+ }
+ while (ret == -EINTR);
+
+ return ret;
Review Comment:
```suggestion
int ret;
do
{
/* Take the semaphore (perhaps waiting) */
ret = _SEM_WAIT(mutex);
}
while (ret < 0 && _SEM_ERRNO(ret) == EINTR);
return ret < 0 ? _SEM_ERRNO(ret) : ret;
```
##########
include/nuttx/mutex.h:
##########
@@ -214,7 +224,7 @@ static inline bool nxmutex_is_locked(FAR mutex_t *mutex)
static inline int nxmutex_unlock(FAR mutex_t *mutex)
{
- return nxsem_post(mutex);
+ return _SEM_POST(mutex);
Review Comment:
```suggestion
int ret;
ret = _SEM_POST(mutex);
if (ret < 0)
{
return _SEM_ERRNO(ret);
}
return ret;
```
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
need to check `ECANCELED` path for mutex. I think it is safe, but need to verify
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881249717
##########
drivers/rptun/rptun.c:
##########
@@ -210,54 +209,12 @@ static METAL_DECLARE_LIST(g_rptun_priv);
static int rptun_lock(void)
Review Comment:
```suggestion
#define rptun_lock() nxrmutex_lock(&g_rptunlock)
```
##########
drivers/rptun/rptun.c:
##########
@@ -210,54 +209,12 @@ static METAL_DECLARE_LIST(g_rptun_priv);
static int rptun_lock(void)
{
- pid_t me = getpid();
- int ret = OK;
-
- /* Does this thread already hold the semaphore? */
-
- if (g_holder == me)
- {
- /* Yes.. just increment the reference count */
-
- g_count++;
- }
- else
- {
- /* No.. take the semaphore (perhaps waiting) */
-
- ret = nxsem_wait_uninterruptible(&g_rptunlock);
- if (ret >= 0)
- {
- /* Now this thread holds the semaphore */
-
- g_holder = me;
- g_count = 1;
- }
- }
-
- return ret;
+ return nxrmutex_lock(&g_rptunlock);
}
static void rptun_unlock(void)
Review Comment:
```suggestion
#define rptun_unlock() nxrmutex_unlock(&g_rptunlock)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883361902
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
- UNUSED(ret);
- }
-
- /* No we hold the semaphore */
-
- g_modlock.holder = me;
- g_modlock.count = 1;
- }
+ nxrmutex_lock(&g_modlock);
Review Comment:
most likely we need to
```suggestion
while ((ret = nxrmutex_lock(&g_modlock)) < 0)
{
DEBUGASSERT(ret == -ECANCELED);
}
```
##########
include/nuttx/mutex.h:
##########
@@ -89,7 +89,14 @@ extern "C"
static inline int nxmutex_init(FAR mutex_t *mutex)
{
- return nxsem_init(mutex, 0, 1);
+ int ret = _SEM_INIT(mutex, 0, 1);
+
+ if (ret < 0)
+ {
+ return _SEM_ERRNO(ret);
Review Comment:
```suggestion
return -(_SEM_ERRNO(ret));
```
I mean that we need to define what is the return value of `nxmutex_init` and others. Is it `errno` value or `-errno` value
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883362325
##########
include/nuttx/mutex.h:
##########
@@ -89,7 +89,14 @@ extern "C"
static inline int nxmutex_init(FAR mutex_t *mutex)
{
- return nxsem_init(mutex, 0, 1);
+ int ret = _SEM_INIT(mutex, 0, 1);
+
+ if (ret < 0)
+ {
+ return _SEM_ERRNO(ret);
Review Comment:
```suggestion
return -(_SEM_ERRNO(ret));
```
I mean that we need to define what is the return value of `nxmutex_init` and others. Is it `errno` value or `-errno` value. Most likely it is negative
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anjiahao1 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
anjiahao1 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881408940
##########
libs/libc/netdb/lib_dnsinit.c:
##########
@@ -140,56 +137,24 @@ bool dns_initialize(void)
* Name: dns_semtake
*
* Description:
- * Take the DNS semaphore, ignoring errors due to the receipt of signals.
+ * Take the DNS lock, ignoring errors due to the receipt of signals.
*
****************************************************************************/
void dns_semtake(void)
Review Comment:
g_dns_lock is gloal
##########
net/route/net_fileroute.c:
##########
@@ -597,36 +593,10 @@ int net_routesize_ipv6(void)
#ifdef CONFIG_ROUTE_IPv4_FILEROUTE
int net_lockroute_ipv4(void)
Review Comment:
is a globl
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
Review Comment:
g_modlock is a globl
##########
include/nuttx/mutex.h:
##########
@@ -29,13 +29,14 @@
#include <errno.h>
#include <assert.h>
#include <unistd.h>
-#include <nuttx/sched.h>
#include <nuttx/semaphore.h>
Review Comment:
i do it ,but have a complie error about
##########
fs/aio/aio_initialize.c:
##########
@@ -119,53 +117,12 @@ void aio_initialize(void)
int aio_lock(void)
Review Comment:
g_aio_lock is a global variable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882530936
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +174,7 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ return _SEM_TRYWAIT(mutex);
Review Comment:
```suggestion
int ret;
ret = _SEM_TRYWAIT(mutex);
if (ret < 0)
{
return -(_SEM_ERRNO(ret));
}
return ret;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anjiahao1 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
anjiahao1 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882589012
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
you mean add `DEBUGASSERT` to `nxmutex` ？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883458571
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
ECANCELED/EINTR is better catch and ignore inside nxrmutex_lock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883831448
##########
include/nuttx/mutex.h:
##########
@@ -113,7 +120,14 @@ static inline int nxmutex_init(FAR mutex_t *mutex)
static inline int nxmutex_destroy(FAR mutex_t *mutex)
{
- return nxsem_destroy(mutex);
+ int ret = _SEM_DESTROY(mutex);
+
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
Yes, but _SEM_ERRVAL(ret) return the negative value too, but more simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883397865
##########
libs/libc/stdio/lib_libfilesem.c:
##########
@@ -1,126 +0,0 @@
-/****************************************************************************
- * libs/libc/stdio/lib_libfilesem.c
- *
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership. The
- * ASF licenses this file to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance with the
- * License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
- * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
- * License for the specific language governing permissions and limitations
- * under the License.
- *
- ****************************************************************************/
-
-/****************************************************************************
- * Included Files
- ****************************************************************************/
-
-#include <nuttx/config.h>
-
-#include <sys/types.h>
-#include <unistd.h>
-#include <errno.h>
-#include <assert.h>
-
-#include <nuttx/semaphore.h>
-#include <nuttx/fs/fs.h>
-
-#include "libc.h"
-
-#ifndef CONFIG_STDIO_DISABLE_BUFFERING
-
-/****************************************************************************
- * Public Functions
- ****************************************************************************/
-
-/****************************************************************************
- * lib_sem_initialize
- ****************************************************************************/
-
-void lib_sem_initialize(FAR struct file_struct *stream)
-{
- /* Initialize the LIB semaphore to one (to support one-at-a-time access
- * to private data sets.
- */
-
- _SEM_INIT(&stream->fs_sem, 0, 1);
-
- stream->fs_holder = -1;
- stream->fs_counts = 0;
-}
-
-/****************************************************************************
- * lib_take_semaphore
- ****************************************************************************/
-
-void lib_take_semaphore(FAR struct file_struct *stream)
Review Comment:
Seems like we still need `lib_take_semaphore` and `DEBUGASSERT(ret == -ECANCELED);`
##########
net/route/net_fileroute.c:
##########
@@ -636,36 +606,10 @@ int net_lockroute_ipv4(void)
#ifdef CONFIG_ROUTE_IPv6_FILEROUTE
int net_lockroute_ipv6(void)
{
- pid_t me = getpid();
- int ret;
-
- /* Are we already the holder of the lock? */
-
- if (g_ipv6_holder == me)
- {
- /* Yes.. just increment the count of locks held */
-
- g_ipv6_count++;
- ret = OK;
- }
- else
+ int ret = nxrmutex_lock(&g_ipv6_lock);
+ if (ret < 0)
{
- /* No.. wait to get the lock */
-
- ret = nxsem_wait(&g_ipv6_exclsem);
Review Comment:
Here just a note. Previously it was possible to return from `net_lockroute_ipv6` with `EINTR`, but now we wait uninterruptable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883395764
##########
libs/libc/netdb/lib_dnsinit.c:
##########
@@ -140,56 +137,24 @@ bool dns_initialize(void)
* Name: dns_semtake
*
* Description:
- * Take the DNS semaphore, ignoring errors due to the receipt of signals.
+ * Take the DNS lock, ignoring errors due to the receipt of signals.
*
****************************************************************************/
void dns_semtake(void)
{
- pid_t me = getpid();
- int errcode = 0;
- int ret;
-
- /* Does this thread already hold the semaphore? */
-
- if (g_dns_holder == me)
- {
- /* Yes.. just increment the reference count */
-
- g_dns_count++;
- return;
- }
-
- do
- {
- ret = _SEM_WAIT(&g_dns_sem);
- if (ret < 0)
- {
- errcode = _SEM_ERRNO(ret);
- DEBUGASSERT(errcode == EINTR || errcode == ECANCELED);
- }
- }
- while (ret < 0 && errcode == EINTR);
-
- g_dns_holder = me;
- g_dns_count = 1;
+ nxrmutex_lock(&g_dns_lock);
Review Comment:
Please add a loop and `DEBUGASSERT(ret == -ECANCELED);`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883394498
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +174,7 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ return _SEM_TRYWAIT(mutex);
Review Comment:
Do you plan to integrate this suggestion?
##########
include/nuttx/mutex.h:
##########
@@ -214,7 +224,7 @@ static inline bool nxmutex_is_locked(FAR mutex_t *mutex)
static inline int nxmutex_unlock(FAR mutex_t *mutex)
{
- return nxsem_post(mutex);
+ return _SEM_POST(mutex);
Review Comment:
```suggestion
int ret;
ret = _SEM_POST(mutex);
if (ret < 0)
{
return -(_SEM_ERRNO(ret));
}
return ret;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881923294
##########
include/nuttx/mutex.h:
##########
@@ -38,6 +37,7 @@
#define MUTEX_INITIALIZER SEM_INITIALIZER(1)
#define RMUTEX_INITIALIZER {SEM_INITIALIZER(1), INVALID_PROCESS_ID, 0}
+#define RMUTEX_NO_HOLDER (pid_t)-1
Review Comment:
```suggestion
#define RMUTEX_NO_HOLDER (pid_t)-1
#define MUTEX_INITIALIZER SEM_INITIALIZER(1)
#define RMUTEX_INITIALIZER {SEM_INITIALIZER(1), RMUTEX_NO_HOLDER, 0}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r884285772
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +190,15 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ int ret;
+
+ ret = _SEM_TRYWAIT(mutex);
Review Comment:
```
int ret;
for (; ; )
{
/* Try to take the semaphore */
ret = _SEM_TRYWAIT(mutex);
if (ret >= 0)
{
break;
}
ret = _SEM_ERRVAL(ret);
if (ret != -EINTR && ret != -ECANCELED)
{
break;
}
}
return ret;
```
##########
fs/tmpfs/fs_tmpfs.c:
##########
@@ -55,27 +55,12 @@
# warning CONFIG_FS_TMPFS_FILE_FREEGUARD needs to be > ALLOCGUARD
#endif
-#define tmpfs_lock_file(tfo) \
- (tmpfs_lock_object((FAR struct tmpfs_object_s *)tfo))
-#define tmpfs_lock_directory(tdo) \
- (tmpfs_lock_object((FAR struct tmpfs_object_s *)tdo))
-#define tmpfs_unlock_file(tfo) \
- (tmpfs_unlock_object((FAR struct tmpfs_object_s *)tfo))
-#define tmpfs_unlock_directory(tdo) \
- (tmpfs_unlock_object((FAR struct tmpfs_object_s *)tdo))
-
Review Comment:
It's better to keep these macros:
```
#define tmpfs_lock(fs) \
nxrmutex_lock(&fs->tfs_lock)
#define tmpfs_lock_file(tfo) \
nxrmutex_lock(&tfo->tfo_lock)
#define tmpfs_lock_directory(tdo) \
nxrmutex_lock(&tdo->tdo_lock)
#define tmpfs_unlock(fs) \
nxrmutex_unlock(&fs->tfs_lock)
#define tmpfs_unlock_file(tfo) \
nxrmutex_unlock(&tfo->tfo_lock)
#define tmpfs_unlock_directory(tdo) \
nxrmutex_unlock(&tdo->tdo_lock)
```
to avoid the cast spread in the whole source code.
##########
include/nuttx/mutex.h:
##########
@@ -138,7 +152,19 @@ static inline int nxmutex_destroy(FAR mutex_t *mutex)
static inline int nxmutex_lock(FAR mutex_t *mutex)
{
- return nxsem_wait_uninterruptible(mutex);
+ int ret;
+
+ do
+ {
Review Comment:
change to:
```
int ret;
for (; ; )
{
/* Take the semaphore (perhaps waiting) */
ret = _SEM_WAIT(mutex);
if (ret >= 0)
{
break;
}
ret = _SEM_ERRVAL(ret);
if (ret != -EINTR && ret != -ECANCELED)
{
break;
}
}
return ret;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r884532547
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +197,26 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ int ret;
+
+ for (; ; )
Review Comment:
But the caller doesn't expect to receive -EINTR/-ECANCEL since nxmutext_lock doesn't return them too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881508666
##########
include/nuttx/fs/fs.h:
##########
@@ -34,6 +34,7 @@
#include <stdbool.h>
#include <time.h>
+#include <nuttx/mutex.h>
Review Comment:
Yeah... Seems that the easiest way is to remove `#include <nuttx/sched.h>` from `mutex.h`. But let's also avoid usage of `INVALID_PROCESS_ID` in `mutex.h` and use `#define MUTEX_NO_HOLDER (pid_t)-1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883415932
##########
net/route/net_fileroute.c:
##########
@@ -636,36 +606,10 @@ int net_lockroute_ipv4(void)
#ifdef CONFIG_ROUTE_IPv6_FILEROUTE
int net_lockroute_ipv6(void)
{
- pid_t me = getpid();
- int ret;
-
- /* Are we already the holder of the lock? */
-
- if (g_ipv6_holder == me)
- {
- /* Yes.. just increment the count of locks held */
-
- g_ipv6_count++;
- ret = OK;
- }
- else
+ int ret = nxrmutex_lock(&g_ipv6_lock);
+ if (ret < 0)
{
- /* No.. wait to get the lock */
-
- ret = nxsem_wait(&g_ipv6_exclsem);
Review Comment:
@xiaoxiang781216 what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883857522
##########
libs/libc/stdio/lib_libfilesem.c:
##########
@@ -62,36 +59,11 @@ void lib_sem_initialize(FAR struct file_struct *stream)
void lib_take_semaphore(FAR struct file_struct *stream)
{
- pid_t my_pid = getpid();
int ret;
- /* Do I already have the semaphore? */
-
- if (stream->fs_holder == my_pid)
- {
- /* Yes, just increment the number of references that I have */
-
- stream->fs_counts++;
- }
- else
+ while ((ret = nxrmutex_lock(&stream->fs_lock)) < 0)
Review Comment:
We need to check for `ECANCELED`. This could be placed inside the lock
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anjiahao1 commented on pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
anjiahao1 commented on PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#issuecomment-1138560304
maybe we need `MUTEX_INITIALIZER` -> `NXMUTEX_INITIALIZER`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#issuecomment-1138565935
Wow, that is something new. Need to check mainline builds
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881434405
##########
fs/tmpfs/fs_tmpfs.c:
##########
@@ -55,6 +55,11 @@
# warning CONFIG_FS_TMPFS_FILE_FREEGUARD needs to be > ALLOCGUARD
#endif
+#define tmpfs_lock(fs) nxrmutex_lock(&(fs)->tfs_lock)
Review Comment:
Optional, but now seems that all wrappers can be just replaced with `nxrmutex_lock`/`nxrmutex_unlock`, so we do not need defines at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881431353
##########
include/nuttx/mutex.h:
##########
@@ -29,13 +29,14 @@
#include <errno.h>
#include <assert.h>
#include <unistd.h>
-#include <nuttx/sched.h>
#include <nuttx/semaphore.h>
Review Comment:
seems no issue now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anjiahao1 commented on pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
anjiahao1 commented on PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#issuecomment-1138129859
What is the difference between `_SEM_DOSTRY ` and`nxrmutex_destroy`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anjiahao1 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
anjiahao1 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883402723
##########
net/route/net_fileroute.c:
##########
@@ -636,36 +606,10 @@ int net_lockroute_ipv4(void)
#ifdef CONFIG_ROUTE_IPv6_FILEROUTE
int net_lockroute_ipv6(void)
{
- pid_t me = getpid();
- int ret;
-
- /* Are we already the holder of the lock? */
-
- if (g_ipv6_holder == me)
- {
- /* Yes.. just increment the count of locks held */
-
- g_ipv6_count++;
- ret = OK;
- }
- else
+ int ret = nxrmutex_lock(&g_ipv6_lock);
+ if (ret < 0)
{
- /* No.. wait to get the lock */
-
- ret = nxsem_wait(&g_ipv6_exclsem);
Review Comment:
maybe we can add a new api like `nxmutex_lock_interruptible` and `nxrmutex_lock_interruptible`？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#issuecomment-1138294431
>  What is the difference between `_SEM_DOSTRY ` and`nxrmutex_destroy`
```
#if !defined(CONFIG_BUILD_FLAT) && defined(__KERNEL__)
# define _SEM_DESTROY(s) nxsem_destroy(s)
#else
# define _SEM_DESTROY(s) sem_destroy(s)
#endif
```
So the libc layer should be `_SEM_DESTROY` and not `nxsem_destroy`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r884522681
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +197,26 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ int ret;
+
+ for (; ; )
Review Comment:
I think we do not need `for` in `trylock`
##########
fs/tmpfs/fs_tmpfs.c:
##########
@@ -612,9 +514,8 @@ static FAR struct tmpfs_file_s *tmpfs_alloc_file(void)
tfo->tfo_size = 0;
tfo->tfo_data = NULL;
- tfo->tfo_exclsem.ts_holder = getpid();
- tfo->tfo_exclsem.ts_count = 1;
- nxsem_init(&tfo->tfo_exclsem.ts_sem, 0, 0);
+ nxrmutex_init(&tfo->tfo_lock);
+ nxrmutex_lock(&tfo->tfo_lock);
Review Comment:
```suggestion
tmpfs_lock_file(tfo);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anjiahao1 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
anjiahao1 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r884574529
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +197,26 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ int ret;
+
+ for (; ; )
Review Comment:
yes,you are right,sem_trywait not return this errcode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r884571885
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +197,26 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ int ret;
+
+ for (; ; )
Review Comment:
I need to check, but if I recall correctly those error codes are not returned by trywait.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r884736120
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +197,26 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ int ret;
+
+ for (; ; )
Review Comment:
If so, let's keep the simple one. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882606915
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
I mean that `modlib_registry_lock` spins in while until success and return code was checked vs `ECANCELED`. The `EINTR` is handled inside the `nxrmutex_lock`, so the question is only about `ECANCELED` and `DEBUGASSERT`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882617263
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
I mean that it is not a question to you, but more. POSIX does not define `ECANCELED` error code for `sem_wait`, so I will start a thread in a mailing list to clarify why NuttX violate standard in this case
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
I mean that it is not a question to you, but more about that POSIX does not define `ECANCELED` error code for `sem_wait`, so I will start a thread in a mailing list to clarify why NuttX violate standard in this case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881285838
##########
net/route/net_fileroute.c:
##########
@@ -597,36 +593,10 @@ int net_routesize_ipv6(void)
#ifdef CONFIG_ROUTE_IPv4_FILEROUTE
int net_lockroute_ipv4(void)
Review Comment:
we can `#define net_lockroute_ipv4() nxrmutex_lock(&g_ipv4_lock)`
##########
libs/libc/stdio/lib_libfilesem.c:
##########
@@ -46,14 +46,11 @@
void lib_sem_initialize(FAR struct file_struct *stream)
Review Comment:
all this APIs can be changed to defines
```
# define lib_sem_initialize(s) nxrmutex_init(&(s)->fs_lock)
# define lib_take_semaphore(s) nxrmutex_lock(&(s)->fs_lock)
# define lib_give_semaphore(s) nxrmutex_unlock(&(s)->fs_lock)
```
##########
fs/tmpfs/fs_tmpfs.c:
##########
@@ -193,50 +191,13 @@ const struct mountpt_operations tmpfs_operations =
* Private Functions
****************************************************************************/
-/****************************************************************************
- * Name: tmpfs_lock_reentrant
- ****************************************************************************/
-
-static int tmpfs_lock_reentrant(FAR struct tmpfs_sem_s *sem)
-{
- pid_t me;
- int ret = OK;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == sem->ts_holder)
- {
- /* Yes... just increment the count */
-
- sem->ts_count++;
- DEBUGASSERT(sem->ts_count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- ret = nxsem_wait_uninterruptible(&sem->ts_sem);
- if (ret >= 0)
- {
- /* No we hold the semaphore */
-
- sem->ts_holder = me;
- sem->ts_count = 1;
- }
- }
-
- return ret;
-}
-
/****************************************************************************
* Name: tmpfs_lock
****************************************************************************/
static int tmpfs_lock(FAR struct tmpfs_s *fs)
Review Comment:
can be a define
##########
libs/libc/netdb/lib_dnsinit.c:
##########
@@ -140,56 +137,24 @@ bool dns_initialize(void)
* Name: dns_semtake
*
* Description:
- * Take the DNS semaphore, ignoring errors due to the receipt of signals.
+ * Take the DNS lock, ignoring errors due to the receipt of signals.
*
****************************************************************************/
void dns_semtake(void)
Review Comment:
Can be a `#define dns_semtake() nxrmutex_lock(&g_dns_lock)`
##########
drivers/syslog/syslog_device.c:
##########
@@ -129,38 +124,7 @@ static const uint8_t g_syscrlf[2] =
static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
Review Comment:
```
#define syslog_dev_takesem(s) nxrmutex_lock(&(s)->sl_lock)
```
##########
fs/inode/fs_inode.c:
##########
@@ -138,23 +90,5 @@ int inode_semtake(void)
void inode_semgive(void)
{
- DEBUGASSERT(g_inode_sem.holder == getpid());
-
- /* Is this our last count on the semaphore? */
-
- if (g_inode_sem.count > 1)
- {
- /* No.. just decrement the count */
-
- g_inode_sem.count--;
- }
-
- /* Yes.. then we can really release the semaphore */
-
- else
- {
- g_inode_sem.holder = NO_HOLDER;
- g_inode_sem.count = 0;
- nxsem_post(&g_inode_sem.sem);
- }
+ nxrmutex_unlock(&g_inode_lock);
Review Comment:
```suggestion
DEBUGVERIFY(nxrmutex_unlock(&g_inode_lock));
```
This is to get back `DEBUGASSERT(g_inode_sem.holder == getpid());`
##########
include/nuttx/mutex.h:
##########
@@ -29,13 +29,14 @@
#include <errno.h>
#include <assert.h>
#include <unistd.h>
-#include <nuttx/sched.h>
#include <nuttx/semaphore.h>
Review Comment:
```suggestion
#include <nuttx/sched.h>
#include <nuttx/semaphore.h>
```
##########
drivers/syslog/syslog_device.c:
##########
@@ -169,15 +133,7 @@ static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
static inline void syslog_dev_givesem(FAR struct syslog_dev_s *syslog_dev)
Review Comment:
```
#define syslog_dev_givesem(s) DEBUGVERIFY(nxrmutex_unlock(&(s)->sl_lock))
```
##########
include/nuttx/mutex.h:
##########
@@ -29,13 +29,14 @@
#include <errno.h>
#include <assert.h>
#include <unistd.h>
-#include <nuttx/sched.h>
#include <nuttx/semaphore.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+#define INVALID_PROCESS_ID (pid_t)-1
+#define MUTEX_INITIALIZER SEM_INITIALIZER(1);
Review Comment:
```suggestion
```
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
Review Comment:
can be a define
##########
fs/aio/aio_initialize.c:
##########
@@ -119,53 +117,12 @@ void aio_initialize(void)
int aio_lock(void)
Review Comment:
can be a define
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881429009
##########
drivers/usbhost/usbhost_max3421e.c:
##########
@@ -1136,59 +1135,12 @@ static int max3421e_takesem(FAR sem_t *sem)
static int max3421e_take_exclsem(FAR struct max3421e_usbhost_s *priv)
Review Comment:
this can be a define
##########
drivers/rptun/rptun.c:
##########
@@ -53,6 +54,9 @@
# define ALIGN_UP(s, a) (((s) + (a) - 1) & ~((a) - 1))
#endif
+#define rptun_lock() nxrmutex_lock(&g_rptunlock)
+#define rptun_unlock() nxrmutex_unlock(&g_rptunlock)
+
#define RPTUNIOC_NONE 0
#define NO_HOLDER (INVALID_PROCESS_ID)
Review Comment:
```suggestion
```
##########
drivers/rptun/rptun.c:
##########
@@ -53,6 +54,9 @@
# define ALIGN_UP(s, a) (((s) + (a) - 1) & ~((a) - 1))
#endif
+#define rptun_lock() nxrmutex_lock(&g_rptunlock)
+#define rptun_unlock() nxrmutex_unlock(&g_rptunlock)
Review Comment:
```suggestion
#define rptun_lock() nxrmutex_lock(&g_rptunlock)
#define rptun_unlock() nxrmutex_unlock(&g_rptunlock)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883739082
##########
include/nuttx/mutex.h:
##########
@@ -138,7 +152,19 @@ static inline int nxmutex_destroy(FAR mutex_t *mutex)
static inline int nxmutex_lock(FAR mutex_t *mutex)
{
- return nxsem_wait_uninterruptible(mutex);
+ int ret;
+
+ do
+ {
+ /* Take the semaphore (perhaps waiting) */
+
+ ret = _SEM_WAIT(mutex);
+ }
+ while (ret < 0 &&
+ (_SEM_ERRNO(ret) == EINTR ||
+ _SEM_ERRNO(ret) == ECANCELED));
+
+ return ret < 0 ? -(_SEM_ERRNO(ret)) : ret;
Review Comment:
_SEM_ERRVAL(ret)
##########
libs/libc/stdio/lib_libfilesem.c:
##########
@@ -62,36 +59,11 @@ void lib_sem_initialize(FAR struct file_struct *stream)
void lib_take_semaphore(FAR struct file_struct *stream)
{
- pid_t my_pid = getpid();
int ret;
- /* Do I already have the semaphore? */
-
- if (stream->fs_holder == my_pid)
- {
- /* Yes, just increment the number of references that I have */
-
- stream->fs_counts++;
- }
- else
+ while ((ret = nxrmutex_lock(&stream->fs_lock)) < 0)
Review Comment:
let's remove the while loop
##########
include/nuttx/mutex.h:
##########
@@ -214,7 +248,15 @@ static inline bool nxmutex_is_locked(FAR mutex_t *mutex)
static inline int nxmutex_unlock(FAR mutex_t *mutex)
{
- return nxsem_post(mutex);
+ int ret;
+
+ ret = _SEM_POST(mutex);
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
_SEM_ERRVAL(ret)
##########
include/nuttx/mutex.h:
##########
@@ -164,7 +190,15 @@ static inline int nxmutex_lock(FAR mutex_t *mutex)
static inline int nxmutex_trylock(FAR mutex_t *mutex)
{
- return nxsem_trywait(mutex);
+ int ret;
+
+ ret = _SEM_TRYWAIT(mutex);
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
_SEM_ERRVAL(ret)
##########
include/nuttx/mutex.h:
##########
@@ -89,7 +89,14 @@ extern "C"
static inline int nxmutex_init(FAR mutex_t *mutex)
{
- return nxsem_init(mutex, 0, 1);
+ int ret = _SEM_INIT(mutex, 0, 1);
+
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
_SEM_ERRVAL(ret)
##########
include/nuttx/mutex.h:
##########
@@ -113,7 +120,14 @@ static inline int nxmutex_init(FAR mutex_t *mutex)
static inline int nxmutex_destroy(FAR mutex_t *mutex)
{
- return nxsem_destroy(mutex);
+ int ret = _SEM_DESTROY(mutex);
+
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
_SEM_ERRVAL(ret)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883837620
##########
include/nuttx/mutex.h:
##########
@@ -113,7 +120,14 @@ static inline int nxmutex_init(FAR mutex_t *mutex)
static inline int nxmutex_destroy(FAR mutex_t *mutex)
{
- return nxsem_destroy(mutex);
+ int ret = _SEM_DESTROY(mutex);
+
+ if (ret < 0)
+ {
+ return -(_SEM_ERRNO(ret));
Review Comment:
Let me check. Latest time I was looking into it I think it was returning positive value
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883394908
##########
include/nuttx/mutex.h:
##########
@@ -214,7 +224,7 @@ static inline bool nxmutex_is_locked(FAR mutex_t *mutex)
static inline int nxmutex_unlock(FAR mutex_t *mutex)
{
- return nxsem_post(mutex);
+ return _SEM_POST(mutex);
Review Comment:
Do you plan to integrate this suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r882607879
##########
libs/libc/modlib/modlib_registry.c:
##########
@@ -82,40 +68,7 @@ static FAR struct module_s *g_mod_registry;
void modlib_registry_lock(void)
{
- pid_t me;
- int ret;
-
- /* Do we already hold the semaphore? */
-
- me = getpid();
- if (me == g_modlock.holder)
- {
- /* Yes... just increment the count */
-
- g_modlock.count++;
- DEBUGASSERT(g_modlock.count > 0);
- }
-
- /* Take the semaphore (perhaps waiting) */
-
- else
- {
- while ((ret = _SEM_WAIT(&g_modlock.lock)) < 0)
- {
- /* The only case that an error should occur here is if
- * the wait was awakened by a signal.
- */
-
- DEBUGASSERT(_SEM_ERRNO(ret) == EINTR ||
- _SEM_ERRNO(ret) == ECANCELED);
Review Comment:
The same for `dns_semtake`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883455283
##########
net/route/net_fileroute.c:
##########
@@ -636,36 +606,10 @@ int net_lockroute_ipv4(void)
#ifdef CONFIG_ROUTE_IPv6_FILEROUTE
int net_lockroute_ipv6(void)
{
- pid_t me = getpid();
- int ret;
-
- /* Are we already the holder of the lock? */
-
- if (g_ipv6_holder == me)
- {
- /* Yes.. just increment the count of locks held */
-
- g_ipv6_count++;
- ret = OK;
- }
- else
+ int ret = nxrmutex_lock(&g_ipv6_lock);
+ if (ret < 0)
{
- /* No.. wait to get the lock */
-
- ret = nxsem_wait(&g_ipv6_exclsem);
Review Comment:
Except the code error, it isn't expect to interrupt the lock. I believe the interrupt is used to break out the waiting
of an event, not a lock In most case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r883455283
##########
net/route/net_fileroute.c:
##########
@@ -636,36 +606,10 @@ int net_lockroute_ipv4(void)
#ifdef CONFIG_ROUTE_IPv6_FILEROUTE
int net_lockroute_ipv6(void)
{
- pid_t me = getpid();
- int ret;
-
- /* Are we already the holder of the lock? */
-
- if (g_ipv6_holder == me)
- {
- /* Yes.. just increment the count of locks held */
-
- g_ipv6_count++;
- ret = OK;
- }
- else
+ int ret = nxrmutex_lock(&g_ipv6_lock);
+ if (ret < 0)
{
- /* No.. wait to get the lock */
-
- ret = nxsem_wait(&g_ipv6_exclsem);
Review Comment:
Except the code error, it isn't expect to interrupt the lock. I think the interrupt is used for event not lock In most case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r895219754
##########
drivers/syslog/syslog_device.c:
##########
@@ -123,63 +120,6 @@ static const uint8_t g_syscrlf[2] =
* Private Functions
****************************************************************************/
-/****************************************************************************
- * Name: syslog_dev_takesem
- ****************************************************************************/
-
-static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
-{
- pid_t me = getpid();
- int ret;
-
- /* Does this thread already hold the semaphore? That could happen if
- * we were called recursively, i.e., if the logic kicked off by
- * file_write() where to generate more debug output. Return an
- * error in that case.
- */
-
- if (syslog_dev->sl_holder == me)
- {
- /* Return an error (instead of deadlocking) */
-
- return -EWOULDBLOCK;
- }
-
- /* Either the semaphore is available or is currently held by another
- * thread. Wait for it to become available.
- */
-
- ret = nxsem_wait(&syslog_dev->sl_sem);
- if (ret < 0)
- {
- return ret;
- }
-
- /* We hold the semaphore. We can safely mark ourself as the holder
- * of the semaphore.
- */
-
- syslog_dev->sl_holder = me;
- return OK;
-}
Review Comment:
@xiaoxiang781216 I observe deadlock after I migrate my application to the latest NuttX master. I narrow down the issue and see that it happens when multiple tasks try to use `syslog`. I have a syslog configuration with 2 channels:
1. Channel attached to console
2. Channel attached to file on `/mnt/sdcard0/nuttx.log`
When the issue happens I observe next picture:
I've noticed that before this change `syslog_dev_takesem` returned `return -EWOULDBLOCK;` with a comment `/* Return an error (instead of deadlocking) */` and seems like this functionality has been broken now.
In general I think that we rush too much with replacing of semaphores with mutexes. For example the FLAT build functioning changed after this change since all `nxsem_` API calls became replaced with `sem_` API calls and introduced unnecessary cancellation points in kernel. I'm not sure about the side effect of this, but still investigating.
I think we need to revert part of the changes from this PR especially syslog part and part related to replacement in libc and get back `nxmutex_` to use `nxsem_` APIs only and use it in kernel part only for now. Then make a step-by-step change to user space.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r895222450
##########
drivers/syslog/syslog_device.c:
##########
@@ -123,63 +120,6 @@ static const uint8_t g_syscrlf[2] =
* Private Functions
****************************************************************************/
-/****************************************************************************
- * Name: syslog_dev_takesem
- ****************************************************************************/
-
-static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
-{
- pid_t me = getpid();
- int ret;
-
- /* Does this thread already hold the semaphore? That could happen if
- * we were called recursively, i.e., if the logic kicked off by
- * file_write() where to generate more debug output. Return an
- * error in that case.
- */
-
- if (syslog_dev->sl_holder == me)
- {
- /* Return an error (instead of deadlocking) */
-
- return -EWOULDBLOCK;
- }
-
- /* Either the semaphore is available or is currently held by another
- * thread. Wait for it to become available.
- */
-
- ret = nxsem_wait(&syslog_dev->sl_sem);
- if (ret < 0)
- {
- return ret;
- }
-
- /* We hold the semaphore. We can safely mark ourself as the holder
- * of the semaphore.
- */
-
- syslog_dev->sl_holder = me;
- return OK;
-}
Review Comment:
I hope I did all correctly in https://github.com/apache/incubator-nuttx/pull/6414
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r881502596
##########
include/nuttx/fs/fs.h:
##########
@@ -34,6 +34,7 @@
#include <stdbool.h>
#include <time.h>
+#include <nuttx/mutex.h>
Review Comment:
Here is the source of your build error. You are having circular dependency here. `#include <nuttx/sched.h>` -> `#include <nuttx/fs/fs.h>` -> `#include <nuttx/mutex.h>` -> `#include <nuttx/sched.h>`.
Now we need to think what to do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org