From patchwork Fri Dec 4 06:11:01 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 57699 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp357809lbb; Thu, 3 Dec 2015 22:11:52 -0800 (PST) X-Received: by 10.66.254.73 with SMTP id ag9mr19326524pad.116.1449209512623; Thu, 03 Dec 2015 22:11:52 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w77si17136797pfi.227.2015.12.03.22.11.52; Thu, 03 Dec 2015 22:11:52 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dkim=neutral (body hash did not verify) header.i=@linaro-org.20150623.gappssmtp.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751172AbbLDGLH (ORCPT + 11 others); Fri, 4 Dec 2015 01:11:07 -0500 Received: from mail-pf0-f178.google.com ([209.85.192.178]:32801 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750929AbbLDGLG (ORCPT ); Fri, 4 Dec 2015 01:11:06 -0500 Received: by pfnn128 with SMTP id n128so21079036pfn.0 for ; Thu, 03 Dec 2015 22:11:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=RBkU5AVz9K273RLg5phkrDkDTrjWYUlwE8BYlUqxAEU=; b=sMSy2xD/MUb1kPfVhVKhXMGgk9y4yxrIgEDDrPU5wsXozC0vAW1KTNXZv/61nx5nTR O9UuwKIWmO+V41TlQsSQyOkNSY7BWrVyrVb1DmXKYn2c6e26SKVmOk0j3d6oWLZ+bZk4 PPx2ofWZEQSsjayHipyQIVyBdyRbbdZ7dsdd4HpHgv4j+MCfGlI3M1sxBq3Oj8MifhBB rfOWnEu15mmzp9IyfPi0Cz6qvJVj3cG2VRnIWORldwcaidvTM30jGD9VXpimBarAydV6 NfydE+UdYyXTlYLcQs1x4U9Uek+Y9br+GzzEjG9RUUAR+CYGGnbyXwiLG/6+AUxCJQME BKAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=RBkU5AVz9K273RLg5phkrDkDTrjWYUlwE8BYlUqxAEU=; b=gKDzn/ew+VKZr50Sq9nNCwTgZ7cZ7KJpojVf9F07QbGqJkzVKcv8DfHNCvrsTn2hqt j6PR4NbCRaiWzVrt9RigWS3i/oVhttYedh2Jm1b6J+c3ehLSlGeXsN8OvyfuDIbXDeS2 7Pifzv7ftMn3+P5iEEGi8+VMhLp+jYMm1NWN+qLiUFlvQ7SB66BVn9resWqyP9A4hq8x HTn9zW7ukFR3pUq870v8Oc8spkVlPZ2L6f59kzmbZL22vRz+R6t+iMwjMt9QF5XXV2zS zSSbcQBfwRmGC5JPuVUue7w7zvaiy0ZgaefisbCzv4cMDz8jft4lgTu7xVo927PO1+C+ 9neg== X-Gm-Message-State: ALoCoQnCWV0uGJW+au1bLu6G0D0MJ2OvAxTW+X1WLTZ6O+GXP2UuWKfDMf+0tQVBOioyR/H3kRrn X-Received: by 10.98.18.130 with SMTP id 2mr18844804pfs.114.1449209464831; Thu, 03 Dec 2015 22:11:04 -0800 (PST) Received: from localhost ([122.172.170.156]) by smtp.gmail.com with ESMTPSA id tm4sm14434050pab.3.2015.12.03.22.11.03 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 03 Dec 2015 22:11:04 -0800 (PST) Date: Fri, 4 Dec 2015 11:41:01 +0530 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, ashwin.chaugule@linaro.org, "Rafael J. Wysocki" , open list Subject: Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers Message-ID: <20151204061101.GA3430@ubuntu> References: <691a3524afaa8d08d61987b1c2913948b4e59f01.1449115453.git.viresh.kumar@linaro.org> <10439879.00aCyM9quW@vostro.rjw.lan> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <10439879.00aCyM9quW@vostro.rjw.lan> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On 04-12-15, 02:18, Rafael J. Wysocki wrote: > > + shared->skip_work--; > > Is there any reason for incrementing and decrementing this instead of setting > it to either 0 or 1 (or maybe either 'true' or 'false' for that matter)? > > If my reading of the patch is correct, it can only be either 0 or 1 anyway, right? No. It can be 0, 1 or 2. If the timer handler is running on any CPU, we increment skip_work, so its value is 1. If at the same time, we try to stop the governor, we increment it again and its value is 2 now. Once timer-handler finishes, it decrements it and its value become 1. Which guarantees that no other timer handler starts executing at this point of time and we can safely do gov_cancel_timers(). And once we are sure that we don't have any work/timer left, we make it 0 (as we aren't sure of the current value, which can be 0 (if the timer handler wasn't running when we stopped the governor) or 1 (if the timer handler was running while stopping the governor)). Hope this clarifies it. > > +static void dbs_timer_handler(unsigned long data) > > +{ > > + struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data; > > + struct cpu_common_dbs_info *shared = cdbs->shared; > > + struct cpufreq_policy *policy; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&shared->timer_lock, flags); > > + policy = shared->policy; > > Why do we need policy here? > > > + > > + /* > > + * Timer handler isn't allowed to queue work at the moment, because: > > + * - Another timer handler has done that > > + * - We are stopping the governor > > + * - Or we are updating the sampling rate of ondemand governor > > + */ > > + if (shared->skip_work) > > + goto unlock; > > + > > + shared->skip_work++; > > + queue_work(system_wq, &shared->work); > > > > unlock: > > What about writing the above as > > if (!shared->work_in_progress) { > shared->work_in_progress = true; > queue_work(system_wq, &shared->work); > } > > and then you won't need the unlock label. Here is a diff for that: I will resend this patch now. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index a3f9bc9b98e9..c9e420bd0eec 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -265,11 +265,9 @@ static void dbs_timer_handler(unsigned long data) { struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data; struct cpu_common_dbs_info *shared = cdbs->shared; - struct cpufreq_policy *policy; unsigned long flags; spin_lock_irqsave(&shared->timer_lock, flags); - policy = shared->policy; /* * Timer handler isn't allowed to queue work at the moment, because: @@ -277,13 +275,11 @@ static void dbs_timer_handler(unsigned long data) * - We are stopping the governor * - Or we are updating the sampling rate of ondemand governor */ - if (shared->skip_work) - goto unlock; - - shared->skip_work++; - queue_work(system_wq, &shared->work); + if (!shared->skip_work) { + shared->skip_work++; + queue_work(system_wq, &shared->work); + } -unlock: spin_unlock_irqrestore(&shared->timer_lock, flags); }