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 DC08AC3ABC3 for ; Mon, 12 May 2025 14:14:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1398C6B0145; Mon, 12 May 2025 10:14:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0E9136B0146; Mon, 12 May 2025 10:14:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ECB546B0147; Mon, 12 May 2025 10:14:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id D1AD56B0145 for ; Mon, 12 May 2025 10:14:16 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id AC6691D0C86 for ; Mon, 12 May 2025 14:14:17 +0000 (UTC) X-FDA: 83434450554.21.C6546B2 Received: from mail-yb1-f172.google.com (mail-yb1-f172.google.com [209.85.219.172]) by imf07.hostedemail.com (Postfix) with ESMTP id CEC5F40005 for ; Mon, 12 May 2025 14:14:15 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ODf3Gkkw; spf=pass (imf07.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.219.172 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1747059255; a=rsa-sha256; cv=none; b=Lqmb0Lo5tgGws7LzSJM1ngwjI2BQEA1znyR5mbSMNk5rOItaZQxtZHIPfKdjljDG+AP4C8 vl7O4fkTBKI7uJzxSlXYO1ZNcExW9Y/gTfuTmT0K/Z7fhjGTKTEj9oFlihaZuobErC2sPN aVt1twGMPRBdXwIS1P3070RDnZCxzgc= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ODf3Gkkw; spf=pass (imf07.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.219.172 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=1747059255; 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=nLMJ71c2dzKPX20nCpHU00lcLyc+0oXcOaqjf4y3tFU=; b=prj8vZEH+/TGzPxYJM8Gs+33n32OzmWyC7n+ZLzobp97uS23TNyQC8V/7hCwsyci/a4xrY xuChvr+SQI1Uj9fWEjgktU4r+o0Z7dW2aXftyuLL+IJGVHACcglnTdfbYmOvIZIdoBMi3M OcBjrW/qREd3pNSJGx9tAYa2YvKEHRE= Received: by mail-yb1-f172.google.com with SMTP id 3f1490d57ef6-e7904da218aso2156486276.2 for ; Mon, 12 May 2025 07:14:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1747059255; x=1747664055; 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=nLMJ71c2dzKPX20nCpHU00lcLyc+0oXcOaqjf4y3tFU=; b=ODf3GkkwUWv7hFjSd2l9WCJ13cdkx4pVf+ror7oVLlgCmTV9GCwOxvCxGAeEGl3amz TnsGTqjLc3awM4qd08P9bdwmzF78MVCf5PwsDCgOJh6QZrdsQHbqpvsIkfAQn3Q+ZUql TTo9NvoObj57odSyvKoGSPYP1/XNjeAXxUvaRDWQhfTPTZnlZqi9U2SKvMrmW/5oXJpb /VCJGvZrBjAfP9IIDxZrnyyNR4FIGqdsgbr9UG4WVI2WsDC0yFZo5o1JJFbKXes0Fvfz /B582VxcIRVVdKXKJPyJDPrjQXbCM5kP74YPW4yy7UvV+Iyca+r2d2JzDiT/JP4XLF9B NQcw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1747059255; x=1747664055; 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=nLMJ71c2dzKPX20nCpHU00lcLyc+0oXcOaqjf4y3tFU=; b=UnPLCukoyRUVYjHpLAO7gfB6rG1RUD37A84/D7WNhw8NSEJFeYmdxuIv2P7DFp6cOK 3Hz6Xid0Cp7Mn8ryTH/vEtVBnvX2iFkkdSG3Y//+1vnFky/d5HSGatM1zx2peCe/bd9y zE9LBSeqw5YVmCeeN7Muk9SD2TwGqBIUdZFtcMk3NDhP6AT/kwhWQFEI/BvdDF4lsVsE zdw113CfigNaYYqtt2fEv5+z5Dx1Tt9AgL2kuf6qbc4W+UX9NpfC4qNXP5X4GbknLXVk epXo6vFRjCpCtNi9D/v4A68LpFa8h3yfz7FWna8NjwHemDdqy2OW66agYWhEzpe3W0Wi B+Tg== X-Forwarded-Encrypted: i=1; AJvYcCXNk3TO32lXH71Ijo0xh31oqBo+Tb++rjifhy+cUAkemog8aYfa/n+cqY+tmhgutvVuaYJaNKPa7g==@kvack.org X-Gm-Message-State: AOJu0YytZPndo9uj5kWNXigNAs93+nINTHSCcmb4ixOCKg/cidtrnsld x5ChQY1lOsxeVIf296UpabN3hyfA7PaESe+Fah/JexZuEyzB3ng5 X-Gm-Gg: ASbGnctU75AVkLvr3TZFXg5bNr821PnKIemfngOxSZ16rwTCcZvwMCtpLfAf6Rnv4uJ 7mcyYIW6dq8MI6XSk1FZdu0Z6dKfJev3TlmQSI/xbG3/QY2K5bedBEviD8o/BZqd0AI1E4szET6 iqpNmxr36YNdNyfzKZ+x2XuPrh2BrHl2E8O58QtU4381BXUSfzXz6tFxE6+Pz3YNxcRnzXvD9qA wQkLlLKen68dITrLM6yrHY8P+Gmysl2tlOSH26/GgkxYzH6P67tCDwSYYZ0poXDjOWXhQplEeVx /1RuYPo2RFzQpECmuF1FxGxeTlxdBjCNSdrq7vbFGwlDSIDgNg== X-Google-Smtp-Source: AGHT+IHjDqzivQqgRJLVu4GNkjokOZb4ORWM1bq7SBTyXEQQYnYWJZR5erICaS3dhy7pWKIZzBJsWA== X-Received: by 2002:a05:6902:845:b0:e7a:f768:43d0 with SMTP id 3f1490d57ef6-e7af768440emr560169276.8.1747059254519; Mon, 12 May 2025 07:14:14 -0700 (PDT) Received: from localhost ([2a03:2880:25ff:3::]) by smtp.gmail.com with ESMTPSA id 3f1490d57ef6-e78fd651b3asm2113880276.40.2025.05.12.07.14.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 May 2025 07:14:14 -0700 (PDT) From: Joshua Hahn To: Honggyu Kim Cc: Andrew Morton , kernel_team@skhynix.com, "Huang, Ying" , gourry@gourry.net, yunjeong.mun@sk.com, gregkh@linuxfoundation.org, rafael@kernel.org, lenb@kernel.org, dan.j.williams@intel.com, Jonathan.Cameron@huawei.com, dave.jiang@intel.com, horen.chuang@linux.dev, hannes@cmpxchg.org, osalvador@suse.de, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, linux-mm@kvack.org, kernel-team@meta.com Subject: Re: [PATCH v8] mm/mempolicy: Weighted Interleave Auto-tuning Date: Mon, 12 May 2025 07:14:12 -0700 Message-ID: <20250512141412.3792050-1-joshua.hahnjy@gmail.com> X-Mailer: git-send-email 2.47.1 In-Reply-To: <2e030edd-b3e4-4b79-9e1d-1be0c6b0d0b5@sk.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: CEC5F40005 X-Stat-Signature: enbfdrhsa5tjhyuqhdyadrakebkui787 X-HE-Tag: 1747059255-122205 X-HE-Meta: U2FsdGVkX1+8JMmFX6KveFqUJkZTPwJZs/Rr4vbO4yfSuW90VEiO5rLUKVnmKhHznPsAbVMUg/DmUb8WW3zNKqzIdWuffJEzQ+oRgy2neld5KjPvhh2l794vzijhLNwxUgrhbrj6NwPl+ofS+CfgvUZXODjhxSAUCzOdP+eMrQUKkiosT5CBdBMg/yuDeq1F6CWzZQJTEofYEPLPf2FRYAGkPmvKgxuAuxM8/jkJA0gsCsYhW/660/h968BZ4mEOt5bkg6q+AoOnO4E+8f72ej/fX0HEoc90oXfqtmbLcsqsLfph5jUkYqU7yg/AgmnNc3LA1ElI3xoQVvgjqjmGuPOz+qZBt16wSP0fTu11c0l7K8Hq8H44A9nA9C03vffwwUucVQzuKlUblB3Bq4h82HlsMjorGQGqROjxSLJakq2ZNW6kGEk0M1y7PVi7e8l3sk5H8mwsUp1EpYChHrhp9ks5tSjrRIGvHz7xB2sVqawYw/XgexPc0WX5i0aCKm9M47dZRRQa9euuljwlE2qA1FbzOK7USLUCUT1OZN/w3f7k3Ep3Y5lZb9jhVfeAzWGz2hUqIC74dGlDyGwc2V5q/dvo0bZfqkEsgrHVL/1Pr5vfbRUf7LrkkpK6tA4wGORA3TCcYfjjab2zObuHh8dgUUSZ8ND42g2oRyzfN8JZlykGqWqPes0zIdxOg+95E5ymUdTXrge5oCy18skS6VMN27HMPhEqnBE+qAY8h37H7VHXTyYSS9SAB3c3BhEu6u2AcSCewh8fKibi0ryzskPGKCWbkQyUOn6AgH7rOM7gaGsx7Og7EbJbZUR830Z4g+l6nCk28W+k85iVN09ZeQ524wdJmrQpv6t99BMy+GgZsYYWURFWFAd0nvbHefmiYFefnTfpX9UsGOsFx/UKXZ8Vl1PKscbM5gj35PPIwvO9SdhgIXyzXH0LoKPFlWYCygNdBbhWcz528BIE0yzdQ4P cWVOJ2uW M43VMZpDlHep+E/eIof3572LxoH799+XohYeydcFX2TF6BnAUkzhRaXhld+VppR/NegHdhyUgGWSsWDL69QTIi9uJ3Ad2OLQgssuboVoQZi114Aj680dKUVcvTWQ7b/0puoQHhKg7jhwzOR76BdA+u55X8GN7kgu7pTYUWUHJA0UDGxgmx44xc06XEeGjOuoqIZXyiW4B015NnYVn2Droms/kwtL4nt7hsGheHQGuo6wI/dwdV/AsaIh5V9s1rk5aw9wuEEBYVswwjvtyQXqb1XwUWj0oqjZkoUDkfsF1b/FpILPh1ResvMj5nKGWQR4pEGltEKK4l0qucLI87Kq7IK/JQ29xhCeiwPDY4QwOaYF864ihVnXE7xnmew8nnCSQ/qb/FiU50mwPXFDZdTVVcK/lZuTksaOzJ74cWIR289bg1maSzvJXwCR0DAJZw1PgniN/G843VaN6oqGZYe0bdK/PAELvUA8fsiDTaEf+egP1wl6Jhjb2UHp3RGwFra1sVa4T/bc/BnW7tArOxvmAdylpglrotKHmVyjY2ULLA2nX8VltWNh6dKmKEVZtOJQ8rxNb 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: Hi Honggyu, thank you for reviewing & testing my patch (again)! On Sun, 11 May 2025 21:56:20 +0900 Honggyu Kim wrote: > Hi Joshua, > > Thanks for the update this patch and it looks good to me. > > I've applied your v8 patch with your fixup here together, then tested it in my > server, which has 8ch of DRAM with 4ch of CXL memory in each socket. > > I can confirm that it shows decent ratio with this auto weight configuration as > follows. > > $ ls /sys/kernel/mm/mempolicy/weighted_interleave/ > auto node0 node1 node2 node3 > > $ cat /sys/kernel/mm/mempolicy/weighted_interleave/* > true > 3 > 3 > 2 > 2 > > Hi Andrew, > > I'm not sure if Joshua is better to post v9, but if you want to fold and update, > then could you please add my tags as follows when you fold this change? > > Reviewed-by: Honggyu Kim > Tested-by: Honggyu Kim > > I added the same tags in v7 but not included in v8 somehow. > https://lore.kernel.org/linux-mm/5fdd7db9-96fb-49ea-9803-977158cb0132@sk.com I must have missed including these tags. Sorry about the confusion -- hopefully we can incorporate them into v8! > Thanks, > Honggyu > > On 5/11/2025 11:58 AM, Joshua Hahn wrote: > > Hello Andrew, > > > > I was hoping to fold this fixlet in with the patch this belongs to. It includes > > some wordsmithing changes, some code simplification/cleanups, and makes sure > > that the code behavior matches that of the ABI I described. I've kept the > > original message below as well, where Ying suggested the changes present in > > this fixlet. > > > > Please let me know if this fixlet is too big, and you would rather prefer a > > new version instead. Thank you as always for your patience and support! > > Joshua > > > > diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > > index ec13382c606f..649c0e9b895c 100644 > > --- a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > > +++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > > @@ -24,7 +24,7 @@ Description: Weight configuration interface for nodeN > > empty string, ...) will return -EINVAL. > > > > Changing the weight to a valid value will automatically > > - update the system to manual mode as well. > > + switch the system to manual mode as well. > > > > What: /sys/kernel/mm/mempolicy/weighted_interleave/auto > > Date: May 2025 > > diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h > > index 3e8da8ba1146..0fe96f3ab3ef 100644 > > --- a/include/linux/mempolicy.h > > +++ b/include/linux/mempolicy.h > > @@ -57,15 +57,6 @@ struct mempolicy { > > } w; > > }; > > > > -/* > > - * A null weighted_interleave_state is interpted as having .mode = "auto", > > - * and .iw_table is interpreted as an array of 1s with length nr_node_ids. > > - */ > > -struct weighted_interleave_state { > > - bool mode_auto; > > - u8 iw_table[]; > > -}; > > - > > /* > > * Support for managing mempolicy data objects (clone, copy, destroy) > > * The default fast path of a NULL MPOL_DEFAULT policy is always inlined. > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > > index f542691b7123..0624d735a2e7 100644 > > --- a/mm/mempolicy.c > > +++ b/mm/mempolicy.c > > @@ -148,6 +148,14 @@ static struct mempolicy preferred_node_policy[MAX_NUMNODES]; > > */ > > static const int weightiness = 32; > > > > +/* > > + * A null weighted_interleave_state is interpreted as having .mode="auto", > > + * and .iw_table is interpreted as an array of 1s with length nr_node_ids. > > + */ > > +struct weighted_interleave_state { > > + bool mode_auto; > > + u8 iw_table[]; > > +}; > > static struct weighted_interleave_state __rcu *wi_state; > > static unsigned int *node_bw_table; > > > > @@ -3561,9 +3569,8 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr, > > int i; > > > > node_attr = container_of(attr, struct iw_node_attr, kobj_attr); > > - if (count == 0 || sysfs_streq(buf, "")) > > - weight = 0; > > - else if (kstrtou8(buf, 0, &weight) || weight == 0) > > + if (count == 0 || sysfs_streq(buf, "") || > > + kstrtou8(buf, 0, &weight) || weight == 0) > > return -EINVAL; > > > > new_wi_state = kzalloc(struct_size(new_wi_state, iw_table, nr_node_ids), > > @@ -3630,9 +3637,15 @@ static ssize_t weighted_interleave_auto_store(struct kobject *kobj, > > if (!input) { > > old_wi_state = rcu_dereference_protected(wi_state, > > lockdep_is_held(&wi_state_lock)); > > - if (old_wi_state) > > - memcpy(new_wi_state->iw_table, old_wi_state->iw_table, > > - nr_node_ids * sizeof(u8)); > > + if (!old_wi_state) > > + goto update_wi_state; > > + if (input == old_wi_state->mode_auto) { > > + mutex_unlock(&wi_state_lock); > > + return count; > > + } > > + > > + memcpy(new_wi_state->iw_table, old_wi_state->iw_table, > > + nr_node_ids * sizeof(u8)); > > goto update_wi_state; > > } > > > > @@ -3707,8 +3720,12 @@ static void wi_state_free(void) > > kfree(&wi_group->wi_kobj); > > } > > > > +static struct kobj_attribute wi_auto_attr = > > + __ATTR(auto, 0664, weighted_interleave_auto_show, > > + weighted_interleave_auto_store); > > + > > static void wi_cleanup(void) { > > - sysfs_remove_file(&wi_group->wi_kobj, &wi_group->auto_kobj_attr.attr); > > + sysfs_remove_file(&wi_group->wi_kobj, &wi_auto_attr.attr); > > sysfs_wi_node_delete_all(); > > wi_state_free(); > > } > > @@ -3798,10 +3815,6 @@ static int wi_node_notifier(struct notifier_block *nb, > > return NOTIFY_OK; > > } > > > > -static struct kobj_attribute wi_auto_attr = > > - __ATTR(auto, 0664, weighted_interleave_auto_show, > > - weighted_interleave_auto_store); > > - > > static int __init add_weighted_interleave_group(struct kobject *mempolicy_kobj) > > { > > int nid, err; > > > > > > On Sat, 10 May 2025 11:51:50 -0700 Joshua Hahn wrote: > > > >> On Sat, 10 May 2025 13:25:32 +0800 "Huang, Ying" wrote: > >> > >> Hi Ying, > >> Thank you for reviewing my patch, as always! > >> > >>> Hi, Joshua, > >>> > >>> Thank you for updated version! And sorry for late reply. > >>> > >>> Joshua Hahn writes: > >> > >> [...snip...] > >> > >>>> diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > >>>> index 0b7972de04e9..ec13382c606f 100644 > >>>> --- a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > >>>> +++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > >>>> @@ -20,6 +20,35 @@ Description: Weight configuration interface for nodeN > >>>> Minimum weight: 1 > >>>> Maximum weight: 255 > >>>> > >>>> - Writing an empty string or `0` will reset the weight to the > >>>> - system default. The system default may be set by the kernel > >>>> - or drivers at boot or during hotplug events. > >>>> + Writing invalid values (i.e. any values not in [1,255], > >>>> + empty string, ...) will return -EINVAL. > >>>> + > >>>> + Changing the weight to a valid value will automatically > >>>> + update the system to manual mode as well. > >>> > >>> s/update/switch/ ? > >>> > >>> But my English is poor, please keep your version if you think that it's > >>> better. > >> > >> I have no particular preference here, whatever will make it easiest for the > >> users to understand what is happening. I'll take your suggestion! > >> > >> [...snip...] > >> > >>>> +/* > >>>> + * A null weighted_interleave_state is interpted as having .mode = "auto", > >>>> + * and .iw_table is interpreted as an array of 1s with length nr_node_ids. > >>>> + */ > >>> > >>> Better to move the comments to above "wi_state" definition? > >>> > >>>> +struct weighted_interleave_state { > >>>> + bool mode_auto; > >>>> + u8 iw_table[]; > >>>> +}; > >>>> + > >>> > >>> Why do you put the type definition in mempolicy.h instead of > >>> mempolicy.c? I don't find other users except mempolicy.c. > >> > >> Good point, I'll move the definition to mempolicy.c and move the comment > >> to the wi_state definition as well. > >> > >> [...snip...] > >> > >>>> @@ -3450,31 +3555,104 @@ static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr, > >>>> static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr, > >>>> const char *buf, size_t count) > >>>> { > >>>> + struct weighted_interleave_state *new_wi_state, *old_wi_state = NULL; > >>>> struct iw_node_attr *node_attr; > >>>> - u8 *new; > >>>> - u8 *old; > >>>> u8 weight = 0; > >>>> + int i; > >>>> > >>>> node_attr = container_of(attr, struct iw_node_attr, kobj_attr); > >>>> if (count == 0 || sysfs_streq(buf, "")) > >>>> weight = 0; > >>> > >>> According to revised ABI, we should return -EINVAL here? > >> > >> Great catch, I completely ignored the ABI description that I wrote... > >> I'll go ahead and just return -EINVAL here! > >> > >> [...snip...] > >> > >>>> +static ssize_t weighted_interleave_auto_store(struct kobject *kobj, > >>>> + struct kobj_attribute *attr, const char *buf, size_t count) > >>>> +{ > >>>> + struct weighted_interleave_state *new_wi_state, *old_wi_state = NULL; > >>>> + unsigned int *bw; > >>>> + bool input; > >>>> + int i; > >>>> + > >>>> + if (kstrtobool(buf, &input)) > >>>> + return -EINVAL; > >>>> + > >>>> + new_wi_state = kzalloc(struct_size(new_wi_state, iw_table, nr_node_ids), > >>>> + GFP_KERNEL); > >>>> + if (!new_wi_state) > >>>> + return -ENOMEM; > >>>> + for (i = 0; i < nr_node_ids; i++) > >>>> + new_wi_state->iw_table[i] = 1; > >>>> + > >>>> + mutex_lock(&wi_state_lock); > >>>> + if (!input) { > >>> > >>> If input == old_wi_state->mode_auto, we can return directly? > >> > >> Yes, that makes sense to me. > >> > >>>> static void wi_cleanup(void) { > >>>> + sysfs_remove_file(&wi_group->wi_kobj, &wi_group->auto_kobj_attr.attr); > >>> > >>> Why not just > >>> > >>> sysfs_remove_file(&wi_group->wi_kobj, &wi_auto_attr.attr); > >>> > >>> ? > >> > >> Also makes sense!! > >> > >>> --- > >>> Best Regards, > >>> Huang, Ying > >> > >> Thank you for your great feedback Ying, I'll make changes based on > >> your suggestions and shortly send up a v9. I hope you have a great day! > >> Joshua > >> > >