From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EA5F7371CE0 for ; Mon, 8 Jun 2026 13:30:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780925460; cv=none; b=A1yuIZtwm22/lxNp2Ur+4Oixa5bhWcry1KIs03DkXq8zeew328V2p/Q58/9V/6nkDnj0qH6JVmw4jSbqafr1VGcRUdNeIYobYXaZ2Dax9lHTOAivRTDQuV+bnYZ2371QckAkjusbxP5XCyXlZdrb20qPwMTMZeYKNtuo8wEpGMQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780925460; c=relaxed/simple; bh=GwCuKLbifrVUjBrnmgsr5H16eGrctid/Y/EhKUyp1Rg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tY9NpH9hRL5FGNcx+qEmpp62CgQ1vLulCRrn9w3DHBEUtzchTWLbZ4n9kxUCfOYU/fHIH0k6TyHkiCQjHcnPujcNKwMN8FT3UETvU56ia6jRgwiHX7BWlu4qjEmz+xccL4tLMa6IUuNmXIP1tDuembqRf2ZWT1zc2t044pUumNY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=PZv3Ffsu; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=TU1nMlST; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="PZv3Ffsu"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="TU1nMlST" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1780925458; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=iewDQSsA13YrnQ4UInBbfMz/SjkA5BPlaeZcu83emSo=; b=PZv3FfsuVNPXtzwkZOfoRwyBaf5CWkLpufzIO68Q+YEM5fh7k/7AvFNECKQi9TH5jl2sc4 //zSK+nosD9FTD4IamFBJMEAEQsWStZciW6q2lUPy3X8Lqzsj3fH0UwXchoQNZOGXZ87gY FGtnkSg4EMrYFMomaOjXLsacrvPjb1c= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-682-LlLbGOFmNDG7-rIVUdEbzQ-1; Mon, 08 Jun 2026 09:30:55 -0400 X-MC-Unique: LlLbGOFmNDG7-rIVUdEbzQ-1 X-Mimecast-MFC-AGG-ID: LlLbGOFmNDG7-rIVUdEbzQ_1780925454 Received: by mail-qk1-f198.google.com with SMTP id af79cd13be357-915c364ae3bso339533185a.0 for ; Mon, 08 Jun 2026 06:30:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1780925454; x=1781530254; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=iewDQSsA13YrnQ4UInBbfMz/SjkA5BPlaeZcu83emSo=; b=TU1nMlSTCfYGSHCu7jPMylA75f+JztQO0uyr2MtpK3dWryj6+uCM4j2bx+rY63NnoH sgiX/m0SyTDJ0YORzCUYLeHqmWtdsIV7Yw9StpSe+qvCKmTf3b1aF8eFjKTVVuhPqFp4 4thsDEQSNqMGLFP8XP1AYCqlw5co28t6k9BVydnyVgWHb956yc6bOOXg55836bWvh6PP X9P8Vc0q1Y8+nxAEN6mWwaJ9eWc+ugBUqbLdHghW8qj4Z/eAhnbu6Nov2Ltuj2Lfw1cT ZsMrc8+NEL3Ff4YyGGo+P4YT7JO8dmo+fY7t1QzJA18fGOOkFIUNIxTQwLFAnjB8CA32 o+LQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780925454; x=1781530254; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=iewDQSsA13YrnQ4UInBbfMz/SjkA5BPlaeZcu83emSo=; b=qGQy5kgiZeBDUayVZrqIwYXTnqEYOMz/KpKkjkQLIc/7w4fFuLDS8gyjLRitm2SM3h 5UTI4gdXf+tk8GaA7GmT/Gk8pHG3C2oGrSvMDe38CQrU+2Y1ugI/C9Ay8YGdmLlUgjUO dsKxqdIpdWClktLv9fOcVMJBfADkzEIbKWy2W3XY3cqVTQIs+JSw5k5i9DT798U7qEEW ItSHrEYHWxOREjOJMYvrR31WIRrnckjq2/4cCTT/pxZNUD/si9w79cKx8uHTaeN9sdfH rjFGj2sUhrGQPiQ/lUvmHIJPpGogY6g+wqHLa1CEx8AK2aUquKAauIjfz8NoWqItPEQx WDKQ== X-Forwarded-Encrypted: i=1; AFNElJ8b9V24Ibu5poCGlQEihh91xI+abvqV9CJO/jhZNjnDnZjcDNnqxZg3DUINyvp0tXXFHngBlyVdJo669ho=@vger.kernel.org X-Gm-Message-State: AOJu0YyRPPvhMXtOEjtpWB+zrnQUpRR3GZnHx2htX3Ty8Yme+ebk6g2c 2rYedij5eFKCViIv+d6xpzlOAhyVnwxdo5R5y8tw777IEaaHXFTE3LvvLJbR8A9KOvJeTKzxAqj /bPWDzv6BhKs5fVh+pi5uTlDgkHCmjIt2/EEoRyJGaM/IqzIT55nVIHTup0dVIfNhdQ== X-Gm-Gg: Acq92OHbeqNUUYpKuR/aBWC6YZLXGrbNZRIIaD7sGQtMPDvclSq6kjuYEJFeqbbw+MR O7N2wGjwxciFj+0cpe5QAmBDbZNRtp2vkDg32brUqPaBi5IYhpJazuyFQaDDpmxLXnja6x4Q4Ci 8OjkxS2e+7kIv8KhGJ6thzC7sWXMOaXxTWlozZvCNbUnX1HA44WF279xw3eC9y0mvoVwNEpUFMv Hqzqyk+Z6LGEHEd9QWpV1eaAHbrENzQ+ZMMRJpGIpTNgCDxGNLNXluntVupxKLKQMi9edtrhPpU 9DpwUiYkInav6vMFLVx0zBBX/iMQH+/QieQqdl2soEYsFn1L68jZ2qtMi+sYHBw8asY0qX08fMl pfzgnk/iyZ85pfvHkuzfp9iG0Ufn5Jmc08vMAEvh7xOu7vCsAJG0oDo8gCr9JLaqjSJya9RKgp3 2M X-Received: by 2002:a05:620a:4546:b0:915:cf88:1e3b with SMTP id af79cd13be357-915cf882096mr770202085a.47.1780925453592; Mon, 08 Jun 2026 06:30:53 -0700 (PDT) X-Received: by 2002:a05:620a:4546:b0:915:cf88:1e3b with SMTP id af79cd13be357-915cf882096mr769780685a.47.1780925431588; Mon, 08 Jun 2026 06:30:31 -0700 (PDT) Received: from localhost (pool-100-17-17-231.bstnma.fios.verizon.net. [100.17.17.231]) by smtp.gmail.com with ESMTPSA id af79cd13be357-9158a238f8esm1762107885a.15.2026.06.08.06.30.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jun 2026 06:30:30 -0700 (PDT) Date: Mon, 8 Jun 2026 09:30:29 -0400 From: Eric Chanudet To: Natalie Vock Cc: Maarten Lankhorst , Maxime Ripard , Tejun Heo , Johannes Weiner , Michal =?utf-8?Q?Koutn=C3=BD?= , cgroups@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Albert Esteve Subject: Re: [PATCH] cgroup/dmem: accept only one region per limit write Message-ID: References: <20260605-cgroup-dmem-write-single-region-v1-1-9137f296579c@redhat.com> <271b1c16-3c3c-4a1e-b09e-c4361c63814c@gmx.de> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <271b1c16-3c3c-4a1e-b09e-c4361c63814c@gmx.de> On Sat, Jun 06, 2026 at 06:31:53PM +0200, Natalie Vock wrote: > On 6/6/26 00:44, Eric Chanudet wrote: > > Accept only one "region value" pair entry for the dmem.max, dmem.min, > > dmem.low files.> > > This changes the UAPI that otherwise accepted multiple lines for setting > > multiple entries in one write. No existing user is known to rely on > > writing multiple regions in a single write. > > Ugh, shoot. > > For dmem.low specifically, there already are some userspace thingies > floating around that may write more than one region/value pairs. > > These thingies all depend on that one patchset for dmemcg protection that I > should really get around to merging[1]. Since the userspace utilities depend > on not-yet-merged patches, they sort of have to expect stuff changing under > their belts, so I wouldn't really consider those users a blocker by > necessity. > > As I see it, we could go down one of two paths: > 1. We go ahead with the patch as proposed, and I make sure that the users I > know of adapt. Could be a bit icky wrt. "do not break userspace" rules, but > since the already use non-merged UAPIs in one place, you can argue that > these users kind of have to expect breakage. > 2. We use the old handling allowing multiple lines for dmem.min and dmem.low > only. This preserves compatibility but uglifies the code by quite a bit. > > All things considered, I think I personally would prefer going with 1. and > taking the patch as proposed and just having one codepath handling every > limit file. Just highlighting this so we don't do it on accident. > > [1] https://patchwork.freedesktop.org/series/163183/ > > Some more review comments inline. > > > > > Processing multiple regions in dmemcg_limit_write() could quietly change > > first limits before failing on a later one and returning an error to the > > writer, with no indication some changes occurred. > > > > Signed-off-by: Eric Chanudet > > --- > > Follow up from discussions on a previous thread[1]. > > If Albert's series[2] lands, I can cleanup and prepare some kunits for > > these as well. > > [1] https://lore.kernel.org/all/158bc103-7f99-4df4-8d3b-2da9b04ac0ed@lankhorst.se/ > > [2] https://lore.kernel.org/all/20260519-kunit_cgroups-v4-1-f6c2f498fae4@redhat.com/ > > --- > > kernel/cgroup/dmem.c | 70 +++++++++++++++++++--------------------------------- > > 1 file changed, 26 insertions(+), 44 deletions(-) > > > > diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c > > index 6430c7ce1e0372f59f1313163fb7630ce49ac1ef..113ee88e276296bccb4def546adf5cc175d7f0be 100644 > > --- a/kernel/cgroup/dmem.c > > +++ b/kernel/cgroup/dmem.c > > @@ -734,57 +734,39 @@ static ssize_t dmemcg_limit_write(struct kernfs_open_file *of, > > void (*apply)(struct dmem_cgroup_pool_state *, u64)) > > { > > struct dmemcg_state *dmemcs = css_to_dmemcs(of_css(of)); > > - int err = 0; > > - > > - while (buf && !err) { > > - struct dmem_cgroup_pool_state *pool = NULL; > > - char *options, *region_name; > > - struct dmem_cgroup_region *region; > > - u64 new_limit; > > - > > - options = buf; > > - buf = strchr(buf, '\n'); > > - if (buf) > > - *buf++ = '\0'; > > - > > - options = strstrip(options); > > - > > - /* eat empty lines */ > > - if (!options[0]) > > - continue; > > - > > - region_name = strsep(&options, " \t"); > > - if (!region_name[0]) > > - continue; > > - > > - if (!options || !*options) > > - return -EINVAL; > > + struct dmem_cgroup_pool_state *pool; > > + struct dmem_cgroup_region *region; > > + char *region_name; > > + u64 new_limit; > > + int err; > > - rcu_read_lock(); > > - region = dmemcg_get_region_by_name(region_name); > > - rcu_read_unlock(); > > + buf = strstrip(buf); > > + region_name = strsep(&buf, " \t"); > > + if (!region_name[0] || !buf) > > If buf is NULL, isn't strsep(&buf, ...) also NULL? region_name[0] would > therefore be a NULL pointer deref. Flipping the order of the logical or > should be enough to prevent this. > I can do a v2 with that today. I added it if there are no delimiter found (e.g, if only the region name is passed and strstrip() ate any trailing space). Although, buf can't be NULL in the write callback iirc, it's either pre-allocated or kmalloc'ed. > > + return -EINVAL; > > - if (!region) > > - return -EINVAL; > > + rcu_read_lock(); > > + region = dmemcg_get_region_by_name(region_name); > > + rcu_read_unlock(); > > + if (!region) > > + return -EINVAL; > > - err = dmemcg_parse_limit(options, &new_limit); > > - if (err < 0) > > - goto out_put; > > + buf = strstrip(buf); > > Do we start allowing extra spaces between region and limit as well? Would > also be fine by me, it doesn't break anything, just highlighting that it's a > change in behavior. Should perhaps be documented in the commit message, too. > > Also, you should be able to use skip_spaces() here for an equivalent result. > I'm not strongly opinionated on either way, but using skip_spaces() > indicates more clearly that this can only remove spaces at the start. Same I can add to v2, I failed to notice it wasn't allowed in the original logic. Thank you for the review. Best, > > Best, > Natalie > > > + err = dmemcg_parse_limit(buf, &new_limit); > > + if (err < 0) > > + goto out_put; > > - pool = get_cg_pool_unlocked(dmemcs, region); > > - if (IS_ERR(pool)) { > > - err = PTR_ERR(pool); > > - goto out_put; > > - } > > + pool = get_cg_pool_unlocked(dmemcs, region); > > + if (IS_ERR(pool)) { > > + err = PTR_ERR(pool); > > + goto out_put; > > + } > > - /* And commit */ > > - apply(pool, new_limit); > > - dmemcg_pool_put(pool); > > + apply(pool, new_limit); > > + dmemcg_pool_put(pool); > > out_put: > > - kref_put(®ion->ref, dmemcg_free_region); > > - } > > - > > + kref_put(®ion->ref, dmemcg_free_region); > > return err ?: nbytes; > > } > > > > --- > > base-commit: 640c57d6ca1346a1c2363a3f473b405af979e046 > > change-id: 20260605-cgroup-dmem-write-single-region-9bf05b6d995d > > > > Best regards, > -- Eric Chanudet