Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290849 > unrolled thread

[PATCH 1/2] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn Joonsoo Kim <js1304@gmail.com> - 2015-12-14 06:10 +0100
    Re: [PATCH 1/2] mm/compaction: fix invalid free_pfn and  compact_cached_free_pfn Vlastimil Babka <vbabka@suse.cz> - 2015-12-14 11:10 +0100
      Re: [PATCH 1/2] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn Joonsoo Kim <js1304@gmail.com> - 2015-12-14 16:30 +0100
        Re: [PATCH 1/2] mm/compaction: fix invalid free_pfn and  compact_cached_free_pfn Vlastimil Babka <vbabka@suse.cz> - 2015-12-15 09:40 +0100
          Re: [PATCH 1/2] mm/compaction: fix invalid free_pfn and  compact_cached_free_pfn Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2015-12-16 06:50 +0100

#1290849 — [PATCH 1/2] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn

free_pfn and compact_cached_free_pfn are the pointer that remember
restart position of freepage scanner. When they are reset or invalid,
we set them to zone_end_pfn because freepage scanner works in reverse
direction. But, because zone range is defined as [zone_start_pfn,
zone_end_pfn), zone_end_pfn is invalid to access. Therefore, we should
not store it to free_pfn and compact_cached_free_pfn. Instead, we need
to store zone_end_pfn - 1 to them. There is one more thing we should
consider. Freepage scanner scan reversely by pageblock unit. If free_pfn
and compact_cached_free_pfn are set to middle of pageblock, it regards
that sitiation as that it already scans front part of pageblock so we
lose opportunity to scan there. To fix-up, this patch do round_down()
to guarantee that reset position will be pageblock aligned.

Note that thanks to the current pageblock_pfn_to_page() implementation,
actual access to zone_end_pfn doesn't happen until now. But, following
patch will change pageblock_pfn_to_page() so this patch is needed
from now on.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 mm/compaction.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 585de54..56fa321 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -200,7 +200,8 @@ static void reset_cached_positions(struct zone *zone)
 {
 	zone->compact_cached_migrate_pfn[0] = zone->zone_start_pfn;
 	zone->compact_cached_migrate_pfn[1] = zone->zone_start_pfn;
-	zone->compact_cached_free_pfn = zone_end_pfn(zone);
+	zone->compact_cached_free_pfn =
+			round_down(zone_end_pfn(zone) - 1, pageblock_nr_pages);
 }
 
 /*
@@ -1371,11 +1372,11 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 	 */
 	cc->migrate_pfn = zone->compact_cached_migrate_pfn[sync];
 	cc->free_pfn = zone->compact_cached_free_pfn;
