lkml.org 
[lkml]   [2018]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH V2] powercap/drivers/idle_injection: Add an idle injection framework
On Fri, May 11, 2018 at 03:02:21PM +0530, viresh kumar wrote:
> On 10-05-18, 14:26, Daniel Lezcano wrote:
> > Initially, the cpu_cooling device for ARM was changed by adding a new
> > policy inserting idle cycles. The intel_powerclamp driver does a
> > similar action.
> >
> > Instead of implementing idle injections privately in the cpu_cooling
> > device, move the idle injection code in a dedicated framework and give
> > the opportunity to other frameworks to make use of it.
>
> Maybe move the above explanation in the comments section below, as it
> doesn't belong to the commit log really.

Yes that makes sense.

[ ... ]

> > diff --git a/drivers/powercap/idle_injection.c b/drivers/powercap/idle_injection.c
> > new file mode 100644
> > index 0000000..825ffac
> > --- /dev/null
> > +++ b/drivers/powercap/idle_injection.c
> > @@ -0,0 +1,331 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * drivers/powercap/idle_injection.c
> > + *
>
> What's the purpose of this particular line ? Yeah, I have seen it
> quite a number of times and might have added that myself as well
> somewhere :)

I think it is a hundredth monkey effect :)

[ ... ]

> > + /*
> > + * Boolean used by the smpboot mainloop and used as a flip-flop
>
> s/mainloop/main loop/ ?
>
> > + * in this function
> > + */
> > + iit->should_run = 0;
>
> What is the purpose of this field ? Just wanted to check on how
> smpboot stuff uses it.

This field is used as a boolean for the smpboot main loop.

You should look at the function smpboot_thread_fn().

The function idle_injection_thread_fn() idle is called every idle injection
period, sets a timer for the next idle injection deadline and tells the
smpboot main loop to schedule. The way to tell the smpboot main loop to
schedule and not call the idle_injection_thread_fn() again is by setting this
boolean to false.

static int smpboot_thread_fn(void *data)
{
while (1) {

[ ... ]

if (!ht->thread_should_run(td->cpu)) {
preempt_enable_no_resched();
schedule();
} else {
__set_current_state(TASK_RUNNING);
preempt_enable();
ht->thread_fn(td->cpu);
}

[ ... ]
}
}

[ ... ]

> > + */
> > +void idle_injection_set_duration(struct idle_injection_device *ii_dev,
> > + unsigned int run_duration_ms,
> > + unsigned int idle_duration_ms)
> > +{
> > + atomic_set(&ii_dev->run_duration_ms, run_duration_ms);
> > + atomic_set(&ii_dev->idle_duration_ms, idle_duration_ms);
> > +}
> > +
> > +
>
> Two blank lines here ?

Ah, yes.

[ ... ]

