On Thu, Feb 10, 2022 at 12:14:34AM -0800, Shakeel Butt wrote:
> The function mem_cgroup_oom returns enum which has four possible values
> but the caller does not care about such values and only care if the
> return value is OOM_SUCCESS or not. So, remove the enum altogether and
> make mem_cgroup_oom returns a simple bool.
>
> Signed-off-by: Shakeel Butt <shakeelb@xxxxxxxxxx>
Nice!
Reviewed-by: Roman Gushchin <guro@xxxxxx>
> ---
> mm/memcontrol.c | 40 +++++++++++++---------------------------
> 1 file changed, 13 insertions(+), 27 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a0e9d9f12cf5..c40c27822802 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1795,20 +1795,12 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
> __wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
> }
>
> -enum oom_status {
> - OOM_SUCCESS,
> - OOM_FAILED,
> - OOM_ASYNC,
> - OOM_SKIPPED
> -};
> -
The only thing, I'd add a small comment on the return value here. E.g.
"returns true if one or more tasks have been successfully killed" or something
like this.
Thanks!
> -static enum oom_status mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int order)
> +static bool mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int order)
> {
> - enum oom_status ret;
> - bool locked;
> + bool locked, ret = false;
>
> if (order > PAGE_ALLOC_COSTLY_ORDER)
> - return OOM_SKIPPED;
> + return ret;
>
> memcg_memory_event(memcg, MEMCG_OOM);
>
> @@ -1831,14 +1823,13 @@ static enum oom_status mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int
> * victim and then we have to bail out from the charge path.
> */
> if (memcg->oom_kill_disable) {
> - if (!current->in_user_fault)
> - return OOM_SKIPPED;
> - css_get(&memcg->css);
> - current->memcg_in_oom = memcg;
> - current->memcg_oom_gfp_mask = mask;
> - current->memcg_oom_order = order;
> -
> - return OOM_ASYNC;
> + if (current->in_user_fault) {
> + css_get(&memcg->css);
> + current->memcg_in_oom = memcg;
> + current->memcg_oom_gfp_mask = mask;
> + current->memcg_oom_order = order;
> + }
> + return ret;
> }
>
> mem_cgroup_mark_under_oom(memcg);
> @@ -1849,10 +1840,7 @@ static enum oom_status mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int
> mem_cgroup_oom_notify(memcg);
>
> mem_cgroup_unmark_under_oom(memcg);
> - if (mem_cgroup_out_of_memory(memcg, mask, order))
> - ret = OOM_SUCCESS;
> - else
> - ret = OOM_FAILED;
> + ret = mem_cgroup_out_of_memory(memcg, mask, order);
>
> if (locked)
> mem_cgroup_oom_unlock(memcg);
> @@ -2545,7 +2533,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
> int nr_retries = MAX_RECLAIM_RETRIES;
> struct mem_cgroup *mem_over_limit;
> struct page_counter *counter;
> - enum oom_status oom_status;
> unsigned long nr_reclaimed;
> bool passed_oom = false;
> bool may_swap = true;
> @@ -2648,9 +2635,8 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
> * a forward progress or bypass the charge if the oom killer
> * couldn't make any progress.
> */
> - oom_status = mem_cgroup_oom(mem_over_limit, gfp_mask,
> - get_order(nr_pages * PAGE_SIZE));
> - if (oom_status == OOM_SUCCESS) {
> + if (mem_cgroup_oom(mem_over_limit, gfp_mask,
> + get_order(nr_pages * PAGE_SIZE))) {
> passed_oom = true;
> nr_retries = MAX_RECLAIM_RETRIES;
> goto retry;
> --
> 2.35.1.265.g69c8d7142f-goog
>
