From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753021AbcHOKcy (ORCPT ); Mon, 15 Aug 2016 06:32:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58282 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752650AbcHOKcx (ORCPT ); Mon, 15 Aug 2016 06:32:53 -0400 Date: Mon, 15 Aug 2016 12:29:49 +0200 From: Stanislaw Gruszka To: Mel Gorman Cc: Giovanni Gherdovich , Ingo Molnar , Ingo Molnar , Peter Zijlstra , Mike Galbraith , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1] sched/cputime: Mitigate performance regression in times()/clock_gettime() Message-ID: <20160815102948.GC19741@redhat.com> References: <1470385316-15027-1-git-send-email-ggherdovich@suse.cz> <1470385316-15027-2-git-send-email-ggherdovich@suse.cz> <20160810112641.GA30126@gmail.com> <20160812121010.GA30199@redhat.com> <1471247345.1776.2.camel@suse.cz> <20160815083349.GE8119@techsingularity.net> <20160815091900.GA19741@redhat.com> <20160815095804.GF8119@techsingularity.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160815095804.GF8119@techsingularity.net> User-Agent: Mutt/1.5.23 (2014-03-12) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 15 Aug 2016 10:32:52 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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