Re: [PATCH -mm] Allow selective freezing of the system for different events

[Gautham R Shenoy <ego@in.ibm.com>]

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!"