Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Feb 2022 18:44:07 -0800
From:   Roman Gushchin <guro@fb.com>
To:     Shakeel Butt <shakeelb@google.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
Message-ID: <YgXNd11XK94PM89+@carbon.dhcp.thefacebook.com>
References: <20220210081437.1884008-1-shakeelb@google.com>
 <20220210081437.1884008-5-shakeelb@google.com>
 <YgVyZrDPxVgP6OLG@carbon.dhcp.thefacebook.com>
 <CALvZod5xFmCVV_AZO1be8pYakmDvYh-QXmNYtTNT4zvCw-m4bQ@mail.gmail.com>
 <YgWf0wL5RoSpNrWn@carbon.dhcp.thefacebook.com>
 <CALvZod5XstvNB_=qprW6_bx33h8JeyE5TjvX3UDoqG8XHBr14w@mail.gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CALvZod5XstvNB_=qprW6_bx33h8JeyE5TjvX3UDoqG8XHBr14w@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?4Yqz/CIHjPCeiqrXBU5pApn+pqiKvAJQuTJnRjyNFRS+v4Y49oUs5oQLnS3w?=
 =?us-ascii?Q?iPvK9faNo4lfENb/qi3DuhN6aF7wOQxG7zAxbViWlXl9ReWrfAt3lhAFYkkh?=
 =?us-ascii?Q?gn9WaY7jK6tcpROZYWLchkPer6JWtWejbRseEbW20etlWjzHYlKA27PY2jvX?=
 =?us-ascii?Q?koZqTox8qMqd0gLY6uvnnXdMA6zdxxpTEJs0jfUVIhM7gf8jM33uPuuuTimN?=
 =?us-ascii?Q?g9wJOU63lIle3ENBWRvgYcJMfw0EUdAsY2tuZHonxRyzLngdrE80Uz++a7pa?=
 =?us-ascii?Q?A486kvSHE/r8JQi9HQJaL/taFGMCWYDHaPyEAZA/i1n4vgjQTrxBbm0rJOP5?=
 =?us-ascii?Q?MptbIgc+M8Axr+UEHnIvj6d+agdpIgw7t+82RyT7m7NoGNAoHO15KS30Ca12?=
 =?us-ascii?Q?3XtxcGd6vtdKMYkXps02Yi8SxUoXt6C9kR0SaL1h3MZQ/WpOEWAWe6+S4Dkz?=
 =?us-ascii?Q?49LZ6ynUFkH6UgCnBcDAOKxllJDAE+FntLuD7PHbBboahtBZ9rG3RUN8tKE9?=
 =?us-ascii?Q?EAkbIuYgOqtTQjAUOqHV6sO/XNW/AyrAFmDjhCdYygJ9ufihOp5G0m7COJhL?=
 =?us-ascii?Q?x/WXMcOsfWVfkYXwbk+78zEZUMe32oKiCq1fbGZRToEXXErCDY6hUy3uGUgB?=
 =?us-ascii?Q?mH/vwcJfKzYR29RQAe/+zIblbSpVkKBgzpw+PjU3bRtXjUqt/UhBYZ8P7vgz?=
 =?us-ascii?Q?GRZO3hYvcHE94Ukqkau77zHwTRkczZJ60IkAZ/FPEu8HNmwOaPPHJM5PBeGW?=
 =?us-ascii?Q?ldvYbzDnYVInC25Ail03QY7DbvPegd5A0gIouf+/F6Ki9/QJHyJetv8dRX5l?=
 =?us-ascii?Q?AMwS/Im3l1UoROEXU3jvlAsJVKPxwhozKDfEy1IKrENHycr6gSxopOYvgQG+?=
 =?us-ascii?Q?NYOYxP1CubDCFzIC7kn1Vw8OmHuDOuDY7XtH26IWSajltDo1dR42FV/4J8yC?=
 =?us-ascii?Q?KZ1gcyCIUmE+h1PExDbyT3LhPN6EYqv3jT7oTlKveBuLTOZwUY5fAdKFmUgO?=
 =?us-ascii?Q?B+xyjgpLhgUaNTtuvmmUNLGhf14+4SeqU2C9ZAd7+Vqg8W1vFB1KqJ6NU5om?=
 =?us-ascii?Q?40uljyiZ/UsG3T78r1Fw32F3WPF08jaFGMqIEZTHDJdFsvrTXXf9zCthR6xE?=
 =?us-ascii?Q?U0QnglVDWzubaV9weZxc0zPJtHz4kh+BdsAVSZZUWWF9RXj3QV6noWsb4Oaq?=
 =?us-ascii?Q?bIetp+/vgcjkVP/plLGYRNhP5qRR6iIZel4Zrbr3WNLKwoESXrhOVVJq7ELG?=
 =?us-ascii?Q?fmOkp7mgth5F1IXZROyCmQ+z1/OL6BbGtR69B7pqkExQjF4E3vPST+Ys4Hkz?=
 =?us-ascii?Q?7yNq/pSyjReIFZ2OXlVvjp8Sx2ra10VdwR+HBNCcrAlcNojX395E15av9QZ1?=
 =?us-ascii?Q?dlVMpCjQXebNYKe5J4HC/o/0cP5xHmvkUEEZlSt/Ua4KPyZWPuw96HSigWFs?=
 =?us-ascii?Q?PCwolUcDGZhoU5SQF2gDYOz9cGNIFCaQ0x8EhZk05AAMkDUFVzZ3VjZlGAaA?=
 =?us-ascii?Q?U2TTa/5MbwdXk0P5NYVfXAEPtop08H2nt7XzzCBqG2HzWZvwP6uXScmHe/Uy?=
 =?us-ascii?Q?k7b7OJrX4J9dIPh64/+z5Yy/+r7aveCHaH5r8dlt?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7da21205-775c-422b-3839-08d9ed086282
