| Message ID | 20200119030636.11899-8-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/migrate.c: cleanup on do_pages_move() | expand |
On Sun 19-01-20 11:06:35, Wei Yang wrote: > When page is not successfully queued for migration, we would move pages > on pagelist immediately. Actually, this could be done in the next > iteration by telling it we need some help. > > This patch adds a new variable need_move to be an indication. After > this, we only need to call move_pages_and_store_status() twice. No! Not another iterator. There are two and this makes the function quite hard to follow already. We do not want to add a third one. > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > --- > mm/migrate.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index aee5aeb082c4..2a857fec65b6 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1610,6 +1610,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > LIST_HEAD(pagelist); > int start, i; > int err = 0, err1; > + int need_move = 0; > > migrate_prep(); > > @@ -1641,13 +1642,15 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > if (current_node == NUMA_NO_NODE) { > current_node = node; > start = i; > - } else if (node != current_node) { > + } else if (node != current_node || need_move) { > err = move_pages_and_store_status(mm, current_node, > - &pagelist, status, start, i - start); > + &pagelist, status, start, > + i - start - need_move); > if (err) > goto out; > start = i; > current_node = node; > + need_move = 0; > } > > /* > @@ -1662,6 +1665,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > continue; > } > > + /* Ask next iteration to move us */ > + need_move = 1; > + > /* > * Two possible cases for err here: > * == 0: page is already on the target node, then store > @@ -1671,17 +1677,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > err = store_status(status, i, err ? : current_node, 1); > if (err) > goto out_flush; > - > - err = move_pages_and_store_status(mm, current_node, &pagelist, > - status, start, i - start); > - if (err) > - goto out; > - current_node = NUMA_NO_NODE; > } > out_flush: > /* Make sure we do not overwrite the existing error */ > err1 = move_pages_and_store_status(mm, current_node, &pagelist, > - status, start, i - start); > + status, start, i - start - need_move); > if (err >= 0) > err = err1; > out: > -- > 2.17.1 >
On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote: >On Sun 19-01-20 11:06:35, Wei Yang wrote: >> When page is not successfully queued for migration, we would move pages >> on pagelist immediately. Actually, this could be done in the next >> iteration by telling it we need some help. >> >> This patch adds a new variable need_move to be an indication. After >> this, we only need to call move_pages_and_store_status() twice. > >No! Not another iterator. There are two and this makes the function >quite hard to follow already. We do not want to add a third one. > Two iterators here are? I don't get your point. >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> --- >> mm/migrate.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index aee5aeb082c4..2a857fec65b6 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1610,6 +1610,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> LIST_HEAD(pagelist); >> int start, i; >> int err = 0, err1; >> + int need_move = 0; >> >> migrate_prep(); >> >> @@ -1641,13 +1642,15 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> if (current_node == NUMA_NO_NODE) { >> current_node = node; >> start = i; >> - } else if (node != current_node) { >> + } else if (node != current_node || need_move) { >> err = move_pages_and_store_status(mm, current_node, >> - &pagelist, status, start, i - start); >> + &pagelist, status, start, >> + i - start - need_move); >> if (err) >> goto out; >> start = i; >> current_node = node; >> + need_move = 0; >> } >> >> /* >> @@ -1662,6 +1665,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> continue; >> } >> >> + /* Ask next iteration to move us */ >> + need_move = 1; >> + >> /* >> * Two possible cases for err here: >> * == 0: page is already on the target node, then store >> @@ -1671,17 +1677,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> err = store_status(status, i, err ? : current_node, 1); >> if (err) >> goto out_flush; >> - >> - err = move_pages_and_store_status(mm, current_node, &pagelist, >> - status, start, i - start); >> - if (err) >> - goto out; >> - current_node = NUMA_NO_NODE; >> } >> out_flush: >> /* Make sure we do not overwrite the existing error */ >> err1 = move_pages_and_store_status(mm, current_node, &pagelist, >> - status, start, i - start); >> + status, start, i - start - need_move); >> if (err >= 0) >> err = err1; >> out: >> -- >> 2.17.1 >> > >-- >Michal Hocko >SUSE Labs
On Tue 21-01-20 09:22:00, Wei Yang wrote: > On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote: > >On Sun 19-01-20 11:06:35, Wei Yang wrote: > >> When page is not successfully queued for migration, we would move pages > >> on pagelist immediately. Actually, this could be done in the next > >> iteration by telling it we need some help. > >> > >> This patch adds a new variable need_move to be an indication. After > >> this, we only need to call move_pages_and_store_status() twice. > > > >No! Not another iterator. There are two and this makes the function > >quite hard to follow already. We do not want to add a third one. > > > > Two iterators here are? I don't get your point. i is the main iterator to process the whole imput. start is another one to control the batch to migrate. We do not need/want more. More clear?
On Tue, Jan 21, 2020 at 09:43:52AM +0100, Michal Hocko wrote: >On Tue 21-01-20 09:22:00, Wei Yang wrote: >> On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote: >> >On Sun 19-01-20 11:06:35, Wei Yang wrote: >> >> When page is not successfully queued for migration, we would move pages >> >> on pagelist immediately. Actually, this could be done in the next >> >> iteration by telling it we need some help. >> >> >> >> This patch adds a new variable need_move to be an indication. After >> >> this, we only need to call move_pages_and_store_status() twice. >> > >> >No! Not another iterator. There are two and this makes the function >> >quite hard to follow already. We do not want to add a third one. >> > >> >> Two iterators here are? I don't get your point. > >i is the main iterator to process the whole imput. start is another one >to control the batch to migrate. We do not need/want more. More clear? Yes, more clear. I hope you are angry with me. Sorry for my poor English. >-- >Michal Hocko >SUSE Labs
On Wed 22-01-20 08:40:12, Wei Yang wrote: > On Tue, Jan 21, 2020 at 09:43:52AM +0100, Michal Hocko wrote: > >On Tue 21-01-20 09:22:00, Wei Yang wrote: > >> On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote: > >> >On Sun 19-01-20 11:06:35, Wei Yang wrote: > >> >> When page is not successfully queued for migration, we would move pages > >> >> on pagelist immediately. Actually, this could be done in the next > >> >> iteration by telling it we need some help. > >> >> > >> >> This patch adds a new variable need_move to be an indication. After > >> >> this, we only need to call move_pages_and_store_status() twice. > >> > > >> >No! Not another iterator. There are two and this makes the function > >> >quite hard to follow already. We do not want to add a third one. > >> > > >> > >> Two iterators here are? I don't get your point. > > > >i is the main iterator to process the whole imput. start is another one > >to control the batch to migrate. We do not need/want more. More clear? > > Yes, more clear. Great > I hope you are angry with me. Sorry for my poor English. Heh, not at all.
diff --git a/mm/migrate.c b/mm/migrate.c index aee5aeb082c4..2a857fec65b6 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1610,6 +1610,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, LIST_HEAD(pagelist); int start, i; int err = 0, err1; + int need_move = 0; migrate_prep(); @@ -1641,13 +1642,15 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, if (current_node == NUMA_NO_NODE) { current_node = node; start = i; - } else if (node != current_node) { + } else if (node != current_node || need_move) { err = move_pages_and_store_status(mm, current_node, - &pagelist, status, start, i - start); + &pagelist, status, start, + i - start - need_move); if (err) goto out; start = i; current_node = node; + need_move = 0; } /* @@ -1662,6 +1665,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, continue; } + /* Ask next iteration to move us */ + need_move = 1; + /* * Two possible cases for err here: * == 0: page is already on the target node, then store @@ -1671,17 +1677,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err = store_status(status, i, err ? : current_node, 1); if (err) goto out_flush; - - err = move_pages_and_store_status(mm, current_node, &pagelist, - status, start, i - start); - if (err) - goto out; - current_node = NUMA_NO_NODE; } out_flush: /* Make sure we do not overwrite the existing error */ err1 = move_pages_and_store_status(mm, current_node, &pagelist, - status, start, i - start); + status, start, i - start - need_move); if (err >= 0) err = err1; out:
When page is not successfully queued for migration, we would move pages on pagelist immediately. Actually, this could be done in the next iteration by telling it we need some help. This patch adds a new variable need_move to be an indication. After this, we only need to call move_pages_and_store_status() twice. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- mm/migrate.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)