| Message ID | 20211110114721.133808-1-alexs@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] mm/damon: remove damon_lock | expand |
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. > > 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)) > 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); > > return err; > } This would let multiple concurrent threads seeing nr_running_ctxs of zero and therefore proceed together. Thanks, SJ
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
On Wed, Nov 10, 2021 at 9:41 PM Alex Shi <seakeel@gmail.com> wrote: > > 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)) 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
On Wed, 10 Nov 2021 22:04:55 +0800 Alex Shi <seakeel@gmail.com> wrote: > On Wed, Nov 10, 2021 at 9:41 PM Alex Shi <seakeel@gmail.com> wrote: > > > > 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. Sure. > > > > > > > > > > > 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)) > > 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 >
On Wed, Nov 10, 2021 at 10:35 PM SeongJae Park <sj@kernel.org> wrote: > 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. > hi SJ, Thanks for the quick reply! Yes, it's fine to use mutex on a slow path, it won't cost much. but I just feel itchy while looking at the code, especially since it only guards an int... Anyway, it's up to you. Thanks Alex
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 @@ -26,8 +26,7 @@ /* Get a random number in [l, r) */ #define damon_rand(l, r) (l + prandom_u32_max(r - l)) -static DEFINE_MUTEX(damon_lock); -static int nr_running_ctxs; +atomic_t nr_running_ctxs; /* * Construct a damon_region struct @@ -356,20 +355,6 @@ int damon_set_schemes(struct damon_ctx *ctx, struct damos **schemes, return 0; } -/** - * damon_nr_running_ctxs() - Return number of currently running contexts. - */ -int damon_nr_running_ctxs(void) -{ - int nr_ctxs; - - mutex_lock(&damon_lock); - nr_ctxs = nr_running_ctxs; - mutex_unlock(&damon_lock); - - return nr_ctxs; -} - /* Returns the size upper limit for each monitoring region */ static unsigned long damon_region_sz_limit(struct damon_ctx *ctx) { @@ -408,7 +393,7 @@ static int __damon_start(struct damon_ctx *ctx) if (!ctx->kdamond) { err = 0; ctx->kdamond = kthread_run(kdamond_fn, ctx, "kdamond.%d", - nr_running_ctxs); + atomic_read(&nr_running_ctxs)); if (IS_ERR(ctx->kdamond)) { err = PTR_ERR(ctx->kdamond); ctx->kdamond = NULL; @@ -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)) 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); return err; } @@ -1078,9 +1059,7 @@ static int kdamond_fn(void *data) ctx->kdamond = NULL; mutex_unlock(&ctx->kdamond_lock); - mutex_lock(&damon_lock); - nr_running_ctxs--; - mutex_unlock(&damon_lock); + atomic_dec(&nr_running_ctxs); return 0; } diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c index eccc14b34901..7182a46f6a2a 100644 --- a/mm/damon/dbgfs.c +++ b/mm/damon/dbgfs.c @@ -20,6 +20,8 @@ static int dbgfs_nr_ctxs; static struct dentry **dbgfs_dirs; static DEFINE_MUTEX(damon_dbgfs_lock); +extern atomic_t nr_running_ctxs; + /* * Returns non-empty string on success, negative error code otherwise. */ @@ -688,7 +690,7 @@ static int dbgfs_mk_context(char *name) struct dentry *root, **new_dirs, *new_dir; struct damon_ctx **new_ctxs, *new_ctx; - if (damon_nr_running_ctxs()) + if (atomic_read(&nr_running_ctxs)) return -EBUSY; new_ctxs = krealloc(dbgfs_ctxs, sizeof(*dbgfs_ctxs) * @@ -772,7 +774,7 @@ static int dbgfs_rm_context(char *name) struct damon_ctx **new_ctxs; int i, j; - if (damon_nr_running_ctxs()) + if (atomic_read(&nr_running_ctxs)) return -EBUSY; root = dbgfs_dirs[0]; @@ -853,7 +855,7 @@ static ssize_t dbgfs_monitor_on_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) { char monitor_on_buf[5]; - bool monitor_on = damon_nr_running_ctxs() != 0; + bool monitor_on = atomic_read(&nr_running_ctxs) != 0; int len; len = scnprintf(monitor_on_buf, 5, monitor_on ? "on\n" : "off\n");