[PATCH trivial] include/linux/memcontrol.h: Clean up code only

[PATCH trivial] include/linux/memcontrol.h: Clean up code only

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

Merge several statements to one return statement, since the new return
statement is still simple enough.

Try to let the second line function parameters almost align with the
first line parameter (try to be within 80 columns, and in one line).

The comments can fully use 80 columns which can save one line.

Use parameter name newpage instead of new (which will be key word color
in vim) for dummy mem_cgroup_migrate(), and real mem_cgroup_migrate()
already uses newpage.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 include/linux/memcontrol.h | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 2d03975..a03204e 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -327,10 +327,7 @@ void mem_cgroup_iter_break(struct mem_cgroup *, struct mem_cgroup *);
 
 static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
 {
-	if (mem_cgroup_disabled())
-		return 0;
-
-	return memcg->css.id;
+	return mem_cgroup_disabled() ? 0 : memcg->css.id;
 }
 
 /**
@@ -341,10 +338,7 @@ static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
  */
 static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short id)
 {
-	struct cgroup_subsys_state *css;
-
-	css = css_from_id(id, &memory_cgrp_subsys);
-	return mem_cgroup_from_css(css);
+	return mem_cgroup_from_css(css_from_id(id, &memory_cgrp_subsys));
 }
 
 /**
@@ -390,9 +384,7 @@ ino_t page_cgroup_ino(struct page *page);
 
 static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
 {
-	if (mem_cgroup_disabled())
-		return true;
-	return !!(memcg->css.flags & CSS_ONLINE);
+	return mem_cgroup_disabled() || (memcg->css.flags & CSS_ONLINE);
 }
 
 /*
@@ -401,7 +393,7 @@ static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
 int mem_cgroup_select_victim_node(struct mem_cgroup *memcg);
 
 void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru,
-		int nr_pages);
+				int nr_pages);
 
 unsigned long mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
 					   int nid, unsigned int lru_mask);
@@ -452,9 +444,8 @@ void unlock_page_memcg(struct page *page);
  * @idx: page state item to account
  * @val: number of pages (positive or negative)
  *
- * The @page must be locked or the caller must use lock_page_memcg()
- * to prevent double accounting when the page is concurrently being
- * moved to another memcg:
+ * The @page must be locked or the caller must use lock_page_memcg() to prevent
+ * double accounting when the page is concurrently being moved to another memcg:
  *
  *   lock_page(page) or lock_page_memcg(page)
  *   if (TestClearPageState(page))
@@ -462,7 +453,7 @@ void unlock_page_memcg(struct page *page);
  *   unlock_page(page) or unlock_page_memcg(page)
  */
 static inline void mem_cgroup_update_page_stat(struct page *page,
-				 enum mem_cgroup_stat_index idx, int val)
+					enum mem_cgroup_stat_index idx, int val)
 {
 	VM_BUG_ON(!(rcu_read_lock_held() || PageLocked(page)));
 
@@ -569,7 +560,7 @@ static inline void mem_cgroup_uncharge_list(struct list_head *page_list)
 {
 }
 
-static inline void mem_cgroup_migrate(struct page *old, struct page *new)
+static inline void mem_cgroup_migrate(struct page *old, struct page *newpage)
 {
 }
 
@@ -586,7 +577,7 @@ static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
 }
 
 static inline bool mm_match_cgroup(struct mm_struct *mm,
-		struct mem_cgroup *memcg)
+				   struct mem_cgroup *memcg)
 {
 	return true;
 }
@@ -798,7 +789,7 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
  * @val: number of pages (positive or negative)
  */
 static inline void memcg_kmem_update_page_stat(struct page *page,
-				enum mem_cgroup_stat_index idx, int val)
+					enum mem_cgroup_stat_index idx, int val)
 {
 	if (memcg_kmem_enabled() && page->mem_cgroup)
 		this_cpu_add(page->mem_cgroup->stat->count[idx], val);
@@ -827,7 +818,7 @@ static inline void memcg_put_cache_ids(void)
 }
 
 static inline void memcg_kmem_update_page_stat(struct page *page,
-				enum mem_cgroup_stat_index idx, int val)
+					enum mem_cgroup_stat_index idx, int val)
 {
 }
 #endif /* CONFIG_MEMCG && !CONFIG_SLOB */
