xiaoxiang781216 commented on code in PR #6965:
URL: https://github.com/apache/incubator-nuttx/pull/6965#discussion_r985881307
##########
arch/arm/src/cxd56xx/cxd56_udmac.c:
##########
@@ -238,7 +239,7 @@ void cxd56_udmainitialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, CXD56_DMA_NCHANNELS);
Review Comment:
> I think that is not a good way. If I understand correctly we will have a
Kconfig option so users can select either enable or disable by default.
If we enable priority inheritance by default, this simple POSIX compliant
code will panic:
```
#include <pthread.h>
#include <semaphore.h>
#include <unistd.h>
static sem_t g_sem;
static void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
static void *thread2_cb(void *arg)
{
sleep(2);
sem_wait(&g_sem);
return NULL;
}
static void *thread3_cb(void *arg)
{
sleep(1);
sem_post(&g_sem);
sleep(2);
sem_post(&g_sem);
return NULL;
}
int main(int argc, char *argv[])
{
sem_init(&g_sem);
thread1 = pthread_create(thread1_cb...);
thread2 = pthread_create(thread2_cb...);
thread3 = pthread_create(thread3_cb...);
pthread_join(&thread1);
pthread_join(&thread2);
pthread_join(&thread3);
sem_destroy(&g_sem);
return 0;
}
```
semaphore can be used either as lock or as signal. Priority inheritance can
only work well when semaphore is used as lock. So it's wrong to enable it by
default and even worse that the default behavior is controlled by an Kconfig.
> Having explicit calls will break this dependency. Unless we will have
Kconfig applied only for nxmutex. In such a way I agree.
The current behavior break all programs if they don't call NuttX specific
nxsem_set_protocol, when they use semaphore as an event notification or
resource counting.
So, the default behavior must be changed to compliant POSIX standard:
1. Semaphore disable priority inheritance after initialization
2. Caller has to call nxsem_set_protocol(SEM_PRIO_INHERIT) if the semaphore
is used as lock
Of course, the caller is better to use:
1. nxmutex_t or nxrmutex_t as lock for kernel code
2. or pthread_mutex_t as lock for userspace code
All these type will call nxsem_set_protocol(SEM_PRIO_INHERIT) automatically
for caller.
##########
arch/arm/src/cxd56xx/cxd56_udmac.c:
##########
@@ -238,7 +239,7 @@ void cxd56_udmainitialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, CXD56_DMA_NCHANNELS);
Review Comment:
> I think that is not a good way. If I understand correctly we will have a
Kconfig option so users can select either enable or disable by default.
If we enable priority inheritance by default, this simple POSIX compliant
code will panic:
```
#include <pthread.h>
#include <semaphore.h>
#include <unistd.h>
static sem_t g_sem;
static void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
static void *thread2_cb(void *arg)
{
sleep(2);
sem_wait(&g_sem);
return NULL;
}
static void *thread3_cb(void *arg)
{
sleep(1);
sem_post(&g_sem);
sleep(2);
sem_post(&g_sem);
return NULL;
}
int main(int argc, char *argv[])
{
sem_init(&g_sem);
thread1 = pthread_create(thread1_cb...);
thread2 = pthread_create(thread2_cb...);
thread3 = pthread_create(thread3_cb...);
pthread_join(&thread1);
pthread_join(&thread2);
pthread_join(&thread3);
sem_destroy(&g_sem);
return 0;
}
```
Semaphore can be used either as lock or as signal. Priority inheritance can
only work well when semaphore is used as lock. So it's wrong to enable it by
default and even worse that the default behavior is controlled by an Kconfig.
> Having explicit calls will break this dependency. Unless we will have
Kconfig applied only for nxmutex. In such a way I agree.
The current behavior break all programs if they don't call NuttX specific
nxsem_set_protocol, when they use semaphore as an event notification or
resource counting.
So, the default behavior must be changed to compliant POSIX standard:
1. Semaphore disable priority inheritance after initialization
2. Caller has to call nxsem_set_protocol(SEM_PRIO_INHERIT) if the semaphore
is used as lock
Of course, the caller is better to use:
1. nxmutex_t or nxrmutex_t as lock for kernel code
2. or pthread_mutex_t as lock for userspace code
All these type will call nxsem_set_protocol(SEM_PRIO_INHERIT) automatically
for caller.
##########
arch/arm/src/cxd56xx/cxd56_udmac.c:
##########
@@ -238,7 +239,7 @@ void cxd56_udmainitialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, CXD56_DMA_NCHANNELS);
Review Comment:
> I think that is not a good way. If I understand correctly we will have a
Kconfig option so users can select either enable or disable by default.
If we enable priority inheritance by default, this simple POSIX compliant
code will panic:
```
#include <pthread.h>
#include <semaphore.h>
#include <unistd.h>
static sem_t g_sem;
static void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
static void *thread2_cb(void *arg)
{
sleep(2);
sem_wait(&g_sem);
return NULL;
}
static void *thread3_cb(void *arg)
{
sleep(1);
sem_post(&g_sem);
sleep(2);
sem_post(&g_sem);
return NULL;
}
int main(int argc, char *argv[])
{
sem_init(&g_sem);
thread1 = pthread_create(thread1_cb...);
thread2 = pthread_create(thread2_cb...);
thread3 = pthread_create(thread3_cb...);
pthread_join(&thread1);
pthread_join(&thread2);
pthread_join(&thread3);
sem_destroy(&g_sem);
return 0;
}
```
Semaphore can be used either as lock or as signal. Priority inheritance can
only work well when semaphore is used as lock. So it's wrong to enable it by
default and even worse that the default behavior is controlled by an Kconfig.
> Having explicit calls will break this dependency. Unless we will have
Kconfig applied only for nxmutex. In such a way I agree.
The current behavior break all programs if they don't call NuttX specific
nxsem_set_protocol(SEM_PRIO_NONE), when they use semaphore as an event
notification or resource counting.
So, the default behavior must be changed to compliant POSIX standard:
1. Semaphore disable priority inheritance after initialization
2. Caller has to call nxsem_set_protocol(SEM_PRIO_INHERIT) if the semaphore
is used as lock
Of course, the caller is better to use:
1. nxmutex_t or nxrmutex_t as lock for kernel code
2. or pthread_mutex_t as lock for userspace code
All these type will call nxsem_set_protocol(SEM_PRIO_INHERIT) automatically
for caller.
##########
arch/arm/src/cxd56xx/cxd56_udmac.c:
##########
@@ -238,7 +239,7 @@ void cxd56_udmainitialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, CXD56_DMA_NCHANNELS);
Review Comment:
> I think that is not a good way. If I understand correctly we will have a
Kconfig option so users can select either enable or disable by default.
If we enable priority inheritance by default, this simple POSIX compliant
code will panic:
```
#include <pthread.h>
#include <semaphore.h>
#include <unistd.h>
static sem_t g_sem;
static void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
static void *thread2_cb(void *arg)
{
sleep(2);
sem_wait(&g_sem);
return NULL;
}
static void *thread3_cb(void *arg)
{
sleep(1);
sem_post(&g_sem);
sleep(2);
sem_post(&g_sem);
return NULL;
}
int main(int argc, char *argv[])
{
sem_init(&g_sem);
thread1 = pthread_create(thread1_cb...);
thread2 = pthread_create(thread2_cb...);
thread3 = pthread_create(thread3_cb...);
pthread_join(&thread1);
pthread_join(&thread2);
pthread_join(&thread3);
sem_destroy(&g_sem);
return 0;
}
```
Semaphore can be used either as lock or as signal. Priority inheritance can
only work well when semaphore is used as lock. So it's wrong to enable it by
default and even worse that the default behavior is controlled by an Kconfig.
> Having explicit calls will break this dependency. Unless we will have
Kconfig applied only for nxmutex. In such a way I agree.
The current behavior break all programs if they don't call NuttX specific
nxsem_set_protocol(SEM_PRIO_NONE), when they use semaphore as an event
notification or resource counting.
So, the default behavior must be changed to compliant POSIX standard:
1. Semaphore disable priority inheritance after initialization
2. Caller has to call nxsem_set_protocol(SEM_PRIO_INHERIT) if the semaphore
is used as lock
Of course, the caller is better to use:
1. nxmutex_t or nxrmutex_t as lock for kernel code
2. pthread_mutex_t as lock for userspace code
All these type will call nxsem_set_protocol(SEM_PRIO_INHERIT) automatically
for caller.
##########
arch/arm/src/cxd56xx/cxd56_udmac.c:
##########
@@ -238,7 +239,7 @@ void cxd56_udmainitialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, CXD56_DMA_NCHANNELS);
Review Comment:
> I think that is not a good way. If I understand correctly we will have a
Kconfig option so users can select either enable or disable by default.
If we enable priority inheritance by default, this simple POSIX compliant
code will panic:
```
#include <pthread.h>
#include <semaphore.h>
#include <unistd.h>
static sem_t g_sem;
static void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
static void *thread2_cb(void *arg)
{
sleep(2);
sem_wait(&g_sem);
return NULL;
}
static void *thread3_cb(void *arg)
{
sleep(1);
sem_post(&g_sem);
sleep(2);
sem_post(&g_sem);
return NULL;
}
int main(int argc, char *argv[])
{
sem_init(&g_sem);
thread1 = pthread_create(thread1_cb...);
thread2 = pthread_create(thread2_cb...);
thread3 = pthread_create(thread3_cb...);
pthread_join(&thread1);
pthread_join(&thread2);
pthread_join(&thread3);
sem_destroy(&g_sem);
return 0;
}
```
Semaphore can be used either as lock or as signal. Priority inheritance can
only work well when semaphore is used as lock. So it's wrong to enable it by
default and even worse that the default behavior is controlled by an Kconfig.
> Having explicit calls will break this dependency. Unless we will have
Kconfig applied only for nxmutex. In such a way I agree.
The current behavior break all programs if they don't call NuttX specific
nxsem_set_protocol(SEM_PRIO_NONE), when they use semaphore as an event
notification or resource counting.
So, the default behavior must be changed to compliant POSIX standard:
1. Semaphore disable priority inheritance after initialization
2. Caller has to call nxsem_set_protocol(SEM_PRIO_INHERIT) if the semaphore
is used as lock
Of course, the caller is better to use:
1. nxmutex_t or nxrmutex_t as lock for kernel code
2. pthread_mutex_t as lock for userspace code
All these types will call nxsem_set_protocol(SEM_PRIO_INHERIT) automatically
for caller.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
