Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAGjdHu=8pGmyBpVMpf-V=6w7hZHLmG4WH5EsNchn_+GqikTDxQ@mail.gmail.com>
 <20220914080404.58913-1-sj@kernel.org>
In-Reply-To: <20220914080404.58913-1-sj@kernel.org>
From:   Kaixu Xia <xiakaixu1987@gmail.com>
Date:   Wed, 14 Sep 2022 16:21:35 +0800
Message-ID: <CAGjdHu=rixLvmd37eXxpH8R_V8x1RaTH-f-G=WtJuCGDz+N5DA@mail.gmail.com>
Subject: Re: [PATCH 4/4] mm/damon/vaddr: indicate the target is invalid when
 'nr_regions' is zero
To:     SeongJae Park <sj@kernel.org>
Cc:     akpm@linux-foundation.org, damon@lists.linux.dev,
        linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>,
        Kaixu Xia <kaixuxia@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Sep 14, 2022 at 4:04 PM SeongJae Park <sj@kernel.org> wrote:
>
> On Wed, 14 Sep 2022 12:02:05 +0800 Kaixu Xia <xiakaixu1987@gmail.com> wrote:
>
> > On Tue, Sep 13, 2022 at 11:11 PM SeongJae Park <sj@kernel.org> wrote:
> > >
> > > On Tue, 13 Sep 2022 17:11:27 +0800 xiakaixu1987@gmail.com wrote:
> > >
> > > > From: Kaixu Xia <kaixuxia@tencent.com>
> > > >
> > > > When 'init()' and 'update()' DAMON operations failed and the number
> > > > of the damon_target regions is zero,
> > >
> > > Well, I think that could be a temporal failure.  In the case, later call of
> > > 'update()' could success?
> > >
> > Yeah, the kdamond while() loop calls 'update()' periodically to fix this
> > temporary failure. But for extreme scenarios that 'update()' continues to fail,
> > we should have some ways to detect this case.
>
> Even in the case, kdamond will do nothing but continuing the main loop while
> sleeping sample_aggr interval (5ms by default) for each iteration, and calling
> 'update()' for every update interval (100ms by default).  Waste is waste, but I
> don't think that's a real issue.  Further, continuous 'update()' failures mean
> the process is in some weird state anyway, so I'd assume the process would be
> finished soon.  kdamond will also finish as soon as the process finishes.
> Users could also find the strange situation (nothing in the monitoring results)
> and finish kdamond on their own.
>
> Anything I'm missing?
>
That sounds reasonable. Thanks for your detailed explanation.


Thanks,
Kaixu
>
> Andrew, I found you merged this patch in mm-unstable.  Could you please hold it
> until we finish this discussion?
>
>
> Thanks,
> SJ
>
> >
> > Thanks,
> > Kaixu
> > >
> > > Thanks,
> > > SJ
> > >
> > > > the kdamond would do nothing
> > > > to this monitoring target in this case. It makes no sense to run
> > > > kdamond when all of monitoring targets have no regions. So add the
> > > > judgement in 'target_valid()' operation to indicate the target is
> > > > invalid when 'nr_regions' is zero.
> > >
> > > >
> > > > Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> > > > ---
> > > >  mm/damon/vaddr.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > > > index 39ea48d9cc15..65ff98d49ec0 100644
> > > > --- a/mm/damon/vaddr.c
> > > > +++ b/mm/damon/vaddr.c
> > > > @@ -598,6 +598,9 @@ static bool damon_va_target_valid(void *target)
> > > >       struct damon_target *t = target;
> > > >       struct task_struct *task;
> > > >
> > > > +     if (!damon_nr_regions(t))
> > > > +             return false;
> > > > +
> > > >       task = damon_get_task_struct(t);
> > > >       if (task) {
> > > >               put_task_struct(task);
> > > > --
> > > > 2.27.0
> >