| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090703022518.GB26976@Krystal>
Date: Thu, 2 Jul 2009 22:25:18 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To: "Pallipadi, Venkatesh" <venkatesh.pallipadi@...el.com>
Cc: Dave Jones <davej@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
"kernel-testers@...r.kernel.org" <kernel-testers@...r.kernel.org>,
Ingo Molnar <mingo@...e.hu>, "Rafael J. Wysocki" <rjw@...k.pl>,
Dave Young <hidave.darkstar@...il.com>,
Pekka Enberg <penberg@...helsinki.fi>,
Thomas Renninger <trenn@...e.de>
Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in
cpufreq
* Pallipadi, Venkatesh (venkatesh.pallipadi@...el.com) wrote:
>
>
> >-----Original Message-----
> >From: Mathieu Desnoyers [mailto:mathieu.desnoyers@...ymtl.ca]
> >Sent: Thursday, July 02, 2009 6:07 PM
> >To: Pallipadi, Venkatesh
> >Cc: Dave Jones; linux-kernel@...r.kernel.org;
> >cpufreq@...r.kernel.org; kernel-testers@...r.kernel.org; Ingo
> >Molnar; Rafael J. Wysocki; Dave Young; Pekka Enberg; Thomas Renninger
> >Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep
> >warnings in cpufreq
> >
> >* venkatesh.pallipadi@...el.com (venkatesh.pallipadi@...el.com) wrote:
> >> Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very
> >> much needed to properly cleanup ondemand timer, opened-up a
> >can of worms
> >> related to locking dependencies in cpufreq.
> >>
> >> Patch here defines the need for dbs_mutex and cleans up its usage in
> >> ondemand governor. This also resolves the lockdep warnings
> >reported here
> >>
> >> http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/01925.html
> >> http://lkml.indiana.edu/hypermail/linux/kernel/0907.0/00820.html
> >>
> >> and few others..
> >>
> >> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@...el.com>
> >> ---
> >> drivers/cpufreq/cpufreq.c | 4 ++--
> >> drivers/cpufreq/cpufreq_conservative.c | 27
> >+++++++++++----------------
> >> drivers/cpufreq/cpufreq_ondemand.c | 27
> >+++++++++++----------------
> >> 3 files changed, 24 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index 6e2ec0b..c7fe16e 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct
> >sys_device *sys_dev)
> >> spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> >> #endif
> >>
> >> - unlock_policy_rwsem_write(cpu);
> >> -
> >> if (cpufreq_driver->target)
> >> __cpufreq_governor(data, CPUFREQ_GOV_STOP);
> >>
> >> @@ -1088,6 +1086,8 @@ static int __cpufreq_remove_dev(struct
> >sys_device *sys_dev)
> >> if (cpufreq_driver->exit)
> >> cpufreq_driver->exit(data);
> >>
> >> + unlock_policy_rwsem_write(cpu);
> >> +
> >> free_cpumask_var(data->related_cpus);
> >> free_cpumask_var(data->cpus);
> >> kfree(data);
> >> diff --git a/drivers/cpufreq/cpufreq_conservative.c
> >b/drivers/cpufreq/cpufreq_conservative.c
> >> index 7fc58af..58889f2 100644
> >> --- a/drivers/cpufreq/cpufreq_conservative.c
> >> +++ b/drivers/cpufreq/cpufreq_conservative.c
> >> @@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct
> >cpu_dbs_info_s, cpu_dbs_info);
> >> static unsigned int dbs_enable; /* number of CPUs using
> >this policy */
> >>
> >> /*
> >> - * DEADLOCK ALERT! There is a ordering requirement between
> >cpu_hotplug
> >> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> >> - * dbs_mutex. If any function that can potentially take
> >cpu_hotplug lock
> >> - * (like __cpufreq_driver_target()) is being called with
> >dbs_mutex taken, then
> >> - * cpu_hotplug lock should be taken before that. Note that
> >cpu_hotplug lock
> >> - * is recursive for the same process. -Venki
> >> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the
> >dbs_mutex, because it
> >> - * would deadlock with cancel_delayed_work_sync(), which is
> >needed for proper
> >> - * raceless workqueue teardown.
> >> + * dbs_mutex protects data in dbs_tuners_ins from
> >concurrent changes on
> >> + * different CPUs. It protects dbs_enable in governor
> >start/stop. It also
> >> + * serializes governor limit_change with do_dbs_timer. We
> >do not want
> >> + * do_dbs_timer to run when user is changing the governor or limits.
> >> */
> >> static DEFINE_MUTEX(dbs_mutex);
> >>
> >> @@ -488,18 +483,17 @@ static void do_dbs_timer(struct
> >work_struct *work)
> >>
> >> delay -= jiffies % delay;
> >>
> >> - if (lock_policy_rwsem_write(cpu) < 0)
> >> - return;
> >> + mutex_lock(&dbs_mutex);
> >
> >OK, I now have absolutely no idea what the rwsem mutex is protecting
> >anymore.
> >
> >You should probably describe the new world order not just in terms of
> >what the dbs_mutex is protecting, but also about what the rwsem is
> >doing. I'm worried that this rwsem is there to protect against
> >more than
> >what is protected by the dbs_mutex local to the ondemand/conservative
> >governors.
> >
> >See below,
> >
> >>
> >> if (!dbs_info->enable) {
> >> - unlock_policy_rwsem_write(cpu);
> >> + mutex_unlock(&dbs_mutex);
> >> return;
> >> }
> >>
> >> dbs_check_cpu(dbs_info);
> >>
> >> queue_delayed_work_on(cpu, kconservative_wq,
> >&dbs_info->work, delay);
> >> - unlock_policy_rwsem_write(cpu);
> >> + mutex_unlock(&dbs_mutex);
> >> }
> >>
> >> static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> >> @@ -590,15 +584,16 @@ static int cpufreq_governor_dbs(struct
> >cpufreq_policy *policy,
> >> &dbs_cpufreq_notifier_block,
> >> CPUFREQ_TRANSITION_NOTIFIER);
> >> }
> >> - dbs_timer_init(this_dbs_info);
> >> -
> >> mutex_unlock(&dbs_mutex);
> >>
> >> + dbs_timer_init(this_dbs_info);
> >> +
> >> break;
> >>
> >> case CPUFREQ_GOV_STOP:
> >> - mutex_lock(&dbs_mutex);
> >> dbs_timer_exit(this_dbs_info);
> >
> >So now the only thing that seems to prevent the init and exit to race
> >with each other is the rwsem. But this does not seem to be described
> >anywhere.
>
> Mathieu,
>
> Yes. rwsem in cpufreq core makes sure that START and STOP happen sequentially. There
> Is no way for START and STOP for a CPU to happen at the same time as cpufreq core holds
> per policy rwsem lock before making any change to the policy. I can add a comment to
> that effect in cpufreq.c. This is a clean seperation across cpufreq core and governor,
> as cpufreq core takes care of all the policy changes. With that, do you see any
> Issues/races with this patchset?
>
I'll code a less intrusive patchset that should hopefully fix the
problem tonight with less possible side-effects. I'll need help for
testing though.
Matheu
> Thanks,
> Venki
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists