All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mel Gorman <mgorman@techsingularity.net>
To: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Giovanni Gherdovich <ggherdovich@suse.cz>,
	Ingo Molnar <mingo@kernel.org>, Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Mike Galbraith <mgalbraith@suse.de>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime()
Date: Mon, 15 Aug 2016 10:58:04 +0100	[thread overview]
Message-ID: <20160815095804.GF8119@techsingularity.net> (raw)
In-Reply-To: <20160815091900.GA19741@redhat.com>

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

  reply	other threads:[~2016-08-15  9:58 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-05  8:21 [PATCH 0/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Giovanni Gherdovich
2016-08-05  8:21 ` [PATCH 1/1] " Giovanni Gherdovich
2016-08-10 11:26   ` Ingo Molnar
2016-08-10 13:02     ` Giovanni Gherdovich
2016-08-12 12:10     ` Stanislaw Gruszka
2016-08-15  7:49       ` Giovanni Gherdovich
2016-08-15  8:33         ` Mel Gorman
2016-08-15  9:19           ` Stanislaw Gruszka
2016-08-15  9:58             ` Mel Gorman [this message]
2016-08-15 10:29               ` Stanislaw Gruszka
2016-08-15  9:13       ` Wanpeng Li
2016-08-15  9:21         ` Stanislaw Gruszka
2016-08-15  9:28           ` Wanpeng Li
2016-08-10 18:00   ` [tip:sched/core] " tip-bot for Giovanni Gherdovich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160815095804.GF8119@techsingularity.net \
    --to=mgorman@techsingularity.net \
    --cc=ggherdovich@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgalbraith@suse.de \
    --cc=mingo@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=sgruszka@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.