From: Shakeel Butt <shakeelb@google.com> To: Roman Gushchin <guro@fb.com> Cc: Johannes Weiner <hannes@cmpxchg.org>, Michal Hocko <mhocko@suse.com>, Chris Down <chris@chrisdown.name>, Andrew Morton <akpm@linux-foundation.org>, Cgroups <cgroups@vger.kernel.org>, Linux MM <linux-mm@kvack.org>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 4/4] memcg: synchronously enforce memory.high Date: Thu, 10 Feb 2022 14:22:36 -0800 [thread overview] Message-ID: <CALvZod5xFmCVV_AZO1be8pYakmDvYh-QXmNYtTNT4zvCw-m4bQ@mail.gmail.com> (raw) In-Reply-To: <YgVyZrDPxVgP6OLG@carbon.dhcp.thefacebook.com> On Thu, Feb 10, 2022 at 12:15 PM Roman Gushchin <guro@fb.com> wrote: > [...] > > Has this approach been extensively tested in the production? > > Injecting sleeps at return-to-userspace moment is safe in terms of priority > inversions: a slowed down task will unlikely affect the rest of the system. > > It way less predictable for a random allocation in the kernel mode, what if > the task is already holding a system-wide resource? > > Someone might argue that it's not better than a system-wide memory shortage > and the same allocation might go into a direct reclaim anyway, but with > the way how memory.high is used it will happen way more often. > Thanks for the review. This patchset is tested in the test environment for now and I do plan to test this in production but that is a slow process and will take some time. Let me answer the main concern you have raised i.e. the safety of throttling a task synchronously in the charge code path. Please note that synchronous memory reclaim and oom-killing can already cause the priority inversion issues you have mentioned. The way we usually tackle such issues are through userspace controllers. For example oomd is the userspace solution for catering such issues related to oom-killing. Here we have a similar userspace daemon monitoring the workload and deciding if it should let the workload grow or kill it. Now should we keep the current high limit enforcement implementation and let it be ineffective for some real workloads or should we make the enforcement more robust and let the userspace tackle some corner case priority inversion issues. I think we should follow the second option as we already have precedence of doing the same for reclaim and oom-killing. thanks, Shakeel
WARNING: multiple messages have this Message-ID (diff)
From: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> To: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org> Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>, Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>, Chris Down <chris-6Bi1550iOqEnzZ6mRAm98g@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Cgroups <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux MM <linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org>, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH 4/4] memcg: synchronously enforce memory.high Date: Thu, 10 Feb 2022 14:22:36 -0800 [thread overview] Message-ID: <CALvZod5xFmCVV_AZO1be8pYakmDvYh-QXmNYtTNT4zvCw-m4bQ@mail.gmail.com> (raw) In-Reply-To: <YgVyZrDPxVgP6OLG-cx5fftMpWqeCjSd+JxjunQ2O0Ztt9esIQQ4Iyu8u01E@public.gmane.org> On Thu, Feb 10, 2022 at 12:15 PM Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org> wrote: > [...] > > Has this approach been extensively tested in the production? > > Injecting sleeps at return-to-userspace moment is safe in terms of priority > inversions: a slowed down task will unlikely affect the rest of the system. > > It way less predictable for a random allocation in the kernel mode, what if > the task is already holding a system-wide resource? > > Someone might argue that it's not better than a system-wide memory shortage > and the same allocation might go into a direct reclaim anyway, but with > the way how memory.high is used it will happen way more often. > Thanks for the review. This patchset is tested in the test environment for now and I do plan to test this in production but that is a slow process and will take some time. Let me answer the main concern you have raised i.e. the safety of throttling a task synchronously in the charge code path. Please note that synchronous memory reclaim and oom-killing can already cause the priority inversion issues you have mentioned. The way we usually tackle such issues are through userspace controllers. For example oomd is the userspace solution for catering such issues related to oom-killing. Here we have a similar userspace daemon monitoring the workload and deciding if it should let the workload grow or kill it. Now should we keep the current high limit enforcement implementation and let it be ineffective for some real workloads or should we make the enforcement more robust and let the userspace tackle some corner case priority inversion issues. I think we should follow the second option as we already have precedence of doing the same for reclaim and oom-killing. thanks, Shakeel
next prev parent reply other threads:[~2022-02-10 22:22 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-10 8:14 [PATCH 0/4] memcg: robust enforcement of memory.high Shakeel Butt 2022-02-10 8:14 ` Shakeel Butt 2022-02-10 8:14 ` [PATCH 1/4] memcg: refactor mem_cgroup_oom Shakeel Butt 2022-02-10 8:14 ` Shakeel Butt 2022-02-10 19:52 ` Roman Gushchin 2022-02-10 19:52 ` Roman Gushchin 2022-02-10 22:23 ` Shakeel Butt 2022-02-10 8:14 ` [PATCH 2/4] memcg: unify force charging conditions Shakeel Butt 2022-02-10 8:14 ` Shakeel Butt 2022-02-10 20:03 ` Roman Gushchin 2022-02-10 20:03 ` Roman Gushchin 2022-02-10 22:25 ` Shakeel Butt 2022-02-10 22:25 ` Shakeel Butt 2022-02-10 23:15 ` Roman Gushchin 2022-02-10 8:14 ` [PATCH 3/4] selftests: memcg: test high limit for single entry allocation Shakeel Butt 2022-02-10 8:14 ` Shakeel Butt 2022-02-10 8:14 ` [PATCH 4/4] memcg: synchronously enforce memory.high Shakeel Butt 2022-02-10 8:14 ` Shakeel Butt 2022-02-10 20:15 ` Roman Gushchin 2022-02-10 20:15 ` Roman Gushchin 2022-02-10 22:22 ` Shakeel Butt [this message] 2022-02-10 22:22 ` Shakeel Butt 2022-02-10 23:29 ` Roman Gushchin 2022-02-10 23:29 ` Roman Gushchin 2022-02-10 23:53 ` Shakeel Butt 2022-02-11 2:44 ` Roman Gushchin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CALvZod5xFmCVV_AZO1be8pYakmDvYh-QXmNYtTNT4zvCw-m4bQ@mail.gmail.com \ --to=shakeelb@google.com \ --cc=akpm@linux-foundation.org \ --cc=cgroups@vger.kernel.org \ --cc=chris@chrisdown.name \ --cc=guro@fb.com \ --cc=hannes@cmpxchg.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.