2013-06-10 06:29:20

by Yinghai Lu

[permalink] [raw]
Subject: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge
during add range) broke mtrr cleanup on his setup in 3.9.5.
corresponding commit in upstream is fbe06b7bae7c.

The reason is add_range_with_merge could generate blank spot.

We could avoid that by searching new expanded start/end, that
new range should include all connected ranges in range array.
At last add the new expanded start/end to the range array.
Also move up left array so do not add new blank slot in the
range array.

-v2: move left array to avoid enhance add_range()

Reported-by: Joshua Covington <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
Cc: <[email protected]> v3.9

---
arch/x86/kernel/cpu/mtrr/cleanup.c | 2 +-
kernel/range.c | 21 +++++++++++----------
2 files changed, 12 insertions(+), 11 deletions(-)

Index: linux-2.6/kernel/range.c
===================================================================
--- linux-2.6.orig/kernel/range.c
+++ linux-2.6/kernel/range.c
@@ -32,9 +32,8 @@ int add_range_with_merge(struct range *r
if (start >= end)
return nr_range;

- /* Try to merge it with old one: */
+ /* get new start/end: */
for (i = 0; i < nr_range; i++) {
- u64 final_start, final_end;
u64 common_start, common_end;

if (!range[i].end)
@@ -45,14 +44,16 @@ int add_range_with_merge(struct range *r
if (common_start > common_end)
continue;

- final_start = min(range[i].start, start);
- final_end = max(range[i].end, end);
-
- /* clear it and add it back for further merge */
- range[i].start = 0;
- range[i].end = 0;
- return add_range_with_merge(range, az, nr_range,
- final_start, final_end);
+ /* new start/end, will add it back at last */
+ start = min(range[i].start, start);
+ end = max(range[i].end, end);
+
+ memmove(&range[i], &range[i + 1],
+ (nr_range - (i + 1)) * sizeof(range[i]));
+ range[nr_range - 1].start = 0;
+ range[nr_range - 1].end = 0;
+ nr_range--;
+ i--;
}

/* Need to add it: */


2013-06-10 10:56:05

by Sergey S. Kostyliov

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

Hi,

On 10 June 2013 09:29, Yinghai Lu <[email protected]> wrote:
> Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge
> during add range) broke mtrr cleanup on his setup in 3.9.5.
> corresponding commit in upstream is fbe06b7bae7c.
>
> The reason is add_range_with_merge could generate blank spot.
>
> We could avoid that by searching new expanded start/end, that
> new range should include all connected ranges in range array.
> At last add the new expanded start/end to the range array.
> Also move up left array so do not add new blank slot in the
> range array.
>
> -v2: move left array to avoid enhance add_range()
>
> Reported-by: Joshua Covington <[email protected]>
> Signed-off-by: Yinghai Lu <[email protected]>
> Cc: <[email protected]> v3.9
>
> ---
> arch/x86/kernel/cpu/mtrr/cleanup.c | 2 +-
> kernel/range.c | 21 +++++++++++----------
> 2 files changed, 12 insertions(+), 11 deletions(-)
>
> Index: linux-2.6/kernel/range.c
> ===================================================================
> --- linux-2.6.orig/kernel/range.c
> +++ linux-2.6/kernel/range.c
> @@ -32,9 +32,8 @@ int add_range_with_merge(struct range *r
> if (start >= end)
> return nr_range;
>
> - /* Try to merge it with old one: */
> + /* get new start/end: */
> for (i = 0; i < nr_range; i++) {
> - u64 final_start, final_end;
> u64 common_start, common_end;
>
> if (!range[i].end)
> @@ -45,14 +44,16 @@ int add_range_with_merge(struct range *r
> if (common_start > common_end)
> continue;
>
> - final_start = min(range[i].start, start);
> - final_end = max(range[i].end, end);
> -
> - /* clear it and add it back for further merge */
> - range[i].start = 0;
> - range[i].end = 0;
> - return add_range_with_merge(range, az, nr_range,
> - final_start, final_end);
> + /* new start/end, will add it back at last */
> + start = min(range[i].start, start);
> + end = max(range[i].end, end);
> +
> + memmove(&range[i], &range[i + 1],
> + (nr_range - (i + 1)) * sizeof(range[i]));
> + range[nr_range - 1].start = 0;
> + range[nr_range - 1].end = 0;
> + nr_range--;
> + i--;
> }
>
> /* Need to add it: */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

patches 1,2(latest one) have not helped me on 3.9.5:

rathamahata@piledriver /usr/local/src/linux-3.9.5 $ cat /proc/cmdline
BOOT_IMAGE=/boot/linux-3.9.5 root=/dev/sda1 rootflags=commit=9
elevator=cfq enable_mtrr_cleanup mtrr_spare_reg_nr=1
rathamahata@piledriver /usr/local/src/linux-3.9.5 $ cat /proc/mtrr
reg00: base=0x000000000 ( 0MB), size= 2048MB, count=1: write-back
reg01: base=0x080000000 ( 2048MB), size= 256MB, count=1: write-back
reg02: base=0x08f800000 ( 2296MB), size= 8MB, count=1: uncachable
reg03: base=0x0b0000000 ( 2816MB), size= 256MB, count=1: write-combining
reg04: base=0x0c0000000 ( 3072MB), size= 256MB, count=1: write-combining
rathamahata@piledriver /usr/local/src/linux-3.9.5 $ free -m
total used free shared buffers cached
Mem: 7370 714 6656 0 35 243
-/+ buffers/cache: 434 6935
Swap: 3659 0 3659
rathamahata@piledriver /usr/local/src/linux-3.9.5 $

2013-06-10 17:38:19

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

On Mon, Jun 10, 2013 at 3:55 AM, Sergey Meirovich <[email protected]> wrote:

> patches 1,2(latest one) have not helped me on 3.9.5:

So v1:
https://patchwork.kernel.org/patch/2694981/
https://patchwork.kernel.org/patch/2694971/

and v2:
https://patchwork.kernel.org/patch/2695891/
https://patchwork.kernel.org/patch/2695881/

Neither of two versions fix the problem on your setup?

Can you post boot log with mtrr_cleanup_debug on 3.9.4?

Thanks

Yinghai

2013-06-11 09:21:11

by Joshua C.

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

2013/6/10 Yinghai Lu <[email protected]>:
> On Mon, Jun 10, 2013 at 3:55 AM, Sergey Meirovich <[email protected]> wrote:
>
>> patches 1,2(latest one) have not helped me on 3.9.5:
>
> So v1:
> https://patchwork.kernel.org/patch/2694981/
> https://patchwork.kernel.org/patch/2694971/
>
> and v2:
> https://patchwork.kernel.org/patch/2695891/
> https://patchwork.kernel.org/patch/2695881/
>
> Neither of two versions fix the problem on your setup?
>
> Can you post boot log with mtrr_cleanup_debug on 3.9.4?
>
> Thanks
>
> Yinghai

I got some warnings when trying to build v2 of the patches:

# make -s ARCH=x86_64 V=1 -j4 bzImage
kernel/range.c: In function 'add_range_with_merge':
kernel/range.c:51:3: error: implicit declaration of function 'memmove'
[-Werror=implicit-function-declaration]
kernel/range.c:51:3: warning: incompatible implicit declaration of
built-in function 'memmove' [enabled by default]
cc1: some warnings being treated as errors
scripts/Makefile.build:307: recipe for target 'kernel/range.o' failed
make[1]: *** [kernel/range.o] Error 1
Makefile:793: recipe for target 'kernel' failed
make: *** [kernel] Error 2
make: *** Waiting for unfinished jobs....


--
--joshua

2013-06-11 10:04:29

by Joshua C.

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

2013/6/11 Joshua C. <[email protected]>:
> 2013/6/10 Yinghai Lu <[email protected]>:
>> On Mon, Jun 10, 2013 at 3:55 AM, Sergey Meirovich <[email protected]> wrote:
>>
>>> patches 1,2(latest one) have not helped me on 3.9.5:
>>
>> So v1:
>> https://patchwork.kernel.org/patch/2694981/
>> https://patchwork.kernel.org/patch/2694971/
>>
>> and v2:
>> https://patchwork.kernel.org/patch/2695891/
>> https://patchwork.kernel.org/patch/2695881/
>>
>> Neither of two versions fix the problem on your setup?
>>
>> Can you post boot log with mtrr_cleanup_debug on 3.9.4?
>>
>> Thanks
>>
>> Yinghai
>
> I got some warnings when trying to build v2 of the patches:
>
> # make -s ARCH=x86_64 V=1 -j4 bzImage
> kernel/range.c: In function 'add_range_with_merge':
> kernel/range.c:51:3: error: implicit declaration of function 'memmove'
> [-Werror=implicit-function-declaration]
> kernel/range.c:51:3: warning: incompatible implicit declaration of
> built-in function 'memmove' [enabled by default]
> cc1: some warnings being treated as errors
> scripts/Makefile.build:307: recipe for target 'kernel/range.o' failed
> make[1]: *** [kernel/range.o] Error 1
> Makefile:793: recipe for target 'kernel' failed
> make: *** [kernel] Error 2
> make: *** Waiting for unfinished jobs....
>
>
> --
> --joshua

A trivial fix for the above warnings:

--- linux-2.6.orig/kernel/range.c
+++ linux-2.6/kernel/range.c
@@ -4,6 +4,7 @@
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/sort.h>
+#include <linux/string.h>

#include <linux/range.h>



--
--joshua

2013-06-11 16:34:17

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

On Tue, Jun 11, 2013 at 3:04 AM, Joshua C. <[email protected]> wrote:
> 2013/6/11 Joshua C. <[email protected]>:
>> 2013/6/10 Yinghai Lu <[email protected]>:
>>> On Mon, Jun 10, 2013 at 3:55 AM, Sergey Meirovich <[email protected]> wrote:
>>>
>>>> patches 1,2(latest one) have not helped me on 3.9.5:
>>>
>>> So v1:
>>> https://patchwork.kernel.org/patch/2694981/
>>> https://patchwork.kernel.org/patch/2694971/
>>>
>>> and v2:
>>> https://patchwork.kernel.org/patch/2695891/
>>> https://patchwork.kernel.org/patch/2695881/
>>>
>>> Neither of two versions fix the problem on your setup?
>>>
>>> Can you post boot log with mtrr_cleanup_debug on 3.9.4?
>>>
>>> Thanks
>>>
>>> Yinghai
>>
>> I got some warnings when trying to build v2 of the patches:
>>
>> # make -s ARCH=x86_64 V=1 -j4 bzImage
>> kernel/range.c: In function 'add_range_with_merge':
>> kernel/range.c:51:3: error: implicit declaration of function 'memmove'
>> [-Werror=implicit-function-declaration]
>> kernel/range.c:51:3: warning: incompatible implicit declaration of
>> built-in function 'memmove' [enabled by default]
>> cc1: some warnings being treated as errors
>> scripts/Makefile.build:307: recipe for target 'kernel/range.o' failed
>> make[1]: *** [kernel/range.o] Error 1
>> Makefile:793: recipe for target 'kernel' failed
>> make: *** [kernel] Error 2
>> make: *** Waiting for unfinished jobs....
>>
>>
>> --
>> --joshua

it does not report warning when I compile it with

gcc (SUSE Linux) 4.7.2 20130108 [gcc-4_7-branch revision 195012

>
> A trivial fix for the above warnings:
>
> --- linux-2.6.orig/kernel/range.c
> +++ linux-2.6/kernel/range.c
> @@ -4,6 +4,7 @@
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/sort.h>
> +#include <linux/string.h>
>
> #include <linux/range.h>
>

2013-06-11 17:08:30

by Joshua C.

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

2013/6/11 Yinghai Lu <[email protected]>:
> On Tue, Jun 11, 2013 at 3:04 AM, Joshua C. <[email protected]> wrote:
>> 2013/6/11 Joshua C. <[email protected]>:
>>> 2013/6/10 Yinghai Lu <[email protected]>:
>>>> On Mon, Jun 10, 2013 at 3:55 AM, Sergey Meirovich <[email protected]> wrote:
>>>>
>>>>> patches 1,2(latest one) have not helped me on 3.9.5:
>>>>
>>>> So v1:
>>>> https://patchwork.kernel.org/patch/2694981/
>>>> https://patchwork.kernel.org/patch/2694971/
>>>>
>>>> and v2:
>>>> https://patchwork.kernel.org/patch/2695891/
>>>> https://patchwork.kernel.org/patch/2695881/
>>>>
>>>> Neither of two versions fix the problem on your setup?
>>>>
>>>> Can you post boot log with mtrr_cleanup_debug on 3.9.4?
>>>>
>>>> Thanks
>>>>
>>>> Yinghai
>>>
>>> I got some warnings when trying to build v2 of the patches:
>>>
>>> # make -s ARCH=x86_64 V=1 -j4 bzImage
>>> kernel/range.c: In function 'add_range_with_merge':
>>> kernel/range.c:51:3: error: implicit declaration of function 'memmove'
>>> [-Werror=implicit-function-declaration]
>>> kernel/range.c:51:3: warning: incompatible implicit declaration of
>>> built-in function 'memmove' [enabled by default]
>>> cc1: some warnings being treated as errors
>>> scripts/Makefile.build:307: recipe for target 'kernel/range.o' failed
>>> make[1]: *** [kernel/range.o] Error 1
>>> Makefile:793: recipe for target 'kernel' failed
>>> make: *** [kernel] Error 2
>>> make: *** Waiting for unfinished jobs....
>>>
>>>
>>> --
>>> --joshua
>
> it does not report warning when I compile it with
>
> gcc (SUSE Linux) 4.7.2 20130108 [gcc-4_7-branch revision 195012
>
>>
>> A trivial fix for the above warnings:
>>
>> --- linux-2.6.orig/kernel/range.c
>> +++ linux-2.6/kernel/range.c
>> @@ -4,6 +4,7 @@
>> #include <linux/kernel.h>
>> #include <linux/init.h>
>> #include <linux/sort.h>
>> +#include <linux/string.h>
>>
>> #include <linux/range.h>
>>

I use fedora17 with gcc version 4.7.3 (git checkout on 20130411). I
have no idea where this came from...


--
--joshua

2013-06-11 18:30:05

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

On Tue, Jun 11, 2013 at 10:08 AM, Joshua C. <[email protected]> wrote:
> 2013/6/11 Yinghai Lu <[email protected]>:
>> On Tue, Jun 11, 2013 at 3:04 AM, Joshua C. <[email protected]> wrote:
>>> 2013/6/11 Joshua C. <[email protected]>:
>>>> 2013/6/10 Yinghai Lu <[email protected]>:
>>>>> On Mon, Jun 10, 2013 at 3:55 AM, Sergey Meirovich <[email protected]> wrote:
>>>>>
>>>>>> patches 1,2(latest one) have not helped me on 3.9.5:
>>>>>
>>>>> So v1:
>>>>> https://patchwork.kernel.org/patch/2694981/
>>>>> https://patchwork.kernel.org/patch/2694971/
>>>>>
>>>>> and v2:
>>>>> https://patchwork.kernel.org/patch/2695891/
>>>>> https://patchwork.kernel.org/patch/2695881/
>>>>>
>>>>> Neither of two versions fix the problem on your setup?
>>>>>
>>>>> Can you post boot log with mtrr_cleanup_debug on 3.9.4?
>>>>>
>>>>> Thanks
>>>>>
>>>>> Yinghai
>>>>
>>>> I got some warnings when trying to build v2 of the patches:
>>>>
>>>> # make -s ARCH=x86_64 V=1 -j4 bzImage
>>>> kernel/range.c: In function 'add_range_with_merge':
>>>> kernel/range.c:51:3: error: implicit declaration of function 'memmove'
>>>> [-Werror=implicit-function-declaration]
>>>> kernel/range.c:51:3: warning: incompatible implicit declaration of
>>>> built-in function 'memmove' [enabled by default]
>>>> cc1: some warnings being treated as errors
>>>> scripts/Makefile.build:307: recipe for target 'kernel/range.o' failed
>>>> make[1]: *** [kernel/range.o] Error 1
>>>> Makefile:793: recipe for target 'kernel' failed
>>>> make: *** [kernel] Error 2
>>>> make: *** Waiting for unfinished jobs....
>>>>
>>>>
>>>> --
>>>> --joshua
>>
>> it does not report warning when I compile it with
>>
>> gcc (SUSE Linux) 4.7.2 20130108 [gcc-4_7-branch revision 195012
>>
>>>
>>> A trivial fix for the above warnings:
>>>
>>> --- linux-2.6.orig/kernel/range.c
>>> +++ linux-2.6/kernel/range.c
>>> @@ -4,6 +4,7 @@
>>> #include <linux/kernel.h>
>>> #include <linux/init.h>
>>> #include <linux/sort.h>
>>> +#include <linux/string.h>
>>>
>>> #include <linux/range.h>
>>>
>
> I use fedora17 with gcc version 4.7.3 (git checkout on 20130411). I
> have no idea where this came from...

Ok. Your config should have CONFIG_DYNAMIC_DEBUG defined,
and my config does not have that defined.

we have linux/kernel.h
linux/printk.h
linux/dynamic_debug.h

dynamic_debug.h will include linux/string.h when CONFIG_DYNAMIC_DEBUG
is not defined.

Hi, Andrew,

How to fix this inconsistency caused by dynamic_debug.h

1. let linux/kernel.h include string.h directly
2. or let dynamic_debug include string.h always
3. let printk.h include string.h.
...

Yinghai

2013-06-12 21:12:41

by Joshua C.

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] x86, range: Do not add new blank slot with add_range_with_merge

2013/6/11 Yinghai Lu <[email protected]>:
> On Tue, Jun 11, 2013 at 10:08 AM, Joshua C. <[email protected]> wrote:
>> 2013/6/11 Yinghai Lu <[email protected]>:
>>> On Tue, Jun 11, 2013 at 3:04 AM, Joshua C. <[email protected]> wrote:
>>>> 2013/6/11 Joshua C. <[email protected]>:
>>>>> 2013/6/10 Yinghai Lu <[email protected]>:
>>>>>> On Mon, Jun 10, 2013 at 3:55 AM, Sergey Meirovich <[email protected]> wrote:
>>>>>>
>>>>>>> patches 1,2(latest one) have not helped me on 3.9.5:
>>>>>>
>>>>>> So v1:
>>>>>> https://patchwork.kernel.org/patch/2694981/
>>>>>> https://patchwork.kernel.org/patch/2694971/
>>>>>>
>>>>>> and v2:
>>>>>> https://patchwork.kernel.org/patch/2695891/
>>>>>> https://patchwork.kernel.org/patch/2695881/
>>>>>>
>>>>>> Neither of two versions fix the problem on your setup?
>>>>>>
>>>>>> Can you post boot log with mtrr_cleanup_debug on 3.9.4?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Yinghai
>>>>>
>>>>> I got some warnings when trying to build v2 of the patches:
>>>>>
>>>>> # make -s ARCH=x86_64 V=1 -j4 bzImage
>>>>> kernel/range.c: In function 'add_range_with_merge':
>>>>> kernel/range.c:51:3: error: implicit declaration of function 'memmove'
>>>>> [-Werror=implicit-function-declaration]
>>>>> kernel/range.c:51:3: warning: incompatible implicit declaration of
>>>>> built-in function 'memmove' [enabled by default]
>>>>> cc1: some warnings being treated as errors
>>>>> scripts/Makefile.build:307: recipe for target 'kernel/range.o' failed
>>>>> make[1]: *** [kernel/range.o] Error 1
>>>>> Makefile:793: recipe for target 'kernel' failed
>>>>> make: *** [kernel] Error 2
>>>>> make: *** Waiting for unfinished jobs....
>>>>>
>>>>>
>>>>> --
>>>>> --joshua
>>>
>>> it does not report warning when I compile it with
>>>
>>> gcc (SUSE Linux) 4.7.2 20130108 [gcc-4_7-branch revision 195012
>>>
>>>>
>>>> A trivial fix for the above warnings:
>>>>
>>>> --- linux-2.6.orig/kernel/range.c
>>>> +++ linux-2.6/kernel/range.c
>>>> @@ -4,6 +4,7 @@
>>>> #include <linux/kernel.h>
>>>> #include <linux/init.h>
>>>> #include <linux/sort.h>
>>>> +#include <linux/string.h>
>>>>
>>>> #include <linux/range.h>
>>>>
>>
>> I use fedora17 with gcc version 4.7.3 (git checkout on 20130411). I
>> have no idea where this came from...
>
> Ok. Your config should have CONFIG_DYNAMIC_DEBUG defined,
> and my config does not have that defined.
>
> we have linux/kernel.h
> linux/printk.h
> linux/dynamic_debug.h
>
> dynamic_debug.h will include linux/string.h when CONFIG_DYNAMIC_DEBUG
> is not defined.
>
> Hi, Andrew,
>
> How to fix this inconsistency caused by dynamic_debug.h
>
> 1. let linux/kernel.h include string.h directly
> 2. or let dynamic_debug include string.h always
> 3. let printk.h include string.h.
> ...
>
> Yinghai

You guys are better at kernel coding than I am, but can't we just
directly add 'string.h' just like the small patch I posted? In the
worst case the file will be included twice... Maybe this is not a good
coding style but it solves the problem and doesn't cause any side
effects. So why not do it the way I did?

--
--joshua