Skip to content

posix: semaphore: implement named semaphore functions #67007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions MAINTAINERS.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2730,6 +2730,8 @@ POSIX API layer:
status: maintained
maintainers:
- cfriedt
collaborators:
- ycsin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

files:
- include/zephyr/posix/
- lib/posix/
Expand Down
6 changes: 3 additions & 3 deletions doc/services/portability/posix/option_groups/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -367,14 +367,14 @@ POSIX_SEMAPHORES
:header: API, Supported
:widths: 50,10

sem_close(),
sem_close(),yes
sem_destroy(),yes
sem_getvalue(),yes
sem_init(),yes
sem_open(),
sem_open(),yes
sem_post(),yes
sem_trywait(),yes
sem_unlink(),
sem_unlink(),yes
sem_wait(),yes

.. _posix_option_group_spin_locks:
Expand Down
5 changes: 5 additions & 0 deletions include/zephyr/posix/semaphore.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,18 @@
extern "C" {
#endif

#define SEM_FAILED ((sem_t *) 0)

int sem_destroy(sem_t *semaphore);
int sem_getvalue(sem_t *ZRESTRICT semaphore, int *ZRESTRICT value);
int sem_init(sem_t *semaphore, int pshared, unsigned int value);
int sem_post(sem_t *semaphore);
int sem_timedwait(sem_t *ZRESTRICT semaphore, struct timespec *ZRESTRICT abstime);
int sem_trywait(sem_t *semaphore);
int sem_wait(sem_t *semaphore);
sem_t *sem_open(const char *name, int oflags, ...);
int sem_unlink(const char *name);
int sem_close(sem_t *sem);

#ifdef __cplusplus
}
Expand Down
8 changes: 8 additions & 0 deletions lib/posix/Kconfig.semaphore
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,11 @@ config SEM_VALUE_MAX
range 1 32767
help
Maximum semaphore count in POSIX compliant Application.

config SEM_NAMELEN_MAX
int "Maximum name length"
default 16
range 2 255
help
Maximum length of name for a named semaphore.
The max value of 255 corresponds to {NAME_MAX}.
236 changes: 236 additions & 0 deletions lib/posix/semaphore.c
Original file line number Diff line number Diff line change
@@ -1,11 +1,78 @@
/*
* Copyright (c) 2018 Intel Corporation
* Copyright (c) 2023 Meta
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <errno.h>
#include <zephyr/kernel.h>
#include <zephyr/sys/atomic.h>
#include <zephyr/posix/fcntl.h>
#include <zephyr/posix/pthread.h>
#include <zephyr/posix/semaphore.h>

struct nsem_obj {
sys_snode_t snode;
sem_t sem;
int ref_count;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npitre I've modified other part that uses ref_count to int as well, and changed the overflow test from __ASSERT_NO_MSG(nsem->ref_count != UINT_MAX) to __ASSERT_NO_MSG(nsem->ref_count != INT_MAX)

char *name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could avoid malloc if allocated like this

char name[CONFIG_SEM_NAMELEN_MAX];

I think we have this pattern in many places in the code space.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna gather more comments for this one, doing so will allocate a fixed sized array regardless of the actual length of the name.

Alternatively we can allocate the buffer for the nsem_obj and the name array in one go and the total memory allocated will depend on the name length. Ideally we would be able to k_realloc the buffer once the sem is unlinked, however the k_realloc isn't available (#41151), so I opted for the current implementation as the name buffer can be freed separately upon sem_unlink.

Copy link
Member

@cfriedt cfriedt Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say a good follow-up PR would be to add static allocation support.

I'm ok with dynamic going in first and static coming in later, but I think both use cases are valuable for Zephyr.

The existing pattern that doesn't use malloc should be fine for filling the static gap later. Thanks for your help ❤️

};

/* Initialize the list */
static sys_slist_t nsem_list = SYS_SLIST_STATIC_INIT(&nsem_list);

static K_MUTEX_DEFINE(nsem_mutex);

static inline void nsem_list_lock(void)
{
k_mutex_lock(&nsem_mutex, K_FOREVER);
}

static inline void nsem_list_unlock(void)
{
k_mutex_unlock(&nsem_mutex);
}

static struct nsem_obj *nsem_find(const char *name)
{
struct nsem_obj *nsem;

SYS_SLIST_FOR_EACH_CONTAINER(&nsem_list, nsem, snode) {
if ((nsem->name != NULL) && (strcmp(nsem->name, name) == 0)) {
return nsem;
}
}

return NULL;
}

/* Clean up a named semaphore object completely (incl its `name` buffer) */
static void nsem_cleanup(struct nsem_obj *nsem)
{
if (nsem != NULL) {
if (nsem->name != NULL) {
k_free(nsem->name);
}
k_free(nsem);
}
}

