On Sun, Apr 29, 2007 at 07:51:04PM +0200, Rafael J. Wysocki wrote:
> Hi,
>
> Sorry for the delay.
No problems! Even I was out for the weekend.
> > /*
> > * Tell the freezer to exempt this task from freezing
> > + * for events in freeze_event_mask.
> > */
> > -static inline void freezer_exempt(struct task_struct *p)
>
> I, personally, would introduce
>
> static inline void freezer_exempt_event(struct task_struct *p,
> unsigned long freeze_event_mask)
> {
> atomic_set_mask(freeze_event_mask, &p->freezer_flags);
> }
>
> and then
>
> static inline void freezer_exempt(struct task_struct *p)
> {
> freezer_exempt_event(p, FE_ALL);
> }
>
> The patch would be shorter. ;-)
>
Agreed. Will do that.
> [In that case I'd probably rename freezer_should_exempt() to
> freezer_should_exempt_event(), for symmetry.]
>
Ok.
> > +
> > +static inline int thawable(struct task_struct *p)
> > +{
> > + if (!freezeable(p))
> > + return 0;
> > +
> > + /* Thaw p iff it is frozen for current_freezer_event alone */
> > + if (process_frozen_event_mask(p) & ~current_freezer_event)
> > + return 0;
> > +
> > + return 1;
>
> I would do
>
> return !(process_frozen_event_mask(p) & ~current_freezer_event);
I was wondering if the statement
if (process_frozen_event_mask(p) & ~current_freezer_event)
return 0;
would be readable in the first place!
Yeah, we can do what you have suggested.
> > -int freeze_processes(void)
> > +int freeze_processes(unsigned long freeze_event)
> > {
> > - unsigned int nr_unfrozen;
> > + unsigned int nr_unfrozen = 0;
> > +
> > + mutex_lock(&freezer_mutex);
> > + if (system_frozen_event_mask & freeze_event)
> > + goto out;
> > +
> > + current_freezer_event = freeze_event;
> >
> > printk("Stopping tasks ... ");
> > nr_unfrozen = try_to_freeze_tasks(FREEZER_USER_SPACE);
> > if (nr_unfrozen)
> > - return nr_unfrozen;
> > + goto out;
> >
> > sys_sync();
> > nr_unfrozen = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
> > if (nr_unfrozen)
> > - return nr_unfrozen;
> > + goto out;
> >
> > + system_frozen_event_mask |= current_freezer_event;
> > printk("done.\n");
> > BUG_ON(in_atomic());
>
> The BUG_ON() is still valid if tasks are already frozen for this event.
Right! So we would need one more label. How about the following?
mutex_lock(&freezer_mutex);
/* check if already frozen for the event */
if (system_frozen_event_mask & freeze_event)
goto out_frozen;
.
.
.
out_frozen:
BUG_ON(in_atomic());
out:
current_freezer_event = 0;
mutex_unlock(&freezer_mutex);
return nr_unfrozen;
}
>
> > -void thaw_processes(void)
> > +void thaw_processes(unsigned long thaw_event)
> > {
> > + mutex_lock(&freezer_mutex);
> > + if (!(system_frozen_event_mask & thaw_event)) {
> > + WARN_ON(1);
>
> Hmm, I wouldn't use the WARN_ON() here. There's nothing wrong in calling
> this twice in a row as long as we do the sanity checking. There's even one
> case in which that may be convenient, actually.
Well, yes. But I put the warn on from the perspective of someone trying
to thaw_processes for the event for which they have not frozen. I hadn't
thought about a double thaw. Will rethink.
Thanks for the Review.
Regards
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