-- 
1.9.3

Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

[Michal Hocko]

On Thu 09-06-16 23:23:52, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> Merge several statements to one return statement, since the new return
> statement is still simple enough.
> 
> Try to let the second line function parameters almost align with the
> first line parameter (try to be within 80 columns, and in one line).
> 
> The comments can fully use 80 columns which can save one line.
> 
> Use parameter name newpage instead of new (which will be key word color
> in vim) for dummy mem_cgroup_migrate(), and real mem_cgroup_migrate()
> already uses newpage.

What is the point of these changes? It removes few lines but does that
actually make the code easier to read? To be honest I am not a big fan
of such a stylist changes unless they are in a series of other changes
which actually tweak the functionality. This just brings more churn
to the git history.

That's being said, I appreciate an interest in making the code cleaner
but try to think whether these changes are actually helpful and who is
going to benefit from them.

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  include/linux/memcontrol.h | 31 +++++++++++--------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 2d03975..a03204e 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -327,10 +327,7 @@ void mem_cgroup_iter_break(struct mem_cgroup *, struct mem_cgroup *);
>  
>  static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
>  {
> -	if (mem_cgroup_disabled())
> -		return 0;
> -
> -	return memcg->css.id;
> +	return mem_cgroup_disabled() ? 0 : memcg->css.id;
>  }
>  
>  /**
> @@ -341,10 +338,7 @@ static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
>   */
>  static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short id)
>  {
> -	struct cgroup_subsys_state *css;
> -
> -	css = css_from_id(id, &memory_cgrp_subsys);
> -	return mem_cgroup_from_css(css);
> +	return mem_cgroup_from_css(css_from_id(id, &memory_cgrp_subsys));
>  }
>  
>  /**
> @@ -390,9 +384,7 @@ ino_t page_cgroup_ino(struct page *page);
>  
>  static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>  {
> -	if (mem_cgroup_disabled())
> -		return true;
> -	return !!(memcg->css.flags & CSS_ONLINE);
> +	return mem_cgroup_disabled() || (memcg->css.flags & CSS_ONLINE);
>  }
>  
>  /*
> @@ -401,7 +393,7 @@ static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>  int mem_cgroup_select_victim_node(struct mem_cgroup *memcg);
>  
>  void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru,
> -		int nr_pages);
> +				int nr_pages);
>  
>  unsigned long mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
>  					   int nid, unsigned int lru_mask);
> @@ -452,9 +444,8 @@ void unlock_page_memcg(struct page *page);
>   * @idx: page state item to account
>   * @val: number of pages (positive or negative)
>   *
> - * The @page must be locked or the caller must use lock_page_memcg()
> - * to prevent double accounting when the page is concurrently being
> - * moved to another memcg:
> + * The @page must be locked or the caller must use lock_page_memcg() to prevent
> + * double accounting when the page is concurrently being moved to another memcg:
>   *
>   *   lock_page(page) or lock_page_memcg(page)
>   *   if (TestClearPageState(page))
> @@ -462,7 +453,7 @@ void unlock_page_memcg(struct page *page);
>   *   unlock_page(page) or unlock_page_memcg(page)
>   */
>  static inline void mem_cgroup_update_page_stat(struct page *page,
> -				 enum mem_cgroup_stat_index idx, int val)
> +					enum mem_cgroup_stat_index idx, int val)
>  {
>  	VM_BUG_ON(!(rcu_read_lock_held() || PageLocked(page)));
>  
> @@ -569,7 +560,7 @@ static inline void mem_cgroup_uncharge_list(struct list_head *page_list)
>  {
>  }
>  
> -static inline void mem_cgroup_migrate(struct page *old, struct page *new)
> +static inline void mem_cgroup_migrate(struct page *old, struct page *newpage)
>  {
>  }
>  
> @@ -586,7 +577,7 @@ static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
>  }
>  
>  static inline bool mm_match_cgroup(struct mm_struct *mm,
> -		struct mem_cgroup *memcg)
> +				   struct mem_cgroup *memcg)
>  {
>  	return true;
>  }
> @@ -798,7 +789,7 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
>   * @val: number of pages (positive or negative)
>   */
>  static inline void memcg_kmem_update_page_stat(struct page *page,
> -				enum mem_cgroup_stat_index idx, int val)
> +					enum mem_cgroup_stat_index idx, int val)
>  {
>  	if (memcg_kmem_enabled() && page->mem_cgroup)
>  		this_cpu_add(page->mem_cgroup->stat->count[idx], val);
> @@ -827,7 +818,7 @@ static inline void memcg_put_cache_ids(void)
>  }
>  
>  static inline void memcg_kmem_update_page_stat(struct page *page,
> -				enum mem_cgroup_stat_index idx, int val)
> +					enum mem_cgroup_stat_index idx, int val)
>  {
>  }
>  #endif /* CONFIG_MEMCG && !CONFIG_SLOB */
> -- 
> 1.9.3

