Messages in this thread | | | Subject | Re: [PATCH 3/3] timer: Reduce unnecessary sighand lock contention | From | Jason Low <> | Date | Thu, 27 Aug 2015 13:29:50 -0700 |
| |
On Thu, 2015-08-27 at 14:53 +0200, Frederic Weisbecker wrote: > On Wed, Aug 26, 2015 at 04:32:34PM -0700, Jason Low wrote: > > On Thu, 2015-08-27 at 00:56 +0200, Frederic Weisbecker wrote: > > > On Tue, Aug 25, 2015 at 08:17:48PM -0700, Jason Low wrote: > > > > It was found while running a database workload on large systems that > > > > significant time was spent trying to acquire the sighand lock. > > > > > > > > The issue was that whenever an itimer expired, many threads ended up > > > > simultaneously trying to send the signal. Most of the time, nothing > > > > happened after acquiring the sighand lock because another thread > > > > had already sent the signal and updated the "next expire" time. The > > > > fastpath_timer_check() didn't help much since the "next expire" time > > > > was updated later. > > > > > > > > This patch addresses this by having the thread_group_cputimer structure > > > > maintain a boolean to signify when a thread in the group is already > > > > checking for process wide timers, and adds extra logic in the fastpath > > > > to check the boolean. > > > > > > > > Signed-off-by: Jason Low <jason.low2@hp.com> > > > > --- > > > > include/linux/init_task.h | 1 + > > > > include/linux/sched.h | 3 +++ > > > > kernel/time/posix-cpu-timers.c | 19 +++++++++++++++++-- > > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/linux/init_task.h b/include/linux/init_task.h > > > > index d0b380e..3350c77 100644 > > > > --- a/include/linux/init_task.h > > > > +++ b/include/linux/init_task.h > > > > @@ -53,6 +53,7 @@ extern struct fs_struct init_fs; > > > > .cputimer = { \ > > > > .cputime_atomic = INIT_CPUTIME_ATOMIC, \ > > > > .running = 0, \ > > > > + .checking_timer = 0, \ > > > > }, \ > > > > INIT_PREV_CPUTIME(sig) \ > > > > .cred_guard_mutex = \ > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > > > index 119823d..a6c8334 100644 > > > > --- a/include/linux/sched.h > > > > +++ b/include/linux/sched.h > > > > @@ -619,6 +619,8 @@ struct task_cputime_atomic { > > > > * @cputime_atomic: atomic thread group interval timers. > > > > * @running: non-zero when there are timers running and > > > > * @cputime receives updates. > > > > + * @checking_timer: non-zero when a thread is in the process of > > > > + * checking for thread group timers. > > > > * > > > > * This structure contains the version of task_cputime, above, that is > > > > * used for thread group CPU timer calculations. > > > > @@ -626,6 +628,7 @@ struct task_cputime_atomic { > > > > struct thread_group_cputimer { > > > > struct task_cputime_atomic cputime_atomic; > > > > int running; > > > > + int checking_timer; > > > > > > How about a flag in the "running" field instead? > > > > > > 1) Space in signal_struct is not as important as in task_strut but it > > > still matters. > > > > George Spelvin suggested that we convert them to booleans which would > > make them take up 2 bytes. > > > > > 2) We already read the "running" field locklessly. Adding a new field like > > > checking_timer gets even more complicated. Ideally there should be at > > > least a paired memory barrier between both. Let's just simplify that > > > with a single field. > > > > hmmm, so having 1 "flag" where we access bits for the "running" and > > "checking_timer"? > > Sure, like: > > #define CPUTIMER_RUNNING 0x1 > #define CPUTIMER_CHECKING 0x2 > > struct thread_group_cputimer { > struct task_cputime_atomic cputime_atomic; > int status; > } > > So from cputimer_running() you just need to check: > > if (cputimer->status & CPUTIMER_RUNNING) > > And from run_posix_cpu_timer() fast-path: > > if (cputimer->status == CPUTIMER_RUNNING) > > so that ignores CPUTIMER_CHECKING case.
Right, having just 1 "status" field can simply things a bit. The (cputimer->status == CPUTIMER_RUNNING) check does appear misleading though, since we're technically not only checking for if the "cputimer is running".
Maybe something like:
int status = cputimer->status; if ((status & CPUTIMER_RUNNING) && !(status & CPUTIMER_CHECKING))
makes it more obvious what's going on here.
| |