On Wed, Nov 10, 2021 at 9:41 PM Alex Shi <seakeel@xxxxxxxxx> wrote:
>
> On Wed, Nov 10, 2021 at 8:40 PM SeongJae Park <sj@xxxxxxxxxx> wrote:
> >
> > Thank you for this patch, Alex!
> >
> > On Wed, 10 Nov 2021 19:47:21 +0800 alexs@xxxxxxxxxx wrote:
> >
> > > From: Alex Shi <alexs@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Cc: SeongJae Park <sj@xxxxxxxxxx>
> > > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: linux-mm@xxxxxxxxx
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > ---
> > > 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?
>
> Thanks
>
> > > return err;
> > > }
> >
> > This would let multiple concurrent threads seeing nr_running_ctxs of zero and
> > therefore proceed together.
> >
> >
> > Thanks,
> > SJ
