Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 13 Oct 2021 16:15:27 -0700
From:   Roman Gushchin <guro@fb.com>
To:     Shakeel Butt <shakeelb@google.com>
CC:     Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        Uladzislau Rezki <urezki@gmail.com>,
        Vasily Averin <vvs@virtuozzo.com>,
        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] memcg: page_alloc: skip bulk allocator for __GFP_ACCOUNT
Message-ID: <YWdoj9FZy2B4oLj8@carbon.DHCP.thefacebook.com>
References: <20211013194338.1804247-1-shakeelb@google.com>
 <YWdXv+RBjXvdmsK+@carbon.DHCP.thefacebook.com>
 <CALvZod6ZppPNk2XfvKFfdPhrsSF6NbSBKrOOOc6UyJMfDEfKoQ@mail.gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CALvZod6ZppPNk2XfvKFfdPhrsSF6NbSBKrOOOc6UyJMfDEfKoQ@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?SC+m4W9IUhEXsOFwWCGi+XCvvyeM7cr/XqhfKqqBmiUD1I/WKwMP9S5VKoMF?=
 =?us-ascii?Q?tOdXp+mRQsPCWGEngrDErV8TmsDpZhI5oXK10tdA+3sl2NWNgA9oB/zqsvt/?=
 =?us-ascii?Q?qN+Xgt3Qp9E/JihjnLq4SsCKl2WCiNf6hAaNyISEEUv76177pIUetu+vw5Rw?=
 =?us-ascii?Q?CbKPCXyHTIu9CDHbKFqwgyd5a/zLnwHGFHIKDsWXm/Yjz6oLT22RGeV1xRL+?=
 =?us-ascii?Q?4eO2fO4uHdKDVwZBkuvmJwoPsVLY/8ISh8VDtUHaw1Cx0S2btnb5OumGzXUQ?=
 =?us-ascii?Q?O/QOzPp3ocyxVASR90s36mCKPbPQRakg2aZKlJVjk46wpfh60FTnb0kcNaBF?=
 =?us-ascii?Q?PoeV/yyoX1apyTvVQ+3vXq259pySJNZTgdBIVtojvXbKHsQkxJlnWYi+/cAg?=
 =?us-ascii?Q?2EwCZnDgmlVwt2v67LUkZDP/0YvqX32JHKgpp9EdSLFCYx+xV171wLSpA+QU?=
 =?us-ascii?Q?YJJ3bNL4NHoaENMe2vCNCuIM14JJjz2m9E+LRJKZx5HULf0aaqSOLKx+9ImW?=
 =?us-ascii?Q?wRvoF6ZQgz235rhrIZqAAQAwhgVDWgyw7Ab8roEjCkNfjLgrpUvJfbsysYRM?=
 =?us-ascii?Q?LrTsTzZzxwg1lWnyo/s6NVvJIlKDosjvHIWD4vUL6XqEAygREZCUHjH7QoJz?=
 =?us-ascii?Q?YgZ7wV8n2abFcwzvn9yPOt/7C2p/pkd3SK4MNJEVTuHAm1ryuBPZxVJuAmPL?=
 =?us-ascii?Q?XMDG2oBFOd8NjlSXF3FEtLkQKQiFQjDB+kyfqaWt9fmz/XkFih20y8k6rNwg?=
 =?us-ascii?Q?mcKN/xC0yCQRUC1O8RC9eHDToEfs+2TZtqoYQLyfiqqhLtxjyya7Gj0lT3/v?=
 =?us-ascii?Q?75wx2/7BVF6MmCJXJsmhRUUeqljLhPhjxalrcanfJGO2/12LGdGbKXtiH3SR?=
 =?us-ascii?Q?N+RVxFuBoA60dqUzmrLxeLyUUmHqrklMUZRzmDfc2LwSXHVSXxdFYSmjCjhL?=
 =?us-ascii?Q?/uDVLh29YtAt6sE7zSMSXJm8VTHLc0Kwn9GMcOXz3XF3r323ncB0F8r/MYYU?=
 =?us-ascii?Q?mfFld8E7SugSBVC5mQZYIMHQUZwXC3zLJmHP5xYLq7/M11aW0vR62pst3R9R?=
 =?us-ascii?Q?+JaycukRR2D4w3aYo/sgOqHl5QJm54uwq9QVyKApFAZhZunXyML8shmmA+3L?=
 =?us-ascii?Q?DrXluZkGSUZb0YZhFJYkfuP7QhFlFX4epFokMQ7NpkSb43FBmyAQSYXrg+Gz?=
 =?us-ascii?Q?RJuSVB+zeSDbOwoPcOtHNsuhLE3YDySyZajzwMm1nF9mPyLIdzlGkZ7y797S?=
 =?us-ascii?Q?27DLzSChZXMb9z5EF913kvhu0XcmGJb2Yljls5hetiC2/bylc94kwQMkG1WB?=
 =?us-ascii?Q?RhFfUj15mdixfOTz67u8IX0xhJ5rrp1WnQ4rWfXMhjFvRg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 572e3394-bc00-446b-73f5-08d98e9f5a13
