From: Kaixu Xia <[email protected]>
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 <[email protected]>
---
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
Hi Kaixu,
On Thu, 8 Sep 2022 00:31:02 +0800 [email protected] wrote:
> From: Kaixu Xia <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
Hi SJ,
On Thu, Sep 8, 2022 at 1:42 AM SeongJae Park <[email protected]> wrote:
>
> Hi Kaixu,
>
> On Thu, 8 Sep 2022 00:31:02 +0800 [email protected] wrote:
>
> > From: Kaixu Xia <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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