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 E33DCC02198 for ; Wed, 12 Feb 2025 17:06:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 78DC16B0083; Wed, 12 Feb 2025 12:06:52 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 73E076B0085; Wed, 12 Feb 2025 12:06:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5DDD16B0088; Wed, 12 Feb 2025 12:06:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 3B4396B0083 for ; Wed, 12 Feb 2025 12:06:52 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 8252A1A1A99 for ; Wed, 12 Feb 2025 17:06:51 +0000 (UTC) X-FDA: 83111922222.24.D4D3C37 Received: from mail-yw1-f171.google.com (mail-yw1-f171.google.com [209.85.128.171]) by imf02.hostedemail.com (Postfix) with ESMTP id 502BD80014 for ; Wed, 12 Feb 2025 17:06:49 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=D9QkAMOh; spf=pass (imf02.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.128.171 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=1739380009; 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=EHhgDa+15Ow4EU32iMlqezS/s+Dmcp5L9qnpQ95X31U=; b=zzuNhsAQg/uq7e7mUe3fl+YfgUvpnzTwqtCacXmSri0Zw1LLdaGYqgfuzX3bh8G66gd3KN EeJUhGXodwK5IHIEQu14cxoI4RPdD+EO4ZB2WmkH6Io4JhzArdTRlGonDHnXtf4Iw+RSYk w7bmoZ0v2GqBfoQ1t62cbdv/KsbXPWw= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=D9QkAMOh; spf=pass (imf02.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.128.171 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=1739380009; a=rsa-sha256; cv=none; b=4Ef0GQ6U5FyvTXPKNEwaDuBc/RDcls+XG1PPdsittCCaJOSRfhLH8y6bF4s1Mq1Fwbg0uE 2ruPbhCIze3dfn4QSSe1XeGoKY7GYPy5GSefdleKN/NNwxHJhLXwdYxHgH1H0tuo70fkPp GzSKV5dTTKTOwQlj3S8y+PZux2DiAWM= Received: by mail-yw1-f171.google.com with SMTP id 00721157ae682-6f88509dad2so65250747b3.3 for ; Wed, 12 Feb 2025 09:06:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1739380008; x=1739984808; 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=EHhgDa+15Ow4EU32iMlqezS/s+Dmcp5L9qnpQ95X31U=; b=D9QkAMOh8Yxq9QqDLEhpvc3XTxA3WUNLihLhABmcQt4l8RCWDkb+pRykEaF3voati+ QijshjUzylDjTrYHpgNdQEzNDHsIO35ZE22DHFGMTzyIOKhCcboog1dIe4LQmkPTvgR2 GDlpUg4F8NSWHxhb+StGRG3EywZwYcgdpT0wnrq6IULxiYZtt2zHVhKwOJeckmZoSYgg KDDtdcrJAZxIORIHBJq1nYXE7WvRuK4BPhJIaFu+/DdqCC7XkfYUDM0EjMiRk0Gvn1FL wclUInM4jal0l1M383SIzHkYfLAAgh2+8X2oH3j8tdMp+LjfRGUnrchEBH1QkVYu4guf osYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739380008; x=1739984808; 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=EHhgDa+15Ow4EU32iMlqezS/s+Dmcp5L9qnpQ95X31U=; b=DmerPiPBjBlo49jUyMHaVYmTmmUxghEtQrmxvS4Qq2BcwMKAdx6h5yIZG5SizDquuX pz30qldbYhESQiRqUEbpU8Hq9PDPFCRlIADa1k7L90q6j724ls7dEJNgFm0zNKgIUegk WkE8ij1GtCxt03MkVPblcAAHMoExynmKRK6sauhq2ms+nyNbxL/Y7laH/oqatQHFFxMs eD3BxQH+8ErUCDF6eI51hCOftCKFVhT2qUIlq4ulohH7OEX8PdHl1IQh+tgNNiXR6Jwf VGHvXDQLaWBkBMTHEcev3dYSY/mEXgiL0OB3od0Qegd7ggkYJvoq4cpup/zBIEbm0STd 7z7A== X-Forwarded-Encrypted: i=1; AJvYcCVFAGLRykP0SCa/b0qu5oGmzUKHLyyrXMKrCn8l5wf3xV9o1kmJsMTZllxtvbtrImtd+PQwraXl6Q==@kvack.org X-Gm-Message-State: AOJu0Yzp35KoW5aOXQT5g+0bkm5x34MplQXkiNl4w9W1g5vvF9sRzZmN xozdXsrWgB4wyYlEVuLehs3k3SeyaIYVD25dIkphpiyN5OkMosZn X-Gm-Gg: ASbGncvh5XhbcTExKB57FdyIG8An1eR9naHrGK1iLaWr/PrV4uF+CvynilN9mGdhPx7 bNcKvUO+r4UQi4RMtLuFBM9ImojA0/DfaapuTAAWToSvBZbCweF9BrSEozpT9tmNXqMEAHt3YJa R6V6gRS6/vESmFPp49e2lpZ5kSKW9heg9p8nI9mycTgXKMT7cSIbXE3o8a9kteJ5jMiaaSM+73M NZ0KFf5QQljStzVHNT4+A3vTwXyn4Ka7atrLWuDOf/DIEKGke/1lbFEIx0c4PfFL2qT7zEPv5vz eFOppE+CR2PPbQ== X-Google-Smtp-Source: AGHT+IHUNoQgf2R9a7OphhBMiQ7H3h8YLQWV053bqd5METYdW0bV3GZsxihxsz4cEOxVwaUfLVto4A== X-Received: by 2002:a05:690c:6b89:b0:6f9:8916:3b66 with SMTP id 00721157ae682-6fb1f5a6fffmr43129347b3.12.1739380008194; Wed, 12 Feb 2025 09:06:48 -0800 (PST) Received: from localhost ([2a03:2880:25ff:d::]) by smtp.gmail.com with ESMTPSA id 00721157ae682-6f99fd3c323sm27237247b3.49.2025.02.12.09.06.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Feb 2025 09:06:47 -0800 (PST) From: Joshua Hahn To: "Huang, Ying" Cc: gourry@gourry.net, hyeonggon.yoo@sk.com, honggyu.kim@sk.com, akpm@linux-foundation.org, rafael@kernel.org, lenb@kernel.org, gregkh@linuxfoundation.org, rakie.kim@sk.com, dan.j.williams@intel.com, Jonathan.Cameron@huawei.com, dave.jiang@intel.com, horen.chuang@linux.dev, hannes@cmpxchg.org, linux-kernel@vger.org, linux-acpi@vger.kernel.org, linux-mm@kvack.org, kernel-team@meta.com Subject: Re: [PATCH v5] mm/mempolicy: Weighted Interleave Auto-tuning Date: Wed, 12 Feb 2025 09:06:40 -0800 Message-ID: <20250212170645.1080771-1-joshua.hahnjy@gmail.com> X-Mailer: git-send-email 2.43.5 In-Reply-To: <87a5ar3mir.fsf@DESKTOP-5N7EMDA> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Stat-Signature: 3a8yyuek43bhbj4aqaqqis5j1fwrufy5 X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 502BD80014 X-HE-Tag: 1739380009-34025 X-HE-Meta: U2FsdGVkX1/VAzEI72s5pJrdl9SUjQ+jKLzmIJe0NuF4r6brtraIVCJr5GAQ56oocr8qcBnu2+lL4gfZvwU/vieDv3PJ1fvKgY7sg9Wku1DQn0/sulNj7MAibjPp26SzjmNJpR0kpcVkIZRyWpQ9AAuj8qxhyzWZKINPc4xnze8zrrM2BZWocnCtU/ZyXKZQFC960ez2OX8e34h5MSIi5rxjIj7LtwD+go7b1/AOtg7edDY6VGwJccN9RMKra+0tAKCU3oekT9o4LYO1OTPvs1n4wgfItufb4NsmfU8EY1kVYqMFdjVjh6sY8ePUPY9F7Ib3IXPTEyYpQS3aeXox7sSma/DKPFWBDz+0MJjbkdcoCRHrh3yOdojHVPb2rkNU6XJUGNb5Kzd5UKrMykNl0TPG2ta36u7ZWB5Y+TK6ijzjH6q5+4Jx81uBIGoAJ76JKn5Dc6giUYxm5SjiScFsVgD6fjtCTq07sswbMeLcvW1axtQwtB3YNE938gnb9eL7a5n2XB86u0cGKRo95H2m99MdPRn5MSbH9mnYNy37lx6oIqbpliRdChz+djXdbbXK2SgDkQIgNoKlOequXJod3u9ZpLXVFNG0EaqNeA3zfArg81liflNFArq0wGFIVO1mz51srAvxv5oSE3GtCj9dtkamsAguJ3/JiO6jWXA/3Qx4kZiv8X2Ewejmd6QfaI3c8YG4dm0xGWagzIOCrO2+Q1LkhQa9l+JoFoGFckZxZVLF3iFWvRdCFl+Tv9uPEJdKZaOeSm30ZI9IMOw3xMnVNG2XIjdGRV/LmZMtehmsl73zvxa0+bpK3irNJ73ZTumXIWnSsnLqsN92Ei6643+Vep7zTM4GBSvMJi0O6/z7fyIHJSPFa5NQ3z8iUPXXccZWUHRtkHFH7KxdoXg4cCOUMremfgm225ni06G9qxtQdhU94c/08g/yXVBxsZGgAhrNafUcOF0qWFHBUgLsbtI fG/3G0uF RROJ0KzDOc8VYhpkAcWtgzqlP6oebQrpNXzUvWfmuVAwUFwNNYfzXNeCYrsRAjcxt5Rn+TYz88NQ18tSCweVvwVF2gJkYxBo0/unIyeqORS1m4veq6NPDFVdY1tLxNaxyhQpa7p9y6dZTRFJ7/gIknTiQ/JsT9oZEAZttTV0KU4o0yCN+f62zvfrDbBaOxSBl7GfCVZdOjcPgJSlj2yMQjw6fKtgNnLkhnMyyvJk4cqYP1DedAoh+p+mEQwAo6ZXcyBIJe3GH0I3Y28RxMbvgGp+BkEahWLZPTIBmA38AjcIP6MjyXgxPQrrvuJKsgcYXZOwSQOWSa57vexi1lOjNmj6f98uzwnuJBIK02hyLetCGbslf+oy03xvZHBXlbya0kojDBgFRxsFa2+k7P2Rg5Ne1n27bei7DQD9X/jRmpeBzIpk5VHxeSwSSfP1WeWGzD1aSgqWuu0H/qkcO8eULSb+BiNON07xco7uCuQm/dInAZwERCaCxxgQG4PucZygoqmYfJFAYnoxwnY5f8aAHkxvcSviTTWdMljQdWcZ4aWDxKN7SEMxQ/O63v4VxHgggXk6P 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 Feb 2025 10:49:32 +0800 "Huang, Ying" wrote: > Hi, Joshua, > > Thanks for your patch and sorry for late reply. Hi Ying, no worries! Thank you for taking time to review this patch. > Joshua Hahn writes: [...snip...] > > This patch introduces an auto-configuration mode for the interleave > > weights that aims to balance the two goals of setting node weights to be > > proportional to their bandwidths and keeping the weight values low. > > In order to perform the weight re-scaling, we use an internal > > "weightiness" value (fixed to 32) that defines interleave aggression. > > As asked by Andrew in another thread, you may need to make it more > explicit about why we need this patch. Yes, I think that makes a lot of sense. Probably some details about why we use 32 as the default weightiness value will also help with clarity. [...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..ef43228d135d 100644 > > --- a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > > +++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave > > @@ -20,6 +20,38 @@ 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. > > + > > +What: /sys/kernel/mm/mempolicy/weighted_interleave/auto > > +Date: February 2025 > > +Contact: Linux memory management mailing list > > +Description: Auto-weighting configuration interface > > + > > + Configuration mode for weighted interleave. A 'Y' indicates > > + that the system is in auto mode, and a 'N' indicates that > > + the system is in manual mode. All other values are invalid. > > + > > + In auto mode, all node weights are re-calculated and overwritten > > + (visible via the nodeN interfaces) whenever new bandwidth data > > + is made available during either boot or hotplug events. > > + > > + In manual mode, node weights can only be updated by the user. > > + Note that nodes that are onlined with previously set / defaulted > > Sorry, I don't understand this well. What is "defaulted" weights? Why > not just "previously set"? This is just poor word choice on my end. I meant to say that if they had been using auto mode previously and the node contained system-calculated weights, then offlining and onlining while in manual mode would bring back the weights from the system. This is the scenario: Auto: weights are set to [1,2,3] Mode changes from auto --> manual Manual: weights are still [1,2,3] Manual: update node0 to be [7,2,3] Offline node2 Manual: weights are still [7,2,3], even though node2 is offline Online node2 Manual: weights are still [7,2,3] ^ I just wanted to highlight that it will online with a weight of 3, which is a value set by the system. > > + weights will inherit those weights. If they were not previously > > + set or are onlined with missing bandwidth data, they will be > > + defaulted to a weight of 1. > > + > > + Writing Y or 1 to the interface will enable auto mode, while > > + writing N or 0 will enable manual mode. All other strings will > > + be ignored, and -EINVAL will be returned. > > + > > + If Y or 1 is written to the interface but the recalculation or > > + updates fail at any point (-ENOMEM or -ENODEV), then the system > > + will remain in manual mode. > > IMHO, it's unnecessary to make this a part of the interface definition. > However, I have no strong opinion on this. Thank you for your input. I was just trying to be as explicit and thorough as possible, but I can see how this might feel a bit cluttered. > > + Writing a new weight to a node directly via the nodeN interface > > + will also automatically update the system to manual mode. > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c > > index 80a3481c0470..cc94cba112dd 100644 > > --- a/drivers/acpi/numa/hmat.c > > +++ b/drivers/acpi/numa/hmat.c > > @@ -20,6 +20,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > This change should be removed? Ah I see. Harry (Hyeonggon) had pointed this out in v4 of the patch, but I misread the response because I thought his comment was about the include in node.c, not in hmat.c. Sorry, Harry! This is correct; this include statement should not be here. [...snip...] > > struct mempolicy {}; > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > > index 04f35659717a..51edd3663667 100644 > > --- a/mm/mempolicy.c > > +++ b/mm/mempolicy.c > > @@ -109,6 +109,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -138,16 +139,18 @@ static struct mempolicy default_policy = { > > > > static struct mempolicy preferred_node_policy[MAX_NUMNODES]; > > > > +static uint64_t *node_bw_table; > > Usually, we use "u64" instead of "uint64_t" in kernel. It appears that > we don't need u64. "unsigned int" should be enough because "struct > access_coordinate" uses that too. > > It's better to move this after iw_table definition below. And make it > explicit that iw_table_lock protects all interleave weight global > variables. I see, that makes sense. I will change it to unsigned int and move it below, I agree that the scope of the lock should be clearer as well. > > + > > /* > > - * iw_table is the sysfs-set interleave weight table, a value of 0 denotes > > - * system-default value should be used. A NULL iw_table also denotes that > > - * system-default values should be used. Until the system-default table > > - * is implemented, the system-default is always 1. > > - * > > + * iw_table is the interleave weight table. > > + * If bandwidth data is available and the user is in auto mode, the table > ~~~~ > system? Thank you, I'll fix this. > > + * is populated with default values in [1,255]. > > If system is in manual mode, the weight value is in [1, 255] too? > > NULL iw_table is still a valid configuration, so we still need to > describe its behavior. Right? Yes, that is true. I just wanted to explicilty note that the default values selected by the system in auto mode will be in that range. I'll also include what happens when iw_table is null. It only happens when there is absolutely no bandwidth information available, so I will note that in the description as well. > > * iw_table is RCU protected > > */ > > static u8 __rcu *iw_table; > > static DEFINE_MUTEX(iw_table_lock); > > +static const int weightiness = 32; > > +static bool weighted_interleave_auto = true; > > > > static u8 get_il_weight(int node) > > { > > @@ -156,14 +159,114 @@ static u8 get_il_weight(int node) > > > > rcu_read_lock(); > > table = rcu_dereference(iw_table); > > - /* if no iw_table, use system default */ > > It appears that this applies at some degree. Yes -- but this is only possible if there is no bandwidth data available (and therefore, mempolicy_set_node_perf is never called). If this is the case, then all nodes will have the same weight. Setting the default weight to 1 means weighted interleave is reduced to unweighted interleave, but setting the default weight to 0 will mean no memory can be allocated. [...snip...] > > + if (old_bw) > > + memcpy(new_bw, old_bw, nr_node_ids * sizeof(uint64_t)); > > + new_bw[node] = bw_val; > > + node_bw_table = new_bw; > > + > > + if (weighted_interleave_auto) { > > + reduce_interleave_weights(new_bw, new_iw); > > + } else if (old_iw) { > > + /* > > + * The first time mempolicy_set_node_perf is called, old_iw > > + * (iw_table) is null. If that is the case, assign a zeroed > > + * table to it. Otherwise, free the newly allocated iw_table. > > We shouldn't assign a zeroed iw_table? Because 0 isn't valid now. > Please check changes in weighted_interleave_nid() below. Thank you for the catch! I will fix this and write that it will contain default values of 1 instead!. > > + */ > > + mutex_unlock(&iw_table_lock); > > + kfree(new_iw); > > + kfree(old_bw); > > + return 0; > > + } > > + > > + rcu_assign_pointer(iw_table, new_iw); > > + mutex_unlock(&iw_table_lock); > > + synchronize_rcu(); > > + kfree(old_iw); > > If you want to save one synchronize_rcu() if unnecessary, you can use > > if (old_iw) { > synchronize_rcu(); > kfree(old_iw); > } I see, that makes sense. Thank you! [...snip...] > > +static ssize_t weighted_interleave_mode_show(struct kobject *kobj, > > IMHO, it's better to name the function as > weighted_interleave_auto_show()? That makes sense, this was an artifact of when we named the interface to be "mode", but it seems like I overlooked renaming this function. > > + struct kobj_attribute *attr, char *buf) > > +{ > > + if (weighted_interleave_auto) > > + return sysfs_emit(buf, "Y\n"); > > + else > > + return sysfs_emit(buf, "N\n"); > > str_true_false() can be used here. Thank you! > > +} > > + > > +static ssize_t weighted_interleave_mode_store(struct kobject *kobj, > > + struct kobj_attribute *attr, const char *buf, size_t count) > > +{ > > + uint64_t *bw; > > + u8 *old_iw, *new_iw; > > + > > + if (count == 0) > > + return -EINVAL; > > + > > + if (sysfs_streq(buf, "N") || sysfs_streq(buf, "0")) { > > kstrtobool() can be used here. It can deal with 'count == 0' case too. These kernel string tools are very helpful, thank you for bringing them to my attention : -) > > + weighted_interleave_auto = false; > > + return count; > > + } else if (!sysfs_streq(buf, "Y") && !sysfs_streq(buf, "1")) { > > + return -EINVAL; > > + } > > + > > + new_iw = kcalloc(nr_node_ids, sizeof(u8), GFP_KERNEL); > > + if (!new_iw) > > + return -ENOMEM; > > + > > + mutex_lock(&iw_table_lock); > > + bw = node_bw_table; > > + > > + if (!bw) { > > + mutex_unlock(&iw_table_lock); > > + kfree(new_iw); > > + return -ENODEV; > > + } > > + > > + old_iw = rcu_dereference_protected(iw_table, > > + lockdep_is_held(&iw_table_lock)); > > + > > + reduce_interleave_weights(bw, new_iw); > > + rcu_assign_pointer(iw_table, new_iw); > > + mutex_unlock(&iw_table_lock); > > + > > + synchronize_rcu(); > > + kfree(old_iw); > > + > > + weighted_interleave_auto = true; > > Why assign weighted_interleave_auto after synchronize_rcu()? To reduce > the race window, it's better to change weighted_interleave_auto and > iw_table together? Is it better to put them into a data structure and > change them together always? > > struct weighted_interleave_state { > bool weighted_interleave_auto; > u8 iw_table[0] > }; I see, I think your explanation makes sense. For the first question, I think your point makes sense, so I will move the updating to be inside the rcu section. As for the combined data structure, I think that this makes sense, but I have a few thoughts. First, there are some times when we don't update both of them, like moving from auto --> manual, and whenever we just update iw_table, we don't need to update the weighted_interleave auto field. I also have a concern that this might make the code a bit harder to read, but that is just my humble opinion. > --- > Best Regards, > Huang, Ying Thank you Ying for your detailed comments! It seems like there are a lot of things that I missed, thank you for reviewing my patch and catching the mistakes. Please let me know if there are any other changes that you see fit for the patch. Have a great day! Joshua Sent using hkml (https://github.com/sjp38/hackermail)