jlaitine commented on code in PR #16030: URL: https://github.com/apache/nuttx/pull/16030#discussion_r2014904748
########## libs/libc/semaphore/sem_wait.c: ########## @@ -98,3 +98,43 @@ int sem_wait(FAR sem_t *sem) leave_cancellation_point(); return ERROR; } + +/**************************************************************************** + * Name: nxsem_wait + * + * Description: + * This function attempts to lock the semaphore referenced by 'sem'. If + * the semaphore value is (<=) zero, then the calling task will not return + * until it successfully acquires the lock. + * + * This is an internal OS interface. It is functionally equivalent to + * sem_wait except that: + * + * - It is not a cancellation point, and + * - It does not modify the errno value. + * + * Input Parameters: + * sem - Semaphore descriptor. + * + * Returned Value: + * This is an internal OS interface and should not be used by applications. + * It follows the NuttX internal error return policy: Zero (OK) is + * returned on success. A negated errno value is returned on failure. + * Possible returned errors: + * + * - EINVAL: Invalid attempt to get the semaphore + * - EINTR: The wait was interrupted by the receipt of a signal. + * + ****************************************************************************/ + +#if !defined(CONFIG_BUILD_FLAT) && !defined(__KERNEL__) +int nxsem_wait(FAR sem_t *sem) Review Comment: > > That was my original idea. However: > > > > * Using inline for nxsem_wait, nxsem_trywait and nxsem_post would increase code size significantly. There will still be additional code handling the mutex's holder directly in here as well to further optimize the priority inheritance case. > > I don't see any reason that the code size will increase significantly. nxsem_wait is identical to nxsem_trywait_slow except DEBUGASSERT which is no-op in release build. > I don't understand what you are talking about. If you *inline* sem_wait, sem_trywait and sem_post, obviously the code size will increase since all that code will be included in every compilation unit where these funcitions are used (kernel drivers, kernel nxmutex, libc nxmutex, libc sem.... In addition that will be a mess because of atomic.h including in all of those places. > > * libc and kernel neede different "decorations" - kernel requires DEBUGASSERTs and libc should (IMHO) check if sem != NULL. Putting these into inilne functions would make it messy (would require ifdef **KERNEL** etc..) > > DEBUGASSERT can be called in userspace too, why do you need add `__KERNEL__`? sem_xxx could check sem != NULL before invoking nxsem_xxx, nxsem_xxx could DEBUGASSERT before invoking nsem_xxx_slow. > Yes, but why? You still need two c-files, so why not keep just the common code in the current "inline" and leave the libc specifics in libc c-file and kernel specifics in kernel c-file. > > * That would require the inline functions to be included in all places where nxsem_* functions are used; essentially putting the functions into nuttx/include/semaphore.h. This wouldn't work, because atomic.h can't be in cluded from there, it would break several places, especially with cxx. > > Since nxsem_xxx_slow become the syscall function but nxsem_xxx as the normal function, you can implment nxsem_xxx as non inline function and put them into libs/libc/semaphore/ and remove the version under sched/semaphore/ directly. > > > It is better to have the functions contained in single compilation unit in libc (in memory protected builds) and in kernel. This is better for code size (and cache locality) > > if you really want you can change nxsem_post_impl to static inline function and move it into libs/libc/semaphore/sem_post.c which alredy contain both sem_post and nxsem_post. and move these prototype into include/nuttx/semaphore.h: Of course I can include the _slow prototypes into include/nuttx/semaphore.h. But why? These are now not supposed to be called from anywhere else than the corresponding inline functions. And those inline functions are put into an own header to avoid nasty atomics dependencies. It is easier to include them only in those C files where they are actually used. And you *can't* put the code only into libc. That is the whole point why I made them originally macros and later inline functions. You need them *both in kernel and in libc*. The inline functions are made only for the purpose that the code doesn't need to be duplicated, but it can be included in the corresponding C files, in kernel and in libc. In protected builds, you can't call libc function from kernel or kernel function from libc. You must have nxsem_wait, nxsem_trywait and nxsem_post in BOTH libc AND kernel. -- 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
