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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
