[PATCH] mm/damon/vaddr: remove unnecessary switch case DAMOS_STAT

[PATCH] mm/damon/vaddr: remove unnecessary switch case DAMOS_STAT

[xiakaixu1987]

From: Kaixu Xia <kaixuxia@tencent.com>

The switch case DAMOS_STAT and switch case default have same
return value in damon_va_apply_scheme(), so we can combine them.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.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;
 	default:
 		return 0;
 	}
-- 
2.27.0

Re: [PATCH] mm/damon/vaddr: remove unnecessary switch case DAMOS_STAT

[SeongJae Park]

Hi Kaixu,

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

> From: Kaixu Xia <kaixuxia@tencent.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@tencent.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

Re: [PATCH] mm/damon/vaddr: remove unnecessary switch case DAMOS_STAT

[Kaixu Xia]

Hi SJ,

On Thu, Sep 8, 2022 at 1:42 AM SeongJae Park <sj@kernel.org> wrote:
>
> Hi Kaixu,
>
> On Thu, 8 Sep 2022 00:31:02 +0800 xiakaixu1987@gmail.com wrote:
>
> > From: Kaixu Xia <kaixuxia@tencent.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@tencent.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'.
>
Yeah,  it might make sense to add a comment here, thanks.
> >       }
> > --
> > 2.27.0
>
>
> Thanks,
> SJ