X-MS-Exchange-CrossTenant-AuthSource: BYAPR15MB4136.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2021 23:15:31.3103
 (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: oinaLRDLRRYTjfKdM6e73k3LYOhm9pN8YE71Po5oOr6chXNQPtM5hkAV6j7wdtuP
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR15MB3555
X-OriginatorOrg: fb.com
X-Proofpoint-ORIG-GUID: LVddqxF0aKtttWtdhrLvC7anDQN37YcM
X-Proofpoint-GUID: LVddqxF0aKtttWtdhrLvC7anDQN37YcM
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475
 definitions=2021-10-13_09,2021-10-13_02,2020-04-07_01
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 impostorscore=0
 malwarescore=0 mlxscore=0 suspectscore=0 adultscore=0 priorityscore=1501
 phishscore=0 spamscore=0 mlxlogscore=846 lowpriorityscore=0 clxscore=1015
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109230001 definitions=main-2110130141
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Oct 13, 2021 at 03:26:11PM -0700, Shakeel Butt wrote:
> On Wed, Oct 13, 2021 at 3:03 PM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Wed, Oct 13, 2021 at 12:43:38PM -0700, Shakeel Butt wrote:
> > > The commit 5c1f4e690eec ("mm/vmalloc: switch to bulk allocator in
> > > __vmalloc_area_node()") switched to bulk page allocator for order 0
> > > allocation backing vmalloc. However bulk page allocator does not support
> > > __GFP_ACCOUNT allocations and there are several users of
> > > kvmalloc(__GFP_ACCOUNT).
> > >
> > > For now make __GFP_ACCOUNT allocations bypass bulk page allocator. In
> > > future if there is workload that can be significantly improved with the
> > > bulk page allocator with __GFP_ACCCOUNT support, we can revisit the
> > > decision.
> > >
> > > Fixes: 5c1f4e690eec ("mm/vmalloc: switch to bulk allocator in __vmalloc_area_node()")
> > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > ---
> > >  mm/page_alloc.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 668edb16446a..b3acad4615d3 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -5215,6 +5215,10 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> > >       unsigned int alloc_flags = ALLOC_WMARK_LOW;
> > >       int nr_populated = 0, nr_account = 0;
> > >
> > > +     /* Bulk allocator does not support memcg accounting. */
> > > +     if (unlikely(gfp & __GFP_ACCOUNT))
> > > +             goto out;
> > > +
> >
> > Isn't it a bit too aggressive?
> >
> > How about
> >     if (WARN_ON_ONCE(gfp & __GFP_ACCOUNT))
> 
> We actually know that kvmalloc(__GFP_ACCOUNT) users exist and can
> trigger bulk page allocator through vmalloc, so I don't think the
> warning would be any helpful.
> 
> >        gfp &= ~__GFP_ACCOUNT;
> 
> Bulk allocator is best effort, so callers have adequate fallbacks.
> Transparently disabling accounting would be unexpected.

I see...

Shouldn't we then move this check to an upper level?

E.g.:

if (!(gfp & __GFP_ACCOUNT))
   call_into_bulk_allocator();
else
   call_into_per_page_allocator();

Not a big deal, I'm just worried about potential silent memory allocation
failures.

Thanks!