lkml.org 
[lkml]   [2007]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2007-04-30 21:23    [W:0.044 / U:1.000 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site