/* Remove a named semaphore if it isn't unsed */
static void nsem_unref(struct nsem_obj *nsem)
{
nsem->ref_count -= 1;
__ASSERT(nsem->ref_count >= 0, "ref_count may not be negative");

if (nsem->ref_count == 0) {
__ASSERT(nsem->name == NULL, "ref_count is 0 but sem is not unlinked");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npitre This means that the following behavior is guaranteed only when CONFIG_ASSERT is enabled, is that intended?

If the semaphore has not been removed with a successful call to sem_unlink(), then sem_close() has no effect on the state of the semaphore.

i.e.: a named semaphore can be closed without being unlinked:

operation                refcount change          refcount state
----------------------------------------------------------------

sem_open()               = 2 (created)            2
sem_close()              - 1                      1
sem_close()              - 1                      0


sys_slist_find_and_remove(&nsem_list, (sys_snode_t *) nsem);

/* Free nsem */
nsem_cleanup(nsem);
}
}

/**
* @brief Destroy semaphore.
Expand Down Expand Up @@ -148,3 +215,172 @@ int sem_wait(sem_t *semaphore)
(void)k_sem_take(semaphore, K_FOREVER);
return 0;
}

sem_t *sem_open(const char *name, int oflags, ...)
{
va_list va;
mode_t mode;
unsigned int value;
struct nsem_obj *nsem = NULL;
size_t namelen;

va_start(va, oflags);
BUILD_ASSERT(sizeof(mode_t) <= sizeof(int));
mode = va_arg(va, int);
value = va_arg(va, unsigned int);
va_end(va);

if (value > CONFIG_SEM_VALUE_MAX) {
errno = EINVAL;
return (sem_t *)SEM_FAILED;
}

if (name == NULL) {
errno = EINVAL;
return (sem_t *)SEM_FAILED;
}

namelen = strlen(name);
if ((namelen + 1) > CONFIG_SEM_NAMELEN_MAX) {
errno = ENAMETOOLONG;
return (sem_t *)SEM_FAILED;
}

/* Lock before checking to make sure that the call is atomic */
nsem_list_lock();

/* Check if the named semaphore exists */
nsem = nsem_find(name);

if (nsem != NULL) { /* Named semaphore exists */
if (((oflags & O_CREAT) != 0) && ((oflags & O_EXCL) != 0)) {
errno = EEXIST;
goto error_unlock;
}

__ASSERT_NO_MSG(nsem->ref_count != INT_MAX);
nsem->ref_count++;
goto unlock;
}

/* Named sempahore doesn't exist, try to create new one */

if ((oflags & O_CREAT) == 0) {
errno = ENOENT;
goto error_unlock;
}

nsem = k_calloc(1, sizeof(struct nsem_obj));
if (nsem == NULL) {
errno = ENOSPC;
goto error_unlock;
}

/* goto `cleanup_error_unlock` past this point to avoid memory leak */

nsem->name = k_calloc(namelen + 1, sizeof(uint8_t));
if (nsem->name == NULL) {
errno = ENOSPC;
goto cleanup_error_unlock;
}

strcpy(nsem->name, name);

/* 1 for this open instance, +1 for the linked name */
nsem->ref_count = 2;

(void)k_sem_init(&nsem->sem, value, CONFIG_SEM_VALUE_MAX);

sys_slist_append(&nsem_list, (sys_snode_t *)&(nsem->snode));

goto unlock;

cleanup_error_unlock:
nsem_cleanup(nsem);

error_unlock:
nsem = NULL;

unlock:
nsem_list_unlock();
return nsem == NULL ? SEM_FAILED : &nsem->sem;
}

int sem_unlink(const char *name)
{
int ret = 0;
struct nsem_obj *nsem;

if (name == NULL) {
errno = EINVAL;
return -1;
}

if ((strlen(name) + 1) > CONFIG_SEM_NAMELEN_MAX) {
errno = ENAMETOOLONG;
return -1;
}

nsem_list_lock();

/* Check if queue already exists */
nsem = nsem_find(name);
if (nsem == NULL) {
ret = -1;
errno = ENOENT;
goto unlock;
}

k_free(nsem->name);
nsem->name = NULL;
nsem_unref(nsem);

unlock:
nsem_list_unlock();
return ret;
}

int sem_close(sem_t *sem)
{
struct nsem_obj *nsem = CONTAINER_OF(sem, struct nsem_obj, sem);

if (sem == NULL) {
errno = EINVAL;
return -1;
}

nsem_list_lock();
nsem_unref(nsem);
nsem_list_unlock();
return 0;
}

#ifdef CONFIG_ZTEST
/* Used by ztest to get the ref count of a named semaphore */
int nsem_get_ref_count(sem_t *sem)
{
struct nsem_obj *nsem = CONTAINER_OF(sem, struct nsem_obj, sem);
int ref_count;

__ASSERT_NO_MSG(sem != NULL);
__ASSERT_NO_MSG(nsem != NULL);

nsem_list_lock();
ref_count = nsem->ref_count;
nsem_list_unlock();

return ref_count;
}

/* Used by ztest to get the length of the named semaphore */
size_t nsem_get_list_len(void)
{
size_t len;

nsem_list_lock();
len = sys_slist_len(&nsem_list);
nsem_list_unlock();

return len;
}
#endif
Loading