From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail203.messagelabs.com (mail203.messagelabs.com [216.82.254.243]) by kanga.kvack.org (Postfix) with ESMTP id BEED88D003B for ; Fri, 22 Apr 2011 01:55:25 -0400 (EDT) Received: from wpaz21.hot.corp.google.com (wpaz21.hot.corp.google.com [172.24.198.85]) by smtp-out.google.com with ESMTP id p3M5tN5t010325 for ; Thu, 21 Apr 2011 22:55:23 -0700 Received: from qwf7 (qwf7.prod.google.com [10.241.194.71]) by wpaz21.hot.corp.google.com with ESMTP id p3M5tM6g004593 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Thu, 21 Apr 2011 22:55:22 -0700 Received: by qwf7 with SMTP id 7so186642qwf.38 for ; Thu, 21 Apr 2011 22:55:22 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20110422134804.FA5E.A69D9226@jp.fujitsu.com> References: <1303446260-21333-1-git-send-email-yinghan@google.com> <1303446260-21333-2-git-send-email-yinghan@google.com> <20110422134804.FA5E.A69D9226@jp.fujitsu.com> Date: Thu, 21 Apr 2011 22:55:21 -0700 Message-ID: Subject: Re: [PATCH V7 1/9] Add kswapd descriptor From: Ying Han Content-Type: multipart/alternative; boundary=0016e65dc8ece0053104a17b80c6 Sender: owner-linux-mm@kvack.org List-ID: To: KOSAKI Motohiro Cc: Minchan Kim , Daisuke Nishimura , Balbir Singh , Tejun Heo , Pavel Emelyanov , KAMEZAWA Hiroyuki , Andrew Morton , Li Zefan , Mel Gorman , Christoph Lameter , Johannes Weiner , Rik van Riel , Hugh Dickins , Michal Hocko , Dave Hansen , Zhu Yanhai , linux-mm@kvack.org --0016e65dc8ece0053104a17b80c6 Content-Type: text/plain; charset=ISO-8859-1 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 --0016e65dc8ece0053104a17b80c6 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

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.=A0


nitpick:

> - =A0 =A0 const struct cpumask *cpumask =3D cpumask_of_node(pgdat->= node_id);
> + =A0 =A0 const struct cpumask *cpumask;
>
> =A0 =A0 =A0 lockdep_set_current_reclaim_state(GFP_KERNEL);
>
> + =A0 =A0 cpumask =3D cpumask_of_node(pgdat->node_id);

no effect change?

yes, will chang= e .


> =A0 =A0 =A0 if (!cpumask_empty(cpumask))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 set_cpus_allowed_ptr(tsk, cpumask);
> =A0 =A0 =A0 current->reclaim_state =3D &reclaim_state;
> @@ -2679,7 +2684,7 @@ static int kswapd(void *p)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 order =3D new_order;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 classzone_idx =3D new_clas= szone_idx;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 kswapd_try_to_sleep(pgdat, o= rder, classzone_idx);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 kswapd_try_to_sleep(kswapd_p= , order, classzone_idx);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 order =3D pgdat->kswapd= _max_order;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 classzone_idx =3D pgdat-&g= t;classzone_idx;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pgdat->kswapd_max_order= =3D 0;
> @@ -2817,12 +2822,20 @@ static int __devinit cpu_callback(struct notif= ier_block *nfb,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 for_each_node_state(nid, N_HIGH_MEMORY) {<= br> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pg_data_t *pgdat =3D NODE_= DATA(nid);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const struct cpumask *mask= ;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct kswapd *kswapd_p;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct task_struct *kswapd_t= sk;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 wait_queue_head_t *wait;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mask =3D cpumask_of_node(p= gdat->node_id);
>
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 wait =3D &pgdat->kswa= pd_wait;

In kswapd_try_to_sleep(), this waitqueue is called wait_h. Can you please keep naming consistency?

Ok.=A0

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 kswapd_p =3D pgdat->kswap= d;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 kswapd_tsk =3D kswapd_p->= kswapd_task;
> +
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (cpumask_any_and(cpu_on= line_mask, mask) < nr_cpu_ids)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* One of = our CPUs online: restore mask */
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 set_cpus_all= owed_ptr(pgdat->kswapd, mask);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (kswapd_t= sk)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 set_cpus_allowed_ptr(kswapd_tsk, mask);

Need adding commnets. What mean kswapd_tsk=3D=3DNULL and When it occu= r.
I'm apologize if it done at later patch.

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

--Ying=A0

--0016e65dc8ece0053104a17b80c6-- -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org