Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1676389
| From | Frans Klaver <fransklaver@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [kernel-sched-cputime] question about probable bug in cputime_adjust() |
| Date | 2017-06-28 07:40 +0200 |
| Message-ID | <tXdRM-2J1-15@gated-at.bofh.it> (permalink) |
| References | <tX7Ml-7kb-3@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Wed, Jun 28, 2017 at 1:03 AM, Gustavo A. R. Silva
<garsilva@embeddedor.com> wrote:
>
> Hello everybody,
>
> While looking into Coverity ID 1371643 I ran into the following piece of
> code at kernel/sched/cputime.c:568:
>
> 568/*
> 569 * Adjust tick based cputime random precision against scheduler runtime
> 570 * accounting.
> 571 *
> 572 * Tick based cputime accounting depend on random scheduling timeslices
> of a
> 573 * task to be interrupted or not by the timer. Depending on these
> 574 * circumstances, the number of these interrupts may be over or
> 575 * under-optimistic, matching the real user and system cputime with a
> variable
> 576 * precision.
> 577 *
> 578 * Fix this by scaling these tick based values against the total runtime
> 579 * accounted by the CFS scheduler.
> 580 *
> 581 * This code provides the following guarantees:
> 582 *
> 583 * stime + utime == rtime
> 584 * stime_i+1 >= stime_i, utime_i+1 >= utime_i
> 585 *
> 586 * Assuming that rtime_i+1 >= rtime_i.
> 587 */
> 588static void cputime_adjust(struct task_cputime *curr,
> 589 struct prev_cputime *prev,
> 590 u64 *ut, u64 *st)
> 591{
> 592 u64 rtime, stime, utime;
> 593 unsigned long flags;
> 594
> 595 /* Serialize concurrent callers such that we can honour our
> guarantees */
> 596 raw_spin_lock_irqsave(&prev->lock, flags);
> 597 rtime = curr->sum_exec_runtime;
> 598
> 599 /*
> 600 * This is possible under two circumstances:
> 601 * - rtime isn't monotonic after all (a bug);
> 602 * - we got reordered by the lock.
> 603 *
> 604 * In both cases this acts as a filter such that the rest of the
> code
> 605 * can assume it is monotonic regardless of anything else.
> 606 */
> 607 if (prev->stime + prev->utime >= rtime)
> 608 goto out;
> 609
> 610 stime = curr->stime;
> 611 utime = curr->utime;
> 612
> 613 /*
> 614 * If either stime or both stime and utime are 0, assume all
> runtime is
> 615 * userspace. Once a task gets some ticks, the monotonicy code at
> 616 * 'update' will ensure things converge to the observed ratio.
> 617 */
> 618 if (stime == 0) {
> 619 utime = rtime;
> 620 goto update;
> 621 }
> 622
> 623 if (utime == 0) {
> 624 stime = rtime;
> 625 goto update;
> 626 }
> 627
> 628 stime = scale_stime(stime, rtime, stime + utime);
> 629
> 630update:
> 631 /*
> 632 * Make sure stime doesn't go backwards; this preserves
> monotonicity
> 633 * for utime because rtime is monotonic.
> 634 *
> 635 * utime_i+1 = rtime_i+1 - stime_i
> 636 * = rtime_i+1 - (rtime_i - utime_i)
> 637 * = (rtime_i+1 - rtime_i) + utime_i
> 638 * >= utime_i
> 639 */
> 640 if (stime < prev->stime)
> 641 stime = prev->stime;
> 642 utime = rtime - stime;
> 643
> 644 /*
> 645 * Make sure utime doesn't go backwards; this still preserves
> 646 * monotonicity for stime, analogous argument to above.
> 647 */
> 648 if (utime < prev->utime) {
> 649 utime = prev->utime;
> 650 stime = rtime - utime;
> 651 }
> 652
> 653 prev->stime = stime;
> 654 prev->utime = utime;
> 655out:
> 656 *ut = prev->utime;
> 657 *st = prev->stime;
> 658 raw_spin_unlock_irqrestore(&prev->lock, flags);
> 659}
>
>
> The issue here is that the value assigned to variable utime at line 619 is
> overwritten at line 642, which would make such variable assignment useless.
It isn't completely useless, since utime is used in line 628 to calculate stime.
> But I'm suspicious that such assignment is actually correct and that line
> 642 should be included into the IF block at line 640. Something similar to
> the following patch:
>
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime *curr,
> * = (rtime_i+1 - rtime_i) + utime_i
> * >= utime_i
> */
> - if (stime < prev->stime)
> + if (stime < prev->stime) {
> stime = prev->stime;
> - utime = rtime - stime;
> + utime = rtime - stime;
> + }
>
>
> If you confirm this, I will send a patch in a full and proper form.
>
> I'd really appreciate your comments.
If you do that, how would you meet the guarantee made in line 583?
Frans
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[kernel-sched-cputime] question about probable bug in cputime_adjust() "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-06-28 01:10 +0200
Re: [kernel-sched-cputime] question about probable bug in cputime_adjust() Frans Klaver <fransklaver@gmail.com> - 2017-06-28 07:40 +0200
Re: [kernel-sched-cputime] question about probable bug in cputime_adjust() Frans Klaver <fransklaver@gmail.com> - 2017-06-28 08:10 +0200
Re: [kernel-sched-cputime] question about probable bug in cputime_adjust() "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-06-29 02:30 +0200
Re: [kernel-sched-cputime] question about probable bug in cputime_adjust() Frans Klaver <fransklaver@gmail.com> - 2017-06-29 07:00 +0200
Re: [kernel-sched-cputime] question about probable bug in cputime_adjust() "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-06-29 20:00 +0200
[PATCH] sched/cputime: code refactoring in cputime_adjust() "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-06-29 20:50 +0200
[tip:sched/core] sched/cputime: Refactor the cputime_adjust() code "tip-bot for Gustavo A. R. Silva" <tipbot@zytor.com> - 2017-06-30 15:20 +0200
Re: [tip:sched/core] sched/cputime: Refactor the cputime_adjust() code Rik van Riel <riel@redhat.com> - 2017-06-30 16:10 +0200
Re: [tip:sched/core] sched/cputime: Refactor the cputime_adjust() code Frans Klaver <fransklaver@gmail.com> - 2017-06-30 16:50 +0200
Re: [tip:sched/core] sched/cputime: Refactor the cputime_adjust() code Frederic Weisbecker <fweisbec@gmail.com> - 2017-06-30 17:50 +0200
Re: [tip:sched/core] sched/cputime: Refactor the cputime_adjust() code Stanislaw Gruszka <sgruszka@redhat.com> - 2017-06-30 18:20 +0200
csiph-web