Sebastian Huber commented: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1479#note_158919 Thank you for the patch. I wrote #5747 and I had a local fix for it on a branch. I opened no merge request for that branch, so your work is the one which lands. The points below come out of the comparison of the two. ### The period object Line 212 deletes the rate monotonic period in `_Regulator_Free_helper()`. This closes a leak which `main` has and which my own patch missed. `rtems_regulator_delete( reg, 0 )` deletes the output thread before that thread reaches `exit_delivery_thread`. The period object then stays allocated for the life of the system. Keep this hunk through any rewrite of the branch. #5760 records the leak. ### The order of the two flags on the exit path `_Regulator_Free_helper()` acts on the instance as soon as `delivery_thread_is_running` is false. Line 163 stores that flag first and line 172 stores `delivery_thread_has_exited` last. A helper which runs on another processor in that span reads a half finished instance. Give `delivery_thread_is_running` its value after every other field of the exit path. The span matters, because line 203 and line 208 are not atomic. The helper tests `delivery_thread_has_exited` and then calls `rtems_task_delete()`. An output thread which reaches `rtems_task_exit()` between the two leaves an identifier which names no task. `rtems_task_delete()` answers `RTEMS_INVALID_ID` and the assertion of line 209 fires. Two orders reach that state. A second `rtems_regulator_delete()` after an `RTEMS_TIMEOUT` finds `delivery_thread_is_running` false and `delivery_thread_has_exited` still false. A delete which runs while the output thread fails its `rtems_rate_monotonic_create()` finds the same pair. Both need a second processor, so no uniprocessor build reaches them. This race is older than your branch. My patch shares it on the `ticks == 0` path, where the outer test reads no flag at all. `main` keeps the two stores together and your line 165 puts a directive call between them, so the branch widens the span. The reorder above is worth it for that reason alone. #5761 records the race. Leave it alone in this merge request. ### One mark for an object which does not exist Line 201 and line 202 test two values for one fact. The comment of lines 339 to 346 names `(rtems_id) -1` alone, so the code and the comment disagree. `calloc()` zeroes the instance, so zero does work for the queue and for the partition. A single named value reads better and covers all three identifiers. ```c /* * RTEMS_ID_NONE is not usable for this, because rtems_task_delete() takes * the identifier zero as the calling task. */ #define REGULATOR_NO_ID ( (rtems_id) UINT32_MAX ) ``` `rtems_regulator_create()` gives `delivery_thread_id`, `queue_id` and `messages_partition_id` that value before the first create. Each guard then tests one value. Line 202 tests a value which no path produces. Line 346 stores `(rtems_id) -1` and line 210 stores zero only where the function goes on to free the instance. ### The commits The branch holds one commit for three problems. #5747 describes two of them and says nothing about the period leak. Split the commit in two. - `cpukit/libmisc: Accept an empty regulator queue` for the assertion of line 125. - `cpukit/libmisc: Clean up a partial regulator` for the helper and the exit path. The body of a commit states the problem and then the solution. The bullet list of the description tells a reader what the diff already shows. The text of #5747 gives you the problem for both commits. #5760 records the period leak. The first commit takes `Update #5747.` The second commit takes `Close #5747.` and `Close #5760.` ### Nit Line 210, line 216, line 252, line 253, line 256, line 267 and line 272 store into an instance which line 276 frees. The helper reaches line 276 on every path which passes them. Line 169 is not one of these, because the helper reads `delivery_thread_period_id` after the output thread stores it. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1479#note_158919 You're receiving this email because of your account on gitlab.rtems.org. Unsubscribe from this thread: https://gitlab.rtems.org/-/sent_notifications/5-dua1pild7do47azq7rko3wvam-1d/unsubscribe | Manage all notifications: https://gitlab.rtems.org/-/profile/notifications | Help: https://gitlab.rtems.org/help
_______________________________________________ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
