From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754386AbbJ2Mof (ORCPT ); Thu, 29 Oct 2015 08:44:35 -0400 Received: from mail-wi0-f180.google.com ([209.85.212.180]:34678 "EHLO mail-wi0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751915AbbJ2Mod (ORCPT ); Thu, 29 Oct 2015 08:44:33 -0400 Date: Thu, 29 Oct 2015 13:44:31 +0100 From: Frederic Weisbecker To: Hiroshi Shimamoto Cc: Peter Zijlstra , Ingo Molnar , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2] cputime: fix invalid gtime Message-ID: <20151029124429.GA8554@lerouge> References: <7F861DC0615E0C47A872E6F3C5FCDDBD05F543FE@BPXM14GP.gisp.nec.co.jp> <20151028161145.GA25958@lerouge> <7F861DC0615E0C47A872E6F3C5FCDDBD05F54FC5@BPXM14GP.gisp.nec.co.jp> <20151029033735.GA30367@lerouge> <7F861DC0615E0C47A872E6F3C5FCDDBD05F55194@BPXM14GP.gisp.nec.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7F861DC0615E0C47A872E6F3C5FCDDBD05F55194@BPXM14GP.gisp.nec.co.jp> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 29, 2015 at 04:30:20AM +0000, Hiroshi Shimamoto wrote: > > Subject: Re: [PATCH v2] cputime: fix invalid gtime > > > > On Thu, Oct 29, 2015 at 01:10:01AM +0000, Hiroshi Shimamoto wrote: > > > > Obviously I completely messed up there. And task_cputime() has a similar issue > > > > but it happens to work due to vtime_snap_whence set to VTIME_SLEEPING when vtime > > > > doesn't run. Still it works at the cost of a seqcount read operation. > > > > > > > > Do you think you could fix it too (along with task_cputime_scaled())? I think those > > > > patches will also need a stable tag. > > > > > > Do you mean that task_cputime() and task_cputime_scaled() don't hit invalid behavior > > > but have some extra operation cost which could be removed? > > > > Exactly. > > > > > > > > Will look into it, and send patches with stable tag. > > > > Thanks a lot! > > > > Oh and another detail: vtime_accounting_enabled() checks if vtime > > accounting is done precisely on the current CPU. That's what we want to check > > when we account the time but not when we want to read the cputime of a task. > > > > For example, CPU 0 never has vtime_accounting_enabled() because it plays the > > role of timekeeper and as such it keeps the tick periodic. So if task A runs on > > CPU 1 that has vtime accounting on, and we read the cputime of task A from CPU 0, > > vtime_accounting_enabled() will be false whereas we need to compute the delta. > > > > So vtime_accounting_enabled() isn't suitable to check if vtime is running on _some_ > > CPU such that we can't return utime/stime with a raw read. > > I see the point, vtime accounting can be enabled on dedicated cpu and there is no > guarantee the reading thread is on the same state. Exactly! > > > > > Ideally we shoud rename vtime_accounting_enabled() to vtime_accounting_cpu_enabled() > > and have vtime_accounting_enabled() to check if vtime runs somewhere. But that would > > be too much an invasive change for a stable patch. So lets just use > > context_tracking_is_enabled() for now instead. > > I have dig the code. > And my understanding is that vtime_accounting_enabled() does check global flag with > context_tracking_is_enabled() and then check current cpu state with > context_tracking_cpu_is_enabled(). For now, we just check global flag to fix current > issue instead of checking both in vtime_accounting_enabled(). In future we should fix > more precisely. > Is that correct? Perfectly correct :-) Thanks!