linux-kernel - Re: [RFC PATCH] mm/damon: remove damon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20211110143535.15809-1-sj@kernel.org>
Date:   Wed, 10 Nov 2021 14:35:35 +0000
From:   SeongJae Park <sj@...nel.org>
To:     Alex Shi <seakeel@...il.com>
Cc:     SeongJae Park <sj@...nel.org>, Alex Shi <alexs@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] mm/damon: remove damon_lock

On Wed, 10 Nov 2021 22:04:55 +0800 Alex Shi <seakeel@...il.com> wrote:

> On Wed, Nov 10, 2021 at 9:41 PM Alex Shi <seakeel@...il.com> wrote:
> >
> > On Wed, Nov 10, 2021 at 8:40 PM SeongJae Park <sj@...nel.org> wrote:
> > >
> > > Thank you for this patch, Alex!
> > >
> > > On Wed, 10 Nov 2021 19:47:21 +0800 alexs@...nel.org wrote:
> > >
> > > > From: Alex Shi <alexs@...nel.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.

Sure.

> > >
> > > >
> > > > Signed-off-by: Alex Shi <alexs@...nel.org>
> > > > Cc: SeongJae Park <sj@...nel.org>
> > > > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > > > Cc: linux-mm@...ck.org
> > > > Cc: linux-kernel@...r.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))
> 
> Ops, my fault. The following should be right?
> 
> Thanks
> 
> int a = 0;
> if (!atomic_try_cmpxchg(&nr_running_ctxs, &a, 1))
> > > >               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?

Yes, it would effectively avoid the problem case.  However, I'm unsure how much
performance gain this change is providing, as apparently the lock is not being
used in performance critical parts.

I'm also unsure if this change is reducing the complexity of the code or not.
For an example, this change allows someone to show non-zero nr_running_ctxs
while no real kdamond is running, before __damon_start() is called, or when it
failed.  I think this would never be a real issue, but might make my poor brain
a little bit confused when debugging.

Also, we might add some more variables and code section that should be mutually
exclusive to concurrent DAMON users in future.

atomic_t is obviously enough for protecting a variable.  But, IMHO, it might
not necessarily be the best choice for non-performance-critical mutex sections.

Please feel free to let me know if I'm missing something.


Thanks,
SJ

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