Re: [PATCH 4/4] mm/damon/vaddr: indicate the target is invalid when 'nr_regions' is zero

[Kaixu Xia]

On Wed, Sep 14, 2022 at 4:04 PM SeongJae Park <sj@xxxxxxxxxx> wrote:
>
> On Wed, 14 Sep 2022 12:02:05 +0800 Kaixu Xia <xiakaixu1987@xxxxxxxxx> wrote:
>
> > On Tue, Sep 13, 2022 at 11:11 PM SeongJae Park <sj@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 13 Sep 2022 17:11:27 +0800 xiakaixu1987@xxxxxxxxx wrote:
> > >
> > > > From: Kaixu Xia <kaixuxia@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx>
> > > > ---
> > > >  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
> >