[PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[chengang]

From: Chen Gang <gang.chen.5i5j@gmail.com>

For migrate_misplaced_page and migrate_misplaced_transhuge_page, they
are pure Boolean functions and are also used as pure Boolean functions,
but the related dummy functions return -EAGAIN.

Also change their related pure Boolean function numamigrate_isolate_page.

For variable isolated in migrate_misplaced_transhuge_page, it need not
be initialized.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 include/linux/migrate.h | 16 ++++++++--------
 mm/migrate.c            | 24 ++++++++++++------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index ae8d475..b5e791d 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -88,34 +88,34 @@ static inline void __ClearPageMovable(struct page *page)
 
 #ifdef CONFIG_NUMA_BALANCING
 extern bool pmd_trans_migrating(pmd_t pmd);
-extern int migrate_misplaced_page(struct page *page,
-				  struct vm_area_struct *vma, int node);
+extern bool migrate_misplaced_page(struct page *page,
+				   struct vm_area_struct *vma, int node);
 #else
 static inline bool pmd_trans_migrating(pmd_t pmd)
 {
 	return false;
 }
-static inline int migrate_misplaced_page(struct page *page,
-					 struct vm_area_struct *vma, int node)
+static inline bool migrate_misplaced_page(struct page *page,
+					  struct vm_area_struct *vma, int node)
 {
-	return -EAGAIN; /* can't migrate now */
+	return false;
 }
 #endif /* CONFIG_NUMA_BALANCING */
 
 #if defined(CONFIG_NUMA_BALANCING) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
-extern int migrate_misplaced_transhuge_page(struct mm_struct *mm,
+extern bool migrate_misplaced_transhuge_page(struct mm_struct *mm,
 			struct vm_area_struct *vma,
 			pmd_t *pmd, pmd_t entry,
 			unsigned long address,
 			struct page *page, int node);
 #else
-static inline int migrate_misplaced_transhuge_page(struct mm_struct *mm,
+static inline bool migrate_misplaced_transhuge_page(struct mm_struct *mm,
 			struct vm_area_struct *vma,
 			pmd_t *pmd, pmd_t entry,
 			unsigned long address,
 			struct page *page, int node)
 {
-	return -EAGAIN;
+	return false;
 }
 #endif /* CONFIG_NUMA_BALANCING && CONFIG_TRANSPARENT_HUGEPAGE*/
 
diff --git a/mm/migrate.c b/mm/migrate.c
index f7ee04a..3cdaa19 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1805,7 +1805,7 @@ static bool numamigrate_update_ratelimit(pg_data_t *pgdat,
 	return false;
 }
 
-static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
+static bool numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
 	int page_lru;
 
@@ -1813,10 +1813,10 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 
 	/* Avoid migrating to a node that is nearly full */
 	if (!migrate_balanced_pgdat(pgdat, 1UL << compound_order(page)))
-		return 0;
+		return false;
 
 	if (isolate_lru_page(page))
-		return 0;
+		return false;
 
 	/*
 	 * migrate_misplaced_transhuge_page() skips page migration's usual
@@ -1827,7 +1827,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 	 */
 	if (PageTransHuge(page) && page_count(page) != 3) {
 		putback_lru_page(page);
-		return 0;
+		return false;
 	}
 
 	page_lru = page_is_file_cache(page);
@@ -1840,7 +1840,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 	 * disappearing underneath us during migration.
 	 */
 	put_page(page);
-	return 1;
+	return true;
 }
 
 bool pmd_trans_migrating(pmd_t pmd)
@@ -1854,11 +1854,11 @@ bool pmd_trans_migrating(pmd_t pmd)
  * node. Caller is expected to have an elevated reference count on
  * the page that will be dropped by this function before returning.
  */
-int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
+bool migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 			   int node)
 {
 	pg_data_t *pgdat = NODE_DATA(node);
-	int isolated;
+	bool isolated;
 	int nr_remaining;
 	LIST_HEAD(migratepages);
 
@@ -1893,7 +1893,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 					page_is_file_cache(page));
 			putback_lru_page(page);
 		}
-		isolated = 0;
+		isolated = false;
 	} else
 		count_vm_numa_event(NUMA_PAGE_MIGRATE);
 	BUG_ON(!list_empty(&migratepages));
@@ -1901,7 +1901,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 
 out:
 	put_page(page);
-	return 0;
+	return false;
 }
 #endif /* CONFIG_NUMA_BALANCING */
 
@@ -1910,7 +1910,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
  * Migrates a THP to a given target node. page must be locked and is unlocked
  * before returning.
  */
