Re: [PATCH v3 22/22] thermal/intel_powerclamp: Convert the kthread to kthread worker API

[Petr Mladek]

On Tue 2016-01-12 08:20:21, Jacob Pan wrote:
> On Tue, 12 Jan 2016 11:11:29 +0100
> Petr Mladek <pmladek@xxxxxxxx> wrote:
> 
> > > > BTW: I wonder if the original code correctly handle freezing after
> > > > the schedule_timeout(). It does not call try_to_freeze()
> > > > there and the forced idle states might block freezing.
> > > > I think that the small overhead of kthread works is worth
> > > > solving such bugs. It makes it easier to maintain these
> > > > sleeping states.  
> > > it is in a while loop, so try_to_freeze() gets called. Am I missing
> > > something?  
> > 
> > But it might take some time until try_to_freeze() is called.
> > If I get it correctly. try_to_freeze_tasks() wakes freezable
> > tasks to get them into the fridge. If clamp_thread() is waken
> > from that schedule_timeout_interruptible(), it still might inject
> > the idle state before calling try_to_freeze(). It means that freezer
> > needs to wait "quite" some time until the kthread ends up in the
> > fridge.
> > 
> > Hmm, even my conversion does not solve this entirely. We might
> > need to call freezing(current) in the
> > 
> >    while (time_before(jiffies, target_jiffies)) {
> > 
> > cycle. And break injecting the idle state when freezing is requested.
> 
> The injection time for each period is very short, default 6ms. While on
> the other side the default freeze timeout is 20 sec. So I think task
> freeze can wait :)
> i.e.
> unsigned int __read_mostly freeze_timeout_msecs = 20 * MSEC_PER_SEC;

You are right. And it does not make sense to add an extra
freezer-specific code if not really necessary.

Otherwise, I will keep the conversion into the kthread worker as is
for now. Please, let me know if you are strongly against the split
into the two works.


Best Regards,
Petr