We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
GDTC_INIT_NO_WB
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
mm/page-writeback.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 481b6bf34c21..09b2b0754cc5 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -154,8 +154,6 @@ struct dirty_throttle_control {
.dom = &global_wb_domain, \
.wb_completions = &(__wb)->completions
-#define GDTC_INIT_NO_WB .dom = &global_wb_domain
-
#define MDTC_INIT(__wb, __gdtc) .wb = (__wb), \
.dom = mem_cgroup_wb_domain(__wb), \
.wb_completions = &(__wb)->memcg_completions, \
@@ -210,7 +208,6 @@ static void wb_min_max_ratio(struct bdi_writeback *wb,
#define GDTC_INIT(__wb) .wb = (__wb), \
.wb_completions = &(__wb)->completions
-#define GDTC_INIT_NO_WB
#define MDTC_INIT(__wb, __gdtc)
static bool mdtc_valid(struct dirty_throttle_control *dtc)
@@ -438,7 +435,7 @@ static void domain_dirty_limits(struct dirty_throttle_control *dtc)
*/
void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
{
- struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
+ struct dirty_throttle_control gdtc = { };
gdtc.avail = global_dirtyable_memory();
domain_dirty_limits(&gdtc);
@@ -895,7 +892,7 @@ unsigned long wb_calc_thresh(struct bdi_writeback *wb, unsigned long thresh)
unsigned long wb_calc_cg_thresh(struct bdi_writeback *wb)
{
- struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
+ struct dirty_throttle_control gdtc = { };
struct dirty_throttle_control mdtc = { MDTC_INIT(wb, &gdtc) };
unsigned long filepages, headroom, writeback;
--
2.30.0On Wed 20-03-24 19:02:22, Kemeng Shi wrote:
> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
> GDTC_INIT_NO_WB
>
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Please no, this leaves a trap for the future. If anything, I'd teach
GDTC_INIT() that 'wb' can be NULL and replace GDTC_INIT_NO_WB with
GDTC_INIT(NULL).
Honza
> ---
> mm/page-writeback.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 481b6bf34c21..09b2b0754cc5 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -154,8 +154,6 @@ struct dirty_throttle_control {
> .dom = &global_wb_domain, \
> .wb_completions = &(__wb)->completions
>
> -#define GDTC_INIT_NO_WB .dom = &global_wb_domain
> -
> #define MDTC_INIT(__wb, __gdtc) .wb = (__wb), \
> .dom = mem_cgroup_wb_domain(__wb), \
> .wb_completions = &(__wb)->memcg_completions, \
> @@ -210,7 +208,6 @@ static void wb_min_max_ratio(struct bdi_writeback *wb,
>
> #define GDTC_INIT(__wb) .wb = (__wb), \
> .wb_completions = &(__wb)->completions
> -#define GDTC_INIT_NO_WB
> #define MDTC_INIT(__wb, __gdtc)
>
> static bool mdtc_valid(struct dirty_throttle_control *dtc)
> @@ -438,7 +435,7 @@ static void domain_dirty_limits(struct dirty_throttle_control *dtc)
> */
> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
> {
> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
> + struct dirty_throttle_control gdtc = { };
>
> gdtc.avail = global_dirtyable_memory();
> domain_dirty_limits(&gdtc);
> @@ -895,7 +892,7 @@ unsigned long wb_calc_thresh(struct bdi_writeback *wb, unsigned long thresh)
>
> unsigned long wb_calc_cg_thresh(struct bdi_writeback *wb)
> {
> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
> + struct dirty_throttle_control gdtc = { };
> struct dirty_throttle_control mdtc = { MDTC_INIT(wb, &gdtc) };
> unsigned long filepages, headroom, writeback;
>
> --
> 2.30.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
on 3/26/2024 8:35 PM, Jan Kara wrote:
> On Wed 20-03-24 19:02:22, Kemeng Shi wrote:
>> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
>> GDTC_INIT_NO_WB
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>
> Please no, this leaves a trap for the future. If anything, I'd teach
> GDTC_INIT() that 'wb' can be NULL and replace GDTC_INIT_NO_WB with
> GDTC_INIT(NULL).
Would it be acceptable to define GDTC_INIT_NO_WB to null for now as
discussed in [1].
[1] https://lore.kernel.org/lkml/becdb16b-a318-ec05-61d2-d190541ae997@huaweicloud.com/
Thanks,
Kemeng
>
> Honza
>
>> ---
>> mm/page-writeback.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> index 481b6bf34c21..09b2b0754cc5 100644
>> --- a/mm/page-writeback.c
>> +++ b/mm/page-writeback.c
>> @@ -154,8 +154,6 @@ struct dirty_throttle_control {
>> .dom = &global_wb_domain, \
>> .wb_completions = &(__wb)->completions
>>
>> -#define GDTC_INIT_NO_WB .dom = &global_wb_domain
>> -
>> #define MDTC_INIT(__wb, __gdtc) .wb = (__wb), \
>> .dom = mem_cgroup_wb_domain(__wb), \
>> .wb_completions = &(__wb)->memcg_completions, \
>> @@ -210,7 +208,6 @@ static void wb_min_max_ratio(struct bdi_writeback *wb,
>>
>> #define GDTC_INIT(__wb) .wb = (__wb), \
>> .wb_completions = &(__wb)->completions
>> -#define GDTC_INIT_NO_WB
>> #define MDTC_INIT(__wb, __gdtc)
>>
>> static bool mdtc_valid(struct dirty_throttle_control *dtc)
>> @@ -438,7 +435,7 @@ static void domain_dirty_limits(struct dirty_throttle_control *dtc)
>> */
>> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
>> {
>> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>> + struct dirty_throttle_control gdtc = { };
>>
>> gdtc.avail = global_dirtyable_memory();
>> domain_dirty_limits(&gdtc);
>> @@ -895,7 +892,7 @@ unsigned long wb_calc_thresh(struct bdi_writeback *wb, unsigned long thresh)
>>
>> unsigned long wb_calc_cg_thresh(struct bdi_writeback *wb)
>> {
>> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>> + struct dirty_throttle_control gdtc = { };
>> struct dirty_throttle_control mdtc = { MDTC_INIT(wb, &gdtc) };
>> unsigned long filepages, headroom, writeback;
>>
>> --
>> 2.30.0
>>
Hello,
On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
> GDTC_INIT_NO_WB
>
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
...
> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
> {
> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
> + struct dirty_throttle_control gdtc = { };
Even if it's currently not referenced, wouldn't it still be better to always
guarantee that a dtc's dom is always initialized? I'm not sure what we get
by removing this.
Thanks.
--
tejun
on 3/20/2024 11:15 PM, Tejun Heo wrote:
> Hello,
>
> On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
>> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
>> GDTC_INIT_NO_WB
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ...
>> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
>> {
>> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>> + struct dirty_throttle_control gdtc = { };
>
> Even if it's currently not referenced, wouldn't it still be better to always
> guarantee that a dtc's dom is always initialized? I'm not sure what we get
> by removing this.
As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
calculating dirty limit with domain_dirty_limits, I intuitively think the dirty
limit calculation in domain_dirty_limits is related to global_wb_domain when
CONFIG_WRITEBACK_CGROUP is enabled while the truth is not. So this is a little
confusing to me.
Would it be acceptable to you that we keep useing GDTC_INIT_NO_WB but
define GDTC_INIT_NO_WB to null fow now and redefine GDTC_INIT_NO_WB when some
member of gdtc is really needed.
Of couse I'm not insistent on this. Would like to hear you suggestion. Thanks!
>
> Thanks.
>
On Thu 21-03-24 15:12:21, Kemeng Shi wrote:
>
>
> on 3/20/2024 11:15 PM, Tejun Heo wrote:
> > Hello,
> >
> > On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
> >> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
> >> GDTC_INIT_NO_WB
> >>
> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> > ...
> >> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
> >> {
> >> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
> >> + struct dirty_throttle_control gdtc = { };
> >
> > Even if it's currently not referenced, wouldn't it still be better to always
> > guarantee that a dtc's dom is always initialized? I'm not sure what we get
> > by removing this.
> As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
> calculating dirty limit with domain_dirty_limits, I intuitively think the
> dirty limit calculation in domain_dirty_limits is related to
> global_wb_domain when CONFIG_WRITEBACK_CGROUP is enabled while the truth
> is not. So this is a little confusing to me.
I'm not sure I understand your confusion. domain_dirty_limits() calculates
the dirty limit (and background dirty limit) for the dirty_throttle_control
passed in. If you pass dtc initialized with GDTC_INIT[_NO_WB], it will
compute global dirty limits. If the dtc passed in is initialized with
MDTC_INIT() it will compute cgroup specific dirty limits.
Now because domain_dirty_limits() does not scale the limits based on each
device throughput - that is done only later in __wb_calc_thresh() to avoid
relatively expensive computations when we don't need them - and also
because the effective dirty limit (dtc->dom->dirty_limit) is not updated by
domain_dirty_limits(), domain_dirty_limits() does not need dtc->dom at all.
But that is a technical detail of implementation and I don't want this
technical detail to be relied on by even more code.
What might have confused you is that GDTC_INIT_NO_WB is defined to be empty
when CONFIG_CGROUP_WRITEBACK is disabled. But this is only because in that
case dtc_dom() function unconditionally returns global_wb_domain so we
don't bother with initializing (or even having) the 'dom' field anywhere.
Now I agree this whole code is substantially confusing and complex and it
would all deserve some serious thought how to make it more readable. But
even after thinking about it again I don't think removing GDTC_INIT_NO_WB is
the right way to go.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
on 3/27/2024 5:33 PM, Jan Kara wrote:
> On Thu 21-03-24 15:12:21, Kemeng Shi wrote:
>>
>>
>> on 3/20/2024 11:15 PM, Tejun Heo wrote:
>>> Hello,
>>>
>>> On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
>>>> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
>>>> GDTC_INIT_NO_WB
>>>>
>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>> ...
>>>> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
>>>> {
>>>> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>>>> + struct dirty_throttle_control gdtc = { };
>>>
>>> Even if it's currently not referenced, wouldn't it still be better to always
>>> guarantee that a dtc's dom is always initialized? I'm not sure what we get
>>> by removing this.
>> As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
>> calculating dirty limit with domain_dirty_limits, I intuitively think the
>> dirty limit calculation in domain_dirty_limits is related to
>> global_wb_domain when CONFIG_WRITEBACK_CGROUP is enabled while the truth
>> is not. So this is a little confusing to me.
>
Hi Jan,
> I'm not sure I understand your confusion. domain_dirty_limits() calculates
> the dirty limit (and background dirty limit) for the dirty_throttle_control
> passed in. If you pass dtc initialized with GDTC_INIT[_NO_WB], it will
> compute global dirty limits. If the dtc passed in is initialized with
> MDTC_INIT() it will compute cgroup specific dirty limits.
No doubt about this.
>
> Now because domain_dirty_limits() does not scale the limits based on each
> device throughput - that is done only later in __wb_calc_thresh() to avoid> relatively expensive computations when we don't need them - and also
> because the effective dirty limit (dtc->dom->dirty_limit) is not updated by
> domain_dirty_limits(), domain_dirty_limits() does not need dtc->dom at all.
Acutally, here is the thing confusing me. For wb_calc_thresh, we always pass
dtc initialized with a wb (GDTC_INIT(wb) or MDTC_INIT(wb,..). The dtc
initialized with _NO_WB is only passed to domain_dirty_limits. However, The
dom initialized by _NO_WB for domain_dirty_limits is not needed at all.
> But that is a technical detail of implementation and I don't want this
> technical detail to be relied on by even more code.
Yes, I agree with this. So I wonder if it's acceptable to simply define
GDTC_INIT_NO_WB to empty for now instead of remove defination of
GDTC_INIT_NO_WB. When implementation of domain_dirty_limits() or any
other low level function in future using GDTC_INIT(_NO_WB) changes to
need dtc->domain, we re-define GDTC_INIT_NO_WB to proper value.
As this only looks confusing to me. I will drop this one in next version
if you still prefer to keep definatino of GDTC_INIT_NO_WB in the old way.
Thanks,
Kemeng
>
> What might have confused you is that GDTC_INIT_NO_WB is defined to be empty
> when CONFIG_CGROUP_WRITEBACK is disabled. But this is only because in that
> case dtc_dom() function unconditionally returns global_wb_domain so we
> don't bother with initializing (or even having) the 'dom' field anywhere.
>
> Now I agree this whole code is substantially confusing and complex and it
> would all deserve some serious thought how to make it more readable. But
> even after thinking about it again I don't think removing GDTC_INIT_NO_WB is
> the right way to go.
>
> Honza
>
On Thu 28-03-24 09:49:59, Kemeng Shi wrote:
> on 3/27/2024 5:33 PM, Jan Kara wrote:
> > On Thu 21-03-24 15:12:21, Kemeng Shi wrote:
> >>
> >>
> >> on 3/20/2024 11:15 PM, Tejun Heo wrote:
> >>> Hello,
> >>>
> >>> On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
> >>>> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
> >>>> GDTC_INIT_NO_WB
> >>>>
> >>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> >>> ...
> >>>> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
> >>>> {
> >>>> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
> >>>> + struct dirty_throttle_control gdtc = { };
> >>>
> >>> Even if it's currently not referenced, wouldn't it still be better to always
> >>> guarantee that a dtc's dom is always initialized? I'm not sure what we get
> >>> by removing this.
> >> As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
> >> calculating dirty limit with domain_dirty_limits, I intuitively think the
> >> dirty limit calculation in domain_dirty_limits is related to
> >> global_wb_domain when CONFIG_WRITEBACK_CGROUP is enabled while the truth
> >> is not. So this is a little confusing to me.
> >
> Hi Jan,
> > I'm not sure I understand your confusion. domain_dirty_limits() calculates
> > the dirty limit (and background dirty limit) for the dirty_throttle_control
> > passed in. If you pass dtc initialized with GDTC_INIT[_NO_WB], it will
> > compute global dirty limits. If the dtc passed in is initialized with
> > MDTC_INIT() it will compute cgroup specific dirty limits.
> No doubt about this.
> >
> > Now because domain_dirty_limits() does not scale the limits based on each
> > device throughput - that is done only later in __wb_calc_thresh() to avoid> relatively expensive computations when we don't need them - and also
> > because the effective dirty limit (dtc->dom->dirty_limit) is not updated by
> > domain_dirty_limits(), domain_dirty_limits() does not need dtc->dom at all.
> Acutally, here is the thing confusing me. For wb_calc_thresh, we always pass
> dtc initialized with a wb (GDTC_INIT(wb) or MDTC_INIT(wb,..). The dtc
> initialized with _NO_WB is only passed to domain_dirty_limits. However, The
> dom initialized by _NO_WB for domain_dirty_limits is not needed at all.
> > But that is a technical detail of implementation and I don't want this
> > technical detail to be relied on by even more code.
> Yes, I agree with this. So I wonder if it's acceptable to simply define
> GDTC_INIT_NO_WB to empty for now instead of remove defination of
> GDTC_INIT_NO_WB. When implementation of domain_dirty_limits() or any
> other low level function in future using GDTC_INIT(_NO_WB) changes to
> need dtc->domain, we re-define GDTC_INIT_NO_WB to proper value.
> As this only looks confusing to me. I will drop this one in next version
> if you still prefer to keep definatino of GDTC_INIT_NO_WB in the old way.
Yeah, please keep the code as is for now. I agree this needs some cleanups
but what you suggest is IMHO not an improvement.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
on 4/2/2024 9:53 PM, Jan Kara wrote:
> On Thu 28-03-24 09:49:59, Kemeng Shi wrote:
>> on 3/27/2024 5:33 PM, Jan Kara wrote:
>>> On Thu 21-03-24 15:12:21, Kemeng Shi wrote:
>>>>
>>>>
>>>> on 3/20/2024 11:15 PM, Tejun Heo wrote:
>>>>> Hello,
>>>>>
>>>>> On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
>>>>>> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
>>>>>> GDTC_INIT_NO_WB
>>>>>>
>>>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>>>> ...
>>>>>> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
>>>>>> {
>>>>>> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>>>>>> + struct dirty_throttle_control gdtc = { };
>>>>>
>>>>> Even if it's currently not referenced, wouldn't it still be better to always
>>>>> guarantee that a dtc's dom is always initialized? I'm not sure what we get
>>>>> by removing this.
>>>> As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
>>>> calculating dirty limit with domain_dirty_limits, I intuitively think the
>>>> dirty limit calculation in domain_dirty_limits is related to
>>>> global_wb_domain when CONFIG_WRITEBACK_CGROUP is enabled while the truth
>>>> is not. So this is a little confusing to me.
>>>
>> Hi Jan,
>>> I'm not sure I understand your confusion. domain_dirty_limits() calculates
>>> the dirty limit (and background dirty limit) for the dirty_throttle_control
>>> passed in. If you pass dtc initialized with GDTC_INIT[_NO_WB], it will
>>> compute global dirty limits. If the dtc passed in is initialized with
>>> MDTC_INIT() it will compute cgroup specific dirty limits.
>> No doubt about this.
>>>
>>> Now because domain_dirty_limits() does not scale the limits based on each
>>> device throughput - that is done only later in __wb_calc_thresh() to avoid> relatively expensive computations when we don't need them - and also
>>> because the effective dirty limit (dtc->dom->dirty_limit) is not updated by
>>> domain_dirty_limits(), domain_dirty_limits() does not need dtc->dom at all.
>> Acutally, here is the thing confusing me. For wb_calc_thresh, we always pass
>> dtc initialized with a wb (GDTC_INIT(wb) or MDTC_INIT(wb,..). The dtc
>> initialized with _NO_WB is only passed to domain_dirty_limits. However, The
>> dom initialized by _NO_WB for domain_dirty_limits is not needed at all.
>>> But that is a technical detail of implementation and I don't want this
>>> technical detail to be relied on by even more code.
>> Yes, I agree with this. So I wonder if it's acceptable to simply define
>> GDTC_INIT_NO_WB to empty for now instead of remove defination of
>> GDTC_INIT_NO_WB. When implementation of domain_dirty_limits() or any
>> other low level function in future using GDTC_INIT(_NO_WB) changes to
>> need dtc->domain, we re-define GDTC_INIT_NO_WB to proper value.
>> As this only looks confusing to me. I will drop this one in next version
>> if you still prefer to keep definatino of GDTC_INIT_NO_WB in the old way.
>
> Yeah, please keep the code as is for now. I agree this needs some cleanups
> but what you suggest is IMHO not an improvement.
Sure, will drop this in next version.
Thanks,
Kemeng
>
> Honza
>
On Thu, Mar 21, 2024 at 03:12:21PM +0800, Kemeng Shi wrote:
>
>
> on 3/20/2024 11:15 PM, Tejun Heo wrote:
> > Hello,
> >
> > On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
> >> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
> >> GDTC_INIT_NO_WB
> >>
> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> > ...
> >> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
> >> {
> >> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
> >> + struct dirty_throttle_control gdtc = { };
> >
> > Even if it's currently not referenced, wouldn't it still be better to always
> > guarantee that a dtc's dom is always initialized? I'm not sure what we get
> > by removing this.
> As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
> calculating dirty limit with domain_dirty_limits, I intuitively think the dirty
> limit calculation in domain_dirty_limits is related to global_wb_domain when
> CONFIG_WRITEBACK_CGROUP is enabled while the truth is not. So this is a little
> confusing to me.
> Would it be acceptable to you that we keep useing GDTC_INIT_NO_WB but
> define GDTC_INIT_NO_WB to null fow now and redefine GDTC_INIT_NO_WB when some
> member of gdtc is really needed.
> Of couse I'm not insistent on this. Would like to hear you suggestion. Thanks!
Ah, I see. In that case, the proposed change of removing GDTC_INIT_NO_WB
looks good to me.
Thanks.
--
tejun
on 3/26/2024 4:26 AM, Tejun Heo wrote:
> On Thu, Mar 21, 2024 at 03:12:21PM +0800, Kemeng Shi wrote:
>>
>>
>> on 3/20/2024 11:15 PM, Tejun Heo wrote:
>>> Hello,
>>>
>>> On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
>>>> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
>>>> GDTC_INIT_NO_WB
>>>>
>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>> ...
>>>> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
>>>> {
>>>> - struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>>>> + struct dirty_throttle_control gdtc = { };
>>>
>>> Even if it's currently not referenced, wouldn't it still be better to always
>>> guarantee that a dtc's dom is always initialized? I'm not sure what we get
>>> by removing this.
>> As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
>> calculating dirty limit with domain_dirty_limits, I intuitively think the dirty
>> limit calculation in domain_dirty_limits is related to global_wb_domain when
>> CONFIG_WRITEBACK_CGROUP is enabled while the truth is not. So this is a little
>> confusing to me.
>> Would it be acceptable to you that we keep useing GDTC_INIT_NO_WB but
>> define GDTC_INIT_NO_WB to null fow now and redefine GDTC_INIT_NO_WB when some
>> member of gdtc is really needed.
>> Of couse I'm not insistent on this. Would like to hear you suggestion. Thanks!
>
> Ah, I see. In that case, the proposed change of removing GDTC_INIT_NO_WB
> looks good to me.
Sure, will do it in next version. Thanks!
>
> Thanks.
>
© 2016 - 2026 Red Hat, Inc.