-- 
Michal Hocko
SUSE Labs

Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 5088 bytes --]

On Thu, 2016-06-09 at 23:23 +0800, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> Merge several statements to one return statement, since the new
> return
> statement is still simple enough.

This code is not simple, and any change that
makes it harder to read needs a good reason.

At least in my opinion, your return statement
merging thing makes the code harder to read.

> Try to let the second line function parameters almost align with the
> first line parameter (try to be within 80 columns, and in one line).
> 
> The comments can fully use 80 columns which can save one line.
> 
> Use parameter name newpage instead of new (which will be key word
> color
> in vim) for dummy mem_cgroup_migrate(), and real mem_cgroup_migrate()
> already uses newpage.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  include/linux/memcontrol.h | 31 +++++++++++--------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 2d03975..a03204e 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -327,10 +327,7 @@ void mem_cgroup_iter_break(struct mem_cgroup *,
> struct mem_cgroup *);
>  
>  static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
>  {
> -	if (mem_cgroup_disabled())
> -		return 0;
> -
> -	return memcg->css.id;
> +	return mem_cgroup_disabled() ? 0 : memcg->css.id;
>  }
>  
>  /**
> @@ -341,10 +338,7 @@ static inline unsigned short
> mem_cgroup_id(struct mem_cgroup *memcg)
>   */
>  static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short
> id)
>  {
> -	struct cgroup_subsys_state *css;
> -
> -	css = css_from_id(id, &memory_cgrp_subsys);
> -	return mem_cgroup_from_css(css);
> +	return mem_cgroup_from_css(css_from_id(id,
> &memory_cgrp_subsys));
>  }
>  
>  /**
> @@ -390,9 +384,7 @@ ino_t page_cgroup_ino(struct page *page);
>  
>  static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>  {
> -	if (mem_cgroup_disabled())
> -		return true;
> -	return !!(memcg->css.flags & CSS_ONLINE);
> +	return mem_cgroup_disabled() || (memcg->css.flags &
> CSS_ONLINE);
>  }
>  
>  /*
> @@ -401,7 +393,7 @@ static inline bool mem_cgroup_online(struct
> mem_cgroup *memcg)
>  int mem_cgroup_select_victim_node(struct mem_cgroup *memcg);
>  
>  void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list
> lru,
> -		int nr_pages);
> +				int nr_pages);
>  
>  unsigned long mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
>  					   int nid, unsigned int
> lru_mask);
> @@ -452,9 +444,8 @@ void unlock_page_memcg(struct page *page);
>   * @idx: page state item to account
>   * @val: number of pages (positive or negative)
>   *
> - * The @page must be locked or the caller must use lock_page_memcg()
> - * to prevent double accounting when the page is concurrently being
> - * moved to another memcg:
> + * The @page must be locked or the caller must use lock_page_memcg()
> to prevent
> + * double accounting when the page is concurrently being moved to
> another memcg:
>   *
>   *   lock_page(page) or lock_page_memcg(page)
>   *   if (TestClearPageState(page))
> @@ -462,7 +453,7 @@ void unlock_page_memcg(struct page *page);
>   *   unlock_page(page) or unlock_page_memcg(page)
>   */
>  static inline void mem_cgroup_update_page_stat(struct page *page,
> -				 enum mem_cgroup_stat_index idx, int
> val)
> +					enum mem_cgroup_stat_index
> idx, int val)
>  {
>  	VM_BUG_ON(!(rcu_read_lock_held() || PageLocked(page)));
>  
> @@ -569,7 +560,7 @@ static inline void
> mem_cgroup_uncharge_list(struct list_head *page_list)
>  {
>  }
>  
> -static inline void mem_cgroup_migrate(struct page *old, struct page
> *new)
> +static inline void mem_cgroup_migrate(struct page *old, struct page
> *newpage)
>  {
>  }
>  
> @@ -586,7 +577,7 @@ static inline struct lruvec
> *mem_cgroup_page_lruvec(struct page *page,
>  }
>  
>  static inline bool mm_match_cgroup(struct mm_struct *mm,
> -		struct mem_cgroup *memcg)
> +				   struct mem_cgroup *memcg)
>  {
>  	return true;
>  }
> @@ -798,7 +789,7 @@ static inline int memcg_cache_id(struct
> mem_cgroup *memcg)
>   * @val: number of pages (positive or negative)
>   */
>  static inline void memcg_kmem_update_page_stat(struct page *page,
> -				enum mem_cgroup_stat_index idx, int
> val)
> +					enum mem_cgroup_stat_index
> idx, int val)
>  {
>  	if (memcg_kmem_enabled() && page->mem_cgroup)
>  		this_cpu_add(page->mem_cgroup->stat->count[idx],
> val);
> @@ -827,7 +818,7 @@ static inline void memcg_put_cache_ids(void)
>  }
>  
>  static inline void memcg_kmem_update_page_stat(struct page *page,
> -				enum mem_cgroup_stat_index idx, int
> val)
> +					enum mem_cgroup_stat_index
> idx, int val)
>  {
>  }
>  #endif /* CONFIG_MEMCG && !CONFIG_SLOB */
-- 
All rights reversed

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

