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

[Rik van Riel]

On Thu, 2016-06-09 at 23:23 +0800, chengang@xxxxxxxxxxxxxxxx wrote:
> From: Chen Gang <chengang@xxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxx>
> ---
> Â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: signature.asc
Description: This is a digitally signed message part