lkml.org 
[lkml]   [2007]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/8] Enhance process freezer interface for usage beyond software suspend
Hi!

> > > +/* Per process freezer specific flags */
> > > +#define PF_FE_SUSPEND 0x00008000 /* This thread should not be frozen
> > > + * for suspend
> > > + */
> > > +
> > > +#define PF_FE_KPROBES 0x00000010 /* This thread should not be frozen
> > > + * for Kprobes
> > > + */
> >
> > Just put the comment before the define for long comments?
>
> Agreed.

(Actually it would be nice to say

/* This thread should not be frozen for suspend, becuase it is needed
for getting image saved to disk */

> > > -#ifdef CONFIG_PM
> > > +#if defined(CONFIG_PM) || defined(CONFIG_HOTPLUG_CPU) || \
> > > + defined(CONFIG_KPROBES)
> >
> > Should we create CONFIG_FREEZER?
>
> Why do you think so? I think the freezer should be compiled automatically
> if any of the above is set, which is what this directive really means.

Kconfig can do that. ("select statement"). If we have one such ifdef,
it is okay, but if it would be more of them.

> > Hmmm, I do not really like softlockup watchdog running during suspend.
> > Can we make this freezeable and make watchdog shut itself off while
> > suspending?
>
> Generally, I agree, but this patch only replaces the existing instances
> of PF_NOFREEZE with the new mechanism. The changes you're talking about
> require a separate patch series (or at least one separate patch), I think, and
> they need not be so simple to make.

Agreed about separate patch series.

> > > - current->flags |= PF_NOFREEZE;
> > > + freezer_exempt(FE_ALL);
> > > pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
> > > if (pid > 0) {
> > > while (pid != sys_wait4(-1, NULL, 0, NULL))
> >
> > Does this mean we have userland /linuxrc running with PF_NOFREEZE?
> > That would be very bad...
>
> No, actually it is _required_ for the userland resume to work. Well, perhaps
> I should place a comment in there so that I don't have to explain this again
> and again. :-)

Can you put big bold comment there?

Why is it needed? Freezer never freezes _current_ task.

> > > @@ -104,7 +104,7 @@ static int __kprobes check_safety(void)
> > > {
> > > int ret = 0;
> > > #if defined(CONFIG_PREEMPT) && defined(CONFIG_PM)
> >
> > Eh? Why does kprobes code depend on config_pm?
>
> Because it uses the freezer? ;-)

That is no longer true after this patch... Ugly ifdef above makes sure
freezer is there for kprobes. I'm trying to say that #if above is
now broken. Actually it was probably always broken, but it just became
more so.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-04-06 16:05    [W:0.115 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site