On Tue, Aug 23, 2022 at 2:36 AM Michal Hocko mhocko@suse.com wrote:
On Mon 22-08-22 17:20:17, Yu Zhao wrote:
On Mon, Aug 22, 2022 at 5:16 PM Andrew Morton akpm@linux-foundation.org wrote:
On Mon, 22 Aug 2022 16:59:29 -0600 Yu Zhao yuzhao@google.com wrote:
@@ -4109,7 +4109,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long start, unsigned long end,
walk_pmd_range(&val, addr, next, args);
if (mm_is_oom_victim(args->mm))
if (test_bit(MMF_OOM_REAP_QUEUED, &args->mm->flags)) return 1; /* a racy check to curtail the waiting time */
Oh. Why? What does this change do?
The MMF_OOM_REAP_QUEUED flag is similar to the deleted MMF_OOM_VICTIM flag, but it's set at a later stage during an OOM kill.
When either is set, the OOM reaper is probably already freeing the memory of this mm_struct, or at least it's going to. So there is no need to dwell on it in the reclaim path, hence not about correctness.
Thanks. That sounds worthy of some code comments?
Will do. Thanks.
I would rather not see this abuse.
I understand where you're coming from, however, I don't share this POV. I see it as cooperation -- the page reclaim and the oom/reaper can't (or at least shouldn't) operate in isolation.
You cannot really make any assumptions about oom_reaper and how quickly it is going to free the memory.
Agreed. But here we are talking about heuristics, not dependencies on certain behaviors. Assume we are playing a guessing game: there are multiple mm_structs available for reclaim, would the oom-killed ones be more profitable on average? I'd say no, because I assume it's more likely than unlikely that the oom reaper is doing/to do its work. Note that the assumption is about likelihood, hence arguably valid.
If this is really worth it (and I have to say I doubt it) then it should be a separate patch with numbers justifying it.
I definitely can artificially create a test case that runs oom a few times per second, to prove this two-liner is beneficial to that scenario. Then there is the question how much it would benefit the real-world scenarios.
I'd recommend keeping this two-liner if we still had mm_is_oom_victim(), because it's simple, clear and intuitive. With MMF_OOM_REAP_QUEUED, I don't have a strong opinion. Since you do, I'll just delete it.