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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 452EEC282DE for ; Thu, 13 Mar 2025 06:35:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EFF9E280002; Thu, 13 Mar 2025 02:35:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EAFC6280001; Thu, 13 Mar 2025 02:35:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D7782280002; Thu, 13 Mar 2025 02:35:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id BDD24280001 for ; Thu, 13 Mar 2025 02:35:20 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 2C63B161B9E for ; Thu, 13 Mar 2025 06:35:22 +0000 (UTC) X-FDA: 83215566084.29.B644B0D Received: from invmail4.hynix.com (exvmail4.skhynix.com [166.125.252.92]) by imf08.hostedemail.com (Postfix) with ESMTP id 3CD0A160002 for ; Thu, 13 Mar 2025 06:35:19 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf08.hostedemail.com: domain of rakie.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=rakie.kim@sk.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741847720; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ju/I7B/o5SmvKWOw0gOBfBHi+o+c3jj8w7OxVssez+A=; b=Qyx1T+NSUOmU1ES7MA2Hb1K4Enp1e9v4Wvo4vBsbO6+CNMt1l3yMqdmcnt+7txGKko7aLd SfNykXMBTlqvlDMEVLHjROMzxuDEIPZkqpFWzh5DEqkg9boInZZBru4PEGo7DCghyXTf96 opuB9GPFKM9Rl4oOTS0ax3cGsVoEj30= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741847720; a=rsa-sha256; cv=none; b=CkoCuIPduS+lVHkf9b4STwzbSIv3GMWywB62hlztSCQYK02lSBESLJOhXTvlMYrokYULtz INyk26cXYyrEXTcFi9nyYOffaAgeosLnSMbm93J6DsYRyixkOMrPdFkZI0CMVsYCPCnXiC 9QE34luEhu12ITeJQB2m4E4PCWl8jvU= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf08.hostedemail.com: domain of rakie.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=rakie.kim@sk.com X-AuditID: a67dfc5b-681ff7000002311f-23-67d27ca66d1f From: Rakie Kim To: Joshua Hahn Cc: gourry@gourry.net, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org, dan.j.williams@intel.com, ying.huang@linux.alibaba.com, kernel_team@skhynix.com, honggyu.kim@sk.com, yunjeong.mun@sk.com, Rakie Kim Subject: Re: [PATCH v2 4/4] mm/mempolicy: Fix duplicate node addition in sysfs for weighted interleave Date: Thu, 13 Mar 2025 15:34:37 +0900 Message-ID: <20250313063511.714-1-rakie.kim@sk.com> X-Mailer: git-send-email 2.48.1.windows.1 In-Reply-To: <20250312150440.2301373-1-joshua.hahnjy@gmail.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrLLMWRmVeSWpSXmKPExsXC9ZZnoe7ymkvpBlNPM1nMWb+GzWL61AuM Fj/vHme3OL51HrvF+VmnWCwu75rDZnFvzX9Wi9VrMhw4PHbOusvu0d12md1j8Z6XTB6bPk1i 9zgx4zeLx86Hlh6fN8kFsEdx2aSk5mSWpRbp2yVwZRz88Yit4L5cxaL5t5gbGN+KdzFyckgI mEhcvzaNHcb+vWMXUxcjBwebgJLEsb0xIGERAU2JE62TmLsYuTiYBVYySbQtOM0MkhAWSJM4 fuw4K4jNIqAq8fdgNwuIzStgLLH+ZRPUTE2Jhkv3mEBsTgF7iU29/YwgtpAAj8SrDfsZIeoF JU7OfALWyywgL9G8dTbYMgmBy2wSUy4+Y4QYJClxcMUNlgmM/LOQ9MxC0rOAkWkVo1BmXllu YmaOiV5GZV5mhV5yfu4mRmAgL6v9E72D8dOF4EOMAhyMSjy8CZMvpguxJpYVV+YeYpTgYFYS 4V1teyFdiDclsbIqtSg/vqg0J7X4EKM0B4uSOK/Rt/IUIYH0xJLU7NTUgtQimCwTB6dUA6PQ lQ9isxg8pvfzLpjO3HpI/i9XSW5ft2qO0amnOtUr9gYLTLvtcEGXn0XFqtrd1tDovBbTnoLw Nz3vcyXb+DNFgprO+TbGqMRLTzJSZWtVj5/YtlG69KTM1kbm226vHu2rdri8WVjr14NZ3ote +P04+kbr2MuJLOmfe3vXpc///HTvh1u7LJRYijMSDbWYi4oTAcfrV7FgAgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrOLMWRmVeSWpSXmKPExsXCNUNNS3dZzaV0g5uHLSzmrF/DZjF96gVG i593j7NbfH72mtni+NZ57BaH555ktTg/6xSLxeVdc9gs7q35z2px6NpzVovVazIsfm9bwebA 47Fz1l12j+62y+wei/e8ZPLY9GkSu8eJGb9ZPHY+tPT4dtvDY/GLD0wenzfJBXBGcdmkpOZk lqUW6dslcGUc/PGIreC+XMWi+beYGxjfincxcnJICJhI/N6xi6mLkYODTUBJ4tjeGJCwiICm xInWScxdjFwczAIrmSTaFpxmBkkIC6RJHD92nBXEZhFQlfh7sJsFxOYVMJZY/7KJHWKmpkTD pXtMIDangL3Ept5+RhBbSIBH4tWG/YwQ9YISJ2c+AetlFpCXaN46m3kCI88sJKlZSFILGJlW MYpk5pXlJmbmmOoVZ2dU5mVW6CXn525iBAbvsto/E3cwfrnsfohRgINRiYc3YfLFdCHWxLLi ytxDjBIczEoivKttL6QL8aYkVlalFuXHF5XmpBYfYpTmYFES5/UKT00QEkhPLEnNTk0tSC2C yTJxcEo1MC4oDJcPXe0zoYGls+ipn2B+lErJt21R26ssnuvaLMzJuGl0z7X4VEagsuafazN3 L9Z5XKvW3hEmvD3kSLOeSafbrS9zwh+wP3y5vc2B0Vp7gsD9g9v3991YZBv84vBOZsf9C9ff nthW9K7SiSNy8yuTKQ+dtM9Mnbjg4brrzzmY9tXslD7wW0uJpTgj0VCLuag4EQAHcwJdWgIA AA== X-CFilter-Loop: Reflected X-Rspam-User: X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: 3CD0A160002 X-Stat-Signature: 7ohd5n9q1q4xrpgj4gz1mdbear5hzcwj X-HE-Tag: 1741847719-156273 X-HE-Meta: U2FsdGVkX1/ohCLYSA6U9Qp7gI7MCou4Let8qB+9ymk4/AKjQqZT6ZY1F9exlealWCXAK2D1Rh0cFHJW8wnv7A/PJlCG1bKqBN0DxYPD2nW8u1NOtzN4xUOAQWaj5cSfu+OMmXz/UXyUWFhHvp4etZcs4cReSac9SY32TJT2ASvZMqi0adKZkkRv2vfFv+TqVglPhu/DyyVsT9AncH5v+xl/ORCztj0sx4WLTdwdpO2Y7OA22Nci9j+hSGZUaBx1sCR3Nvi7BCs9S1kIXEWNYHMofvynLI2A6Ry/0aHIB7GCEusDU13c/elqqJqUG5OZ7RjQf0PR4C34I2eaQqfe4t8oSKfjZ+yU3/1GjOz8Mxd8RRMBVJqKrS5dEtG+5L+yiplj9XLT5qJG8PYfD2+lY4OqYQb1p5Sf7UqOC2OJEjyO8zrkzjO0bGi2s+peAXIUU9FbaeTIrNRtfoIm6ehU/GKjIPN1rOD351J5tPMSX3luyTK3VCnJgpzaxtFPGPmEkn/LbVWZV8dbPNqqeAXNAKXBGV7pDiSlcmo2cC/OgIa2fjY+gZguUBdmajMKhNfN/tJyB35pt4xYdbBg7SSx1bjbaRodDAOEwm3BvzhB+VojrHhj+SxNlj4ZNTMwebRPvjPWZP71sNVlEKXU91Y8wAFA1wEv/R1MYFGrQHQfQaY60t04K256+/kLOGG6VIAYCwEiEQpOvrmIJdhzROZhO9WPstPWBpA+5tDstry6o2VVubSCdJ9N/w7qbjDp7YCuxA8LzmNlMh5SC/ZyILnFQOV6tCWojwu/3dlqAWzVzKqVMEZMg/SvoLhsONNnR45WooYrHEmvecj3U2tg2rs8G3qRg/4TIYDd8XtaNVc1/oSYbgD423wn41MyQiANQDyK8sWLsqStFgRtDvM7eBd8+optJSaG9KZTH+YgUp7aFBROh409rV2UJktu6O6+AwoDeKuchePKXLjwMRL69BO F/NyZ05j tFZJMTKEhYjXOcGFESc/fkEpXew5fkc+0jE0M7zqHPBlFEbD3nuf7UDp5tEJ8oKrQmBcUOl2dixur0oAOTqOCxDcniA== 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: List-Subscribe: List-Unsubscribe: On Wed, 12 Mar 2025 08:04:39 -0700 Joshua Hahn wrote: Hi Joshua Thank you for your response regarding this patch. > Hi Rakie, thank your new revision! > > I think this new ordering of the series makes more sense, since the bug exists > even before your patch is applied! IMHO, it might also make sense to take > patch 1 out of this series, and send it separately (and make patches 2-4 > their own series). > > I have a nit and a few thoughts about this patch and the way we order locking > and allocating: > > > static void sysfs_wi_release(struct kobject *wi_kobj) > > @@ -3464,35 +3470,54 @@ static const struct kobj_type wi_ktype = { > > > > static int sysfs_wi_node_add(int nid) > > { > > - struct iw_node_attr *node_attr; > > + int ret = 0; > > char *name; > > > > - node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL); > > - if (!node_attr) > > - return -ENOMEM; > > + if (nid < 0 || nid >= nr_node_ids) { > > + pr_err("Invalid node id: %d\n", nid); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + mutex_lock(&ngrp->kobj_lock); > > + if (!ngrp->nattrs[nid]) { > > + ngrp->nattrs[nid] = kzalloc(sizeof(struct iw_node_attr), GFP_KERNEL); > > I am unsure if kzallocing with the mutex_lock held is best practice. Even though > two threads won't reach this place simultaneously since *most* calls to this > function are sequential, we should try to keep the code safe since future > patches might overlook this, and later make non-sequential calls : -) > > It also doesn't seem wise to directly assign the result of an allocation > without checking for its success (as I explain below). > > IMHO it is best to allocate first, then acquire the lock and check for > existence, then assign with the lock still held. We can also reduce this code > section into a single if statement for clarity (but if you think it looks > cleaner with the if-else, please keep it!) > > struct iw_node_attr *node_attr; > > ... > > node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL); > if (!node_attr) { > ret = -ENOMEM; > goto out; > } > > mutex_lock(&ngrp->kobj_lock); > if (ngrp->nattrs[nid]) { > mutex_unlock(&ngrp->kobj_lock); > kfree(node_attr); > pr_info("Node [%d] already exists\n"); > goto out; > } > ngrp->attrs[nid] = node_attr; > mutex_unlock(&ngrp->kobj_lock): > Your suggestion makes sense, and I will update this part accordingly to reflect your feedback. > > > + } else { > > + mutex_unlock(&ngrp->kobj_lock); > > + pr_info("Node [%d] is already existed\n", nid); > > NIT: To keep consistency with other parts of the kernel, maybe this can be > rephrased to "Node [%d] already exists\n" I also agree that modifying the wording would improve clarity. I will make the necessary adjustments in the next version. > > > + goto out; > > + } > > + mutex_unlock(&ngrp->kobj_lock); > > + > > + if (!ngrp->nattrs[nid]) { > > + ret = -ENOMEM; > > + goto out; > > + } > > If we make the changes above, we don't have to check for allocation success > *after* already having locked & unlocked and making the unnecessary assignment. > > > > > name = kasprintf(GFP_KERNEL, "node%d", nid); > > if (!name) { > > - kfree(node_attr); > > - return -ENOMEM; > > + kfree(ngrp->nattrs[nid]); > > + ret = -ENOMEM; > > + goto out; > > } > > For the same reasons above, I think it makes sense to make this allocation > together with the allocation of node_attr above, and free / return -ENOMEM > as early as possible if we can. > > [...snip...] > > Thank you again for this patch! Please let me know what you think : -) > Have a great day! > Joshua Thank you for your detailed and thoughtful review. I will incorporate your suggestions and update the next version accordingly. > > Sent using hkml (https://github.com/sjp38/hackermail) >