linux-kernel - Re: [PATCH] mm/damon/vaddr: remove unnecessary switch case DAMOS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20220907174207.61191-1-sj@kernel.org>
Date:   Wed,  7 Sep 2022 17:42:07 +0000
From:   SeongJae Park <sj@...nel.org>
To:     xiakaixu1987@...il.com
Cc:     sj@...nel.org, akpm@...ux-foundation.org, damon@...ts.linux.dev,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Kaixu Xia <kaixuxia@...cent.com>
Subject: Re: [PATCH] mm/damon/vaddr: remove unnecessary switch case DAMOS_STAT

Hi Kaixu,

On Thu, 8 Sep 2022 00:31:02 +0800 xiakaixu1987@...il.com wrote:

> From: Kaixu Xia <kaixuxia@...cent.com>
> 
> The switch case DAMOS_STAT and switch case default have same
> return value in damon_va_apply_scheme(), so we can combine them.

Good point.  I have a comment below, though.

> 
> Signed-off-by: Kaixu Xia <kaixuxia@...cent.com>
> ---
>  mm/damon/vaddr.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 3c7b9d6dca95..94ae8816a912 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -643,8 +643,6 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  	case DAMOS_NOHUGEPAGE:
>  		madv_action = MADV_NOHUGEPAGE;
>  		break;
> -	case DAMOS_STAT:
> -		return 0;

IMHO, keeping the 'case' makes the code easier to read, as we can find what is
the expected flow for DAMOS_STAT here immediately, instead of asking readers to
find what are the actions that not specified here and therefore fall though to
'default'.

Also, my another intention here is to mark 'DAMOS_STAT' is supported by
'vaddr'.

>  	default:
>  		return 0;

That is, 'default' case here is for DAMOS actions that not supported by
'vaddr'.  So, I'd like to keep the code as is.  Maybe we could add a comment
saying 'default' case is for DAMOS actions that not yet supported by 'vaddr'.

>  	}
> -- 
> 2.27.0

Thanks,
SJ