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 13A13C28B2F for ; Wed, 12 Mar 2025 15:04:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F2BE4280004; Wed, 12 Mar 2025 11:04:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EDB2D280001; Wed, 12 Mar 2025 11:04:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D567C280004; Wed, 12 Mar 2025 11:04:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id B9063280001 for ; Wed, 12 Mar 2025 11:04:44 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 07C5D121284 for ; Wed, 12 Mar 2025 15:04:46 +0000 (UTC) X-FDA: 83213220972.19.BD96AD9 Received: from mail-yb1-f178.google.com (mail-yb1-f178.google.com [209.85.219.178]) by imf09.hostedemail.com (Postfix) with ESMTP id 0160114001B for ; Wed, 12 Mar 2025 15:04:43 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=hNxgT6bz; spf=pass (imf09.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.219.178 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741791884; 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:dkim-signature; bh=CWJQHvu5mtEWy0MPTSIyoI0L0d27oEw9ieErduQwILE=; b=mrH4in47GQGww/kQRJ2UAtKWqPRBceUqDbC8P/B+RF7utxz8HfrotketBvS5UUlhroLyp5 ssx+t3THqxJZAnlFMa6VXgxltIyYgA14Xx7RHJADprYL3yLpyx6opY2ZYT74GdgvV+c6pJ Xdrkl1r6Oi8rsXNA+BnL04H3Q/+1W5U= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741791884; a=rsa-sha256; cv=none; b=wnQ7BOkKEgl82R4/HVs5bWz7U6D739qZFrhnexIhwX3KqFxXS/mBwU+MPjDDM5ucFjUkrm tllZfatUOOjYj5Pd70KY0kBbkImz71gJzh2WBVYLuHCJIlwwS8uSeNszLu0UKQHepWUA+3 JYPGdpqtcdBCur6uZLKFLh5S0IY7krY= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=hNxgT6bz; spf=pass (imf09.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.219.178 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-yb1-f178.google.com with SMTP id 3f1490d57ef6-e549be93d5eso6114382276.1 for ; Wed, 12 Mar 2025 08:04:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741791883; x=1742396683; darn=kvack.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=CWJQHvu5mtEWy0MPTSIyoI0L0d27oEw9ieErduQwILE=; b=hNxgT6bzfVYtww4/y02EKSf3fHzdbPuiKFP50ycAns+kZbi9LI/K7Zcq2q7ZucGpwC zpVWq1TMmPI2sqKPLdqcF0ET0gh+1iZflOsBhPtO8p8OgakkO3ynykp+gaAKAZpqUUD2 wN5DTLlzstRr7DZqyqROlkFMFBtTy/6f18E1nxo6KOq2O43/a6P27FB2PA1fiU+bIWEe BRHaXVKsAhT838ha5vozg1fQjrWzBHxROj7GQYEXZCthHeuR4GICC8rCkaWXmVAelBw0 3whvgSMyVD4uO/Gvlzdj+n6pIiXkGP60hQ750dv1+/DMTAtfEg+w882eP2SXpQICpOnM esMA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741791883; x=1742396683; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=CWJQHvu5mtEWy0MPTSIyoI0L0d27oEw9ieErduQwILE=; b=avNs/MNZbIztqHft6thfKt5zK7Qnq2t+dc63gvToSr6EIyiXmoOpJFO9T2vl3+bImr pSXgfCanQAApLf1632/zpTL2r2HL3oWSt7TVCWT9uhuAbsCtyQuq5XcR65a40DzNXGyP 66QkktjKDd3JscA0H7rjgIara5sYi6Q1fWoWU9Gg9rrL4yze8LCEZw9JHqZFp+D4V3wv UH5e8BhINY28qOqRieaMLVf373PvfTtpCnPv5q5KOdNTznEwcgtYud1qzy4zs9LAdg1q LNjm4u6uVPvsSTCzpQ30oB+VjS576mqqOkwRF48JWYXpIPuq/qBiuKJfZikYfPGNEi63 t5fA== X-Forwarded-Encrypted: i=1; AJvYcCUzQ1Kb+jrx8bj98gkOecmCNJUzKrRY6SIOjorVKOOXDP++SO36Ssq/r3wNjsJNOjpZzunTbFRIoQ==@kvack.org X-Gm-Message-State: AOJu0YyuS1k68SVtT0V+OQT5bJvqSS8RzNDl1ntUg257ojMzUScU+j32 1siuh0DYaOsD3kRNz3etgW9aBMiNV/bCabg3qxK1YxgAP2IoVqlo X-Gm-Gg: ASbGnct2O5OwuGICxKOq2UPIkGFz16AzObThwc/Mzvduz09StRyWM4MtPw8P2xHOiEc phvi1XvFkevSzvNzFQxLH7eX6pPG9ZB6nQB3Kwr95ZkDCf6FW/v+sKRnPpHsmIUF+9mBA9qHHUu qHqIbFam4YKDG5Fz7OfgzjupEy61yULxstrsI/EkrJvKUGnjCgaBLU6XHP5jttkmLYKhs4rCfzr lZ3xQ2e+GhSuqA0qWbsk1qS4x1ZdMAVK4YL3UT/fRqyVvdeHrwWqxRYhyqrnnxlXPS5rplcTuWT FplK8T3JFW1cY1iXhxhenzkD3DY399oXNXRIIYndWl57 X-Google-Smtp-Source: AGHT+IF9U6SlWjVO5e8FD64REsmZ+AmpBbsbhviSf5KKYvGmv6Wzl2qnU8103UftabS8nl6n3qlxfg== X-Received: by 2002:a05:690c:4b83:b0:6fd:2b7d:9a4e with SMTP id 00721157ae682-6febf30734bmr331638857b3.18.1741791882857; Wed, 12 Mar 2025 08:04:42 -0700 (PDT) Received: from localhost ([2a03:2880:25ff:71::]) by smtp.gmail.com with ESMTPSA id 00721157ae682-6feb2a1bf6esm31766217b3.19.2025.03.12.08.04.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Mar 2025 08:04:42 -0700 (PDT) From: Joshua Hahn To: Rakie Kim Cc: gourry@gourry.net, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org, joshua.hahnjy@gmail.com, dan.j.williams@intel.com, ying.huang@linux.alibaba.com, kernel_team@skhynix.com, honggyu.kim@sk.com, yunjeong.mun@sk.com Subject: Re: [PATCH v2 4/4] mm/mempolicy: Fix duplicate node addition in sysfs for weighted interleave Date: Wed, 12 Mar 2025 08:04:39 -0700 Message-ID: <20250312150440.2301373-1-joshua.hahnjy@gmail.com> X-Mailer: git-send-email 2.47.1 In-Reply-To: <20250312075628.648-4-rakie.kim@sk.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 0160114001B X-Stat-Signature: kug93ph8ez4yjwpg6ty7gsp1a8boszss X-HE-Tag: 1741791883-724000 X-HE-Meta: U2FsdGVkX1+VflJX0Ueuo3WntSF8DO+c1DNv2bweL0GLW/NowT5RRruDbP4Bh8/ybXcIAhLtIqwBwVQC2SbCyPTEZeimA09GXDxsYzcRV2EjiPcQMQN5Z27zobXi5VvBW2knOJKU1FAO2aSppK2+LjFuV009WyuTx9ct3OTa1WkDSqenspWpMhmczIDwdGqIfap3Ond1I3cVAagqHpRZRtazXcyP6+2BLIbHJZCg67j7oEMBKVh/opLafyjG4v97zgkaB//0EsQlLRjWhrVRtBdlAlVU8N4MxtB7ijU0WyIXJriRJQL0UcGd34tGKkfWziO3mcK1No7KZtgwv8PL/USX80qBYrFJx6kzqDPs5cdSRcEerNZpBF/EJzliTWbDUGtmEscT9vRzDUHsMETXWNLQFzk+D7+V1QhG1FoC2hgFPNlWTEs9csY9M/ND2qbBpmFHV9y0VTF/kOQXE522k0jQ7Gh9TBxoLvK33ZLNVfUQTze7PBzraQ+MU/HzFzdrQGLPRs6xlrJ8AZG73paMKXcO77Rsb8XBgG4tkgcLY7NQQTajvujrgWWGqol5QNlrBNb2415/6QxO4ryGlFgHnA/7Q0QPlvajO5BuCZIl0UiTXYtniXqiQJrvfCH0ZDOVPc1sNkiojx0/1FlkGsg7/Q5xRO2zOrgo2duFztFXdNHlKzy+gVER/xMzOifnjm6opF0tXAmfOyFYTRCRsZuvm9KNubQFURG2zMib/InPTHv3QpY71X2QkYItJzE/ewRT/gBQvmJoQFZn0ZcA/oRHj6FaJ8qzeb8sbTVGLiCpFfTBEn1tzhRqEuGd8BeenCwgUJDkBug5xLHOW+NlWqFDyFLOSGat5PUrWcnXevz6b2bzTOB4A0N4P62sNH8+pP9nD2UGAQGsFGWyIiBq+gLA2H0hLmpQnqpLJ59Oi7GgXChfn98NL127PJEUTM0tYMGtKOcpP2YYj0foWaT+RGi qiQywGBB RpCvbI8dqAuHHn0RzX9aIPFtnbWI19g59iXi6UacWm9cxi07emYYiahU1KAYmvICEeqQ0gfP36usc59Nsf0TL4U9KKBBbrQmxLJBz9f4lxtoPFX5HTSnj51OHztvS1PkIL5eBwiep//UB9vl5iyEJHpUz+UVh8cv3cJvPE0j5rx2HuYWw67XBnjufJXI4mNqxfEZtjlgunsS4nfoWBBZ34rl4Xp+I5i+8uWNpU7B9zvBlBRTOrGPHIYM1de1mtmRGr5GIWF5S4GrLyYtUl19e0HdlxH0YVxvcJDSGJ7EFXiU00+Vx9vi1GnR+4tU00fLJQMbfresBkFikJL7BTb8m4n3z80Ln48TCrIAdNYioeRKSZ9n8lCStR5EMh3/nQf6oV2OPrwJ/7+unVEIdaRJg7Gis7dSxSslbxDV8L/1SxB01NG7IIqNE+VBiTOebdR1x00s3E238QlAPvm4fbfSGtKLQvfEdLrYgo13dRPCAgyj/9bhmgTnh6ovv0r6opZVvKq0w1G6ThYNNLFOhG9g0LrNoTa1lwPiHr+cLISpZFDfH5LjOr32O9TLLIfE7LuZKd0SK X-Bogosity: Ham, tests=bogofilter, spamicity=0.000002, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: 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): > + } 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" > + 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 Sent using hkml (https://github.com/sjp38/hackermail)