[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241129074942.1554-1-honggyu.kim@sk.com>
Date: Fri, 29 Nov 2024 16:49:40 +0900
From: Honggyu Kim <honggyu.kim@...com>
To: SeongJae Park <sj@...nel.org>
Cc: damon@...ts.linux.dev,
linux-mm@...ck.org,
linux-kernel@...r.kernel.org,
Yunjeong Mun <yunjeong.mun@...com>,
kernel_team@...ynix.com,
Honggyu Kim <honggyu.kim@...com>
Subject: Re: [RFC PATCH] mm/damon: explain "effective quota" on kernel-doc comment
On Thu, 28 Nov 2024 09:50:46 -0800 SeongJae Park <sj@...nel.org> wrote:
> On Thu, 28 Nov 2024 18:53:55 +0900 Honggyu Kim <honggyu.kim@...com> wrote:
>
> > On Tue, 26 Nov 2024 11:43:47 -0800 SeongJae Park <sj@...nel.org> wrote:
> > > On Tue, 26 Nov 2024 17:24:33 +0900 Honggyu Kim <honggyu.kim@...com> wrote:
> > >
> > > > Hi SeongJae,
> > > >
> > > > Thanks very much for the quick response.
> > >
> > > No problem, all owing to your kind report!
> > >
> > > > I think it looks great but I
> > > > have some minor comments so please see my inline comments below.
> > > >
> > > > Thanks,
> > > > Honggyu
> > > >
> > > > On Mon, 25 Nov 2024 16:29:21 -0800 SeongJae Park <sj@...nel.org> wrote:
> > > > > The kernel-doc comment for 'struct damos_quota' describes how "effective
> > > > > quota" is calculated, but does not explain what it is. Actually there
> > > > > was an input[1] about it. Add the explanation on the comment.
> > > > >
> > > > > [1] https://github.com/damonitor/damo/issues/17#issuecomment-2497525043
> > > > >
> > > > > Cc: Yunjeong Mun <yunjeong.mun@...com>
> > > > > Cc: Honggyu Kim <honggyu.kim@...com>
> > > > > Signed-off-by: SeongJae Park <sj@...nel.org>
> > > > > ---
> > > > > include/linux/damon.h | 10 +++++++---
> > > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > > > > index a67f2c4940e9..a01bfe2ff616 100644
> > > > > --- a/include/linux/damon.h
> > > > > +++ b/include/linux/damon.h
> > > > > @@ -193,9 +193,13 @@ struct damos_quota_goal {
> > > > > * size quota is set, DAMON tries to apply the action only up to &sz bytes
> > > > > * within &reset_interval.
> > > > > *
> > > > > - * Internally, the time quota is transformed to a size quota using estimated
> > > > > - * throughput of the scheme's action. DAMON then compares it against &sz and
> > > > > - * uses smaller one as the effective quota.
> > > > > + * To convince the different types of quotas and goals, DAMON internally
> > > > > + * converts those into one single size quota called "effective quota". DAMON
> > > >
> > > > Could we use "effective size quota" instead of "effective quota"?
> > > > IMHO, it will better give an idea this is related to "esz" in the code,
> > > > which means effective size.
> > >
> > > The above sentence is saying it as one single "size" quota, so calling it
> > > "effective size quota" here feels like unnecessary duplicates of the word
> > > ("size") to me. I'd like to keep this sentence as is if you don't really mind.
> >
> > Since the time or other goals are eventually transformed into a size
> > quota, I thought the "effective size quota" makes sense but I won't
> > stick to my term here.
>
> I understand your concern. But I want to make it not very strictly fixed and
> well-defined term, but just somewhat understandable with common sense and given
> context, for flexibility and conciseness. So unless this is really makes it
> difficult to understand what it means even with common senses and the context
> I'd like to keep current form.
>
> I believe your answer to the above question is "no" since you mentioned you
> won't stick to your term. Please let me know if I'm misreading you.
OK. I'm fine with the changes with this patch. I won't ask more changes.
> >
> > We originally asked this question about the term "effective" itself as
> > we didn't find an explanation what "effective" means actually in the
> > doc. It'd be better to have more explicit explanation as well.
>
> I think this patch makes the point pretty explicit, so my humble brain is bit
> confused what "more explicit" really means. Could you please clarify what
> changes you want to be added?
Nevermind, your changes here with some grammar and typo fixes are enough
for this patch.
Thanks,
Honggyu
>
> Thanks,
> SJ
>
> [...]
Powered by blists - more mailing lists