Re: [PATCH] mm: fix uninitialized variable warnings

[Jan Kara <jack@suse.cz>]

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@arndb.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@suse.com>
SUSE Labs, CR