Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1461139 > unrolled thread
| Started by | Stanislaw Gruszka <sgruszka@redhat.com> |
|---|---|
| First post | 2016-08-12 14:20 +0200 |
| Last post | 2016-08-15 11:30 +0200 |
| Articles | 9 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Stanislaw Gruszka <sgruszka@redhat.com> - 2016-08-12 14:20 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Giovanni Gherdovich <ggherdovich@suse.cz> - 2016-08-15 09:50 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Mel Gorman <mgorman@techsingularity.net> - 2016-08-15 10:40 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Stanislaw Gruszka <sgruszka@redhat.com> - 2016-08-15 11:30 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Mel Gorman <mgorman@techsingularity.net> - 2016-08-15 12:00 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Stanislaw Gruszka <sgruszka@redhat.com> - 2016-08-15 12:40 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Wanpeng Li <kernellwp@gmail.com> - 2016-08-15 11:20 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Stanislaw Gruszka <sgruszka@redhat.com> - 2016-08-15 11:30 +0200
Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Wanpeng Li <kernellwp@gmail.com> - 2016-08-15 11:30 +0200
| From | Stanislaw Gruszka <sgruszka@redhat.com> |
|---|---|
| Date | 2016-08-12 14:20 +0200 |
| Subject | Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() |
| Message-ID | <s5jBo-1uO-5@gated-at.bofh.it> |
[Multipart message — attachments visible in raw view] — view raw
Hi On Wed, Aug 10, 2016 at 01:26:41PM +0200, Ingo Molnar wrote: > Nice detective work! I'm wondering, where do we stand if compared with a > pre-6e998916dfe3 kernel? > > I admit this is a difficult question: 6e998916dfe3 does not revert cleanly and I > suspect v3.17 does not run easily on a recent distro. Could you attempt to revert > the bad effects of 6e998916dfe3 perhaps, just to get numbers - i.e. don't try to > make the result correct, just see what the performance gap is, roughly. > > If there's still a significant gap then it might make sense to optimize this some > more. I measured (partial) revert performance on 4.7 using mmtest instructions from Giovanni and also tested some other possible fix (draft version): diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index 75f98c5..54fdf6d 100644 --- a/kernel/sched/cputime.c +++ b/kernel/sched/cputime.c @@ -294,6 +294,8 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) unsigned int seq, nextseq; unsigned long flags; + (void) task_sched_runtime(tsk); + rcu_read_lock(); /* Attempt a lockless read on the first round. */ nextseq = 0; @@ -308,7 +310,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) task_cputime(t, &utime, &stime); times->utime += utime; times->stime += stime; - times->sum_exec_runtime += task_sched_runtime(t); + times->sum_exec_runtime += t->se.sum_exec_runtime; } /* If lockless access failed, take the lock. */ nextseq = 1; --- mmtest benchmark results are below (full compare-kernels.sh output is in attachment): vanila-4.7 revert prefetch patch 4.74 ( 0.00%) 3.04 ( 35.93%) 4.09 ( 13.81%) 1.30 ( 72.59%) 5.49 ( 0.00%) 5.00 ( 8.97%) 5.34 ( 2.72%) 1.03 ( 81.16%) 6.12 ( 0.00%) 4.91 ( 19.73%) 5.97 ( 2.40%) 0.90 ( 85.27%) 6.68 ( 0.00%) 4.90 ( 26.66%) 6.02 ( 9.75%) 0.88 ( 86.89%) 7.21 ( 0.00%) 5.13 ( 28.85%) 6.70 ( 7.09%) 0.87 ( 87.91%) 7.66 ( 0.00%) 5.22 ( 31.80%) 7.17 ( 6.39%) 0.92 ( 88.01%) 7.91 ( 0.00%) 5.36 ( 32.22%) 7.30 ( 7.72%) 0.95 ( 87.97%) 7.95 ( 0.00%) 5.35 ( 32.73%) 7.34 ( 7.66%) 1.06 ( 86.66%) 8.00 ( 0.00%) 5.33 ( 33.31%) 7.38 ( 7.73%) 1.13 ( 85.82%) 5.61 ( 0.00%) 3.55 ( 36.76%) 4.53 ( 19.23%) 2.29 ( 59.28%) 5.66 ( 0.00%) 4.32 ( 23.79%) 4.75 ( 16.18%) 3.65 ( 35.46%) 5.98 ( 0.00%) 4.97 ( 16.87%) 5.96 ( 0.35%) 3.62 ( 39.40%) 6.58 ( 0.00%) 4.94 ( 24.93%) 6.04 ( 8.32%) 3.63 ( 44.89%) 7.19 ( 0.00%) 5.18 ( 28.01%) 6.68 ( 7.13%) 3.65 ( 49.22%) 7.67 ( 0.00%) 5.27 ( 31.29%) 7.16 ( 6.63%) 3.62 ( 52.76%) 7.88 ( 0.00%) 5.36 ( 31.98%) 7.28 ( 7.58%) 3.65 ( 53.71%) 7.99 ( 0.00%) 5.39 ( 32.52%) 7.40 ( 7.42%) 3.65 ( 54.25%) Patch works because we we update sum_exec_runtime on current thread what assure we see proper sum_exec_runtime value on different CPUs. I tested it with reproducers from commits 6e998916dfe32 and d670ec13178d0, patch did not break them. I'm going to run some other test. Patch is draft version for early review, task_sched_runtime() will be simplified (since it's called only current thread) and possibly split into two functions: one that call update_curr() and other that return sum_exec_runtime (assure it's consistent on 32 bit arches). Stanislaw
[toc] | [next] | [standalone]
| From | Giovanni Gherdovich <ggherdovich@suse.cz> |
|---|---|
| Date | 2016-08-15 09:50 +0200 |
| Message-ID | <s6kOJ-2Gt-13@gated-at.bofh.it> |
| In reply to | #1461139 |
Hello Stanislaw,
On Fri, 2016-08-12 at 14:10 +0200, Stanislaw Gruszka wrote:
>
> I measured (partial) revert performance on 4.7 using mmtest instructions
> from Giovanni and also tested some other possible fix (draft version):
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 75f98c5..54fdf6d 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -294,6 +294,8 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
> unsigned int seq, nextseq;
> unsigned long flags;
>
> + (void) task_sched_runtime(tsk);
> +
> rcu_read_lock();
> /* Attempt a lockless read on the first round. */
> nextseq = 0;
> @@ -308,7 +310,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
> task_cputime(t, &utime, &stime);
> times->utime += utime;
> times->stime += stime;
> - times->sum_exec_runtime += task_sched_runtime(t);
> + times->sum_exec_runtime += t->se.sum_exec_runtime;
> }
> /* If lockless access failed, take the lock. */
> nextseq = 1;
> ---
> mmtest benchmark results are below (full compare-kernels.sh output is in attachment):
>
> vanila-4.7 revert prefetch patch
> 4.74 ( 0.00%) 3.04 ( 35.93%) 4.09 ( 13.81%) 1.30 ( 72.59%)
> 5.49 ( 0.00%) 5.00 ( 8.97%) 5.34 ( 2.72%) 1.03 ( 81.16%)
> 6.12 ( 0.00%) 4.91 ( 19.73%) 5.97 ( 2.40%) 0.90 ( 85.27%)
> 6.68 ( 0.00%) 4.90 ( 26.66%) 6.02 ( 9.75%) 0.88 ( 86.89%)
> 7.21 ( 0.00%) 5.13 ( 28.85%) 6.70 ( 7.09%) 0.87 ( 87.91%)
> 7.66 ( 0.00%) 5.22 ( 31.80%) 7.17 ( 6.39%) 0.92 ( 88.01%)
> 7.91 ( 0.00%) 5.36 ( 32.22%) 7.30 ( 7.72%) 0.95 ( 87.97%)
> 7.95 ( 0.00%) 5.35 ( 32.73%) 7.34 ( 7.66%) 1.06 ( 86.66%)
> 8.00 ( 0.00%) 5.33 ( 33.31%) 7.38 ( 7.73%) 1.13 ( 85.82%)
> 5.61 ( 0.00%) 3.55 ( 36.76%) 4.53 ( 19.23%) 2.29 ( 59.28%)
> 5.66 ( 0.00%) 4.32 ( 23.79%) 4.75 ( 16.18%) 3.65 ( 35.46%)
> 5.98 ( 0.00%) 4.97 ( 16.87%) 5.96 ( 0.35%) 3.62 ( 39.40%)
> 6.58 ( 0.00%) 4.94 ( 24.93%) 6.04 ( 8.32%) 3.63 ( 44.89%)
> 7.19 ( 0.00%) 5.18 ( 28.01%) 6.68 ( 7.13%) 3.65 ( 49.22%)
> 7.67 ( 0.00%) 5.27 ( 31.29%) 7.16 ( 6.63%) 3.62 ( 52.76%)
> 7.88 ( 0.00%) 5.36 ( 31.98%) 7.28 ( 7.58%) 3.65 ( 53.71%)
> 7.99 ( 0.00%) 5.39 ( 32.52%) 7.40 ( 7.42%) 3.65 ( 54.25%)
>
> Patch works because we we update sum_exec_runtime on current thread
> what assure we see proper sum_exec_runtime value on different CPUs. I
> tested it with reproducers from commits 6e998916dfe32 and d670ec13178d0,
> patch did not break them. I'm going to run some other test.
>
> Patch is draft version for early review, task_sched_runtime() will be
> simplified (since it's called only current thread) and possibly split
> into two functions: one that call update_curr() and other that return
> sum_exec_runtime (assure it's consistent on 32 bit arches).
>
> Stanislaw
Thank you for having a look at this.
Your patch performs very well, even better than the pre-6e998916dfe3
numbers I was aiming for. I confirm your results on my test machine
(Sandy Bridge, 32 cores, 2 NUMA nodes).
I didn't apply on the very latest 4.8-rc but used what I had handy for
comparison (i.e. 4.7-rc7 and the parent of 6e998916dfe3).
As I said, my measurements match yours (my tables follow); looks like
your diff cures the problem while mine cures the symptoms.
clock_gettime():
threads 4.7-rc7 3.18-rc3 4.7-rc7 + prefetch 4.7-rc7 + Stanislaw
(pre-6e998916dfe3)
2 3.48 2.23 ( 35.68%) 3.06 ( 11.83%) 1.08 ( 68.81%)
5 3.33 2.83 ( 14.84%) 3.25 ( 2.40%) 0.71 ( 78.55%)
8 3.37 2.84 ( 15.80%) 3.26 ( 3.30%) 0.56 ( 83.49%)
12 3.32 3.09 ( 6.69%) 3.37 ( -1.60%) 0.42 ( 87.28%)
21 4.01 3.14 ( 21.70%) 3.90 ( 2.74%) 0.35 ( 91.35%)
30 3.63 3.28 ( 9.75%) 3.36 ( 7.41%) 0.28 ( 92.23%)
48 3.71 3.02 ( 18.69%) 3.11 ( 16.27%) 0.39 ( 89.39%)
79 3.75 2.88 ( 23.23%) 3.16 ( 15.74%) 0.46 ( 87.76%)
110 3.81 2.95 ( 22.62%) 3.25 ( 14.80%) 0.56 ( 85.41%)
128 3.88 3.05 ( 21.28%) 3.31 ( 14.76%) 0.62 ( 84.10%)
times():
threads 4.7-rc7 3.18-rc3 4.7-rc7 + prefetch 4.7-rc7 + Stanislaw
(pre-6e998916dfe3)
2 3.65 2.27 ( 37.94%) 3.25 ( 11.03%) 1.62 ( 55.71%)
5 3.45 2.78 ( 19.34%) 3.17 ( 7.92%) 2.33 ( 32.28%)
8 3.52 2.79 ( 20.66%) 3.22 ( 8.69%) 2.06 ( 41.44%)
12 3.29 3.02 ( 8.33%) 3.36 ( -2.04%) 2.00 ( 39.18%)
21 4.07 3.10 ( 23.86%) 3.92 ( 3.78%) 2.07 ( 49.18%)
30 3.87 3.33 ( 13.80%) 3.40 ( 12.17%) 1.89 ( 51.12%)
48 3.79 2.96 ( 21.94%) 3.16 ( 16.61%) 1.69 ( 55.46%)
79 3.88 2.88 ( 25.82%) 3.28 ( 15.42%) 1.60 ( 58.81%)
110 3.90 2.98 ( 23.73%) 3.38 ( 13.35%) 1.73 ( 55.61%)
128 4.00 3.10 ( 22.40%) 3.38 ( 15.45%) 1.66 ( 58.52%)
Regards,
Giovanni
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2016-08-15 10:40 +0200 |
| Message-ID | <s6lB7-3do-3@gated-at.bofh.it> |
| In reply to | #1462611 |
On Mon, Aug 15, 2016 at 09:49:05AM +0200, Giovanni Gherdovich wrote: > > mmtest benchmark results are below (full compare-kernels.sh output is in attachment): > > > > vanila-4.7 revert prefetch patch > > 4.74 ( 0.00%) 3.04 ( 35.93%) 4.09 ( 13.81%) 1.30 ( 72.59%) > > 5.49 ( 0.00%) 5.00 ( 8.97%) 5.34 ( 2.72%) 1.03 ( 81.16%) > > 6.12 ( 0.00%) 4.91 ( 19.73%) 5.97 ( 2.40%) 0.90 ( 85.27%) > > 6.68 ( 0.00%) 4.90 ( 26.66%) 6.02 ( 9.75%) 0.88 ( 86.89%) > > 7.21 ( 0.00%) 5.13 ( 28.85%) 6.70 ( 7.09%) 0.87 ( 87.91%) > > 7.66 ( 0.00%) 5.22 ( 31.80%) 7.17 ( 6.39%) 0.92 ( 88.01%) > > 7.91 ( 0.00%) 5.36 ( 32.22%) 7.30 ( 7.72%) 0.95 ( 87.97%) > > 7.95 ( 0.00%) 5.35 ( 32.73%) 7.34 ( 7.66%) 1.06 ( 86.66%) > > 8.00 ( 0.00%) 5.33 ( 33.31%) 7.38 ( 7.73%) 1.13 ( 85.82%) > > 5.61 ( 0.00%) 3.55 ( 36.76%) 4.53 ( 19.23%) 2.29 ( 59.28%) > > 5.66 ( 0.00%) 4.32 ( 23.79%) 4.75 ( 16.18%) 3.65 ( 35.46%) > > 5.98 ( 0.00%) 4.97 ( 16.87%) 5.96 ( 0.35%) 3.62 ( 39.40%) > > 6.58 ( 0.00%) 4.94 ( 24.93%) 6.04 ( 8.32%) 3.63 ( 44.89%) > > 7.19 ( 0.00%) 5.18 ( 28.01%) 6.68 ( 7.13%) 3.65 ( 49.22%) > > 7.67 ( 0.00%) 5.27 ( 31.29%) 7.16 ( 6.63%) 3.62 ( 52.76%) > > 7.88 ( 0.00%) 5.36 ( 31.98%) 7.28 ( 7.58%) 3.65 ( 53.71%) > > 7.99 ( 0.00%) 5.39 ( 32.52%) 7.40 ( 7.42%) 3.65 ( 54.25%) > > > > Patch works because we we update sum_exec_runtime on current thread > > what assure we see proper sum_exec_runtime value on different CPUs. I > > tested it with reproducers from commits 6e998916dfe32 and d670ec13178d0, > > patch did not break them. I'm going to run some other test. > > > > Patch is draft version for early review, task_sched_runtime() will be > > simplified (since it's called only current thread) and possibly split > > into two functions: one that call update_curr() and other that return > > sum_exec_runtime (assure it's consistent on 32 bit arches). > > > > Stanislaw > Is this really equivalent though? It updates one task instead of all tasks in the group and there is no guarantee that tsk == current. Glancing at it, it should monotonically increase but it looks like it would calculate stale data. -- Mel Gorman SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Stanislaw Gruszka <sgruszka@redhat.com> |
|---|---|
| Date | 2016-08-15 11:30 +0200 |
| Message-ID | <s6mnv-3MG-1@gated-at.bofh.it> |
| In reply to | #1462629 |
On Mon, Aug 15, 2016 at 09:33:49AM +0100, Mel Gorman wrote: > On Mon, Aug 15, 2016 at 09:49:05AM +0200, Giovanni Gherdovich wrote: > > > mmtest benchmark results are below (full compare-kernels.sh output is in attachment): > > > > > > vanila-4.7 revert prefetch patch > > > 4.74 ( 0.00%) 3.04 ( 35.93%) 4.09 ( 13.81%) 1.30 ( 72.59%) > > > 5.49 ( 0.00%) 5.00 ( 8.97%) 5.34 ( 2.72%) 1.03 ( 81.16%) > > > 6.12 ( 0.00%) 4.91 ( 19.73%) 5.97 ( 2.40%) 0.90 ( 85.27%) > > > 6.68 ( 0.00%) 4.90 ( 26.66%) 6.02 ( 9.75%) 0.88 ( 86.89%) > > > 7.21 ( 0.00%) 5.13 ( 28.85%) 6.70 ( 7.09%) 0.87 ( 87.91%) > > > 7.66 ( 0.00%) 5.22 ( 31.80%) 7.17 ( 6.39%) 0.92 ( 88.01%) > > > 7.91 ( 0.00%) 5.36 ( 32.22%) 7.30 ( 7.72%) 0.95 ( 87.97%) > > > 7.95 ( 0.00%) 5.35 ( 32.73%) 7.34 ( 7.66%) 1.06 ( 86.66%) > > > 8.00 ( 0.00%) 5.33 ( 33.31%) 7.38 ( 7.73%) 1.13 ( 85.82%) > > > 5.61 ( 0.00%) 3.55 ( 36.76%) 4.53 ( 19.23%) 2.29 ( 59.28%) > > > 5.66 ( 0.00%) 4.32 ( 23.79%) 4.75 ( 16.18%) 3.65 ( 35.46%) > > > 5.98 ( 0.00%) 4.97 ( 16.87%) 5.96 ( 0.35%) 3.62 ( 39.40%) > > > 6.58 ( 0.00%) 4.94 ( 24.93%) 6.04 ( 8.32%) 3.63 ( 44.89%) > > > 7.19 ( 0.00%) 5.18 ( 28.01%) 6.68 ( 7.13%) 3.65 ( 49.22%) > > > 7.67 ( 0.00%) 5.27 ( 31.29%) 7.16 ( 6.63%) 3.62 ( 52.76%) > > > 7.88 ( 0.00%) 5.36 ( 31.98%) 7.28 ( 7.58%) 3.65 ( 53.71%) > > > 7.99 ( 0.00%) 5.39 ( 32.52%) 7.40 ( 7.42%) 3.65 ( 54.25%) > > > > > > Patch works because we we update sum_exec_runtime on current thread > > > what assure we see proper sum_exec_runtime value on different CPUs. I > > > tested it with reproducers from commits 6e998916dfe32 and d670ec13178d0, > > > patch did not break them. I'm going to run some other test. > > > > > > Patch is draft version for early review, task_sched_runtime() will be > > > simplified (since it's called only current thread) and possibly split > > > into two functions: one that call update_curr() and other that return > > > sum_exec_runtime (assure it's consistent on 32 bit arches). > > > > > > Stanislaw > > > > Is this really equivalent though? It updates one task instead of all > tasks in the group and there is no guarantee that tsk == current. Oh, my intention was to update runtime on current. > Glancing at it, it should monotonically increase but it looks like it > would calculate stale data. Yes, until the next tick on a CPU, the patch does not count partial runtime of thread running on that CPU. However that was the behaviour before commit d670ec13178d0 - that how old thread_group_sched_runtime() function worked: /* - * Return sum_exec_runtime for the thread group. - * In case the task is currently running, return the sum plus current's - * pending runtime that have not been accounted yet. - * - * Note that the thread group might have other running tasks as well, - * so the return value not includes other pending runtime that other - * running tasks might have. - */ -unsigned long long thread_group_sched_runtime(struct task_struct *p) Stanislaw
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2016-08-15 12:00 +0200 |
| Message-ID | <s6mQx-3Wz-1@gated-at.bofh.it> |
| In reply to | #1462670 |
On Mon, Aug 15, 2016 at 11:19:01AM +0200, Stanislaw Gruszka wrote: > > Is this really equivalent though? It updates one task instead of all > > tasks in the group and there is no guarantee that tsk == current. > > Oh, my intention was to update runtime on current. > Ok, so minimally that would need addressing. However, then I would worry that two tasks in a group calling the function at the same time would see different results because each of them updated a different task. Such a situation is inherently race-prone anyway but it's a large enough functional difference to be worth calling out. Minimally, I don't think such a patch is a replacement for Giovanni's which is functionally equivalent to the current code but could be layered on top if it is proven to be ok. > > Glancing at it, it should monotonically increase but it looks like it > > would calculate stale data. > > Yes, until the next tick on a CPU, the patch does not count partial > runtime of thread running on that CPU. However that was the behaviour > before commit d670ec13178d0 - that how old thread_group_sched_runtime() > function worked: > Sure, but does this patch not reintroduce the "SMP wobble" and the problem of "the diff of 'process' should always be >= the diff of 'thread'" ? -- Mel Gorman SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Stanislaw Gruszka <sgruszka@redhat.com> |
|---|---|
| Date | 2016-08-15 12:40 +0200 |
| Message-ID | <s6ntf-4oT-13@gated-at.bofh.it> |
| In reply to | #1462683 |
On Mon, Aug 15, 2016 at 10:58:04AM +0100, Mel Gorman wrote: > On Mon, Aug 15, 2016 at 11:19:01AM +0200, Stanislaw Gruszka wrote: > > > Is this really equivalent though? It updates one task instead of all > > > tasks in the group and there is no guarantee that tsk == current. > > > > Oh, my intention was to update runtime on current. > > > > Ok, so minimally that would need addressing. However, then I would worry > that two tasks in a group calling the function at the same time would > see different results because each of them updated a different task. > Such a situation is inherently race-prone anyway but it's a large enough > functional difference to be worth calling out. It races bacause we don't know which thread will call the clock_gettime() first. But once that happen, second thread will see updated runtime value from first thread as we call update_curr() for it with task_rq_lock (change from commit 6e998916dfe3). > Minimally, I don't think such a patch is a replacement for Giovanni's > which is functionally equivalent to the current code but could be layered > on top if it is proven to be ok. I agree. I wanted to post my patch on top of Giovanni's. > > > Glancing at it, it should monotonically increase but it looks like it > > > would calculate stale data. > > > > Yes, until the next tick on a CPU, the patch does not count partial > > runtime of thread running on that CPU. However that was the behaviour > > before commit d670ec13178d0 - that how old thread_group_sched_runtime() > > function worked: > > > > Sure, but does this patch not reintroduce the "SMP wobble" and the > problem of "the diff of 'process' should always be >= the diff of > 'thread'" ? It should not reintroduce that problem, also because of change from commit 6e998916dfe3. When a thread reads sum_exec_runtime it also update that value, then process reads updated value. I run test case from "SMP wobble" commit and the problem do not happen on my tests. Perhaps I should post patch with a descriptive changelog and things would be clearer ... Stanislaw
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2016-08-15 11:20 +0200 |
| Subject | Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() |
| Message-ID | <s6mdP-3HY-7@gated-at.bofh.it> |
| In reply to | #1461139 |
2016-08-12 20:10 GMT+08:00 Stanislaw Gruszka <sgruszka@redhat.com>: > Hi > > On Wed, Aug 10, 2016 at 01:26:41PM +0200, Ingo Molnar wrote: >> Nice detective work! I'm wondering, where do we stand if compared with a >> pre-6e998916dfe3 kernel? >> >> I admit this is a difficult question: 6e998916dfe3 does not revert cleanly and I >> suspect v3.17 does not run easily on a recent distro. Could you attempt to revert >> the bad effects of 6e998916dfe3 perhaps, just to get numbers - i.e. don't try to >> make the result correct, just see what the performance gap is, roughly. >> >> If there's still a significant gap then it might make sense to optimize this some >> more. > > I measured (partial) revert performance on 4.7 using mmtest instructions > from Giovanni and also tested some other possible fix (draft version): > > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > index 75f98c5..54fdf6d 100644 > --- a/kernel/sched/cputime.c > +++ b/kernel/sched/cputime.c > @@ -294,6 +294,8 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) > unsigned int seq, nextseq; > unsigned long flags; > > + (void) task_sched_runtime(tsk); > + > rcu_read_lock(); > /* Attempt a lockless read on the first round. */ > nextseq = 0; > @@ -308,7 +310,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) > task_cputime(t, &utime, &stime); > times->utime += utime; > times->stime += stime; > - times->sum_exec_runtime += task_sched_runtime(t); > + times->sum_exec_runtime += t->se.sum_exec_runtime; If this will not have updated stats for other threads? Regards, Wanpeng Li
[toc] | [prev] | [next] | [standalone]
| From | Stanislaw Gruszka <sgruszka@redhat.com> |
|---|---|
| Date | 2016-08-15 11:30 +0200 |
| Message-ID | <s6mnv-3MG-7@gated-at.bofh.it> |
| In reply to | #1462663 |
On Mon, Aug 15, 2016 at 05:13:30PM +0800, Wanpeng Li wrote: > 2016-08-12 20:10 GMT+08:00 Stanislaw Gruszka <sgruszka@redhat.com>: > > Hi > > > > On Wed, Aug 10, 2016 at 01:26:41PM +0200, Ingo Molnar wrote: > >> Nice detective work! I'm wondering, where do we stand if compared with a > >> pre-6e998916dfe3 kernel? > >> > >> I admit this is a difficult question: 6e998916dfe3 does not revert cleanly and I > >> suspect v3.17 does not run easily on a recent distro. Could you attempt to revert > >> the bad effects of 6e998916dfe3 perhaps, just to get numbers - i.e. don't try to > >> make the result correct, just see what the performance gap is, roughly. > >> > >> If there's still a significant gap then it might make sense to optimize this some > >> more. > > > > I measured (partial) revert performance on 4.7 using mmtest instructions > > from Giovanni and also tested some other possible fix (draft version): > > > > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > > index 75f98c5..54fdf6d 100644 > > --- a/kernel/sched/cputime.c > > +++ b/kernel/sched/cputime.c > > @@ -294,6 +294,8 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) > > unsigned int seq, nextseq; > > unsigned long flags; > > > > + (void) task_sched_runtime(tsk); > > + > > rcu_read_lock(); > > /* Attempt a lockless read on the first round. */ > > nextseq = 0; > > @@ -308,7 +310,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) > > task_cputime(t, &utime, &stime); > > times->utime += utime; > > times->stime += stime; > > - times->sum_exec_runtime += task_sched_runtime(t); > > + times->sum_exec_runtime += t->se.sum_exec_runtime; > > If this will not have updated stats for other threads? No, until tick/sched() on CPUs running threads. Stanislaw
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2016-08-15 11:30 +0200 |
| Subject | Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() |
| Message-ID | <s6mnw-3MG-11@gated-at.bofh.it> |
| In reply to | #1462669 |
2016-08-15 17:21 GMT+08:00 Stanislaw Gruszka <sgruszka@redhat.com>: > On Mon, Aug 15, 2016 at 05:13:30PM +0800, Wanpeng Li wrote: >> 2016-08-12 20:10 GMT+08:00 Stanislaw Gruszka <sgruszka@redhat.com>: >> > Hi >> > >> > On Wed, Aug 10, 2016 at 01:26:41PM +0200, Ingo Molnar wrote: >> >> Nice detective work! I'm wondering, where do we stand if compared with a >> >> pre-6e998916dfe3 kernel? >> >> >> >> I admit this is a difficult question: 6e998916dfe3 does not revert cleanly and I >> >> suspect v3.17 does not run easily on a recent distro. Could you attempt to revert >> >> the bad effects of 6e998916dfe3 perhaps, just to get numbers - i.e. don't try to >> >> make the result correct, just see what the performance gap is, roughly. >> >> >> >> If there's still a significant gap then it might make sense to optimize this some >> >> more. >> > >> > I measured (partial) revert performance on 4.7 using mmtest instructions >> > from Giovanni and also tested some other possible fix (draft version): >> > >> > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c >> > index 75f98c5..54fdf6d 100644 >> > --- a/kernel/sched/cputime.c >> > +++ b/kernel/sched/cputime.c >> > @@ -294,6 +294,8 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) >> > unsigned int seq, nextseq; >> > unsigned long flags; >> > >> > + (void) task_sched_runtime(tsk); >> > + >> > rcu_read_lock(); >> > /* Attempt a lockless read on the first round. */ >> > nextseq = 0; >> > @@ -308,7 +310,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) >> > task_cputime(t, &utime, &stime); >> > times->utime += utime; >> > times->stime += stime; >> > - times->sum_exec_runtime += task_sched_runtime(t); >> > + times->sum_exec_runtime += t->se.sum_exec_runtime; >> >> If this will not have updated stats for other threads? > > No, until tick/sched() on CPUs running threads. Yeah, I think this change will result in not updated stats for other threads if they are running and before next update_curr() is called. Regards, Wanpeng Li
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web