From: Jan Kara <jack@suse.cz>
To: Kemeng Shi <shikemeng@huaweicloud.com>
Cc: Jan Kara <jack@suse.cz>, Tejun Heo <tj@kernel.org>,
akpm@linux-foundation.org, linux-mm@kvack.org,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
willy@infradead.org, bfoster@redhat.com, dsterba@suse.com,
mjguzik@gmail.com, dhowells@redhat.com, peterz@infradead.org
Subject: Re: [PATCH 6/6] writeback: remove unneeded GDTC_INIT_NO_WB
Date: Tue, 2 Apr 2024 15:53:06 +0200 [thread overview]
Message-ID: <20240402135306.kluke2jjcmh5f4ei@quack3> (raw)
In-Reply-To: <c2fc01e2-f15a-d331-6c4f-64319f3adc8a@huaweicloud.com>
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
next prev parent reply other threads:[~2024-04-02 13:53 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-20 11:02 [PATCH 0/6] Improve visibility of writeback Kemeng Shi
2024-03-20 11:02 ` [PATCH 1/6] writeback: collect stats of all wb of bdi in bdi_debug_stats_show Kemeng Shi
2024-03-20 13:21 ` Brian Foster
2024-03-21 3:44 ` Kemeng Shi
2024-03-21 12:10 ` Brian Foster
2024-03-22 7:32 ` Kemeng Shi
2024-03-21 18:06 ` Jan Kara
2024-03-22 7:51 ` Kemeng Shi
2024-03-22 11:58 ` Brian Foster
2024-03-26 13:16 ` Kemeng Shi
2024-03-20 11:02 ` [PATCH 2/6] writeback: support retrieving per group debug writeback stats of bdi Kemeng Shi
2024-03-20 15:01 ` Tejun Heo
2024-03-21 3:45 ` Kemeng Shi
2024-03-26 12:24 ` Jan Kara
2024-03-26 13:26 ` Kemeng Shi
2024-03-20 11:02 ` [PATCH 3/6] workqueue: remove unnecessary import and function in wq_monitor.py Kemeng Shi
2024-03-20 15:03 ` Tejun Heo
2024-03-21 6:08 ` Kemeng Shi
2024-03-20 11:02 ` [PATCH 4/6] writeback: add wb_monitor.py script to monitor writeback info on bdi Kemeng Shi
2024-03-20 15:12 ` Tejun Heo
2024-03-21 6:22 ` Kemeng Shi
2024-03-20 11:02 ` [PATCH 5/6] writeback: rename nr_reclaimable to nr_dirty in balance_dirty_pages Kemeng Shi
2024-03-26 12:27 ` Jan Kara
2024-03-20 11:02 ` [PATCH 6/6] writeback: remove unneeded GDTC_INIT_NO_WB Kemeng Shi
2024-03-20 15:15 ` Tejun Heo
2024-03-21 7:12 ` Kemeng Shi
2024-03-25 20:26 ` Tejun Heo
2024-03-26 13:17 ` Kemeng Shi
2024-03-27 9:33 ` Jan Kara
2024-03-28 1:49 ` Kemeng Shi
2024-04-02 13:53 ` Jan Kara [this message]
2024-04-03 8:50 ` Kemeng Shi
2024-03-26 12:35 ` Jan Kara
2024-03-26 13:30 ` Kemeng Shi
2024-03-20 15:20 ` [PATCH 0/6] Improve visibility of writeback Tejun Heo
2024-03-20 17:22 ` Jan Kara
2024-03-21 8:12 ` Kemeng Shi
2024-03-21 18:07 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240402135306.kluke2jjcmh5f4ei@quack3 \
--to=jack@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=bfoster@redhat.com \
--cc=dhowells@redhat.com \
--cc=dsterba@suse.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mjguzik@gmail.com \
--cc=peterz@infradead.org \
--cc=shikemeng@huaweicloud.com \
--cc=tj@kernel.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).