You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "pussuw (via GitHub)" <gi...@apache.org> on 2023/11/23 13:56:34 UTC
[PR] Sem to libc [nuttx]
pussuw opened a new pull request, #11257:
URL: https://github.com/apache/nuttx/pull/11257
## Summary
This moves the POSIX regulated parts of semaphores into libc, i.e. cancellation points and errno handling, meaning some of the work can now be done directly in user space without kernel involvement.
## Impact
Architecturally quite significant, otherwise none (assuming no regressions)
## Testing
rv-virt:nsh64 (ostest) / knsh64 / ksmp64 (what can be tested)
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824581063
> > The `_SEM_XX` macros are used from libnx/ , can someone advise which version should be used there ? I assume we want to use the nxsem_ versions so the graphics APIs don't turn into cancellation points ?
>
> _SEM_XX need to be kept since many libxxx can be called from both kernel and userspace.
Yes but this is exactly my question, is any API from libxxx allowed to become a cancellation point via e.g. sem_wait ? What is wrong with just calling the nxsem_ version always from libxxx ?
Now the nxsem_ APIs are all available to libc
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824724210
> _SEM_XX need to be kept since many libxxx can be called from both kernel and userspace.
Sorry, I was not in context and didn't respond well when @xiaoxiang781216 asked me to comment.
There is an unresolved probably withe_SEM_XX in the FLAT build most. Since there is only one copy of libc in that configuration and it will use sem_wait() with the cancellation point in all contexts. That is wrong.
However, it works perfectly in the PROTECTED and KERNEL builds where there are two versions of libc, one for applications and one for the kernel. The kernel version works well in the PROTECTED and KERNEL builds.
Using sem_wait() in the libc is wrong most of the time for application calls because it often creates cancellation points where they are forbidden by POSIX. Calling sem_wait() from within in the OS is always wrong since when called from within the kernel.
Calling nxsem_wait() is usually correct in both spaces provided that the libc function is not relying on sem_wait() to provide the cancellation point (which would be bad, very coupled design).
So I think I agree with you. Calling nxsem_wait() from libc always is the cleaner solution if possible (and also fixes the existing problems in regard to using sem_wait()).
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1827577440
The size difference is almost none, tried with a few targets, pre is before this change, post is after:
rv-virt:nsh64
```
ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ riscv64-unknown-elf-size nuttx_pre
text data bss dec hex filename
175879 697 11024 187600 2dcd0 nuttx_pre
ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ riscv64-unknown-elf-size nuttx_post
text data bss dec hex filename
175811 697 11024 187532 2dc8c nuttx_post
```
sabre-6quad:nsh
```
ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ arm-none-eabi-size nuttx_pre
text data bss dec hex filename
135798 356 24272 160426 272aa nuttx_pre
ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ arm-none-eabi-size nuttx_post
text data bss dec hex filename
135750 356 24272 160378 2727a nuttx_post
ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$
```
icicle:knsh (kernel mode target with system calls)
```
ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ riscv64-unknown-elf-size nuttx
text data bss dec hex filename
1194078 33652 27160 1254890 1325ea nuttx
ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ riscv64-unknown-elf-size nuttx_post
text data bss dec hex filename
1193518 33652 27160 1254330 1323ba nuttx_post
```
The application image size for kernel mode targets will be a bit larger, since libc is duplicated / statically linked to each binary.
@PetervdPerk-NXP was there some specific target or some specific bloaty flags you wanted me to use. The standard bloaty ouput is basically the same as using size.
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824542253
@pussuw besides ostest, it is important to test using the LTP suite to confirm everything is working!
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1826349537
@xiaoxiang781216 I think it is a good idea to wait! NuttX is increasing very fast and I'm afraid that we cannot fit on MCUs with 32KB Flash anymore!
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "PetervdPerk-NXP (via GitHub)" <gi...@apache.org>.
PetervdPerk-NXP commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1826354772
@acassis wouldn't it be great if we can automate a size impact summary in github-actions.
I've seen other projects doing this as well for example https://github.com/prusa3d/Prusa-Firmware/pull/4487#issuecomment-1804580759
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1826037881
> @acassis do we have the LTP testsuite readily integrated, or how do people typically run it ? I have ran some of the test cases manually but that is quite a bit of manual work.
>
> I noticed that nuttx-apps has an application called ltp, and this can be run with the sim:posix_test target. A few questions though:
>
> * Is there a script that runs all of the tests ?
>
> * Are those tests run as part of CI ?
Yes, please take a look at sim/sim/sim/configs/posix_test
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824575887
> The `_SEM_XX` macros are used from libnx/ , can someone advise which version should be used there ? I assume we want to use the nxsem_ versions so the graphics APIs don't turn into cancellation points ?
_SEM_XX need to be kept since many libxxx can be called from both kernel and userspace.
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #11257:
URL: https://github.com/apache/nuttx/pull/11257
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1825226969
> > There is an unresolved probably withe_SEM_XX in the FLAT build most. Since there is only one copy of libc in that configuration and it will use sem_wait() with the cancellation point in all contexts. That is wrong.
>
> sem_wait() also sets the errno value. nxsem_wait() does not. If all calls to _SEM_WAIT are changed to nxsem_wait() we also need to be certain that the calling libc function does not depend on sem_wait() to set the errno value _only_ when called from the application space.
Thanks, the most important reason for me doing this change is to get rid of the _SEM macros, if those cannot be removed the work in this PR is futile and the whole PR should just be closed.
I already changed all _SEM_XX calls from lib_mutex to nxsem_ calls, no errno in use there. I'll change the remaining references and ensure they don't depend on errno either. I don't think this should be the case anyway.
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1825577700
> It is still not clear to me how we have ran LTP tests in the past ?
>
> * Do people create their own environments where the tests are run ?
Yes, Xiaomi run LTP test internally
> * Do we have something ready made ourselves ? I found testing/ltp in nuttx-apps, what is that ?
You can try https://github.com/apache/nuttx/blob/master/boards/sim/sim/sim/configs/posix_test/defconfig
testing/ltp contain the full LTP source code
> * If so, does CI perform the tests automatically ?
You need run the test programs which is huge (1000+) manually now, we plan to enable LTP test in github action in the next couple month.
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824546069
@acassis do we have the LTP testsuite readily integrated, or how do people typically run it ?
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1825914162
> Can you include a bloaty output of the total size change of this PR.
Yes!
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824482423
Continuation of https://github.com/apache/nuttx/pull/11165
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824678276
since the kernel caller don't want to enable cancellation or change error, but the user space caller want. @patacongo could you give more input?
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1825430451
It is still not clear to me how we have ran LTP tests in the past ?
- Do people create their own environments where the tests are run ?
- Do we have something ready made ourselves ? I found testing/ltp in nuttx-apps, what is that ?
- If so, does CI perform the tests automatically ?
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1826235254
should we merge this patch or wait @pussuw 's size report?
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1825621082
> > It is still not clear to me how we have ran LTP tests in the past ?
> >
> > * Do people create their own environments where the tests are run ?
>
> Yes, Xiaomi run LTP test internally
>
> > * Do we have something ready made ourselves ? I found testing/ltp in nuttx-apps, what is that ?
>
> You can try https://github.com/apache/nuttx/blob/master/boards/sim/sim/sim/configs/posix_test/defconfig testing/ltp contain the full LTP source code
>
> > * If so, does CI perform the tests automatically ?
>
> You need run the test programs which is huge (1000+) manually now, we plan to enable LTP test in github action in the next couple month.
Thanks for the reply, so it is not trivial to run those tests right now since I don't have any ready made test framework for this. Manually testing all of the cases is out of the question.
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1826248530
I'll provide the info on Monday @xiaoxiang781216
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824557642
The `_SEM_XX` macros are used from libnx/ , can someone advise which version should be used there ? I assume we want to use the nxsem_ versions so the graphics APIs don't turn into cancellation points ?
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824693683
> Yes but this is exactly my question, is any API from libxxx allowed to become a cancellation point via e.g. sem_wait ? What is wrong with just calling the nxsem_ version (i.e. the system call) always from libxxx ?
POSIX specifies exactly which APIS _must_ be cancellation and all specifies that any other cancellation point function other than those is forbidden. See 2.9.5 Thread Cancellation in https://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_09.html
_"An implementation shall not introduce cancellation points into any other functions specified in this volume of IEEE Std 1003.1-2001."_
If cancellation points can be introduced in any other way, that would be a serious POSIX violation.
--
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
Re: [PR] sched/semaphore: Move POSIX regulated parts of semaphores into libc [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on PR #11257:
URL: https://github.com/apache/nuttx/pull/11257#issuecomment-1824733215
> There is an unresolved probably withe_SEM_XX in the FLAT build most. Since there is only one copy of libc in that configuration and it will use sem_wait() with the cancellation point in all contexts. That is wrong.
sem_wait() also sets the errno value. nxsem_wait() does not. If all calls to _SEM_WAIT are changed to nxsem_wait() we also need to be certain that the the calling libc function does not depend on sem_wait() to set the 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