Messages in this thread | | | Date | Tue, 1 May 2007 00:49:34 +0530 | From | Gautham R Shenoy <> | Subject | Re: [PATCH -mm] Allow selective freezing of the system for different events |
| |
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 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/
| |