From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B4074C433ED for ; Thu, 15 Apr 2021 19:40:14 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 22AD6610EA for ; Thu, 15 Apr 2021 19:40:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 22AD6610EA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cmpxchg.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4D3996B0070; Thu, 15 Apr 2021 15:40:13 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4843A6B0071; Thu, 15 Apr 2021 15:40:13 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2B0FA6B0072; Thu, 15 Apr 2021 15:40:13 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0046.hostedemail.com [216.40.44.46]) by kanga.kvack.org (Postfix) with ESMTP id F2CDA6B0070 for ; Thu, 15 Apr 2021 15:40:12 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id A8046181AEF3C for ; Thu, 15 Apr 2021 19:40:12 +0000 (UTC) X-FDA: 78035617464.01.B1EF9A8 Received: from mail-qv1-f46.google.com (mail-qv1-f46.google.com [209.85.219.46]) by imf20.hostedemail.com (Postfix) with ESMTP id BB5E913A for ; Thu, 15 Apr 2021 19:40:06 +0000 (UTC) Received: by mail-qv1-f46.google.com with SMTP id i3so6765407qvj.7 for ; Thu, 15 Apr 2021 12:40:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=ysi6RRTEAiRn+Yy07GZnMkjfObnhYqIw2YgVv+JxE44=; b=b2A9c+lKNHFTx5ujWSTaeS9zFMdER+ifE7Xz5osy5K6xiMiaNTgWAyMly/+6qpp5tr yRDuZ5mJe1PTVPp/InRtMLkJ2BKi3Xrj/3nluHM7i/iDx0Z/WAVtnMtXAhool/Zs4r7m MUxwDVravbfwueke0nUfYYeStLFeTvTViS1GxMog6kDrxc/NFZFZzZ4kgx5klve3OqdD HLxq6x2jHTz9MI5/1MxdV9MrSJD1djMSnf/MVVy0BF66/E70xCrsL6bmt8FAkFKHifSw /0xvlNjX1zlQ8+imEOXDURfi+m9K0n7A8o9OsU4NvY8QWkqZWIg9TZz3/AtU2wj1nmIs glTg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ysi6RRTEAiRn+Yy07GZnMkjfObnhYqIw2YgVv+JxE44=; b=q8J0EOHVhYyXmZssGEDuqVOcalHrIqhiC1rqDoLGDJp0/DAiW6CJ8YoJaZb/yghFHc tWcrhIv+WDR6Xgv4WrKWs8e2s1Vb57yM6YE8TMX8m4+nKe55XW/X9nEWDuU7VSMtG9Wd VVNIF/iv+gmOj7ZTDjvvidtd3qLhPT0ulwWZIKok6IxtGB+PNn19+Y+5bVCZLOT9IE/Q G/dAkMDcU5cTiu0mFt3SCNaxrJ2ib3o1IeqUwIJVv02vP98pKAteVzQuMfV2X9ryTPQV 2fZZjLbqVMP46gRqj/1ioBrh5KT+WuLeOdxR394YF69gcbPqHhszskTBElIuKqz/7Cfr pzHg== X-Gm-Message-State: AOAM531hTfJmnqa4r6YcS35WU3iBx992f0pnTQMjmpuT4JPG41V47JzV DjR8rqKosSSvGmhQ1hZkd7ow8g== X-Google-Smtp-Source: ABdhPJyuYjLPcmUlKK6SIRy68Qi0woumXcj2z/Cl1pUb56vpFCK/iZjog2Ajb5zWSOa3zOfXnn/u8w== X-Received: by 2002:a0c:c488:: with SMTP id u8mr4868194qvi.47.1618515611398; Thu, 15 Apr 2021 12:40:11 -0700 (PDT) Received: from localhost (70.44.39.90.res-cmts.bus.ptd.net. [70.44.39.90]) by smtp.gmail.com with ESMTPSA id f17sm2529353qtp.39.2021.04.15.12.40.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Apr 2021 12:40:10 -0700 (PDT) Date: Thu, 15 Apr 2021 15:40:10 -0400 From: Johannes Weiner To: Waiman Long Cc: Michal Hocko , Vladimir Davydov , Andrew Morton , Tejun Heo , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Vlastimil Babka , Roman Gushchin , linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org, Shakeel Butt , Muchun Song , Alex Shi , Chris Down , Yafang Shao , Wei Yang , Masayoshi Mizuma , Xing Zhengjun Subject: Re: [PATCH v3 2/5] mm/memcg: Introduce obj_cgroup_uncharge_mod_state() Message-ID: References: <20210414012027.5352-1-longman@redhat.com> <20210414012027.5352-3-longman@redhat.com> <1c85e8f6-e8b9-33e1-e29b-81fbadff959f@redhat.com> <8a104fd5-64c7-3f41-981c-9cfa977c78a6@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8a104fd5-64c7-3f41-981c-9cfa977c78a6@redhat.com> X-Stat-Signature: 6uxqwhemik953jpachegb5xd4gjiudop X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: BB5E913A Received-SPF: none (cmpxchg.org>: No applicable sender policy available) receiver=imf20; identity=mailfrom; envelope-from=""; helo=mail-qv1-f46.google.com; client-ip=209.85.219.46 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1618515606-665542 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Apr 15, 2021 at 02:47:31PM -0400, Waiman Long wrote: > On 4/15/21 2:10 PM, Johannes Weiner wrote: > > On Thu, Apr 15, 2021 at 12:35:45PM -0400, Waiman Long wrote: > > > On 4/15/21 12:30 PM, Johannes Weiner wrote: > > > > On Tue, Apr 13, 2021 at 09:20:24PM -0400, Waiman Long wrote: > > > > > In memcg_slab_free_hook()/pcpu_memcg_free_hook(), obj_cgroup_uncharge() > > > > > is followed by mod_objcg_state()/mod_memcg_state(). Each of these > > > > > function call goes through a separate irq_save/irq_restore cycle. That > > > > > is inefficient. Introduce a new function obj_cgroup_uncharge_mod_state() > > > > > that combines them with a single irq_save/irq_restore cycle. > > > > > > > > > > @@ -3292,6 +3296,25 @@ void obj_cgroup_uncharge(struct obj_cgroup *objcg, size_t size) > > > > > refill_obj_stock(objcg, size); > > > > > } > > > > > +void obj_cgroup_uncharge_mod_state(struct obj_cgroup *objcg, size_t size, > > > > > + struct pglist_data *pgdat, int idx) > > > > The optimization makes sense. > > > > > > > > But please don't combine independent operations like this into a > > > > single function. It makes for an unclear parameter list, it's a pain > > > > in the behind to change the constituent operations later on, and it > > > > has a habit of attracting more random bools over time. E.g. what if > > > > the caller already has irqs disabled? What if it KNOWS that irqs are > > > > enabled and it could use local_irq_disable() instead of save? > > > > > > > > Just provide an __obj_cgroup_uncharge() that assumes irqs are > > > > disabled, combine with the existing __mod_memcg_lruvec_state(), and > > > > bubble the irq handling up to those callsites which know better. > > > > > > > That will also work. However, the reason I did that was because of patch 5 > > > in the series. I could put the get_obj_stock() and put_obj_stock() code in > > > slab.h and allowed them to be used directly in various places, but hiding in > > > one function is easier. > > Yeah it's more obvious after getting to patch 5. > > > > But with the irq disabling gone entirely, is there still an incentive > > to combine the atomic section at all? Disabling preemption is pretty > > cheap, so it wouldn't matter to just do it twice. > > > > I.e. couldn't the final sequence in slab code simply be > > > > objcg_uncharge() > > mod_objcg_state() > > > > again and each function disables preemption (and in the rare case > > irqs) as it sees fit? > > > > You lose the irqsoff batching in the cold path, but as you say, hit > > rates are pretty good, and it doesn't seem worth complicating the code > > for the cold path. > > > That does make sense, though a little bit of performance may be lost. I will > try that out to see how it work out performance wise. Thanks. Even if we still end up doing it, it's great to have that cost isolated, so we know how much extra code complexity corresponds to how much performance gain. It seems the task/irq split could otherwise be a pretty localized change with no API implications.