Skip to content

Conversation

@cvinayak
Copy link
Contributor

@cvinayak cvinayak commented Mar 1, 2021

Fix ticker job to defer itself to avoid recursive
invocation to itself due to ticker interface calls from
inside the ticker operation callbacks.

The recursive use was exposed when using ticker stop
operation callback of stopping an auxiliary PDU to stop
the primary PDU scheduling as part of generation of
Advertising Terminate event.

Signed-off-by: Vinayak Kariappa Chettimada [email protected]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider also moving clearing of the job_guard to after ticker_job_compare_update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this because ticks_slot_previous and ticks_current are updated in ticker_job_compare_update, and the vectoring (swap) to ticker_worker function would use stale values? or do you have other reasons?

(sorry for asking again, from the message here: #31026 (comment), I understand context-safety, but just making double sure :-) ).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this because ticks_slot_previous and ticks_current are updated in ticker_job_compare_update, and the vectoring (swap) to ticker_worker function would use stale values? or do you have other reasons?

Yes, that is exactly why. The suggestion is not based on an actual observation, just based on analysis and being cautious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Your analysis is correct, as the resolution happens in ticker_worker, there are chances that after a current reserving ticker is stopped, it is necessary to have correct value of ticks_slot_previous and ticks_current be used in the ticker_worker.

cvinayak added 2 commits March 2, 2021 13:51
Fix ticker job to defer itself to avoid recursive
invocation to itself due to ticker interface calls from
inside the ticker operation callbacks.

The recursive use was exposed when using ticker stop
operation callback of stopping an auxiliary PDU to stop
the primary PDU scheduling as part of generation of
Advertising Terminate event.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Move the ticker job_guard reset to after
ticker_job_compare_update, so that the ticks_current and
ticks_slot_previous are updated before ticker_worker gets
to execute. Without this fix, there is a possibility that
ticker_worker will use incorrect ticks_slot_previous and
ticks_current value under race conditions.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@cvinayak cvinayak force-pushed the github_ticker_job_recursive branch from 65a2158 to ce32cc4 Compare March 2, 2021 08:22
Copy link
Contributor

@mtpr-ot mtpr-ot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link

@kruithofa kruithofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@carlescufi carlescufi merged commit 21e83e4 into zephyrproject-rtos:master Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants