Re: [PATCH V2] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On Tue, May 15, 2018 at 10:42:41AM +0530, viresh kumar wrote:
> On 11-05-18, 13:55, Daniel Lezcano wrote:
> > On Fri, May 11, 2018 at 03:02:21PM +0530, viresh kumar wrote:
> > > On 10-05-18, 14:26, Daniel Lezcano wrote:
> > > > +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.
> 
> Well, it can. Can you explain the kind of use-cases you have in mind ?
> 
> For example, what I assumed to be the usecase was:
> 
> idle_injection_start(iidev, idle_duration, run_duration);
> 
> and then at some point of time:
> 
> idle_injection_stop(iidev);
> 
> 
> With this, you would be required to stop the idle injection stuff to
> reconfigure the durations. And then you will never have a race which
> you mentioned above.
> 
> What you are trying to do is something like this:
> 
> idle_injection_set_duration(idle_duration, run_duration);
> idle_injection_start(iidev);
> 
> and then at some point of time:
> idle_injection_set_duration(idle_duration2, run_duration2);

Here potentially hundred of times between a start and a stop, and at a high
rate.
 
> and then at some point of time:
> idle_injection_stop(iidev);
> 
> I am not sure if we would ever want to do something like this. Or if
> stopping the idle injection to start it again is that bad of an idea.

It sounds strange, it is like you change the speed of the car by stopping and
restarting the motor.
 
> > > > +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 ?
> 
> Yes.

Ok.

-- 

 <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