[Chen Gang]

On 6/10/16 01:39, Rik van Riel wrote:
> On Thu, 2016-06-09 at 23:23 +0800, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> Merge several statements to one return statement, since the new
>> return
>> statement is still simple enough.
> 
> This code is not simple, and any change that
> makes it harder to read needs a good reason.
> 
> At least in my opinion, your return statement
> merging thing makes the code harder to read.
>

For me, every member has his/her own taste, we need talk about it case
by case.

[...]
 
>>  
>>  static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
>>  {
>> -	if (mem_cgroup_disabled())
>> -		return 0;
>> -
>> -	return memcg->css.id;
>> +	return mem_cgroup_disabled() ? 0 : memcg->css.id;
>>  }
>>  

For me, this is the simplest way for using "? :", so it is easy enough (
I guess, Linux kernel does not completely reject "? :").

>>  /**
>> @@ -341,10 +338,7 @@ static inline unsigned short
>> mem_cgroup_id(struct mem_cgroup *memcg)
>>   */
>>  static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short
>> id)
>>  {
>> -	struct cgroup_subsys_state *css;
>> -
>> -	css = css_from_id(id, &memory_cgrp_subsys);
>> -	return mem_cgroup_from_css(css);
>> +	return mem_cgroup_from_css(css_from_id(id, &memory_cgrp_subsys));
>>  }
>>  

For me, this case may depend on various members' tastes (although it is
simple enough to me -- it is in one line within 80 columns, and related
with 2 functions' call).

For this case, if any of another member suggests to keep original code
no touch, too, I shall send patch v2 for it (keep it no touch).