> > +int idle_injection_start(struct idle_injection_device *ii_dev)
> > +{
> > + if (!atomic_read(&ii_dev->idle_duration_ms))
> > + return -EINVAL;
> > +
> > + if (!atomic_read(&ii_dev->run_duration_ms))
> > + return -EINVAL;
>
> You are required to have them as atomic variables to take care of the
> races while they are set ?

The race is when you change the values, while the idle injection is acting and
you read those values in the idle injection thread function.

> What about getting the durations as arguments to this routine then ?

May be I missed your point but I don't think that will change the above.

[ ... ]

> > +/*
> > + * idle_injection_threads - smp hotplug threads ops
> > + */
> > +static struct smp_hotplug_thread idle_injection_threads = {
> > + .store = &idle_injection_thread.tsk,
> > + .thread_fn = idle_injection_fn,
> > + .thread_should_run = idle_injection_should_run,
> > + .setup = idle_injection_setup,
> > +};
>
> Why should we keep this structure at all and not have these four
> assignments in the below routine itself ? It is unnecessarily copying
> a bigger structure while it is required to copy only a part of it. And
> then we keep wasting memory for this particular instance without any
> use.

With your comment below about registering the smpboot threads with a cpumask, a
single structure should be used, I will fix this.

> > +
> > +/**
> > + * idle_injection_register - idle injection init routine
> > + * @cpumask: the list of CPUs to run the kthreads
> > + * @name: the threads command name
> > + *
> > + * This is the initialization function in charge of creating the
> > + * kthreads, initializing the timer and allocate the structures. It
> > + * does not starts the idle injection cycles
>
> Forgot full stop (.). Please check that across file

Oops, right. I rememeber you did a similar comment on the previous version.

> > + *
> > + * Returns -ENOMEM if an allocation fails, or < 0 if the smpboot
>
> It should be Return:

Ok.

> > + * kthread registering fails.
> > + */
> > +struct idle_injection_device *
> > +idle_injection_register(struct cpumask *cpumask, const char *name)
> > +{
> > + struct idle_injection_device *ii_dev;
> > + struct smp_hotplug_thread *smp_hotplug_thread;
> > + char *idle_injection_comm;
> > + int cpu, ret;
> > +
> > + ret = -ENOMEM;
>
> Maybe merge it earlier only ?

What do you mean ? int ret = -ENOMEM ?

> > +
> > + idle_injection_comm = kasprintf(GFP_KERNEL, "%s/%%u", name);
> > + if (!idle_injection_comm)
> > + goto out;
> > +
> > + smp_hotplug_thread = kmemdup(&idle_injection_threads,
> > + sizeof(*smp_hotplug_thread),
> > + GFP_KERNEL);
> > + if (!smp_hotplug_thread)
> > + goto out_free_thread_comm;
> > +
> > + smp_hotplug_thread->thread_comm = idle_injection_comm;
> > +
> > + ii_dev = kzalloc(sizeof(*ii_dev),
> > + GFP_KERNEL);
>
> Accidental line break ?

grumf ...

> > + if (!ii_dev)
> > + goto out_free_smp_hotplug;
> > +
> > + ii_dev->smp_hotplug_thread = smp_hotplug_thread;
> > +
> > + hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +
> > + ii_dev->timer.function = idle_injection_wakeup_fn;
> > +
> > + for_each_cpu(cpu, cpumask)
> > + per_cpu(idle_injection_device, cpu) = ii_dev;
> > +
> > + ret = smpboot_register_percpu_thread_cpumask(smp_hotplug_thread,
> > + cpumask);
>
> This creates the thread for all online CPUs and unparks only the one
> in the cpumask. I am not sure how the smpboot stuff works, but this
> looks somewhat wrong to me, we may end up creating multiple threads
> per CPU even if this function is called twice for non-intersecting cpu
> masks.

Good point! That must be fixed, calling idle_injection_register()
one time and idle_injection_start() with a cpumask would be more convenient.

> > + if (ret)
> > + goto out_free_idle_inject;
> > +
> > + return ii_dev;
> > +
> > +out_free_idle_inject:
> > + kfree(ii_dev);
>
> What about resetting per-cpu idle_injection_device ? You missed the
> same in unregister routine below as well ?

Ok.

> > +out_free_smp_hotplug:
> > + kfree(smp_hotplug_thread);
> > +out_free_thread_comm:
> > + kfree(idle_injection_comm);
> > +out:
> > + return ERR_PTR(ret);
> > +}
> > +
> > +/**
> > + * idle_injection_unregister - Unregister the idle injection device
> > + * @ii_dev: a pointer to an idle injection device
> > + *
> > + * The function is in charge of stopping the idle injections,
> > + * unregister the kthreads and free the allocated memory in the
> > + * register function.
> > + */
> > +void idle_injection_unregister(struct idle_injection_device *ii_dev)
> > +{
> > + struct smp_hotplug_thread *smp_hotplug_thread;
> > +
> > + idle_injection_stop(ii_dev);
> > + smp_hotplug_thread = ii_dev->smp_hotplug_thread;
> > + smpboot_unregister_percpu_thread(smp_hotplug_thread);
> > + kfree(smp_hotplug_thread->thread_comm);
> > + kfree(smp_hotplug_thread);
> > + kfree(ii_dev);
>
> Ideally it is much more readable if the ordering here is exactly
> opposite of how things are done in registration time. You may need to
> change the order in which things are allocated, and that would be
> worth it :)

Ok.

> > +}
> > diff --git a/include/linux/idle_injection.h b/include/linux/idle_injection.h
> > new file mode 100644
> > index 0000000..084b999
> > --- /dev/null
> > +++ b/include/linux/idle_injection.h
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2018 Linaro Ltd
> > + *
> > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> > + *
> > + */
> > +#ifndef __IDLE_INJECTION_H__
> > +#define __IDLE_INJECTION_H__
> > +
> > +/* private idle injection device structure */
> > +struct idle_injection_device;
> > +
> > +/**
> > + * idle_injection_register - allocates and initializes an idle_injection_device
> > + * @cpumask: all CPUs with a idle injection kthreads
> > + * @name: a const string giving the kthread name
> > + *
> > + * Returns a pointer to a idle_injection_device, ERR_PTR otherwise.
> This needs to be written as "Return: XXXX.", else it wouldn't get
> documented properly in kernel doc.
>
> And I am wondering on why you have added all these kernel doc comments
> in the .h file ? What will kernel doc look like as we will have to doc
> comments for the same function ? Maybe try generating the doc and you
> will see how it looks.

Hmm, right. I will remove the documentation in the header.

Thanks for the review.

-- Daniel

--

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
\
 
 \ /
  Last update: 2018-05-11 13:56    [W:0.042 / U:5.708 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site