[PATCH] writeback: initialize m_dirty to avoid compile warning

[PATCH] writeback: initialize m_dirty to avoid compile warning

[Yang Shi]

When building kernel with gcc 5.2, the below warning is raised:

mm/page-writeback.c: In function 'balance_dirty_pages.isra.10':
mm/page-writeback.c:1545:17: warning: 'm_dirty' may be used uninitialized in this function [-Wmaybe-uninitialized]
   unsigned long m_dirty, m_thresh, m_bg_thresh;

The m_dirty{thresh, bg_thresh} are initialized in the block of "if (mdtc)",
so if mdts is null, they won't be initialized before being used.
Initialize m_dirty to zero, also initialize m_thresh and m_bg_thresh to keep
consistency.

They are used later by if condition:
!mdtc || m_dirty <= dirty_freerun_ceiling(m_thresh, m_bg_thresh)

If mdtc is null, dirty_freerun_ceiling will not be called at all, so the
initialization will not change any behavior other than just ceasing the compile
warning.

Signed-off-by: Yang Shi <yang.shi@linaro.org>
---
 mm/page-writeback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 2c90357..ce726eb 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1542,7 +1542,7 @@ static void balance_dirty_pages(struct address_space *mapping,
 	for (;;) {
 		unsigned long now = jiffies;
 		unsigned long dirty, thresh, bg_thresh;
-		unsigned long m_dirty, m_thresh, m_bg_thresh;
+		unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
 
 		/*
 		 * Unstable writes are a feature of certain networked
-- 
2.0.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Andrew Morton]

On Fri, 13 Nov 2015 10:26:41 -0800 Yang Shi <yang.shi@linaro.org> wrote:

> When building kernel with gcc 5.2, the below warning is raised:
> 
> mm/page-writeback.c: In function 'balance_dirty_pages.isra.10':
> mm/page-writeback.c:1545:17: warning: 'm_dirty' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    unsigned long m_dirty, m_thresh, m_bg_thresh;
> 
> The m_dirty{thresh, bg_thresh} are initialized in the block of "if (mdtc)",
> so if mdts is null, they won't be initialized before being used.
> Initialize m_dirty to zero, also initialize m_thresh and m_bg_thresh to keep
> consistency.
> 
> They are used later by if condition:
> !mdtc || m_dirty <= dirty_freerun_ceiling(m_thresh, m_bg_thresh)
> 
> If mdtc is null, dirty_freerun_ceiling will not be called at all, so the
> initialization will not change any behavior other than just ceasing the compile
> warning.

Geeze I hate that warning.  gcc really could be a bit smarter about it
and this is such a case.

> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1542,7 +1542,7 @@ static void balance_dirty_pages(struct address_space *mapping,
>  	for (;;) {
>  		unsigned long now = jiffies;
>  		unsigned long dirty, thresh, bg_thresh;
> -		unsigned long m_dirty, m_thresh, m_bg_thresh;
> +		unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
>  
>  		/*
>  		 * Unstable writes are a feature of certain networked

Adding runtime overhead to suppress a compile-time warning is Just
Wrong.

With gcc-4.4.4 the above patch actually reduces page-writeback.o's
.text by 36 bytes, lol.  With gcc-4.8.4 the patch saves 19 bytes.  No
idea what's going on there...


And initializing locals in the above fashion can hide real bugs -
looky:

--- a/mm/page-writeback.c~a
+++ a/mm/page-writeback.c
@@ -1544,6 +1544,8 @@ static void balance_dirty_pages(struct a
 		unsigned long dirty, thresh, bg_thresh;
 		unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
 
+		printk("%lu\n", m_dirty);
+
 		/*
 		 * Unstable writes are a feature of certain networked
 		 * filesystems (i.e. NFS) in which data may have been

After the fake initialization there is no warning.  Perhaps it would be
better to initialize these things to some insane value so the kernel
will at least malfunction in some observable fashion if this happens.

I think unintialized_var() is a good solution - it's self-documenting
and adds no overhead.  It still has the can-hide-real-bugs issue, but
it's better than fake initialization.

But Linus chucks a wobbly over unintialized_var() so shrug.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Arnd Bergmann]

On Tuesday 17 November 2015 15:38:55 Andrew Morton wrote:
> On Fri, 13 Nov 2015 10:26:41 -0800 Yang Shi <yang.shi@linaro.org> wrote:
> 
> > When building kernel with gcc 5.2, the below warning is raised:
> > 
> > mm/page-writeback.c: In function 'balance_dirty_pages.isra.10':
> > mm/page-writeback.c:1545:17: warning: 'm_dirty' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >    unsigned long m_dirty, m_thresh, m_bg_thresh;
> > 
> > The m_dirty{thresh, bg_thresh} are initialized in the block of "if (mdtc)",
> > so if mdts is null, they won't be initialized before being used.
> > Initialize m_dirty to zero, also initialize m_thresh and m_bg_thresh to keep
> > consistency.
> > 
> > They are used later by if condition:
> > !mdtc || m_dirty <= dirty_freerun_ceiling(m_thresh, m_bg_thresh)
> > 
> > If mdtc is null, dirty_freerun_ceiling will not be called at all, so the
> > initialization will not change any behavior other than just ceasing the compile
> > warning.
> 
> Geeze I hate that warning.  gcc really could be a bit smarter about it
> and this is such a case.
> 
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -1542,7 +1542,7 @@ static void balance_dirty_pages(struct address_space *mapping,
> >       for (;;) {
> >               unsigned long now = jiffies;
> >               unsigned long dirty, thresh, bg_thresh;
> > -             unsigned long m_dirty, m_thresh, m_bg_thresh;
> > +             unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
> >  
> >               /*
> >                * Unstable writes are a feature of certain networked
> 
> Adding runtime overhead to suppress a compile-time warning is Just
> Wrong.
> 
> With gcc-4.4.4 the above patch actually reduces page-writeback.o's
> .text by 36 bytes, lol.  With gcc-4.8.4 the patch saves 19 bytes.  No
> idea what's going on there...

I've done tons of build tests and never got the warning for the variables
other than m_dirty, and that one also just with very few configurations
(e.g. ARM omap2plus_defconfig).
 
How about initializing only m_dirty but not the others?

	Arnd

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Shi, Yang]

On 11/18/2015 1:53 AM, Arnd Bergmann wrote:
> On Tuesday 17 November 2015 15:38:55 Andrew Morton wrote:
>> On Fri, 13 Nov 2015 10:26:41 -0800 Yang Shi <yang.shi@linaro.org> wrote:
>>
>>> When building kernel with gcc 5.2, the below warning is raised:
>>>
>>> mm/page-writeback.c: In function 'balance_dirty_pages.isra.10':
>>> mm/page-writeback.c:1545:17: warning: 'm_dirty' may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>     unsigned long m_dirty, m_thresh, m_bg_thresh;
>>>
>>> The m_dirty{thresh, bg_thresh} are initialized in the block of "if (mdtc)",
>>> so if mdts is null, they won't be initialized before being used.
>>> Initialize m_dirty to zero, also initialize m_thresh and m_bg_thresh to keep
>>> consistency.
>>>
>>> They are used later by if condition:
>>> !mdtc || m_dirty <= dirty_freerun_ceiling(m_thresh, m_bg_thresh)
>>>
>>> If mdtc is null, dirty_freerun_ceiling will not be called at all, so the
>>> initialization will not change any behavior other than just ceasing the compile
>>> warning.
>>
>> Geeze I hate that warning.  gcc really could be a bit smarter about it
>> and this is such a case.
>>
>>> --- a/mm/page-writeback.c
>>> +++ b/mm/page-writeback.c
>>> @@ -1542,7 +1542,7 @@ static void balance_dirty_pages(struct address_space *mapping,
>>>        for (;;) {
>>>                unsigned long now = jiffies;
>>>                unsigned long dirty, thresh, bg_thresh;
>>> -             unsigned long m_dirty, m_thresh, m_bg_thresh;
>>> +             unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
>>>
>>>                /*
>>>                 * Unstable writes are a feature of certain networked
>>
>> Adding runtime overhead to suppress a compile-time warning is Just
>> Wrong.
>>
>> With gcc-4.4.4 the above patch actually reduces page-writeback.o's
>> .text by 36 bytes, lol.  With gcc-4.8.4 the patch saves 19 bytes.  No
>> idea what's going on there...
>
> I've done tons of build tests and never got the warning for the variables
> other than m_dirty, and that one also just with very few configurations
> (e.g. ARM omap2plus_defconfig).

Yes, I just got the warning for m_dirty too. Just initialize m_thresh 
and m_bg_thresh to keep consistency (not sure if it is necessary). And, 
I'm a little bit confused why gcc just reports m_dirty but ignore others.

>
> How about initializing only m_dirty but not the others?

Fine to me.

Thanks,
Yang

>
> 	Arnd
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Tejun Heo]

Hello,

On Tue, Nov 17, 2015 at 03:38:55PM -0800, Andrew Morton wrote:
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -1542,7 +1542,7 @@ static void balance_dirty_pages(struct address_space *mapping,
> >  	for (;;) {
> >  		unsigned long now = jiffies;
> >  		unsigned long dirty, thresh, bg_thresh;
> > -		unsigned long m_dirty, m_thresh, m_bg_thresh;
> > +		unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
> >  
> >  		/*
> >  		 * Unstable writes are a feature of certain networked
> 
> Adding runtime overhead to suppress a compile-time warning is Just
> Wrong.
> 
> With gcc-4.4.4 the above patch actually reduces page-writeback.o's
> .text by 36 bytes, lol.  With gcc-4.8.4 the patch saves 19 bytes.  No
> idea what's going on there...
> 
> 
> And initializing locals in the above fashion can hide real bugs -
> looky:

This was the main reason the code was structured the way it is.  If
cgroup writeback is not enabled, any derefs of mdtc variables should
trigger warnings.  Ugh... I don't know.  Compiler really should be
able to tell this much.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Shi, Yang]

On 11/18/2015 10:11 AM, Tejun Heo wrote:
> Hello,
>
> On Tue, Nov 17, 2015 at 03:38:55PM -0800, Andrew Morton wrote:
>>> --- a/mm/page-writeback.c
>>> +++ b/mm/page-writeback.c
>>> @@ -1542,7 +1542,7 @@ static void balance_dirty_pages(struct address_space *mapping,
>>>   	for (;;) {
>>>   		unsigned long now = jiffies;
>>>   		unsigned long dirty, thresh, bg_thresh;
>>> -		unsigned long m_dirty, m_thresh, m_bg_thresh;
>>> +		unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
>>>
>>>   		/*
>>>   		 * Unstable writes are a feature of certain networked
>>
>> Adding runtime overhead to suppress a compile-time warning is Just
>> Wrong.
>>
>> With gcc-4.4.4 the above patch actually reduces page-writeback.o's
>> .text by 36 bytes, lol.  With gcc-4.8.4 the patch saves 19 bytes.  No
>> idea what's going on there...
>>
>>
>> And initializing locals in the above fashion can hide real bugs -
>> looky:
>
> This was the main reason the code was structured the way it is.  If
> cgroup writeback is not enabled, any derefs of mdtc variables should
> trigger warnings.  Ugh... I don't know.  Compiler really should be
> able to tell this much.

Thanks for the explanation. It sounds like a compiler problem.

If you think it is still good to cease the compile warning, maybe we 
could just assign it to an insane value as what Andrew suggested, maybe 
0xdeadbeef.

Thanks,
Yang

>
> Thanks.
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Tejun Heo]

Hello,

On Wed, Nov 18, 2015 at 10:27:32AM -0800, Shi, Yang wrote:
> >This was the main reason the code was structured the way it is.  If
> >cgroup writeback is not enabled, any derefs of mdtc variables should
> >trigger warnings.  Ugh... I don't know.  Compiler really should be
> >able to tell this much.
> 
> Thanks for the explanation. It sounds like a compiler problem.
> 
> If you think it is still good to cease the compile warning, maybe we could

If this is gonna be a problem with new gcc versions, I don't think we
have any other options. :(

> just assign it to an insane value as what Andrew suggested, maybe
> 0xdeadbeef.

I'd just keep it at zero.  Whatever we do, the effect is gonna be
difficult to track down - it's not gonna blow up in an obvious way.
Can you please add a comment tho explaining that this is to work
around compiler deficiency?

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Shi, Yang]

On 11/18/2015 10:33 AM, Tejun Heo wrote:
> Hello,
>
> On Wed, Nov 18, 2015 at 10:27:32AM -0800, Shi, Yang wrote:
>>> This was the main reason the code was structured the way it is.  If
>>> cgroup writeback is not enabled, any derefs of mdtc variables should
>>> trigger warnings.  Ugh... I don't know.  Compiler really should be
>>> able to tell this much.
>>
>> Thanks for the explanation. It sounds like a compiler problem.
>>
>> If you think it is still good to cease the compile warning, maybe we could
>
> If this is gonna be a problem with new gcc versions, I don't think we
> have any other options. :(
>
>> just assign it to an insane value as what Andrew suggested, maybe
>> 0xdeadbeef.
>
> I'd just keep it at zero.  Whatever we do, the effect is gonna be
> difficult to track down - it's not gonna blow up in an obvious way.
> Can you please add a comment tho explaining that this is to work
> around compiler deficiency?

Sure.

Other than this, in v2, I will just initialize m_dirty since compiler 
just reports it is uninitialized.

Thanks,
Yang

>
> Thanks.
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Andrew Morton]

On Wed, 18 Nov 2015 10:39:23 -0800 "Shi, Yang" <yang.shi@linaro.org> wrote:

> On 11/18/2015 10:33 AM, Tejun Heo wrote:
> > Hello,
> >
> > On Wed, Nov 18, 2015 at 10:27:32AM -0800, Shi, Yang wrote:
> >>> This was the main reason the code was structured the way it is.  If
> >>> cgroup writeback is not enabled, any derefs of mdtc variables should
> >>> trigger warnings.  Ugh... I don't know.  Compiler really should be
> >>> able to tell this much.
> >>
> >> Thanks for the explanation. It sounds like a compiler problem.
> >>
> >> If you think it is still good to cease the compile warning, maybe we could
> >
> > If this is gonna be a problem with new gcc versions, I don't think we
> > have any other options. :(
> >
> >> just assign it to an insane value as what Andrew suggested, maybe
> >> 0xdeadbeef.
> >
> > I'd just keep it at zero.  Whatever we do, the effect is gonna be
> > difficult to track down - it's not gonna blow up in an obvious way.
> > Can you please add a comment tho explaining that this is to work
> > around compiler deficiency?
> 
> Sure.
> 
> Other than this, in v2, I will just initialize m_dirty since compiler 
> just reports it is uninitialized.

gcc-4.4.4 and gcc-4.8.4 warn about all three variables.


--- a/mm/page-writeback.c~writeback-initialize-m_dirty-to-avoid-compile-warning-fix
+++ a/mm/page-writeback.c
@@ -1542,7 +1542,9 @@ static void balance_dirty_pages(struct a
 	for (;;) {
 		unsigned long now = jiffies;
 		unsigned long dirty, thresh, bg_thresh;
-		unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
+		unsigned long m_dirty = 0;	/* stop bogus uninit warnings */
+		unsigned long m_thresh = 0;
+		unsigned long m_bg_thresh = 0;
 
 		/*
 		 * Unstable writes are a feature of certain networked
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] writeback: initialize m_dirty to avoid compile warning

[Shi, Yang]

On 11/18/2015 10:55 AM, Andrew Morton wrote:
> On Wed, 18 Nov 2015 10:39:23 -0800 "Shi, Yang" <yang.shi@linaro.org> wrote:
>
>> On 11/18/2015 10:33 AM, Tejun Heo wrote:
>>> Hello,
>>>
>>> On Wed, Nov 18, 2015 at 10:27:32AM -0800, Shi, Yang wrote:
>>>>> This was the main reason the code was structured the way it is.  If
>>>>> cgroup writeback is not enabled, any derefs of mdtc variables should
>>>>> trigger warnings.  Ugh... I don't know.  Compiler really should be
>>>>> able to tell this much.
>>>>
>>>> Thanks for the explanation. It sounds like a compiler problem.
>>>>
>>>> If you think it is still good to cease the compile warning, maybe we could
>>>
>>> If this is gonna be a problem with new gcc versions, I don't think we
>>> have any other options. :(
>>>
>>>> just assign it to an insane value as what Andrew suggested, maybe
>>>> 0xdeadbeef.
>>>
>>> I'd just keep it at zero.  Whatever we do, the effect is gonna be
>>> difficult to track down - it's not gonna blow up in an obvious way.
>>> Can you please add a comment tho explaining that this is to work
>>> around compiler deficiency?
>>
>> Sure.
>>
>> Other than this, in v2, I will just initialize m_dirty since compiler
>> just reports it is uninitialized.
>
> gcc-4.4.4 and gcc-4.8.4 warn about all three variables.

It sounds 5.x is smarter :-)
>
>
> --- a/mm/page-writeback.c~writeback-initialize-m_dirty-to-avoid-compile-warning-fix
> +++ a/mm/page-writeback.c
> @@ -1542,7 +1542,9 @@ static void balance_dirty_pages(struct a
>   	for (;;) {
>   		unsigned long now = jiffies;
>   		unsigned long dirty, thresh, bg_thresh;
> -		unsigned long m_dirty = 0, m_thresh = 0, m_bg_thresh = 0;
> +		unsigned long m_dirty = 0;	/* stop bogus uninit warnings */
> +		unsigned long m_thresh = 0;
> +		unsigned long m_bg_thresh = 0;

Still need v2?

Thanks,
Yang

>
>   		/*
>   		 * Unstable writes are a feature of certain networked
> _
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>