-int migrate_misplaced_transhuge_page(struct mm_struct *mm,
+bool migrate_misplaced_transhuge_page(struct mm_struct *mm,
 				struct vm_area_struct *vma,
 				pmd_t *pmd, pmd_t entry,
 				unsigned long address,
@@ -1918,7 +1918,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 {
 	spinlock_t *ptl;
 	pg_data_t *pgdat = NODE_DATA(node);
-	int isolated = 0;
+	bool isolated;
 	struct page *new_page = NULL;
 	int page_lru = page_is_file_cache(page);
 	unsigned long mmun_start = address & HPAGE_PMD_MASK;
@@ -2052,7 +2052,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 out_unlock:
 	unlock_page(page);
 	put_page(page);
-	return 0;
+	return false;
 }
 #endif /* CONFIG_NUMA_BALANCING */
 
-- 
1.9.3

Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[Michal Hocko]

On Sat 17-09-16 15:20:36, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> For migrate_misplaced_page and migrate_misplaced_transhuge_page, they
> are pure Boolean functions and are also used as pure Boolean functions,
> but the related dummy functions return -EAGAIN.

I agree that their return semantic is rather confusing wrt. how they are
used but

> Also change their related pure Boolean function numamigrate_isolate_page.

this is not true. Just look at the current usage

	migrated = migrate_misplaced_page(page, vma, target_nid);
	if (migrated) {
		page_nid = target_nid;
		flags |= TNF_MIGRATED;
	} else
		flags |= TNF_MIGRATE_FAIL;

and now take your change which changes -EAGAIN into false. See the
difference? Now I didn't even try to understand why
CONFIG_NUMA_BALANCING=n pretends a success but then in order to keep the
current semantic your patch should return true in that path. So NAK from
me until you either explain why this is OK or change it.

But to be honest I am not keen of this int -> bool changes much.
Especially if they are bringing a risk of subtle behavior change like
this patch. And without a good changelog explaining why this makes
sense.

> For variable isolated in migrate_misplaced_transhuge_page, it need not
> be initialized.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  include/linux/migrate.h | 16 ++++++++--------
>  mm/migrate.c            | 24 ++++++++++++------------
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index ae8d475..b5e791d 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -88,34 +88,34 @@ static inline void __ClearPageMovable(struct page *page)
>  
>  #ifdef CONFIG_NUMA_BALANCING
>  extern bool pmd_trans_migrating(pmd_t pmd);
> -extern int migrate_misplaced_page(struct page *page,
> -				  struct vm_area_struct *vma, int node);
> +extern bool migrate_misplaced_page(struct page *page,
> +				   struct vm_area_struct *vma, int node);
>  #else
>  static inline bool pmd_trans_migrating(pmd_t pmd)
>  {
>  	return false;
>  }
> -static inline int migrate_misplaced_page(struct page *page,
> -					 struct vm_area_struct *vma, int node)
> +static inline bool migrate_misplaced_page(struct page *page,
> +					  struct vm_area_struct *vma, int node)
>  {
> -	return -EAGAIN; /* can't migrate now */
> +	return false;
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
>  
>  #if defined(CONFIG_NUMA_BALANCING) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
> -extern int migrate_misplaced_transhuge_page(struct mm_struct *mm,
> +extern bool migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  			struct vm_area_struct *vma,
>  			pmd_t *pmd, pmd_t entry,
>  			unsigned long address,
>  			struct page *page, int node);
>  #else
> -static inline int migrate_misplaced_transhuge_page(struct mm_struct *mm,
> +static inline bool migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  			struct vm_area_struct *vma,
>  			pmd_t *pmd, pmd_t entry,
>  			unsigned long address,
>  			struct page *page, int node)
>  {
> -	return -EAGAIN;
> +	return false;
>  }
>  #endif /* CONFIG_NUMA_BALANCING && CONFIG_TRANSPARENT_HUGEPAGE*/
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f7ee04a..3cdaa19 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1805,7 +1805,7 @@ static bool numamigrate_update_ratelimit(pg_data_t *pgdat,
>  	return false;
>  }
>  
> -static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
> +static bool numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  {
>  	int page_lru;
>  
> @@ -1813,10 +1813,10 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  
>  	/* Avoid migrating to a node that is nearly full */
>  	if (!migrate_balanced_pgdat(pgdat, 1UL << compound_order(page)))
> -		return 0;
> +		return false;
>  
>  	if (isolate_lru_page(page))
> -		return 0;
> +		return false;
>  
>  	/*
>  	 * migrate_misplaced_transhuge_page() skips page migration's usual
> @@ -1827,7 +1827,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  	 */
>  	if (PageTransHuge(page) && page_count(page) != 3) {
>  		putback_lru_page(page);
> -		return 0;
> +		return false;
>  	}
>  
>  	page_lru = page_is_file_cache(page);
> @@ -1840,7 +1840,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  	 * disappearing underneath us during migration.
>  	 */
>  	put_page(page);
> -	return 1;
> +	return true;
>  }
>  
>  bool pmd_trans_migrating(pmd_t pmd)
> @@ -1854,11 +1854,11 @@ bool pmd_trans_migrating(pmd_t pmd)
>   * node. Caller is expected to have an elevated reference count on
>   * the page that will be dropped by this function before returning.
>   */
> -int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
> +bool migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
>  			   int node)
>  {
>  	pg_data_t *pgdat = NODE_DATA(node);
> -	int isolated;
> +	bool isolated;
>  	int nr_remaining;
>  	LIST_HEAD(migratepages);
>  
> @@ -1893,7 +1893,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
>  					page_is_file_cache(page));
>  			putback_lru_page(page);
>  		}
> -		isolated = 0;
> +		isolated = false;
>  	} else
>  		count_vm_numa_event(NUMA_PAGE_MIGRATE);
>  	BUG_ON(!list_empty(&migratepages));
> @@ -1901,7 +1901,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
>  
>  out:
>  	put_page(page);
> -	return 0;
> +	return false;
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
>  
> @@ -1910,7 +1910,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
>   * Migrates a THP to a given target node. page must be locked and is unlocked
>   * before returning.
>   */
> -int migrate_misplaced_transhuge_page(struct mm_struct *mm,
> +bool migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  				struct vm_area_struct *vma,
>  				pmd_t *pmd, pmd_t entry,
>  				unsigned long address,
> @@ -1918,7 +1918,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  {
>  	spinlock_t *ptl;
>  	pg_data_t *pgdat = NODE_DATA(node);
> -	int isolated = 0;
> +	bool isolated;
>  	struct page *new_page = NULL;
>  	int page_lru = page_is_file_cache(page);
>  	unsigned long mmun_start = address & HPAGE_PMD_MASK;
> @@ -2052,7 +2052,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  out_unlock:
>  	unlock_page(page);
>  	put_page(page);
> -	return 0;
> +	return false;
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
>  
> -- 
> 1.9.3
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[Chen Gang]

On 9/17/16 23:46, Michal Hocko wrote:
> On Sat 17-09-16 15:20:36, chengang@emindsoft.com.cn wrote:
> 
>> Also change their related pure Boolean function numamigrate_isolate_page.
> 
> this is not true. Just look at the current usage
> 
> 	migrated = migrate_misplaced_page(page, vma, target_nid);
> 	if (migrated) {
> 		page_nid = target_nid;
> 		flags |= TNF_MIGRATED;
> 	} else
> 		flags |= TNF_MIGRATE_FAIL;
> 
> and now take your change which changes -EAGAIN into false. See the
> difference? Now I didn't even try to understand why
> CONFIG_NUMA_BALANCING=n pretends a success but then in order to keep the
> current semantic your patch should return true in that path. So NAK from
> me until you either explain why this is OK or change it.
>

For me, it really need return false:

 - For real implementation, when do nothing, it will return false.

 - I assume that the input page already is in a node (although maybe my
   assumption incorrect), and migrate to the same node. When the real
   implementation fails (e.g. -EAGAIN 10 times), it still returns false.

 - Original dummy implementation always return -EAGAIN, And -EAGAIN in
   real implementation will trigger returning false, after 10 times.

 - After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in
   task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and
   numa_faults_locality, I guess they are only used for statistics.

So for me the dummy implementation need return false instead of -EAGAIN.
 
> But to be honest I am not keen of this int -> bool changes much.
> Especially if they are bringing a risk of subtle behavior change like
> this patch. And without a good changelog explaining why this makes
> sense.
> 

If our original implementation already used bool, our this issue (return
-EAGAIN) would be avoided (compiler would help us to find this issue).


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[Michal Hocko]

On Tue 20-09-16 05:46:58, Chen Gang wrote:
> On 9/17/16 23:46, Michal Hocko wrote:
> > On Sat 17-09-16 15:20:36, chengang@emindsoft.com.cn wrote:
> > 
> >> Also change their related pure Boolean function numamigrate_isolate_page.
> > 
> > this is not true. Just look at the current usage
> > 
> > 	migrated = migrate_misplaced_page(page, vma, target_nid);
> > 	if (migrated) {
> > 		page_nid = target_nid;
> > 		flags |= TNF_MIGRATED;
> > 	} else
> > 		flags |= TNF_MIGRATE_FAIL;
> > 
> > and now take your change which changes -EAGAIN into false. See the
> > difference? Now I didn't even try to understand why
> > CONFIG_NUMA_BALANCING=n pretends a success but then in order to keep the
> > current semantic your patch should return true in that path. So NAK from
> > me until you either explain why this is OK or change it.
> >
> 
> For me, it really need return false:
> 
>  - For real implementation, when do nothing, it will return false.
> 
>  - I assume that the input page already is in a node (although maybe my
>    assumption incorrect), and migrate to the same node. When the real
>    implementation fails (e.g. -EAGAIN 10 times), it still returns false.
> 
>  - Original dummy implementation always return -EAGAIN, And -EAGAIN in
>    real implementation will trigger returning false, after 10 times.
> 
>  - After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in
>    task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and
>    numa_faults_locality, I guess they are only used for statistics.
> 
> So for me the dummy implementation need return false instead of -EAGAIN.

I see that the return value semantic might be really confusing. But I am
not sure why bool would make it all of the sudden any less confusing.
migrate_page returns -EAGAIN on failure and 0 on success, migrate_pages
returns -EAGAIN or number of not migrated pages on failure and 0 on
success. So migrate_misplaced_page doesn't fit into this mode with the
bool return value. So I would argue that the code is not any better.

> > But to be honest I am not keen of this int -> bool changes much.
> > Especially if they are bringing a risk of subtle behavior change like
> > this patch. And without a good changelog explaining why this makes
> > sense.
> > 
> 
> If our original implementation already used bool, our this issue (return
> -EAGAIN) would be avoided (compiler would help us to find this issue).

OK, so you pushed me to look into it deeper and the fact is that
migrate_misplaced_page return value doesn't matter at all for
CONFIG_NUMA_BALANCING=n because task_numa_fault is noop for that
configuration. Moreover the whole do_numa_page should never execute with
that configuration because we will not have numa pte_protnone() ptes in
that path. do_huge_pmd_numa_page seems be in a similar case. So this
doesn't have any real impact on the runtime AFAICS.

So what is the point of this whole exercise? Do not take me wrong, this
area could see some improvements but I believe that doing int->bool
change is not just the right thing to do and worth spending both your
and reviewers time.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[Chen Gang]

On 9/20/16 16:09, Michal Hocko wrote:
> On Tue 20-09-16 05:46:58, Chen Gang wrote:
>>
>> For me, it really need return false:
>>
>>  - For real implementation, when do nothing, it will return false.
>>
>>  - I assume that the input page already is in a node (although maybe my
>>    assumption incorrect), and migrate to the same node. When the real
>>    implementation fails (e.g. -EAGAIN 10 times), it still returns false.
>>
>>  - Original dummy implementation always return -EAGAIN, And -EAGAIN in
>>    real implementation will trigger returning false, after 10 times.
>>
>>  - After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in
>>    task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and
>>    numa_faults_locality, I guess they are only used for statistics.
>>
>> So for me the dummy implementation need return false instead of -EAGAIN.
> 
> I see that the return value semantic might be really confusing. But I am
> not sure why bool would make it all of the sudden any less confusing.
> migrate_page returns -EAGAIN on failure and 0 on success, migrate_pages
> returns -EAGAIN or number of not migrated pages on failure and 0 on
> success. So migrate_misplaced_page doesn't fit into this mode with the
> bool return value. So I would argue that the code is not any better.
> 

I guess, numamigrate_isolate_page can be bool, at least.

And yes, commonly, bool functions are for asking something, and int
functions are for doing something, but not must be. When the caller care
about success, but never care about every failure details, bool is OK.

In our case, for me, numa balancing is for performance. When return
failure, the system has no any negative effect -- only lose a chance for
improving performance.

 - For user, the failure times statistics is enough, they need not care
   about every failure details.

 - For tracer, the failure details statistics are meaningfulness, but
   focusing on each failure details is meaningless. Now, it finishes a
   part of failure details statistics -- which can be improved next.

 - For debugger (or printing log), focusing on each failure details is
   useful. But debugger already can check every details, returning every
   failure details is still a little helpful, but not necessary.

>>
>> If our original implementation already used bool, our this issue (return
>> -EAGAIN) would be avoided (compiler would help us to find this issue).
> 
> OK, so you pushed me to look into it deeper and the fact is that
> migrate_misplaced_page return value doesn't matter at all for
> CONFIG_NUMA_BALANCING=n because task_numa_fault is noop for that
> configuration. Moreover the whole do_numa_page should never execute with
> that configuration because we will not have numa pte_protnone() ptes in
> that path. do_huge_pmd_numa_page seems be in a similar case. So this
> doesn't have any real impact on the runtime AFAICS.
> 

OK, thanks.

> So what is the point of this whole exercise? Do not take me wrong, this
> area could see some improvements but I believe that doing int->bool
> change is not just the right thing to do and worth spending both your
> and reviewers time.
> 

I am not quite sure about that.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[Michal Hocko]

On Wed 21-09-16 06:06:44, Chen Gang wrote:
> On 9/20/16 16:09, Michal Hocko wrote:
[...]

skipping the large part of the email because I do not have a spare time
to discuss this.

> > So what is the point of this whole exercise? Do not take me wrong, this
> > area could see some improvements but I believe that doing int->bool
> > change is not just the right thing to do and worth spending both your
> > and reviewers time.
> > 
> 
> I am not quite sure about that.

Maybe you should listen to the feedback your are getting. I do not think
I am not the first one here.

Look, MM surely needs some man power. There are issues to be solved,
patches to review. Doing the cleanups is really nice but there are more
serious problems to solve first. If you want to help then starting
with review would be much much more helpful and hugely appreciated. We
are really lacking people there a _lot_. Just generating more work for
reviewers with something that doesn't make any real difference in the
runtime is far less helpful IMHO.

Thanks.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[Vlastimil Babka]

On 09/21/2016 10:11 AM, Michal Hocko wrote:
> On Wed 21-09-16 06:06:44, Chen Gang wrote:
>> On 9/20/16 16:09, Michal Hocko wrote:
> [...]
>
> skipping the large part of the email because I do not have a spare time
> to discuss this.
>
>>> So what is the point of this whole exercise? Do not take me wrong, this
>>> area could see some improvements but I believe that doing int->bool
>>> change is not just the right thing to do and worth spending both your
>>> and reviewers time.
>>>
>>
>> I am not quite sure about that.
>
> Maybe you should listen to the feedback your are getting. I do not think
> I am not the first one here.
>
> Look, MM surely needs some man power. There are issues to be solved,
> patches to review. Doing the cleanups is really nice but there are more
> serious problems to solve first. If you want to help then starting
> with review would be much much more helpful and hugely appreciated. We
> are really lacking people there a _lot_. Just generating more work for
> reviewers with something that doesn't make any real difference in the
> runtime is far less helpful IMHO.

Agreed, thanks.

> Thanks.
>

Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions

[Chen Gang]

Firstly, excuse me for replying late -- since I also agree, this patch
is not urgent ;-)
 
On 9/21/16 16:11, Michal Hocko wrote:
> On Wed 21-09-16 06:06:44, Chen Gang wrote:
>> On 9/20/16 16:09, Michal Hocko wrote:
> [...]
> 
> skipping the large part of the email because I do not have a spare time
> to discuss this.
>

I agree, they are not urgent, so if we have no time on it, just leave
it.

But for me, they are still important (not urgent != not important), so
every member can continue to discuss about it, when he/she have time,
e.g. Do we have another better solving way for this issue?

>>> So what is the point of this whole exercise? Do not take me wrong, this
>>> area could see some improvements but I believe that doing int->bool
>>> change is not just the right thing to do and worth spending both your
>>> and reviewers time.
>>>
>>
>> I am not quite sure about that.
> 
> Maybe you should listen to the feedback your are getting. I do not think
> I am not the first one here.
> 

OK, for me, normally, when a mailing list contents 100+ members, every
feedback has not only one member (especially, we have about 10K members).

> Look, MM surely needs some man power. There are issues to be solved,
> patches to review. Doing the cleanups is really nice but there are more
> serious problems to solve first.

OK, we really need a task management, for me, we need notice about the
urgent and important. If the patch or issue is either urgent nor
important, we can just drop it.

If they are not urgent, but still important, just discuss about it when
have time, but do not forget it (I guess, quite a few of volunteers can
not for urgent things -- their time resources are not stable, e.g. me).

>                                  If you want to help then starting
> with review would be much much more helpful and hugely appreciated. We
> are really lacking people there a _lot_.

I guess, I can try (at least, I want to try). But excuse me, in honest,
I am not quite familiar with mm, and my time resources are not stable
enough, either. So I am not quite sure I can do.

>                                          Just generating more work for
> reviewers with something that doesn't make any real difference in the
> runtime is far less helpful IMHO.
> 

For urgent things, really it is less helpful (in fact, it will generate
negative effect).

But if it is related with important things, we need discuss about it
when we have time (do not treat it as urgent thing).

For me, all issues in public header files are important, at least. When
a developer want to put or modify something in public header files, they
need think more -- since the members outside of mm may see them.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.