Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups

[Saravana Kannan]

On 02/04/2016 09:43 AM, Saravana Kannan wrote:
> On 02/04/2016 03:09 AM, Viresh Kumar wrote:
> > On 04-02-16, 00:50, Rafael J. Wysocki wrote:
> > > This is exactly right.  We've avoided one deadlock only to trip into
> > > another one.
> > >
> > > This happens because update_sampling_rate() acquires
> > > od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
> > > cpufreq_governor_dbs().
> > >
> > > Worse yet, a deadlock can still happen without (the new)
> > > dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
> > > update_sampling_rate() runs in parallel with
> > > cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
> > > the race.
> > >
> > > It looks like we need to drop the governor mutex before putting the
> > > kobject in cpufreq_governor_exit().
> >
> > I have tried to explore all possible ways of fixing this, and every
> > other way looked to be racy in some way.
> >
> > Does anyone else have a better idea (untested):
> >
> > -------------------------8<-------------------------
> >
> > Subject: [PATCH] cpufreq: ondemand: Shoot update_sampling_rate with a
> > separate
> >   work
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > ---
> >   drivers/cpufreq/cpufreq_governor.h |  2 ++
> >   drivers/cpufreq/cpufreq_ondemand.c | 39
> > +++++++++++++++++++++++++++++---------
> >   2 files changed, 32 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq_governor.h
> > b/drivers/cpufreq/cpufreq_governor.h
> > index 7bed63e14e7d..97e604356b20 100644
> > --- a/drivers/cpufreq/cpufreq_governor.h
> > +++ b/drivers/cpufreq/cpufreq_governor.h
> > @@ -141,6 +141,8 @@ struct od_dbs_tuners {
> >       unsigned int powersave_bias;
> >       unsigned int io_is_busy;
> >       unsigned int min_sampling_rate;
> > +    struct work_struct work;
> > +    struct dbs_data *dbs_data;
> >   };
> >
> >   struct cs_dbs_tuners {
> > diff --git a/drivers/cpufreq/cpufreq_ondemand.c
> > b/drivers/cpufreq/cpufreq_ondemand.c
> > index 82ed490f7de0..93ad7a226aee 100644
> > --- a/drivers/cpufreq/cpufreq_ondemand.c
> > +++ b/drivers/cpufreq/cpufreq_ondemand.c
> > @@ -242,20 +242,27 @@ static struct common_dbs_data od_dbs_cdata;
> >    * reducing the sampling rate, we need to make the new value effective
> >    * immediately.
> >    */
> > -static void update_sampling_rate(struct dbs_data *dbs_data,
> > -        unsigned int new_rate)
> > +static void update_sampling_rate(struct work_struct *work)
> >   {
> > -    struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> > +    struct od_dbs_tuners *od_tuners = container_of(work, struct
> > +                               od_dbs_tuners, work);
> > +    unsigned int new_rate = od_tuners->sampling_rate;
> > +    struct dbs_data *dbs_data = od_tuners->dbs_data;
> >       struct cpumask cpumask;
> >       int cpu;
> >
> > -    od_tuners->sampling_rate = new_rate = max(new_rate,
> > -            od_tuners->min_sampling_rate);
> > -
> >       /*
> >        * Lock governor so that governor start/stop can't execute in
> > parallel.
> > +     *
> > +     * We can't do a regular mutex_lock() here, as that may deadlock
> > against
> > +     * another thread performing CPUFREQ_GOV_POLICY_EXIT event on the
> > +     * governor, which might have already taken od_dbs_cdata.mutex
> > and is
> > +     * waiting for this work to finish.
> >        */
> > -    mutex_lock(&od_dbs_cdata.mutex);
> > +    if (!mutex_trylock(&od_dbs_cdata.mutex)) {
> > +        queue_work(system_wq, &od_tuners->work);
> > +        return;
> > +    }
> >
> >       cpumask_copy(&cpumask, cpu_online_mask);
> >
> > @@ -311,13 +318,22 @@ static void update_sampling_rate(struct dbs_data
> > *dbs_data,
> >   static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const
> > char *buf,
> >           size_t count)
> >   {
> > +    struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> >       unsigned int input;
> >       int ret;
> >       ret = sscanf(buf, "%u", &input);
> >       if (ret != 1)
> >           return -EINVAL;
> >
> > -    update_sampling_rate(dbs_data, input);
> > +    od_tuners->sampling_rate = max(input, od_tuners->min_sampling_rate);
> > +
> > +    /*
> > +     * update_sampling_rate() requires to hold od_dbs_cdata.mutex,
> > but we
> > +     * can't take that from this thread, otherwise it results in ABBA
> > +     * lockdep between s_active and od_dbs_cdata.mutex locks.
> > +     */
> > +    queue_work(system_wq, &od_tuners->work);
> > +
> >       return count;
> >   }
> >
> > @@ -501,6 +517,8 @@ static int od_init(struct dbs_data *dbs_data, bool
> > notify)
> >       tuners->ignore_nice_load = 0;
> >       tuners->powersave_bias = default_powersave_bias;
> >       tuners->io_is_busy = should_io_be_busy();
> > +    INIT_WORK(&tuners->work, update_sampling_rate);
> > +    tuners->dbs_data = dbs_data;
> >
> >       dbs_data->tuners = tuners;
> >       return 0;
> > @@ -508,7 +526,10 @@ static int od_init(struct dbs_data *dbs_data,
> > bool notify)
> >
> >   static void od_exit(struct dbs_data *dbs_data, bool notify)
> >   {
> > -    kfree(dbs_data->tuners);
> > +    struct od_dbs_tuners *tuners = dbs_data->tuners;
> > +
> > +    cancel_work_sync(&tuners->work);
> > +    kfree(tuners);
> >   }
> >
> >   define_get_cpu_dbs_routines(od_cpu_dbs_info);
>
> No no no no! Let's not open up this can of worms of queuing up the work
> to handle a write to a sysfs file. It *MIGHT* work for this specific
> tunable (I haven't bothered to analyze), but this makes it impossible to
> return a useful/proper error value.

Sent too soon. Not only that, but it can also cause the writes to the 
sysfs files to get processed in a different order and I don't know what 
other issues/races THAT will open up.

-Saravana

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project