Re: [PATCH V7 1/9] Add kswapd descriptor

[Ying Han <yinghan@xxxxxxxxxx>]

On Thu, Apr 21, 2011 at 9:47 PM, KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> 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