All of lore.kernel.org
 help / color / mirror / Atom feed
From: Prarit Bhargava <prarit@redhat.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Saravana Kannan <skannan@codeaurora.org>,
	Stephen Boyd <sboyd@codeaurora.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Lenny Szubowicz <lszubowi@redhat.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
Subject: Re: [PATCH] cpufreq, store_scaling_governor requires policy->rwsem to be held for duration of changing governors [v2]
Date: Wed, 06 Aug 2014 06:09:41 -0400	[thread overview]
Message-ID: <53E1FEE5.80305@redhat.com> (raw)
In-Reply-To: <CAKohpoky1-hj7cwnKc58svh8Z1Yas1=q4uJEK8mWcb56uqAcng@mail.gmail.com>



On 08/06/2014 04:10 AM, Viresh Kumar wrote:
> On 6 August 2014 03:36, Saravana Kannan <skannan@codeaurora.org> wrote:
>> Stephen and I looked into this. This is not a sysfs framework difference.
>> The reason we don't have this issue when we use global tunables is because
>> we add the attribute group to the cpufreq_global_kobject and that kobject
>> doesn't have a kobj_type ops similar to the per policy kobject. So,
>> read/write to those attributes do NOT go through the generic show/store ops
>> that wrap every other cpufreq framework attribute read/writes.
>>
>> So, none of those read/write do any kind of locking. They don't race with
>> POLICY_EXIT (because we remove the sysfs group first thing in POLICY_EXIT)
>> but might still race with START/STOPs (not sure, haven't looked closely
>> yet).
>>
>> For example, writing to sampling_rate of ondemand governor might cause a
>> race in update_sampling_rate(). It could race and happen between a STOP and
>> POLICY_EXIT (triggered by hotplug, gov change, etc).
> 
> This sounds good but I couldn't prove it. Doing this on my dual core exynos
> doesn't give me that crash report and it should?

Are you sure you're not seeing another lockdep warning?  That was my problem --
there was an xfs related lockdep warning which then resulted in lockdep being
disabled from that point on.

P.

