You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "tmedicci (via GitHub)" <gi...@apache.org> on 2025/03/11 12:45:18 UTC
[PR] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
tmedicci opened a new pull request, #15971:
URL: https://github.com/apache/nuttx/pull/15971
## Summary
* fs/vfs/fs_poll: Add noinstrument_function to poll_notify function
Add the noinstrument_function attribute to the poll_notify function to avoid it looping if -finstrument-functions is set to the fs/vfs files.
## Impact
Impact on user:YES. Prevent a failure when instrumentation is enabled.
Impact on build: NO.
Impact on hardware: NO.
Impact on documentation: NO.
Impact on security: NO.
Impact on compatibility: NO.
## Testing
First of all, enable instrumentation for fs/vfs files with the following patch:
```
diff --git a/fs/vfs/Make.defs b/fs/vfs/Make.defs
index 30292d787f..e00b361407 100644
--- a/fs/vfs/Make.defs
+++ b/fs/vfs/Make.defs
@@ -22,6 +22,8 @@
# Common file/socket descriptor support
+CFLAGS += -finstrument-functions
+
CSRCS += fs_chstat.c fs_close.c fs_dup.c fs_dup2.c fs_fcntl.c fs_epoll.c
CSRCS += fs_fchstat.c fs_fstat.c fs_fstatfs.c fs_ioctl.c fs_lseek.c
CSRCS += fs_mkdir.c fs_open.c fs_poll.c fs_pread.c fs_pwrite.c fs_read.c
```
Then, take the `esp32s3-devkit:tickless` default config and enable instrumentation and build it with the following commands:
### Building
```
make -j distclean && ./tools/configure.sh esp32s3-devkit:tickless && \
kconfig-tweak -e CONFIG_SCHED_INSTRUMENTATION && \
kconfig-tweak -e CONFIG_SCHED_INSTRUMENTATION_DUMP && \
kconfig-tweak -e CONFIG_SCHED_INSTRUMENTATION_FILTER && \
kconfig-tweak -e CONFIG_SCHED_INSTRUMENTATION_FUNCTION && \
kconfig-tweak -e CONFIG_SYSTEM_TRACE && \
kconfig-tweak -e CONFIG_SYSTEM_SYSTEM && \
kconfig-tweak --set-val CONFIG_SYSTEM_NSH_PRIORITY 102 && \
kconfig-tweak -e CONFIG_DRIVERS_NOTE && \
kconfig-tweak -e CONFIG_DRIVERS_NOTECTL && \
kconfig-tweak -e CONFIG_DRIVERS_NOTERAM && \
kconfig-tweak --set-val CONFIG_DRIVERS_NOTERAM_BUFSIZE 32768 && \
kconfig-tweak -e CONFIG_DRIVERS_NOTERAM_DEFAULT_NOOVERWRITE && \
make olddefconfig && make flash EXTRAFLAGS="-Wno-cpp -Werror" ESPTOOL_PORT=/dev/ttyUSB0 ESPTOOL_BINDIR=./ -s -j$(nproc) && xtensa-esp32s3-elf-objdump -D nuttx > objdump.flat && minicom -D /dev/ttyUSB0
```
### Running
#### Before this PR
Running it without this PR, the device fails to boot properly. It halts with the following serial log:
```
ESP-ROM:esp32s3-20210327
Build:Mar 27 2021
rst:0x1 (POWERON),boot:0x8 (SPI_FAST_FLASH_BOOT)
SPIWP:0xee
mode:DIO, clock div:2
load:0x3fc94350,len:0xdec
load:0x40374000,len:0x539c
SHA-256 comparison failed:
Calculated: 662c19f6d3c8aa629f7a49122ddff861ccb85ccca96f9d347cd4082718ea9f31
Expected: 00000000309e0000000000000000000000000000000000000000000000000000
Attempting to boot anyway...
entry 0x40374df4
*** Booting NuttX ***
dram: lma 0x00000020 vma 0x3fc94350 len 0xdec (3564)
iram: lma 0x00000e14 vma 0x40374000 len 0x539c (21404)
padd: lma 0x000061c8 vma 0x00000000 len 0x9e30 (40496)
imap: lma 0x00010000 vma 0x42010000 len 0x20cc4 (134340)
padd: lma 0x00030ccc vma 0x00000000 len 0xf32c (62252)
dmap: lma 0x00040000 vma 0x3c050000 len 0x9834 (38964)
total segments stored 6
```
By debugging it using GDB + OpenOCD, it can be seen that the function [`poll_notify`](https://github.com/apache/incubator-nuttx/blob/4e70cc1e696fd6de36da69019411869aeb2bfa73/fs/vfs/fs_poll.c#L275) keeps looping after instrumentation is enabled because this function itself is being instrumented. Check the following GDB backtrace at [`noteram_add`](https://github.com/apache/incubator-nuttx/blob/f22b93b337c2f9de457904596688a04bd1ce12e5/drivers/note/noteram_driver.c#L691):
```
Thread 1 "esp32s3.cpu0" hit Breakpoint 1, noteram_add (driver=0x3fc94b9c <g_note_drivers>, note=0x1, notelen=1070109696) at note/noteram_driver.c:693
+bt
#0 noteram_add (driver=0x3fc94b9c <g_note_drivers>, note=0x1, notelen=1070109696) at note/noteram_driver.c:693
#1 0x42014a98 in sched_note_event_ip (tag=0, ip=1107422816, event=31 '\037', buf=0x0, len=0)
at note/note_driver.c:1524
#2 0x42014534 in note_driver_instrument_enter (this_fn=0x4201ee60 <poll_notify>,
call_site=0x42014981 <noteram_add+225>, arg=0x0) at note/note_initialize.c:60
#3 0x40379331 in __cyg_profile_func_enter (this_fn=0x4201ee60 <poll_notify>, call_site=0x42014981 <noteram_add+225>)
at misc/lib_instrument.c:70
#4 0x4201ee85 in poll_notify (afds=0x3fc94b98 <g_noteram_driver+48>, nfds=1, eventset=1) at vfs/fs_poll.c:282
#5 0x42014981 in noteram_add (driver=0x3fc94b68 <g_noteram_driver>, note=<optimized out>, notelen=20)
at note/noteram_driver.c:741
#6 0x42014a98 in sched_note_event_ip (tag=0, ip=1107422816, event=31 '\037', buf=0x0, len=0)
at note/note_driver.c:1524
#7 0x42014534 in note_driver_instrument_enter (this_fn=0x4201ee60 <poll_notify>, call_site=0x42014981 <noteram_add+225>, arg=0x0) at note/note_initialize.c:60
#8 0x40379331 in __cyg_profile_func_enter (this_fn=0x4201ee60 <poll_notify>, call_site=0x42014981 <noteram_add+225>) at misc/lib_instrument.c:70
#9 0x4201ee85 in poll_notify (afds=0x3fc94b98 <g_noteram_driver+48>, nfds=1, eventset=1) at vfs/fs_poll.c:282
#10 0x42014981 in noteram_add (driver=0x3fc94b68 <g_noteram_driver>, note=<optimized out>, notelen=20) at note/noteram_driver.c:741
#11 0x42014a98 in sched_note_event_ip (tag=0, ip=1107422816, event=31 '\037', buf=0x0, len=0) at note/note_driver.c:1524
#12 0x42014534 in note_driver_instrument_enter (this_fn=0x4201ee60 <poll_notify>, call_site=0x42014981 <noteram_add+225>, arg=0x0) at note/note_initialize.c:60
#13 0x40379331 in __cyg_profile_func_enter (this_fn=0x4201ee60 <poll_notify>, call_site=0x42014981 <noteram_add+225>) at misc/lib_instrument.c:70
#14 0x4201ee85 in poll_notify (afds=0x3fc94b98 <g_noteram_driver+48>, nfds=1, eventset=1) at vfs/fs_poll.c:282
#15 0x42014981 in noteram_add (driver=0x3fc94b68 <g_noteram_driver>, note=<optimized out>, notelen=20) at note/noteram_driver.c:741
#16 0x42014a98 in sched_note_event_ip (tag=0, ip=1107422816, event=31 '\037', buf=0x0, len=0) at note/note_driver.c:1524
#17 0x42014534 in note_driver_instrument_enter (this_fn=0x4201ee60 <poll_notify>, call_site=0x42014981 <noteram_add+225>, arg=0x0) at note/note_initialize.c:60
#18 0x40379331 in __cyg_profile_func_enter (this_fn=0x4201ee60 <poll_notify>, call_site=0x42014981 <noteram_add+225>) at misc/lib_instrument.c:70
#19 0x4201ee85 in poll_notify (afds=0x3fc94b98 <g_noteram_driver+48>, nfds=1, eventset=1) at vfs/fs_poll.c:282
#20 0x42014981 in noteram_add (driver=0x3fc94b68 <g_noteram_driver>, note=<optimized out>, notelen=20) at note/noteram_driver.c:741
#21 0x42014a98 in sched_note_event_ip (tag=0, ip=1107428816, event=31 '\037', buf=0x0, len=0) at note/note_driver.c:1524
#22 0x42014534 in note_driver_instrument_enter (this_fn=0x420205d0 <register_driver>, call_site=0x42014999 <noteram_register+21>, arg=0x0) at note/note_initialize.c:60
#23 0x40379331 in __cyg_profile_func_enter (this_fn=0x420205d0 <register_driver>, call_site=0x42014999 <noteram_register+21>) at misc/lib_instrument.c:70
#24 0x420205ee in register_driver (path=0x3c0501bb "/dev/note/ram", fops=0x3c0501e0 <g_noteram_fops>, mode=438, priv=0x3fc94b68 <g_noteram_driver>) at driver/fs_registerdriver.c:68
#25 0x42014999 in noteram_register () at note/noteram_driver.c:1318
#26 0x4201454a in note_initialize () at note/note_initialize.c:153
#27 0x42014487 in drivers_initialize () at drivers_initialize.c:152
#28 0x42012186 in nx_start () at init/nx_start.c:697
#29 0x40374dee in __esp32s3_start () at chip/esp32s3_start.c:447
#30 0x40374eae in __start () at chip/esp32s3_start.c:500
```
Eventually, the stack would overflow and the firmware will crash. To avoid it, just disable instrumentation for `poll_notify`
### Results
By applying this patch, the device boots successfully:
```
ESP-ROM:esp32s3-20210327
Build:Mar 27 2021
rst:0x1 (POWERON),boot:0x8 (SPI_FAST_FLASH_BOOT)
SPIWP:0xee
mode:DIO, clock div:2
load:0x3fc94350,len:0xdec
load:0x40374000,len:0x539c
SHA-256 comparison failed:
Calculated: 97d616d462ad4ac173f88e9f2f1d2343ee71a5bc0f453d20237d1778180b4939
Expected: 00000000309e0000000000000000000000000000000000000000000000000000
Attempting to boot anyway...
entry 0x40374df4
*** Booting NuttX ***
dram: lma 0x00000020 vma 0x3fc94350 len 0xdec (3564)
iram: lma 0x00000e14 vma 0x40374000 len 0x539c (21404)
padd: lma 0x000061c8 vma 0x00000000 len 0x9e30 (40496)
imap: lma 0x00010000 vma 0x42010000 len 0x20c84 (134276)
padd: lma 0x00030c8c vma 0x00000000 len 0xf36c (62316)
dmap: lma 0x00040000 vma 0x3c050000 len 0x9834 (38964)
total segments stored 6
NuttShell (NSH) NuttX-10.4.0
nsh>
```
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2732548756
> > Thanks for submitting. It helps booting the device, but still gives wrong results. I attached two logs, one with the content of this PR and the other with building #15997 . [trace_vfs_anchao.txt](https://github.com/user-attachments/files/19294159/trace_vfs_anchao.txt) [trace_vfs_tiago.txt](https://github.com/user-attachments/files/19294163/trace_vfs_tiago.txt)
> > They were produced by just dumping the tracing on `/dev/note/ram`. You can check that they have the same number of lines, but the one that implements the recursive lock contains calls to `poll_notify` that "substituted" other calls. For instance, in the first file we have 152 occurrences of `B|1|uio_copyto`, while it occurs only 46 times at the second dump.
> > This is expected because `poll_notify` executes recursively. Consider the following situation illustrated by the following "trace":
> > ```
> > nsh_main-1 [0] 1.870000000: tracing_mark_write: B|1|uio_copyto+0/0x78
> > nsh_main-1 [0] 1.870000000: tracing_mark_write: E|1|uio_copyto+0/0x78
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Now, consider that `poll_notify` is being instrumented and the patch on #15997 applied: when `uio_copyto` is called, it'll set `instr_level_enter` to `1`, but before it's cleaned, [`instrument->enter`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R82) is called, which will end-up calling `poll_notify`, which will trigger `__cyg_profile_func_enter` (the critical section here doesn't prevent nested calls). Fortunately, `instr_level_enter` is set and the function will return, but as soon as `poll_notify` exits, it'll call `__cyg_profile_func_exit`, which will execute as expected (not for `uio_copyto`, but for `poll_notify`). The same mechanism happens when `uio_copyto` clears `instr_level_enter` and `__cyg_profile_func_exit` is called: `poll_notify` will call `__cyg_profile_func_enter` successfully on enter, but `__cyg_profile_func_exit` for `poll_notify` will fail (`instr_level_exit` was set when `uio_copyt
o` exited). That's why we have less (but symetrical) entries for the same function (`uio_copyto` in this case).
> > Basically, the patch is kind of the same I tested here previously: it works for nested functions and enables the device to boot, but gives wrong results if any function called by [`instrument->enter`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R82) or [`instrument->exit`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R121) is being instrumented.
> > #15997 prevents catastrophic failures, but we still need to remove functions from that are called recursively by the instrumentation system from being instrumented. This PR is still necessary.
>
> @tmedicci Thank you so much for your detailed analysis. I have checked the trace results in your attachment. In some scenarios, #15997 indeed requires more detailed demonstration, especially in the context of interrupt or ISR nesting. Let us merge #15971 to address the issues of `poll_notify`. Thank you again for your proactive response for my comments.
Thank you @anchao for reviewing it carefully and contributing to the discussion! #15997 works as expected if none of the functions called by [`instrument->enter`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R82) or [`instrument->exit`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R121) is being instrumented and - more importantly - it prevents infinite recursion. So, it's a valid attempt to try to solve the problem.
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao merged PR #15971:
URL: https://github.com/apache/nuttx/pull/15971
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2725372135
> maybe we can use the same variable in "`instr_level`" and call `atomic_fetch_sub ` to subtract the count in `__cyg_profile_func_exit()`
>
> ```
> __cyg_profile_func_enter
> ->atomic_fetch_add(&tcb->instr_level, 1) == 0
> ->instrument->enter
> ->atomic_set(&tcb->instr_level, 0);
> ```
>
> ```
> __cyg_profile_func_exit
> ->atomic_fetch_sub(&tcb->instr_level, 1) == 1
> ->instrument->leave
> ->atomic_set(&tcb->instr_level, 0);
> ```
>
> I have no objection to this PR, just worried that for the backends enabling the features of other instruments, there may be more functions that need to have the "noinstrument" mark added, and we will also lose track of these functions with such special marks.
>
> Please merge this PR if we don't have a better option.
I tried this change before the one I've suggested (and the device still halts). Either way, the functions that are part of the instrumentation itself should not be instrumented (the same is valid for `__cyg_profile_func_[enter|exist]` which already contains it).
For now, this is the fix that fixes the reported issue without adding any overheads.
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2721294472
> > @xiaoxiang781216 @anjiahao1 I think the correct solution should be to add a nesting counter in the instrument hook to avoid recursion. poll_notify() can only solve the current problem. we cannot predict whether other functions need to add tags.
>
> Can you provide a patch for that, please?
I'm asking you because I didn't fully understand your proposal. Although we can implement a recursion check, I see no reason for instrumenting this function (and I'm not convinced that the recursion check would affect other functions from being called recursively)
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2721466557
> > > @xiaoxiang781216 @anjiahao1 I think the correct solution should be to add a nesting counter in the instrument hook to avoid recursion. poll_notify() can only solve the current problem. we cannot predict whether other functions need to add tags.
> >
> >
> > Can you provide a patch for that, please?
>
> I'm asking you because I didn't fully understand your proposal. Although we can implement a recursion check, I see no reason for instrumenting this function (and I'm not convinced that the recursion check would affect other functions from being called recursively)
```
@@ -56,12 +58,20 @@ __cyg_profile_func_enter(FAR void *this_fn, FAR void *call_site)
{
FAR struct instrument_s *instrument;
FAR sq_entry_t *entry;
+ struct tcb_s *tcb;
if (g_magic != MAIGC_NUMBMER)
{
return;
}
+ tcb = this_task();
+
+ if (atomic_fetch_add(&tcb->instr_level, 1) != 0)
+ {
+ return;
+ }
+
sq_for_every(&g_instrument_queue, entry)
{
instrument = (FAR struct instrument_s *)entry;
@@ -70,6 +80,8 @@ __cyg_profile_func_enter(FAR void *this_fn, FAR void *call_site)
instrument->enter(this_fn, call_site, instrument->arg);
}
}
+
+ atomic_set(&tcb->instr_level, 0);
}
```
Could you tried this change? I don't know if it works.
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2726569771
> > I tried this change before the one I've suggested (and the device still halts). Either way, the functions that are part of the instrumentation itself should not be instrumented (the same is valid for `__cyg_profile_func_[enter|exist]` which already contains it).
>
> I totally disagree with your opinion. It's just that functions that appear in the detection cycle should not be detected, otherwise you will find that more and more functions are added to this list. As a maintainer, we should provide a solution to the framework instead of letting everyone find bugs and solve the problem.
Yes, but the solution you proposed didn't work and my (simpler) solution solved the current known issue. I disagree that a function which is called by the instrumentation subsystem recursively - like `poll_notify` - should be instrumented. It shouldn't.
I understand your concerns, but both approaches are complementary.
> > For now, this is the fix that fixes the reported issue without adding any overheads.
>
> If we want to reduce the overhead, we should not turn on the instrumentation capability. You can check the disassemble code. The overhead of ` __cyg_profile_func_[enter|exist]` is much greater than several checks.
Can you assume this issue and provide a working solution, then? I will be happy to test it.
This PR has 5 approvals and it wasn't merged yet because of this discussion. I don't need my solution to be adopted and merged, but we need a solution for the issue.
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2727223541
> > > I tried this change before the one I've suggested (and the device still halts). Either way, the functions that are part of the instrumentation itself should not be instrumented (the same is valid for `__cyg_profile_func_[enter|exist]` which already contains it).
> >
> >
> > I totally disagree with your opinion. It's just that functions that appear in the detection cycle should not be detected, otherwise you will find that more and more functions are added to this list. As a maintainer, we should provide a solution to the framework instead of letting everyone find bugs and solve the problem.
>
> Yes, but the solution you proposed didn't work and my (simpler) solution solved the current known issue. I disagree that a function which is called by the instrumentation subsystem recursively - like `poll_notify` - should be instrumented. It shouldn't.
>
> I understand your concerns, but both approaches are complementary.
>
> > > For now, this is the fix that fixes the reported issue without adding any overheads.
> >
> >
> > If we want to reduce the overhead, we should not turn on the instrumentation capability. You can check the disassemble code. The overhead of ` __cyg_profile_func_[enter|exist]` is much greater than several checks.
>
> Can you assume this issue and provide a working solution, then? I will be happy to test it.
>
> This PR has 5 approvals and it wasn't merged yet because of this discussion. I don't need my solution to be adopted and merged, but we need a solution for the issue.
try this patch if any helps: https://github.com/apache/nuttx/pull/15997
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2731334540
> Thanks for submitting. It helps booting the device, but still gives wrong results. I attached two logs, one with the content of this PR and the other with building #15997 . [trace_vfs_anchao.txt](https://github.com/user-attachments/files/19294159/trace_vfs_anchao.txt) [trace_vfs_tiago.txt](https://github.com/user-attachments/files/19294163/trace_vfs_tiago.txt)
>
> They were produced by just dumping the tracing on `/dev/note/ram`. You can check that they have the same number of lines, but the one that implements the recursive lock contains calls to `poll_notify` that "substituted" other calls. For instance, in the first file we have 152 occurrences of `B|1|uio_copyto`, while it occurs only 46 times at the second dump.
>
> This is expected because `poll_notify` executes recursively. Consider the following situation illustrated by the following "trace":
>
> ```
> nsh_main-1 [0] 1.870000000: tracing_mark_write: B|1|uio_copyto+0/0x78
> nsh_main-1 [0] 1.870000000: tracing_mark_write: E|1|uio_copyto+0/0x78
> ```
>
> Now, consider that `poll_notify` is being instrumented and the patch on #15997 applied: when `uio_copyto` is called, it'll set `instr_level_enter` to `1`, but before it's cleaned, [`instrument->enter`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R82) is called, which will end-up calling `poll_notify`, which will trigger `__cyg_profile_func_enter` (the critical section here doesn't prevent nested calls). Fortunately, `instr_level_enter` is set and the function will return, but as soon as `poll_notify` exits, it'll call `__cyg_profile_func_exit`, which will execute as expected (not for `uio_copyto`, but for `poll_notify`). The same mechanism happens when `uio_copyto` clears `instr_level_enter` and `__cyg_profile_func_exit` is called: `poll_notify` will call `__cyg_profile_func_enter` successfully on enter, but `__cyg_profile_func_exit` for `poll_notify` will fail (`instr_level_exit` was set when `uio_copyto`
exited). That's why we have less (but symetrical) entries for the same function (`uio_copyto` in this case).
>
> Basically, the patch is kind of the same I tested here previously: it works for nested functions and enables the device to boot, but gives wrong results if any function called by [`instrument->enter`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R82) or [`instrument->exit`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R121) is being instrumented.
>
> #15997 prevents catastrophic failures, but we still need to remove functions from that are called recursively by the instrumentation system from being instrumented. This PR is still necessary.
@tmedicci Thank you so much for your detailed analysis. I have checked the trace results in your attachment. In some scenarios, #15997 indeed requires more detailed demonstration, especially in the context of interrupt or ISR nesting. Let us merge #15971 to address the issues of `poll_notify`. Thank you again for your proactive response for my comments.
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2730549101
> > > > I tried this change before the one I've suggested (and the device still halts). Either way, the functions that are part of the instrumentation itself should not be instrumented (the same is valid for `__cyg_profile_func_[enter|exist]` which already contains it).
> > >
> > >
> > > I totally disagree with your opinion. It's just that functions that appear in the detection cycle should not be detected, otherwise you will find that more and more functions are added to this list. As a maintainer, we should provide a solution to the framework instead of letting everyone find bugs and solve the problem.
> >
> >
> > Yes, but the solution you proposed didn't work and my (simpler) solution solved the current known issue. I disagree that a function which is called by the instrumentation subsystem recursively - like `poll_notify` - should be instrumented. It shouldn't.
> > I understand your concerns, but both approaches are complementary.
> > > > For now, this is the fix that fixes the reported issue without adding any overheads.
> > >
> > >
> > > If we want to reduce the overhead, we should not turn on the instrumentation capability. You can check the disassemble code. The overhead of ` __cyg_profile_func_[enter|exist]` is much greater than several checks.
> >
> >
> > Can you assume this issue and provide a working solution, then? I will be happy to test it.
> > This PR has 5 approvals and it wasn't merged yet because of this discussion. I don't need my solution to be adopted and merged, but we need a solution for the issue.
>
> try this patch if any helps: #15997
Thanks for submitting. It helps booting the device, but still gives wrong results. I attached two logs, one with the content of this PR and the other with building #15997 .
[trace_vfs_anchao.txt](https://github.com/user-attachments/files/19294159/trace_vfs_anchao.txt)
[trace_vfs_tiago.txt](https://github.com/user-attachments/files/19294163/trace_vfs_tiago.txt)
They were produced by just dumping the tracing on `/dev/note/ram`. You can check that they have the same number of lines, but the one that implements the recursive lock contains calls to `poll_notify` that "substituted" other calls. For instance, in the first file we have 152 occurrences of `B|1|uio_copyto`, while it occurs only 46 times at the second dump.
This is expected because `poll_notify` executes recursively. Consider the following situation illustrated by the following "trace":
```
nsh_main-1 [0] 1.870000000: tracing_mark_write: B|1|uio_copyto+0/0x78
nsh_main-1 [0] 1.870000000: tracing_mark_write: E|1|uio_copyto+0/0x78
```
Now, consider that `poll_notify` is being instrumented and the patch on #15997 applied: when `uio_copyto` is called, it'll set `instr_level_enter` to `1`, but before it's cleaned, [`instrument->enter`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R82) is called, which will end-up calling `poll_notify`, which will trigger `__cyg_profile_func_enter` (the critical section here doesn't prevent nested calls). Fortunately, `instr_level_enter` is set and the function will return, but as soon as `poll_notify` exits, it'll call `__cyg_profile_func_exit`, which will execute as expected (not for `uio_copyto`, but for `poll_notify`). The same mechanism happens when `uio_copyto` clears `instr_level_enter` and `__cyg_profile_func_exit` is called: `poll_notify` will call `__cyg_profile_func_enter` successfully on enter, but `__cyg_profile_func_exit` for `poll_notify` will fail (`instr_level_exit` was set when `uio_copyto` e
xited). That's why we have less (but symetrical) entries for the same function (`uio_copyto` in this case).
Basically, the patch is kind of the same I tested here previously: it works for nested functions and enables the device to boot, but gives wrong results if any function called by [`instrument->enter`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R82) or [`instrument->exit`](https://github.com/apache/nuttx/pull/15997/files#diff-2eedc840a1af5e018f10144be3af5de564c255b6fd476ecb2ce4898f36585145R121) is being instrumented.
#15997 prevents catastrophic failures, but we still need to remove functions from that are called recursively by the instrumentation system from being instrumented. This PR is still necessary.
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2724888394
> > > > @xiaoxiang781216 @anjiahao1 I think the correct solution should be to add a nesting counter in the instrument hook to avoid recursion. poll_notify() can only solve the current problem. we cannot predict whether other functions need to add tags.
> > >
> > >
> > > Can you provide a patch for that, please?
> >
> >
> > I'm asking you because I didn't fully understand your proposal. Although we can implement a recursion check, I see no reason for instrumenting this function (and I'm not convinced that the recursion check would affect other functions from being called recursively)
>
> ```
> @@ -56,12 +58,20 @@ __cyg_profile_func_enter(FAR void *this_fn, FAR void *call_site)
> {
> FAR struct instrument_s *instrument;
> FAR sq_entry_t *entry;
> + struct tcb_s *tcb;
>
> if (g_magic != MAIGC_NUMBMER)
> {
> return;
> }
>
> + tcb = this_task();
> +
> + if (atomic_fetch_add(&tcb->instr_level, 1) != 0)
> + {
> + return;
> + }
> +
> sq_for_every(&g_instrument_queue, entry)
> {
> instrument = (FAR struct instrument_s *)entry;
> @@ -70,6 +80,8 @@ __cyg_profile_func_enter(FAR void *this_fn, FAR void *call_site)
> instrument->enter(this_fn, call_site, instrument->arg);
> }
> }
> +
> + atomic_set(&tcb->instr_level, 0);
> }
> ```
>
> Could you tried this change? I don't know if it works.
Thanks!
It works if I insert the same logic to the `__cyg_profile_func_exit` function too, but it doesn't work as expected without this PR. Consider this approach without this PR:
- we need to create the `instr_level_[enter|exit]` variables to the TCB (which, if not a breaking change, is controversial);
- It creates an unnecessary overhead: each time `poll_notify` is called:
- Consider a function being instrumented: when it's called, `__cyg_profile_func_enter` is called too;
- `instr_level_enter` is `0`, then it calls the `instrument->enter` function, which calls `poll_notify`;
- This ends up calling `__cyg_profile_func_enter` again, which returns because we have `instr_level_enter` greater than `0`;
- `poll_notify` then exits and end-up calling `__cyg_profile_func_exit`, which has its own `instr_level_exit` equal to `0` and executes `instrument->leave` function. **Note that the `__cyg_profile_func_exit` ran for the `poll_notify` function, which was not registered by `__cyg_profile_func_enter`**;
- But `__cyg_profile_func_exit` calls `poll_notify` again, which makes `__cyg_profile_func_enter` run again (still exiting earlier because `instr_level_enter`). This same `poll_notify` calls `__cyg_profile_func_exit` again, which exits earlier too.
- Finally, the `__cyg_profile_func_exit` triggered by the first `poll_notify` cleans `instr_level_exit` and `instr_level_enter` is cleaned by the first function being instrumented.
In summary, the problem here is `poll_notify` being instrumented. Although this approach can prevent the device from halting, it still provides wrong results for the instrumentation log and doesn't work if `poll_notify` is instrumented.
My suggestion: let's move on with this PR and I'll open another issue/PR with the proposed changes and we can continue discussing there. What do you think @anchao @acassis @lupyuen @xiaoxiang781216 @pussuw and @anjiahao1 ? Basically, not instrumenting the `poll_notify` solves this issue without any overhead. The recursion check still requires some attention and there are no known issues that would be solved by 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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2726174854
> I tried this change before the one I've suggested (and the device still halts). Either way, the functions that are part of the instrumentation itself should not be instrumented (the same is valid for `__cyg_profile_func_[enter|exist]` which already contains it).
I have a completely different opinion from you. It's just that functions that appear in the detection cycle should not be detected, otherwise you will find that more and more functions are added to this list.
As a maintainer, we should provide a solution to the framework instead of letting everyone find bugs and solve the problem.
> For now, this is the fix that fixes the reported issue without adding any overheads.
If we want to reduce the overhead, we should not turn on the instrumentation capability. You can check the disassemble code. The overhead of ` __cyg_profile_func_[enter|exist]` is much greater than several checks.
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2721000742
> @xiaoxiang781216 @anjiahao1 I think the correct solution should be to add a nesting counter in the instrument hook to avoid recursion. poll_notify() can only solve the current problem. we cannot predict whether other functions need to add tags.
Can you provide a patch for that, please?
--
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] fs/vfs/fs_poll: Add noinstrument_function to poll_notify function [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on PR #15971:
URL: https://github.com/apache/nuttx/pull/15971#issuecomment-2725035884
maybe we can use the same variable in "`instr_level`" and call `atomic_fetch_sub ` to subtract the count in `__cyg_profile_func_exit()`
```
__cyg_profile_func_enter
->atomic_fetch_add(&tcb->instr_level, 1) == 0
->instrument->enter
->atomic_set(&tcb->instr_level, 0);
```
```
__cyg_profile_func_exit
->atomic_fetch_sub(&tcb->instr_level, 1) == 1
->instrument->leave
->atomic_set(&tcb->instr_level, 0);
```
I have no objection to this PR. I have no objection to this PR. I'm just worried that for the backends enabling the features of other instruments, there may be more functions that need to have the "noinstrument" mark added, and we will also lose track of these functions with such special marks.
Please merge this PR if we don't have a better option.
--
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