Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sched_processtimer: use small lock to protect g_timer_interval and g_timer_tick #15129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sched_processtimer: use small lock to protect g_timer_interval and g_timer_tick #15129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions sched/sched/sched_timerexpiration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ static clock_t g_timer_tick;

static unsigned int g_timer_interval;

static spinlock_t g_lock = SP_UNLOCKED;

/****************************************************************************
* Private Functions
****************************************************************************/
Expand Down Expand Up @@ -377,6 +379,7 @@ static clock_t nxsched_timer_process(clock_t ticks, clock_t elapsed,

static clock_t nxsched_timer_start(clock_t ticks, clock_t interval)
{
irqstate_t flags;
int ret;

if (interval > 0)
Expand All @@ -388,6 +391,8 @@ static clock_t nxsched_timer_start(clock_t ticks, clock_t interval)
}
#endif

flags = enter_critical_section();
Copy link
Contributor

Choose a reason for hiding this comment

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

why mix the critical section and spinlock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we need to protect up_timer_tick_start.


#ifdef CONFIG_SCHED_TICKLESS_ALARM
/* Convert the delay to a time in the future (with respect
* to the time when last stopped the timer).
Expand All @@ -400,6 +405,8 @@ static clock_t nxsched_timer_start(clock_t ticks, clock_t interval)
ret = up_timer_tick_start(interval);
#endif

leave_critical_section(flags);

if (ret < 0)
{
serr("ERROR: up_timer_start/up_alarm_start failed: %d\n", ret);
Expand Down Expand Up @@ -443,18 +450,18 @@ void nxsched_alarm_tick_expiration(clock_t ticks)

/* Save the time that the alarm occurred */

flags = enter_critical_section();
flags = spin_lock_irqsave_wo_note(&g_lock);
elapsed = ticks - g_timer_tick;
g_timer_tick = ticks;
leave_critical_section(flags);
spin_unlock_irqrestore_wo_note(&g_lock, flags);

/* Process the timer ticks and set up the next interval (or not) */

nexttime = nxsched_timer_process(ticks, elapsed, false);

flags = enter_critical_section();
flags = spin_lock_irqsave_wo_note(&g_lock);
g_timer_interval = nxsched_timer_start(ticks, nexttime);
leave_critical_section(flags);
spin_unlock_irqrestore_wo_note(&g_lock, flags);
}

void nxsched_alarm_expiration(FAR const struct timespec *ts)
Expand Down Expand Up @@ -494,19 +501,19 @@ void nxsched_timer_expiration(void)

/* Get the interval associated with last expiration */

flags = enter_critical_section();
flags = spin_lock_irqsave_wo_note(&g_lock);
up_timer_gettick(&ticks);
g_timer_tick = ticks;
elapsed = g_timer_interval;
leave_critical_section(flags);
spin_unlock_irqrestore_wo_note(&g_lock, flags);

/* Process the timer ticks and set up the next interval (or not) */

nexttime = nxsched_timer_process(ticks, elapsed, false);

flags = enter_critical_section();
flags = spin_lock_irqsave_wo_note(&g_lock);
g_timer_interval = nxsched_timer_start(ticks, nexttime);
Copy link
Contributor

Choose a reason for hiding this comment

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

nxsched_timer_start() will access the global variables in lower harlf driver, using local spinlock cannot guarantee the same effect as csection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nxsched_timer_start will call up_timer_start or up_alarm_start
I believe that these two functions should be responsible for adding their own critical sections. In fact, this is probably how it's done in most cases.
image
image

However, there may be some overlooked scenarios. Currently, we can add an additional critical section in this function. Once all the timer driver codes have been replaced with small locks in the future, this critical section can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

leave_critical_section(flags);
spin_unlock_irqrestore_wo_note(&g_lock, flags);
}
#endif

Expand Down Expand Up @@ -549,6 +556,7 @@ void nxsched_timer_expiration(void)

void nxsched_reassess_timer(void)
{
irqstate_t flags;
clock_t nexttime;
clock_t ticks;
clock_t elapsed;
Expand All @@ -565,13 +573,18 @@ void nxsched_reassess_timer(void)

/* Convert this to the elapsed time and update clock tickbase */

flags = spin_lock_irqsave_wo_note(&g_lock);
elapsed = ticks - g_timer_tick;
g_timer_tick = ticks;
spin_unlock_irqrestore_wo_note(&g_lock, flags);

/* Process the timer ticks and start next timer */

nexttime = nxsched_timer_process(ticks, elapsed, true);

flags = spin_lock_irqsave_wo_note(&g_lock);
g_timer_interval = nxsched_timer_start(ticks, nexttime);
spin_unlock_irqrestore_wo_note(&g_lock, flags);
}

/****************************************************************************
Expand All @@ -593,9 +606,9 @@ clock_t nxsched_get_next_expired(void)
irqstate_t flags;
sclock_t ret;

flags = enter_critical_section();
flags = spin_lock_irqsave_wo_note(&g_lock);
ret = g_timer_tick + g_timer_interval - clock_systime_ticks();
leave_critical_section(flags);
spin_unlock_irqrestore_wo_note(&g_lock, flags);

return ret < 0 ? 0 : ret;
}
Expand Down
Loading