| Message ID | 20220218102611.31895-3-tome01@ajou.ac.kr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rebase DAMON_RECALIM for NUMA system | expand |
Hello Jonghyeon, On Fri, 18 Feb 2022 19:26:10 +0900 Jonghyeon Kim <tome01@ajou.ac.kr> wrote: > damon_start() function is designed to start multiple damon monitoring > contexts. But, sometimes we need to start monitoring one context. > Although __damon_start() could be considered to start for one monitoring > context, it seems reasonable to adopt a new function that does not need > to handle 'damon_lock' from the caller. > > Signed-off-by: Jonghyeon Kim <tome01@ajou.ac.kr> > --- > include/linux/damon.h | 1 + > mm/damon/core.c | 25 +++++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/include/linux/damon.h b/include/linux/damon.h > index c0adf1566603..069577477662 100644 > --- a/include/linux/damon.h > +++ b/include/linux/damon.h > @@ -511,6 +511,7 @@ int damon_register_ops(struct damon_operations *ops); > int damon_select_ops(struct damon_ctx *ctx, enum damon_ops_id id); > > int damon_start(struct damon_ctx **ctxs, int nr_ctxs); > +int damon_start_one(struct damon_ctx *ctx); > int damon_stop(struct damon_ctx **ctxs, int nr_ctxs); > > #endif /* CONFIG_DAMON */ > diff --git a/mm/damon/core.c b/mm/damon/core.c > index 290c9c0535ee..e43f138a3489 100644 > --- a/mm/damon/core.c > +++ b/mm/damon/core.c > @@ -466,6 +466,31 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs) > return err; > } > > +/** > + * damon_start_one() - Starts the monitorings for one context. > + * @ctx: monitoring context > + * > + * This function starts one monitoring thread for only one monitoring context > + * handling damon_lock. > + * > + * Return: 0 on success, negative error code otherwise. > + */ > +int damon_start_one(struct damon_ctx *ctx) > +{ > + int err = 0; > + > + mutex_lock(&damon_lock); > + err = __damon_start(ctx); > + if (err) { > + mutex_unlock(&damon_lock); > + return err; > + } > + nr_running_ctxs++; > + mutex_unlock(&damon_lock); > + > + return err; > +} > + IMHO, this looks like an unnecessary duplication of code. Unless this is needed for a real usecase, this change might unnecessarily make the code only a little bit more complicated. And to my understanding of the next patch, this is not really needed for this patchset. I will left comments on the patch. If I'm missing something, please clarify why this is really needed. Furthermore, damon_start() starts a set of DAMON contexts in exclusive manner, to ensure there will be no interference. This patch breaks the assumption. That is, contexts that started with damon_start() could be interfered by other contexts that started with damon_start_one(). I have a plan to make damon_start() also work for non-exclusive contexts group[1], though. [1] https://lore.kernel.org/linux-mm/20220217161938.8874-3-sj@kernel.org/ Thanks, SJ > /* > * __damon_stop() - Stops monitoring of given context. > * @ctx: monitoring context > -- > 2.17.1 >
On Wed, Feb 23, 2022 at 02:11:13PM +0900, Jonghyeon Kim wrote: > On Tue, Feb 22, 2022 at 09:53:28AM +0000, SeongJae Park wrote: > > Hello Jonghyeon, > > > > On Fri, 18 Feb 2022 19:26:10 +0900 Jonghyeon Kim <tome01@ajou.ac.kr> wrote: > > > > > damon_start() function is designed to start multiple damon monitoring > > > contexts. But, sometimes we need to start monitoring one context. > > > Although __damon_start() could be considered to start for one monitoring > > > context, it seems reasonable to adopt a new function that does not need > > > to handle 'damon_lock' from the caller. > > > > > > Signed-off-by: Jonghyeon Kim <tome01@ajou.ac.kr> > > > --- > > > include/linux/damon.h | 1 + > > > mm/damon/core.c | 25 +++++++++++++++++++++++++ > > > 2 files changed, 26 insertions(+) > > > > > > diff --git a/include/linux/damon.h b/include/linux/damon.h > > > index c0adf1566603..069577477662 100644 > > > --- a/include/linux/damon.h > > > +++ b/include/linux/damon.h > > > @@ -511,6 +511,7 @@ int damon_register_ops(struct damon_operations *ops); > > > int damon_select_ops(struct damon_ctx *ctx, enum damon_ops_id id); > > > > > > int damon_start(struct damon_ctx **ctxs, int nr_ctxs); > > > +int damon_start_one(struct damon_ctx *ctx); > > > int damon_stop(struct damon_ctx **ctxs, int nr_ctxs); > > > > > > #endif /* CONFIG_DAMON */ > > > diff --git a/mm/damon/core.c b/mm/damon/core.c > > > index 290c9c0535ee..e43f138a3489 100644 > > > --- a/mm/damon/core.c > > > +++ b/mm/damon/core.c > > > @@ -466,6 +466,31 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs) > > > return err; > > > } > > > > > > +/** > > > + * damon_start_one() - Starts the monitorings for one context. > > > + * @ctx: monitoring context > > > + * > > > + * This function starts one monitoring thread for only one monitoring context > > > + * handling damon_lock. > > > + * > > > + * Return: 0 on success, negative error code otherwise. > > > + */ > > > +int damon_start_one(struct damon_ctx *ctx) > > > +{ > > > + int err = 0; > > > + > > > + mutex_lock(&damon_lock); > > > + err = __damon_start(ctx); > > > + if (err) { > > > + mutex_unlock(&damon_lock); > > > + return err; > > > + } > > > + nr_running_ctxs++; > > > + mutex_unlock(&damon_lock); > > > + > > > + return err; > > > +} > > > + > > > > IMHO, this looks like an unnecessary duplication of code. Unless this is > > needed for a real usecase, this change might unnecessarily make the code only a > > little bit more complicated. And to my understanding of the next patch, this > > is not really needed for this patchset. I will left comments on the patch. If > > I'm missing something, please clarify why this is really needed. > > > > Furthermore, damon_start() starts a set of DAMON contexts in exclusive manner, > > to ensure there will be no interference. This patch breaks the assumption. > > That is, contexts that started with damon_start() could be interfered by other > > contexts that started with damon_start_one(). I have a plan to make > > damon_start() also work for non-exclusive contexts group[1], though. > > > > [1] https://lore.kernel.org/linux-mm/20220217161938.8874-3-sj@kernel.org/ > > > > > > Thanks, > > SJ > > > > I understand your opinion, and it makes sense. I will drop this patch in the > next version. > > > Thanks, > Jonghyeon
diff --git a/include/linux/damon.h b/include/linux/damon.h index c0adf1566603..069577477662 100644 --- a/include/linux/damon.h +++ b/include/linux/damon.h @@ -511,6 +511,7 @@ int damon_register_ops(struct damon_operations *ops); int damon_select_ops(struct damon_ctx *ctx, enum damon_ops_id id); int damon_start(struct damon_ctx **ctxs, int nr_ctxs); +int damon_start_one(struct damon_ctx *ctx); int damon_stop(struct damon_ctx **ctxs, int nr_ctxs); #endif /* CONFIG_DAMON */ diff --git a/mm/damon/core.c b/mm/damon/core.c index 290c9c0535ee..e43f138a3489 100644 --- a/mm/damon/core.c +++ b/mm/damon/core.c @@ -466,6 +466,31 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs) return err; } +/** + * damon_start_one() - Starts the monitorings for one context. + * @ctx: monitoring context + * + * This function starts one monitoring thread for only one monitoring context + * handling damon_lock. + * + * Return: 0 on success, negative error code otherwise. + */ +int damon_start_one(struct damon_ctx *ctx) +{ + int err = 0; + + mutex_lock(&damon_lock); + err = __damon_start(ctx); + if (err) { + mutex_unlock(&damon_lock); + return err; + } + nr_running_ctxs++; + mutex_unlock(&damon_lock); + + return err; +} + /* * __damon_stop() - Stops monitoring of given context. * @ctx: monitoring context
damon_start() function is designed to start multiple damon monitoring contexts. But, sometimes we need to start monitoring one context. Although __damon_start() could be considered to start for one monitoring context, it seems reasonable to adopt a new function that does not need to handle 'damon_lock' from the caller. Signed-off-by: Jonghyeon Kim <tome01@ajou.ac.kr> --- include/linux/damon.h | 1 + mm/damon/core.c | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+)