-	if (cc->free_pfn < start_pfn || cc->free_pfn > end_pfn) {
-		cc->free_pfn = end_pfn & ~(pageblock_nr_pages-1);
+	if (cc->free_pfn < start_pfn || cc->free_pfn >= end_pfn) {
+		cc->free_pfn = round_down(end_pfn - 1, pageblock_nr_pages);
 		zone->compact_cached_free_pfn = cc->free_pfn;
 	}
-	if (cc->migrate_pfn < start_pfn || cc->migrate_pfn > end_pfn) {
+	if (cc->migrate_pfn < start_pfn || cc->migrate_pfn >= end_pfn) {
 		cc->migrate_pfn = start_pfn;
 		zone->compact_cached_migrate_pfn[0] = cc->migrate_pfn;
 		zone->compact_cached_migrate_pfn[1] = cc->migrate_pfn;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1291029 — Re: [PATCH 1/2] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn

On 12/14/2015 06:02 AM, Joonsoo Kim wrote:
> free_pfn and compact_cached_free_pfn are the pointer that remember
> restart position of freepage scanner. When they are reset or invalid,
> we set them to zone_end_pfn because freepage scanner works in reverse
> direction. But, because zone range is defined as [zone_start_pfn,
> zone_end_pfn), zone_end_pfn is invalid to access. Therefore, we should
> not store it to free_pfn and compact_cached_free_pfn. Instead, we need
> to store zone_end_pfn - 1 to them. There is one more thing we should
> consider. Freepage scanner scan reversely by pageblock unit. If free_pfn
> and compact_cached_free_pfn are set to middle of pageblock, it regards
> that sitiation as that it already scans front part of pageblock so we
> lose opportunity to scan there. To fix-up, this patch do round_down()
> to guarantee that reset position will be pageblock aligned.
>
> Note that thanks to the current pageblock_pfn_to_page() implementation,
> actual access to zone_end_pfn doesn't happen until now. But, following
> patch will change pageblock_pfn_to_page() so this patch is needed
> from now on.
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Note that until now in compaction we've used basically an open-coded 
round_down(), and ALIGN() for rounding up. You introduce a first use of 
round_down(), and it would be nice to standardize on round_down() and 
round_up() everywhere. I think it's more obvious than open-coding and 
ALIGN() (which doesn't tell the reader if it's aligning up or down). 
Hopefully they really do the same thing and there are no caveats...

> ---
>   mm/compaction.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 585de54..56fa321 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -200,7 +200,8 @@ static void reset_cached_positions(struct zone *zone)
>   {
>   	zone->compact_cached_migrate_pfn[0] = zone->zone_start_pfn;
>   	zone->compact_cached_migrate_pfn[1] = zone->zone_start_pfn;
> -	zone->compact_cached_free_pfn = zone_end_pfn(zone);
> +	zone->compact_cached_free_pfn =
> +			round_down(zone_end_pfn(zone) - 1, pageblock_nr_pages);
>   }
>
>   /*
> @@ -1371,11 +1372,11 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>   	 */
>   	cc->migrate_pfn = zone->compact_cached_migrate_pfn[sync];
>   	cc->free_pfn = zone->compact_cached_free_pfn;
> -	if (cc->free_pfn < start_pfn || cc->free_pfn > end_pfn) {
> -		cc->free_pfn = end_pfn & ~(pageblock_nr_pages-1);
> +	if (cc->free_pfn < start_pfn || cc->free_pfn >= end_pfn) {
> +		cc->free_pfn = round_down(end_pfn - 1, pageblock_nr_pages);
>   		zone->compact_cached_free_pfn = cc->free_pfn;
>   	}
> -	if (cc->migrate_pfn < start_pfn || cc->migrate_pfn > end_pfn) {
> +	if (cc->migrate_pfn < start_pfn || cc->migrate_pfn >= end_pfn) {
>   		cc->migrate_pfn = start_pfn;
>   		zone->compact_cached_migrate_pfn[0] = cc->migrate_pfn;
>   		zone->compact_cached_migrate_pfn[1] = cc->migrate_pfn;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291281

2015-12-14 19:07 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> On 12/14/2015 06:02 AM, Joonsoo Kim wrote:
>>
>> free_pfn and compact_cached_free_pfn are the pointer that remember
>> restart position of freepage scanner. When they are reset or invalid,
>> we set them to zone_end_pfn because freepage scanner works in reverse
>> direction. But, because zone range is defined as [zone_start_pfn,
>> zone_end_pfn), zone_end_pfn is invalid to access. Therefore, we should
>> not store it to free_pfn and compact_cached_free_pfn. Instead, we need
>> to store zone_end_pfn - 1 to them. There is one more thing we should
>> consider. Freepage scanner scan reversely by pageblock unit. If free_pfn
>> and compact_cached_free_pfn are set to middle of pageblock, it regards
>> that sitiation as that it already scans front part of pageblock so we
>> lose opportunity to scan there. To fix-up, this patch do round_down()
>> to guarantee that reset position will be pageblock aligned.
>>
>> Note that thanks to the current pageblock_pfn_to_page() implementation,
>> actual access to zone_end_pfn doesn't happen until now. But, following
>> patch will change pageblock_pfn_to_page() so this patch is needed
>> from now on.
>>
>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>
> Note that until now in compaction we've used basically an open-coded
> round_down(), and ALIGN() for rounding up. You introduce a first use of
> round_down(), and it would be nice to standardize on round_down() and
> round_up() everywhere. I think it's more obvious than open-coding and
> ALIGN() (which doesn't tell the reader if it's aligning up or down).
> Hopefully they really do the same thing and there are no caveats...

Okay. Will send another patch for this clean-up on next spin.

Thanks.

>
>> ---
>>   mm/compaction.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 585de54..56fa321 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -200,7 +200,8 @@ static void reset_cached_positions(struct zone *zone)
>>   {
>>         zone->compact_cached_migrate_pfn[0] = zone->zone_start_pfn;
>>         zone->compact_cached_migrate_pfn[1] = zone->zone_start_pfn;
>> -       zone->compact_cached_free_pfn = zone_end_pfn(zone);
>> +       zone->compact_cached_free_pfn =
>> +                       round_down(zone_end_pfn(zone) - 1,
>> pageblock_nr_pages);
>>   }
>>
>>   /*
>> @@ -1371,11 +1372,11 @@ static int compact_zone(struct zone *zone, struct
>> compact_control *cc)
>>          */
>>         cc->migrate_pfn = zone->compact_cached_migrate_pfn[sync];
>>         cc->free_pfn = zone->compact_cached_free_pfn;
>> -       if (cc->free_pfn < start_pfn || cc->free_pfn > end_pfn) {
>> -               cc->free_pfn = end_pfn & ~(pageblock_nr_pages-1);
>> +       if (cc->free_pfn < start_pfn || cc->free_pfn >= end_pfn) {
>> +               cc->free_pfn = round_down(end_pfn - 1,
>> pageblock_nr_pages);
>>                 zone->compact_cached_free_pfn = cc->free_pfn;
>>         }
>> -       if (cc->migrate_pfn < start_pfn || cc->migrate_pfn > end_pfn) {
>> +       if (cc->migrate_pfn < start_pfn || cc->migrate_pfn >= end_pfn) {
>>                 cc->migrate_pfn = start_pfn;
>>                 zone->compact_cached_migrate_pfn[0] = cc->migrate_pfn;
>>                 zone->compact_cached_migrate_pfn[1] = cc->migrate_pfn;
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291932 — Re: [PATCH 1/2] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn

On 12/14/2015 04:26 PM, Joonsoo Kim wrote:
> 2015-12-14 19:07 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
>> On 12/14/2015 06:02 AM, Joonsoo Kim wrote:
>>>
>>
>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>>
>> Note that until now in compaction we've used basically an open-coded
>> round_down(), and ALIGN() for rounding up. You introduce a first use of
>> round_down(), and it would be nice to standardize on round_down() and
>> round_up() everywhere. I think it's more obvious than open-coding and
>> ALIGN() (which doesn't tell the reader if it's aligning up or down).
>> Hopefully they really do the same thing and there are no caveats...
>
> Okay. Will send another patch for this clean-up on next spin.

Great, I didn't mean that the cleanup is needed right now, but whether 
we agree on an idiom to use whenever doing any changes from now on.
Maybe it would be best to add some defines in the top of compaction.c 
that would also hide away the repeated pageblock_nr_pages everywhere? 
Something like:

#define pageblock_start(pfn) round_down(pfn, pageblock_nr_pages)
#define pageblock_end(pfn) round_up((pfn)+1, pageblock_nr_pages)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292777 — Re: [PATCH 1/2] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn

On Tue, Dec 15, 2015 at 09:31:39AM +0100, Vlastimil Babka wrote:
> On 12/14/2015 04:26 PM, Joonsoo Kim wrote:
> >2015-12-14 19:07 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> >>On 12/14/2015 06:02 AM, Joonsoo Kim wrote:
> >>>
> >>
> >>Acked-by: Vlastimil Babka <vbabka@suse.cz>
> >>
> >>Note that until now in compaction we've used basically an open-coded
> >>round_down(), and ALIGN() for rounding up. You introduce a first use of
> >>round_down(), and it would be nice to standardize on round_down() and
> >>round_up() everywhere. I think it's more obvious than open-coding and
> >>ALIGN() (which doesn't tell the reader if it's aligning up or down).
> >>Hopefully they really do the same thing and there are no caveats...
> >
> >Okay. Will send another patch for this clean-up on next spin.
> 
> Great, I didn't mean that the cleanup is needed right now, but
> whether we agree on an idiom to use whenever doing any changes from
> now on.

Okay.

> Maybe it would be best to add some defines in the top of
> compaction.c that would also hide away the repeated
> pageblock_nr_pages everywhere? Something like:
> 
> #define pageblock_start(pfn) round_down(pfn, pageblock_nr_pages)
> #define pageblock_end(pfn) round_up((pfn)+1, pageblock_nr_pages)

Quick grep shows that there are much more places this new define or
some variant can be used. It would be good clean-up. I will try it
separately.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web