> 
> diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
> index 1e0ec57..027b6f7 100644
> --- a/drivers/cpufreq/exynos-cpufreq.c
> +++ b/drivers/cpufreq/exynos-cpufreq.c
> @@ -139,7 +139,7 @@ static int exynos_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
>  }
> 
>  static struct cpufreq_driver exynos_driver = {
> -       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
> +       .flags          = CPUFREQ_STICKY |
> CPUFREQ_NEED_INITIAL_FREQ_CHECK | CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>         .verify         = cpufreq_generic_frequency_table_verify,
>         .target_index   = exynos_target,
>         .get            = cpufreq_generic_get,
> 

WARNING: multiple messages have this Message-ID (diff)
From: Prarit Bhargava <prarit@redhat.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Saravana Kannan <skannan@codeaurora.org>,
	Stephen Boyd <sboyd@codeaurora.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Lenny Szubowicz <lszubowi@redhat.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
Subject: Re: [PATCH] cpufreq, store_scaling_governor requires policy->rwsem to be held for duration of changing governors [v2]
Date: Wed, 06 Aug 2014 06:09:41 -0400	[thread overview]
Message-ID: <53E1FEE5.80305@redhat.com> (raw)
In-Reply-To: <CAKohpoky1-hj7cwnKc58svh8Z1Yas1=q4uJEK8mWcb56uqAcng@mail.gmail.com>



On 08/06/2014 04:10 AM, Viresh Kumar wrote:
> On 6 August 2014 03:36, Saravana Kannan <skannan@codeaurora.org> wrote:
>> Stephen and I looked into this. This is not a sysfs framework difference.
>> The reason we don't have this issue when we use global tunables is because
>> we add the attribute group to the cpufreq_global_kobject and that kobject
>> doesn't have a kobj_type ops similar to the per policy kobject. So,
>> read/write to those attributes do NOT go through the generic show/store ops
>> that wrap every other cpufreq framework attribute read/writes.
>>
>> So, none of those read/write do any kind of locking. They don't race with
>> POLICY_EXIT (because we remove the sysfs group first thing in POLICY_EXIT)
>> but might still race with START/STOPs (not sure, haven't looked closely
>> yet).
>>
>> For example, writing to sampling_rate of ondemand governor might cause a
>> race in update_sampling_rate(). It could race and happen between a STOP and
>> POLICY_EXIT (triggered by hotplug, gov change, etc).
> 
> This sounds good but I couldn't prove it. Doing this on my dual core exynos
> doesn't give me that crash report and it should?

Are you sure you're not seeing another lockdep warning?  That was my problem --
there was an xfs related lockdep warning which then resulted in lockdep being
disabled from that point on.

P.

> 
> diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
> index 1e0ec57..027b6f7 100644
> --- a/drivers/cpufreq/exynos-cpufreq.c
> +++ b/drivers/cpufreq/exynos-cpufreq.c
> @@ -139,7 +139,7 @@ static int exynos_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
>  }
> 
>  static struct cpufreq_driver exynos_driver = {
> -       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
> +       .flags          = CPUFREQ_STICKY |
> CPUFREQ_NEED_INITIAL_FREQ_CHECK | CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>         .verify         = cpufreq_generic_frequency_table_verify,
>         .target_index   = exynos_target,
>         .get            = cpufreq_generic_get,
> 

  reply	other threads:[~2014-08-06 10:09 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-29 11:46 [PATCH] cpufreq, store_scaling_governor requires policy->rwsem to be held for duration of changing governors [v2] Prarit Bhargava
2014-07-30  0:03 ` Rafael J. Wysocki
2014-07-30 14:18   ` Prarit Bhargava
2014-07-30 21:40     ` Rafael J. Wysocki
2014-07-31  1:36       ` Saravana Kannan
2014-07-31  2:16         ` Rafael J. Wysocki
2014-07-31  2:07           ` Saravana Kannan
2014-07-31 10:16           ` Prarit Bhargava
2014-07-31 10:21             ` Prarit Bhargava
2014-07-31 10:23           ` Prarit Bhargava
2014-07-31 16:36             ` Rafael J. Wysocki
2014-07-31 17:57               ` Prarit Bhargava
2014-07-31 18:38                 ` Rafael J. Wysocki
2014-07-31 18:26                   ` Prarit Bhargava
2014-07-31 20:24                     ` Saravana Kannan
2014-07-31 20:30                       ` Prarit Bhargava
2014-07-31 20:38                         ` Saravana Kannan
2014-07-31 21:08                           ` Prarit Bhargava
2014-07-31 22:13                             ` Saravana Kannan
2014-07-31 22:58                               ` Prarit Bhargava
2014-08-01  0:55                                 ` Saravana Kannan
2014-08-01 10:24                                   ` Prarit Bhargava
2014-08-01 10:27                                   ` Prarit Bhargava
2014-08-01 17:18                                     ` Stephen Boyd
2014-08-01 19:15                                       ` Prarit Bhargava
2014-08-01 19:36                                         ` Stephen Boyd
2014-08-01 19:43                                           ` Prarit Bhargava
2014-08-01 19:54                                             ` Stephen Boyd
2014-08-01 21:25                                               ` Saravana Kannan
2014-08-04 10:11                                                 ` Prarit Bhargava
2014-08-05  7:46                                           ` Viresh Kumar
2014-08-05 10:47                                             ` Prarit Bhargava
2014-08-05 10:53                                               ` Viresh Kumar
2014-08-05 22:06                                                 ` Saravana Kannan
2014-08-05 22:20                                                   ` Prarit Bhargava
2014-08-05 22:40                                                     ` Saravana Kannan
2014-08-05 22:42                                                   ` Prarit Bhargava
2014-08-05 22:51                                                     ` Saravana Kannan
2014-08-13 19:57                                                       ` Prarit Bhargava
2014-08-13 19:57                                                         ` Prarit Bhargava
2014-08-14 18:16                                                         ` Saravana Kannan
2014-08-14 18:16                                                           ` Saravana Kannan
2014-08-06  8:10                                                   ` Viresh Kumar
2014-08-06 10:09                                                     ` Prarit Bhargava [this message]
2014-08-06 10:09                                                       ` Prarit Bhargava
2014-08-06 15:08                                                       ` Stephen Boyd
2014-08-07  6:36                                                         ` Viresh Kumar
2014-08-07 10:12                                                           ` Prarit Bhargava
2014-08-07 10:15                                                             ` Viresh Kumar
2014-08-12  9:03                                                               ` Viresh Kumar
2014-08-12 11:33                                                                 ` Prarit Bhargava
2014-08-13  7:39                                                                   ` Viresh Kumar
2014-08-13  9:58                                                                     ` Prarit Bhargava
2014-08-13  9:58                                                                       ` Prarit Bhargava
2014-08-14  4:19                                                                       ` Viresh Kumar
2014-08-04 10:36                                       ` Viresh Kumar
2014-08-04 12:25                                         ` Prarit Bhargava
2014-08-04 13:38                                           ` Viresh Kumar
2014-08-04 14:00                                             ` Prarit Bhargava
2014-08-04 15:04                                               ` Prarit Bhargava
2014-08-04 20:16                                             ` Saravana Kannan
2014-08-05  6:14                                               ` Viresh Kumar
2014-08-05  6:29                                                 ` skannan
2014-08-05  6:43                                                   ` Viresh Kumar
2014-10-13 10:43                                       ` Viresh Kumar
2014-10-13 11:52                                         ` Prarit Bhargava

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=53E1FEE5.80305@redhat.com \
    --to=prarit@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lszubowi@redhat.com \
    --cc=rjw@rjwysocki.net \
    --cc=sboyd@codeaurora.org \
    --cc=skannan@codeaurora.org \
    --cc=viresh.kumar@linaro.org \
    /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.