Hi SJ, On Thu, Sep 8, 2022 at 1:42 AM SeongJae Park <sj@xxxxxxxxxx> wrote: > > Hi Kaixu, > > On Thu, 8 Sep 2022 00:31:02 +0800 xiakaixu1987@xxxxxxxxx wrote: > > > From: Kaixu Xia <kaixuxia@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxx> > > --- > > 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
