lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 19 Dec 2020 01:41:07 +0800 From: Jacob Wen <jian.w.wen@...cle.com> To: Michal Hocko <mhocko@...e.com> Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org, akpm@...ux-foundation.org Subject: Re: [PATCH] mm/vmscan: DRY cleanup for do_try_to_free_pages() On 12/18/20 10:27 PM, Michal Hocko wrote: > On Fri 18-12-20 21:51:48, Jacob Wen wrote: >> On 12/18/20 6:51 PM, Michal Hocko wrote: >>> On Fri 18-12-20 18:22:17, Jacob Wen wrote: >>>> This patch reduces repetition of set_task_reclaim_state() around >>>> do_try_to_free_pages(). >>> The changelog really should be talking about why this is needed/useful. >>> From the above it is not really clear whether you aimed at doing >>> a clean up or this is a fix for some misbehavior. I do assume the former >>> but this should be clearly articulated. >> How about this? >> >> mm/vmscan: remove duplicate code around do_try_to_free_pages() >> >> This patch moves set_task_reclaim_state() into do_try_to_free_pages() >> to avoid unnecessary repetition. It doesn't introduce functional >> change. > This is still more about what is changed more than why it is changed. I > would go with something like the following: > " > reclaim_state has to be set for all reclaim paths because it acts as a > storage to collect reclaim feedback. Currently set_task_reclaim_state is > called from each highlevel reclaim function. Simplify the code flow by > moving set_task_reclaim_state into core direct reclaim function > (do_try_to_free_pages) for all direct reclaim paths. > " > > To the patch itself. I am not opposed but I do not see an urgent reason > to take it either. The net LOC increases slightly, it makes > do_try_to_free_pages slightly more tricky due to different early return > paths. Highlevel direct reclaim functions do not tend to change a lot. set_task_reclaim_state() is a function with 3 lines of code of which 2 lines contain WARN_ON_ONCE. I am not comfortable with the current repetition. > >>>> Signed-off-by: Jacob Wen <jian.w.wen@...cle.com> >>>> --- >>>> mm/vmscan.c | 27 ++++++++++++++++----------- >>>> 1 file changed, 16 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/mm/vmscan.c b/mm/vmscan.c >>>> index 257cba79a96d..4bc244b23686 100644 >>>> --- a/mm/vmscan.c >>>> +++ b/mm/vmscan.c >>>> @@ -3023,6 +3023,10 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist, >>>> pg_data_t *last_pgdat; >>>> struct zoneref *z; >>>> struct zone *zone; >>>> + unsigned long ret; >>>> + >>>> + set_task_reclaim_state(current, &sc->reclaim_state); >>>> + >>>> retry: >>>> delayacct_freepages_start(); >>>> @@ -3069,12 +3073,16 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist, >>>> delayacct_freepages_end(); >>>> - if (sc->nr_reclaimed) >>>> - return sc->nr_reclaimed; >>>> + if (sc->nr_reclaimed) { >>>> + ret = sc->nr_reclaimed; >>>> + goto out; >>>> + } >>>> /* Aborted reclaim to try compaction? don't OOM, then */ >>>> - if (sc->compaction_ready) >>>> - return 1; >>>> + if (sc->compaction_ready) { >>>> + ret = 1; >>>> + goto out; >>>> + } >>>> /* >>>> * We make inactive:active ratio decisions based on the node's >>>> @@ -3101,7 +3109,10 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist, >>>> goto retry; >>>> } >>>> - return 0; >>>> + ret = 0; >>>> +out: >>>> + set_task_reclaim_state(current, NULL); >>>> + return ret; >>>> } >>>> static bool allow_direct_reclaim(pg_data_t *pgdat) >>>> @@ -3269,13 +3280,11 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order, >>>> if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask)) >>>> return 1; >>>> - set_task_reclaim_state(current, &sc.reclaim_state); >>>> trace_mm_vmscan_direct_reclaim_begin(order, sc.gfp_mask); >>>> nr_reclaimed = do_try_to_free_pages(zonelist, &sc); >>>> trace_mm_vmscan_direct_reclaim_end(nr_reclaimed); >>>> - set_task_reclaim_state(current, NULL); >>>> return nr_reclaimed; >>>> } >>>> @@ -3347,7 +3356,6 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg, >>>> */ >>>> struct zonelist *zonelist = node_zonelist(numa_node_id(), sc.gfp_mask); >>>> - set_task_reclaim_state(current, &sc.reclaim_state); >>>> trace_mm_vmscan_memcg_reclaim_begin(0, sc.gfp_mask); >>>> noreclaim_flag = memalloc_noreclaim_save(); >>>> @@ -3355,7 +3363,6 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg, >>>> memalloc_noreclaim_restore(noreclaim_flag); >>>> trace_mm_vmscan_memcg_reclaim_end(nr_reclaimed); >>>> - set_task_reclaim_state(current, NULL); >>>> return nr_reclaimed; >>>> } >>>> @@ -4023,11 +4030,9 @@ unsigned long shrink_all_memory(unsigned long nr_to_reclaim) >>>> fs_reclaim_acquire(sc.gfp_mask); >>>> noreclaim_flag = memalloc_noreclaim_save(); >>>> - set_task_reclaim_state(current, &sc.reclaim_state); >>>> nr_reclaimed = do_try_to_free_pages(zonelist, &sc); >>>> - set_task_reclaim_state(current, NULL); >>>> memalloc_noreclaim_restore(noreclaim_flag); >>>> fs_reclaim_release(sc.gfp_mask); >>>> -- >>>> 2.25.1 >>>>
Powered by blists - more mailing lists