X-MS-Exchange-CrossTenant-AuthSource: BYAPR15MB4136.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Feb 2022 02:44:11.9576
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3UcBs+zc7BHB2vON6uxZlks4xtCjn24QKKL8Ty7Vk3Vxm4FK/3FSuEJvhCkkBglq
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR15MB2372
X-OriginatorOrg: fb.com
X-Proofpoint-ORIG-GUID: 4OzCqrIRTYwkSBorQU7UMVv7nyqxGUF1
X-Proofpoint-GUID: 4OzCqrIRTYwkSBorQU7UMVv7nyqxGUF1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2022-02-10_11,2022-02-09_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 bulkscore=0
 mlxlogscore=951 priorityscore=1501 spamscore=0 lowpriorityscore=0
 impostorscore=0 mlxscore=0 suspectscore=0 malwarescore=0 adultscore=0
 phishscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2202110010
X-FB-Internal: deliver
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 10, 2022 at 03:53:00PM -0800, Shakeel Butt wrote:
> On Thu, Feb 10, 2022 at 3:29 PM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Thu, Feb 10, 2022 at 02:22:36PM -0800, Shakeel Butt wrote:
> > > 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.
> >
> > Well, in a theory it sounds good and I have no intention to oppose the
> > idea. However in practice we might easily get quite serious problems.
> > So I think we should be extra careful here. In the end we don't want to
> > pull and then revert this patch.
> >
> > The difference between the system-wide direct reclaim and this case is that
> > usually kswapd is doing a good job of refilling the empty buffer, so we don't
> > usually work in the circumstances of the global memory shortage. And when we do,
> > often it's not working out quite well, this is why oomd and other similar
> > solutions are required.
> >.
> > Another option is to use your approach only for special cases (e.g. huge
> > allocations) and keep the existing approach for most other allocations.
> >
> 
> These are not necessarily huge allocations and can be a large number
> of small allocations. However I think we can make this idea work by
> checking current->memcg_nr_pages_over_high. If
> order(current->memcg_nr_pages_over_high) is, let's say, larger than
> PAGE_ALLOC_COSTLY_ORDER, then throttle synchronously. WDYT?

Yes, I really like this idea: the majority of allocations will be handled in
a proven way, however we'll have coverage for large outliers as well.

Not sure about PAGE_ALLOC_COSTLY_ORDER though, I'd probably chose a larger
constant, but we can discuss it separately.

Thanks!