Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211110114721.133808-1-alexs@kernel.org> <20211110124034.2574-1-sj@kernel.org>
In-Reply-To: <20211110124034.2574-1-sj@kernel.org>
From:   Alex Shi <seakeel@gmail.com>
Date:   Wed, 10 Nov 2021 21:41:59 +0800
Message-ID: <CAJy-Amky9AAap=mAzdae-92GFWzjzCXY20UOTA+L_kvWXMgPnw@mail.gmail.com>
Subject: Re: [RFC PATCH] mm/damon: remove damon_lock
To:     SeongJae Park <sj@kernel.org>
Cc:     Alex Shi <alexs@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Nov 10, 2021 at 8:40 PM SeongJae Park <sj@kernel.org> wrote:
>
> Thank you for this patch, Alex!
>
> On Wed, 10 Nov 2021 19:47:21 +0800 alexs@kernel.org wrote:
>
> > From: Alex Shi <alexs@kernel.org>
> >
> > Variable nr_running_ctxs guards by damon_lock, but a lock for a int
> > variable seems a bit heavy, a atomic_t is enough.
>
> The lock is not only for protecting nr_running_ctxs, but also for avoiding
> different users concurrently executing damon_start(), because that could allow
> the users interfering others.

That's right. but it could be resolved by atomic too. like the following.
>
> >
> > Signed-off-by: Alex Shi <alexs@kernel.org>
> > Cc: SeongJae Park <sj@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  include/linux/damon.h |  1 -
> >  mm/damon/core.c       | 31 +++++--------------------------
> >  mm/damon/dbgfs.c      |  8 +++++---
> >  3 files changed, 10 insertions(+), 30 deletions(-)
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index b4d4be3cc987..e5dcc6336ef2 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -453,7 +453,6 @@ int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> >               unsigned long min_nr_reg, unsigned long max_nr_reg);
> >  int damon_set_schemes(struct damon_ctx *ctx,
> >                       struct damos **schemes, ssize_t nr_schemes);
> > -int damon_nr_running_ctxs(void);
> >
> >  int damon_start(struct damon_ctx **ctxs, int nr_ctxs);
> >  int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index c381b3c525d0..e821e36d5c10 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [...]
> > @@ -437,19 +422,15 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs)
> >       int i;
> >       int err = 0;
> >
> > -     mutex_lock(&damon_lock);
> > -     if (nr_running_ctxs) {
> > -             mutex_unlock(&damon_lock);
> > +     if (atomic_read(&nr_running_ctxs))

if (atomic_inc_not_zero(&nr_running_ctxs))
> >               return -EBUSY;
> > -     }
> >
> >       for (i = 0; i < nr_ctxs; i++) {
> >               err = __damon_start(ctxs[i]);
> >               if (err)
> >                       break;
> > -             nr_running_ctxs++;
> > +             atomic_inc(&nr_running_ctxs);
> >       }
> > -     mutex_unlock(&damon_lock);
> >

 atomic_dec(&nr_running_ctxs);

Is it save the multiple ctxs issue?

Thanks

> >       return err;
> >  }
>
> This would let multiple concurrent threads seeing nr_running_ctxs of zero and
> therefore proceed together.
>
>
> Thanks,
> SJ