* [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list [not found] <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcas5p2.samsung.com> @ 2020-04-29 5:59 ` Maninder Singh 2020-04-29 12:09 ` Michal Hocko [not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p6> 0 siblings, 2 replies; 6+ messages in thread From: Maninder Singh @ 2020-04-29 5:59 UTC (permalink / raw) To: mhocko, akpm; +Cc: linux-mm, linux-kernel, a.sahrawat, v.narang, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list 2020-04-29 5:59 ` [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list Maninder Singh @ 2020-04-29 12:09 ` Michal Hocko [not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p6> 1 sibling, 0 replies; 6+ messages in thread From: Michal Hocko @ 2020-04-29 12:09 UTC (permalink / raw) To: Maninder Singh; +Cc: akpm, linux-mm, linux-kernel, a.sahrawat, v.narang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p6>]
* RE:[PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list [not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p6> @ 2020-04-29 12:53 ` Maninder Singh 2020-04-29 13:09 ` [PATCH " Michal Hocko [not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p8> 0 siblings, 2 replies; 6+ messages in thread From: Maninder Singh @ 2020-04-29 12:53 UTC (permalink / raw) To: Michal Hocko; +Cc: akpm, linux-mm, linux-kernel, AMIT SAHRAWAT, Vaneet Narang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list 2020-04-29 12:53 ` Maninder Singh @ 2020-04-29 13:09 ` Michal Hocko [not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p8> 1 sibling, 0 replies; 6+ messages in thread From: Michal Hocko @ 2020-04-29 13:09 UTC (permalink / raw) To: Maninder Singh; +Cc: akpm, linux-mm, linux-kernel, AMIT SAHRAWAT, Vaneet Narang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p8>]
* RE:(2) [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list [not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p8> @ 2020-04-29 13:29 ` Vaneet Narang 2020-04-29 13:35 ` (2) " Michal Hocko 0 siblings, 1 reply; 6+ messages in thread From: Vaneet Narang @ 2020-04-29 13:29 UTC (permalink / raw) To: Michal Hocko, Maninder Singh; +Cc: akpm, linux-mm, linux-kernel, AMIT SAHRAWAT 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: (2) [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list 2020-04-29 13:29 ` Vaneet Narang @ 2020-04-29 13:35 ` Michal Hocko 0 siblings, 0 replies; 6+ messages in thread From: Michal Hocko @ 2020-04-29 13:35 UTC (permalink / raw) To: Vaneet Narang; +Cc: Maninder Singh, akpm, linux-mm, linux-kernel, AMIT SAHRAWAT 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-29 13:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcas5p2.samsung.com>
2020-04-29 5:59 ` [PATCH 1/1] mm/vmscan.c: change prototype for shrink_page_list Maninder Singh
2020-04-29 12:09 ` Michal Hocko
[not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p6>
2020-04-29 12:53 ` Maninder Singh
2020-04-29 13:09 ` [PATCH " Michal Hocko
[not found] ` <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p8>
2020-04-29 13:29 ` Vaneet Narang
2020-04-29 13:35 ` (2) " Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).