On Thu, Apr 21, 2011 at 9:47 PM, KOSAKI Motohiro <
kosaki.motohiro@jp.fujitsu.com> wrote:

> Hi,
>
> This seems to have no ugly parts.
>

Thank you for reviewing.

>
>
> nitpick:
>
> > -     const struct cpumask *cpumask = cpumask_of_node(pgdat->node_id);
> > +     const struct cpumask *cpumask;
> >
> >       lockdep_set_current_reclaim_state(GFP_KERNEL);
> >
> > +     cpumask = cpumask_of_node(pgdat->node_id);
>
> no effect change?
>

yes, will change .

>
>
> >       if (!cpumask_empty(cpumask))
> >               set_cpus_allowed_ptr(tsk, cpumask);
> >       current->reclaim_state = &reclaim_state;
> > @@ -2679,7 +2684,7 @@ static int kswapd(void *p)
> >                       order = new_order;
> >                       classzone_idx = new_classzone_idx;
> >               } else {
> > -                     kswapd_try_to_sleep(pgdat, order, classzone_idx);
> > +                     kswapd_try_to_sleep(kswapd_p, order,
> classzone_idx);
> >                       order = pgdat->kswapd_max_order;
> >                       classzone_idx = pgdat->classzone_idx;
> >                       pgdat->kswapd_max_order = 0;
> > @@ -2817,12 +2822,20 @@ static int __devinit cpu_callback(struct
> notifier_block *nfb,
> >               for_each_node_state(nid, N_HIGH_MEMORY) {
> >                       pg_data_t *pgdat = NODE_DATA(nid);
> >                       const struct cpumask *mask;
> > +                     struct kswapd *kswapd_p;
> > +                     struct task_struct *kswapd_tsk;
> > +                     wait_queue_head_t *wait;
> >
> >                       mask = cpumask_of_node(pgdat->node_id);
> >
> > +                     wait = &pgdat->kswapd_wait;
>
> In kswapd_try_to_sleep(), this waitqueue is called wait_h. Can you
> please keep naming consistency?
>
> Ok.

>
> > +                     kswapd_p = pgdat->kswapd;
> > +                     kswapd_tsk = kswapd_p->kswapd_task;
> > +
> >                       if (cpumask_any_and(cpu_online_mask, mask) <
> nr_cpu_ids)
> >                               /* One of our CPUs online: restore mask */
> > -                             set_cpus_allowed_ptr(pgdat->kswapd, mask);
> > +                             if (kswapd_tsk)
> > +                                     set_cpus_allowed_ptr(kswapd_tsk,
> mask);
>
> Need adding commnets. What mean kswapd_tsk==NULL and When it occur.
> I'm apologize if it done at later patch.
>

I don't think i have comments on later patch. will add.

--Ying