[PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list

[PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list

[Maninder Singh]

'commit 3c710c1ad11b ("mm, vmscan:
extract shrink_page_list reclaim counters into a struct")'

changed data type for the function,
so changing return type for funciton and its caller.

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 mm/internal.h   |  2 +-
 mm/page_alloc.c |  2 +-
 mm/vmscan.c     | 24 ++++++++++++------------
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index b5634e7..c3eeec8 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -527,7 +527,7 @@ extern unsigned long  __must_check vm_mmap_pgoff(struct file *, unsigned long,
         unsigned long, unsigned long);
 
 extern void set_pageblock_order(void);
-unsigned long reclaim_clean_pages_from_list(struct zone *zone,
+unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 					    struct list_head *page_list);
 /* The ALLOC_WMARK bits are used as an index to zone->watermark */
 #define ALLOC_WMARK_MIN		WMARK_MIN
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1385d78..f17d88c6 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8416,7 +8416,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
 					unsigned long start, unsigned long end)
 {
 	/* This function is based on compact_zone() from compaction.c. */
-	unsigned long nr_reclaimed;
+	unsigned int nr_reclaimed;
 	unsigned long pfn = start;
 	unsigned int tries = 0;
 	int ret = 0;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index b06868f..e561ba5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1066,17 +1066,17 @@ static void page_check_dirty_writeback(struct page *page,
 /*
  * shrink_page_list() returns the number of reclaimed pages
  */
-static unsigned long shrink_page_list(struct list_head *page_list,
-				      struct pglist_data *pgdat,
-				      struct scan_control *sc,
-				      enum ttu_flags ttu_flags,
-				      struct reclaim_stat *stat,
-				      bool ignore_references)
+static unsigned int shrink_page_list(struct list_head *page_list,
+				     struct pglist_data *pgdat,
+				     struct scan_control *sc,
+				     enum ttu_flags ttu_flags,
+				     struct reclaim_stat *stat,
+				     bool ignore_references)
 {
 	LIST_HEAD(ret_pages);
 	LIST_HEAD(free_pages);
-	unsigned nr_reclaimed = 0;
-	unsigned pgactivate = 0;
+	unsigned int nr_reclaimed = 0;
+	unsigned int pgactivate = 0;
 
 	memset(stat, 0, sizeof(*stat));
 	cond_resched();
@@ -1483,16 +1483,16 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 	return nr_reclaimed;
 }
 
-unsigned long reclaim_clean_pages_from_list(struct zone *zone,
+unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 					    struct list_head *page_list)
 {
+	unsigned int ret;
 	struct scan_control sc = {
 		.gfp_mask = GFP_KERNEL,
 		.priority = DEF_PRIORITY,
 		.may_unmap = 1,
 	};
 	struct reclaim_stat dummy_stat;
-	unsigned long ret;
 	struct page *page, *next;
 	LIST_HEAD(clean_pages);
 
@@ -1900,13 +1900,13 @@ static int current_may_throttle(void)
 {
 	LIST_HEAD(page_list);
 	unsigned long nr_scanned;
-	unsigned long nr_reclaimed = 0;
 	unsigned long nr_taken;
 	struct reclaim_stat stat;
 	int file = is_file_lru(lru);
 	enum vm_event_item item;
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
 	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
+	unsigned int nr_reclaimed = 0;
 	bool stalled = false;
 
 	while (unlikely(too_many_isolated(pgdat, file, sc))) {
@@ -2096,7 +2096,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
 unsigned long reclaim_pages(struct list_head *page_list)
 {
 	int nid = NUMA_NO_NODE;
-	unsigned long nr_reclaimed = 0;
+	unsigned int nr_reclaimed = 0;
 	LIST_HEAD(node_page_list);
 	struct reclaim_stat dummy_stat;
 	struct page *page;
-- 
1.9.1

Re: [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list

[Michal Hocko]

On Wed 29-04-20 11:29:27, Maninder Singh wrote:
> 'commit 3c710c1ad11b ("mm, vmscan:
> extract shrink_page_list reclaim counters into a struct")'
> 
> changed data type for the function,
> so changing return type for funciton and its caller.
> 
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Is there any reason to move declarations here?

> -unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> +unsigned int reclaim_clean_pages_from_list(struct zone *zone,
>  					    struct list_head *page_list)
>  {
> +	unsigned int ret;
>  	struct scan_control sc = {
>  		.gfp_mask = GFP_KERNEL,
>  		.priority = DEF_PRIORITY,
>  		.may_unmap = 1,
>  	};
>  	struct reclaim_stat dummy_stat;
> -	unsigned long ret;
>  	struct page *page, *next;
>  	LIST_HEAD(clean_pages);
>  
> @@ -1900,13 +1900,13 @@ static int current_may_throttle(void)
>  {
>  	LIST_HEAD(page_list);
>  	unsigned long nr_scanned;
> -	unsigned long nr_reclaimed = 0;
>  	unsigned long nr_taken;
>  	struct reclaim_stat stat;
>  	int file = is_file_lru(lru);
>  	enum vm_event_item item;
>  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>  	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
> +	unsigned int nr_reclaimed = 0;
>  	bool stalled = false;
>  
>  	while (unlikely(too_many_isolated(pgdat, file, sc))) {
> @@ -2096,7 +2096,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
>  unsigned long reclaim_pages(struct list_head *page_list)
>  {
>  	int nid = NUMA_NO_NODE;
> -	unsigned long nr_reclaimed = 0;
> +	unsigned int nr_reclaimed = 0;
>  	LIST_HEAD(node_page_list);
>  	struct reclaim_stat dummy_stat;
>  	struct page *page;
> -- 
> 1.9.1

-- 
Michal Hocko
SUSE Labs

RE:[PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list

[Maninder Singh]

Hi,

>
>Acked-by: Michal Hocko <mhocko@suse.com>
>
>Is there any reason to move declarations here?
>

"unsigned int ret" was changed mistakenely, sending V2.
and "unsigned int nr_reclaimed" is changed to remove hole.
 
>> -unsigned long reclaim_clean_pages_from_list(struct zone *zone,
>> +unsigned int reclaim_clean_pages_from_list(struct zone *zone,
>>  					    struct list_head *page_list)
>>  {
>> +	unsigned int ret;
>>  	struct scan_control sc = {
>>  		.gfp_mask = GFP_KERNEL,
>>  		.priority = DEF_PRIORITY,
>>  		.may_unmap = 1,
>>  	};
>>  	struct reclaim_stat dummy_stat;
>> -	unsigned long ret;
>>  	struct page *page, *next;
>>  	LIST_HEAD(clean_pages);
>>  
>> @@ -1900,13 +1900,13 @@ static int current_may_throttle(void)
>>  {
>>  	LIST_HEAD(page_list);
>>  	unsigned long nr_scanned;
>> -	unsigned long nr_reclaimed = 0;
>>  	unsigned long nr_taken;
>>  	struct reclaim_stat stat;
>>  	int file = is_file_lru(lru);
>>  	enum vm_event_item item;
>>  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>  	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
>> +	unsigned int nr_reclaimed = 0;
>>  	bool stalled = false;
>>  
>>  	while (unlikely(too_many_isolated(pgdat, file, sc))) {


Thanks,
Maninder Singh

Re: [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list

[Michal Hocko]

On Wed 29-04-20 18:23:23, Maninder Singh wrote:
> 
> Hi,
> 
> >
> >Acked-by: Michal Hocko <mhocko@suse.com>
> >
> >Is there any reason to move declarations here?
> >
> 
> "unsigned int ret" was changed mistakenely, sending V2.
> and "unsigned int nr_reclaimed" is changed to remove hole.

Could you be more specific? Have you seen a better stack allocation
footprint?

-- 
Michal Hocko
SUSE Labs

RE:(2) [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list

[Vaneet Narang]

Hi Michal, 

>> >
>> >Acked-by: Michal Hocko <mhocko@suse.com>
>> >
>> >Is there any reason to move declarations here?
>> >
>> 
>> "unsigned int ret" was changed mistakenely, sending V2.
>> and "unsigned int nr_reclaimed" is changed to remove hole.
 
>Could you be more specific? Have you seen a better stack allocation
>footprint?

We didn't check stack allocation footprint, we did changes just by looking into the code.
we thought changing reclaimed type from long to int on 64 bit platform will add 
hole of 4 bytes between two stack variables nr_reclaimed & nr_taken.

So we tried to remove that hole by packing it with bool.

 	unsigned long nr_scanned;                 --> Size and alignment 8 byte for long 
-	unsigned long nr_reclaimed = 0;           --> Changing to int will make its size as 4  
 	unsigned long nr_taken;                   --> nr_taken needs alignment of 8 so will add hole.
 	struct reclaim_stat stat;
 	int file = is_file_lru(lru);
 	enum vm_event_item item;
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
 	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
+	unsigned int nr_reclaimed = 0;            --> So moving to this place to pack it along with bool 
 	bool stalled = false;


Overall stack footprint might not change as compiler makes function stack pointer as 16 byte aligned but we did this 
as we normally follow this coding convention when defining structures or stack variables. 

Thanks & Regards,
Vaneet Narang
 
 

 

Re: (2) [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list

[Michal Hocko]

On Wed 29-04-20 18:59:40, Vaneet Narang wrote:
> Hi Michal, 
> 
> >> >
> >> >Acked-by: Michal Hocko <mhocko@suse.com>
> >> >
> >> >Is there any reason to move declarations here?
> >> >
> >> 
> >> "unsigned int ret" was changed mistakenely, sending V2.
> >> and "unsigned int nr_reclaimed" is changed to remove hole.
>  
> >Could you be more specific? Have you seen a better stack allocation
> >footprint?
> 
> We didn't check stack allocation footprint, we did changes just by looking into the code.
> we thought changing reclaimed type from long to int on 64 bit platform will add 
> hole of 4 bytes between two stack variables nr_reclaimed & nr_taken.
> 
> So we tried to remove that hole by packing it with bool.
> 
>  	unsigned long nr_scanned;                 --> Size and alignment 8 byte for long 
> -	unsigned long nr_reclaimed = 0;           --> Changing to int will make its size as 4  
>  	unsigned long nr_taken;                   --> nr_taken needs alignment of 8 so will add hole.
>  	struct reclaim_stat stat;
>  	int file = is_file_lru(lru);
>  	enum vm_event_item item;
>  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>  	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
> +	unsigned int nr_reclaimed = 0;            --> So moving to this place to pack it along with bool 
>  	bool stalled = false;
> 
> 
> Overall stack footprint might not change as compiler makes function stack pointer as 16 byte aligned but we did this 
> as we normally follow this coding convention when defining structures or stack variables. 

My understanding is that gcc can and does tricks when allocating space
for local variables. It can use registers and there is no dictated
structure on the placing of variable or ordering (unlike for
structures).

Anyway, I would prefer if the patch was doing one thing at the time.
If you can see some (have a look ./scripts/bloat-o-meter) improvements
from reordering, make it a separate patch with some numbers attached.
-- 
Michal Hocko
SUSE Labs