>>  /**
>> @@ -390,9 +384,7 @@ ino_t page_cgroup_ino(struct page *page);
>>  
>>  static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>>  {
>> -	if (mem_cgroup_disabled())
>> -		return true;
>> -	return !!(memcg->css.flags & CSS_ONLINE);
>> +	return mem_cgroup_disabled() || (memcg->css.flags & CSS_ONLINE);
>>  }
>>  

For me, this is almost the simplest way for using "||", we can find
this case in include/linux (60+ cases for '||', and 150+ for '&&'), and
the new return statement is in one line within 80 columns.

The new return statement is not the simplest, but it is still simple
enough (which should be better than original several lines statements).


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

[Chen Gang]

On 6/9/16 23:46, Michal Hocko wrote:
> On Thu 09-06-16 23:23:52, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> Merge several statements to one return statement, since the new return
>> statement is still simple enough.
>>
>> Try to let the second line function parameters almost align with the
>> first line parameter (try to be within 80 columns, and in one line).
>>
>> The comments can fully use 80 columns which can save one line.
>>
>> Use parameter name newpage instead of new (which will be key word color
>> in vim) for dummy mem_cgroup_migrate(), and real mem_cgroup_migrate()
>> already uses newpage.
> 
> What is the point of these changes? It removes few lines but does that
> actually make the code easier to read? To be honest I am not a big fan
> of such a stylist changes unless they are in a series of other changes
> which actually tweak the functionality. This just brings more churn
> to the git history.
> 

For me, if one line is simple enough, we need not use several lines to
express the same thing, especially a line within 80 columns can express
the whole function work in headers.

The public header files do not like body files, they are simple enough,
but their contents are often read through by another readers. So we need
more careful for coding styles, but less careful for git history.

> That's being said, I appreciate an interest in making the code cleaner
> but try to think whether these changes are actually helpful and who is
> going to benefit from them.
> 

For me, another readers can get benefit more or less from it: if read a
simple line can know the whole thing (function work), why need we read
multiple lines?

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

[Michal Hocko]

On Fri 10-06-16 08:40:30, Chen Gang wrote:
> 
> On 6/9/16 23:46, Michal Hocko wrote:
[...]
> > That's being said, I appreciate an interest in making the code cleaner
> > but try to think whether these changes are actually helpful and who is
> > going to benefit from them.
> > 
> 
> For me, another readers can get benefit more or less from it: if read a
> simple line can know the whole thing (function work), why need we read
> multiple lines?

Yes I understand that this is a matter of taste but as I've said above.
I do not see this would add a benefit to justify the change.

If you are doing a reformating or white space cleanups always try to
think about those who want/need to dig into the history to understand
the code and this would add an additional step to get to the original
commit which is added the code. This is just wasting of time.

Now this would be much more tolerable when the code in question was a
maze but this is not the case.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

[Chen Gang]

On 6/10/16 14:14, Michal Hocko wrote:
> On Fri 10-06-16 08:40:30, Chen Gang wrote:
>>
>> On 6/9/16 23:46, Michal Hocko wrote:
> [...]
>>> That's being said, I appreciate an interest in making the code cleaner
>>> but try to think whether these changes are actually helpful and who is
>>> going to benefit from them.
>>>
>>
>> For me, another readers can get benefit more or less from it: if read a
>> simple line can know the whole thing (function work), why need we read
>> multiple lines?
> 
> Yes I understand that this is a matter of taste but as I've said above.
> I do not see this would add a benefit to justify the change.
> 
> If you are doing a reformating or white space cleanups always try to
> think about those who want/need to dig into the history to understand
> the code and this would add an additional step to get to the original
> commit which is added the code. This is just wasting of time.
> 
> Now this would be much more tolerable when the code in question was a
> maze but this is not the case.
> 

OK, thanks.

Cleaning up code in include/* should face to the whole header files, not
only for mm specially. It is not suitable to only focus mm in common
shared header files for cleaning up code only.

So for me, cleaning up code in include/* is still necessary, but I shall
face to all files instead of mm related files.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.