[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20181105090114.GD6953@quack2.suse.cz>
Date: Mon, 5 Nov 2018 10:01:14 +0100
From: Jan Kara <jack@...e.cz>
To: Arnd Bergmann <arnd@...db.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Jan Kara <jack@...e.cz>,
Michal Hocko <mhocko@...e.com>,
Wang Long <wanglong19@...tuan.com>,
Matthew Wilcox <willy@...radead.org>,
Dave Chinner <dchinner@...hat.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH] mm: fix uninitialized variable warnings
On Fri 02-11-18 16:31:06, Arnd Bergmann wrote:
> In a rare randconfig build, I got a warning about possibly uninitialized
> variables:
>
> mm/page-writeback.c: In function 'balance_dirty_pages':
> mm/page-writeback.c:1623:16: error: 'writeback' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> mdtc->dirty += writeback;
> ^~
> mm/page-writeback.c:1624:4: error: 'filepages' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> mdtc_calc_avail(mdtc, filepages, headroom);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> mm/page-writeback.c:1624:4: error: 'headroom' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The compiler evidently fails to notice that the usage is in dead code
> after 'mdtc' is set to NULL when CONFIG_CGROUP_WRITEBACK is disabled.
> Adding an IS_ENABLED() check makes this clear to the compiler.
>
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
I'm surprised the compiler was not able to infer this since:
struct dirty_throttle_control * const mdtc = mdtc_valid(&mdtc_stor) ?
&mdtc_stor : NULL;
and if CONFIG_CGROUP_WRITEBACK is disabled, mdtc_valid() is defined to
'false'. But possibly the function is just too big and the problematic
condition is in the loop so maybe it all confuses the compiler too much.
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 3f690bae6b78..f02535b7731a 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1611,7 +1611,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
> bg_thresh = gdtc->bg_thresh;
> }
>
> - if (mdtc) {
> + if (IS_ENABLED(CONFIG_CGROUP_WRITEBACK) && mdtc) {
> unsigned long filepages, headroom, writeback;
Honestly, I don't like the IS_ENABLED(CONFIG_CGROUP_WRITEBACK) check here.
It just looks too arbitrary. Could we perhaps change the code like
struct dirty_throttle_control * const mdtc = &mdtc_stor;
And then replace checks for !mtdc in the function to !mdtc_valid(mdtc)?
That is the same thing as currently and it should make it obvious to the
compiler as well as human what is going on... Tejun?
Honza
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists