Message ID | 20220506015801.757918-2-liupeng256@huawei.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | null_blk: fix wrong use of nr_online_nodes | expand |
Subject line convention looks like "numa: ..." On Fri, May 06, 2022 at 01:58:00AM +0000, Peng Liu wrote: > Lots of code dose does > node != NUMA_NO_NODE && !node_online(node) > or > node == NUMA_NO_NODE || node_online(node) > so create node_available to do this to simplify code. node_available() I'm not really sure what meaning "node_available" conveys, though. Probably just because I don't understand NUMA. Should the test for NUMA_NO_NODE be folded into node_state() or node_online() directly instead of adding a new node_available() interface? NUMA_NO_NODE is -1. It's not clear to me that node_state()/ node_isset()/test_bit() would do the right thing given -1. I doubt all node_online() callers ensure they don't pass NUMA_NO_NODE. > --- a/include/linux/nodemask.h > +++ b/include/linux/nodemask.h > @@ -70,6 +70,7 @@ > * > * int node_online(node) Is some node online? > * int node_possible(node) Is some node possible? > + * int node_available(node) Is some node available(online or NUMA_NO_NODE)? Existing file generally fits in 80 columns; follow that lead unless you have a really good reason. E.g., maybe this? + * int node_available(node) Node online or NUMA_NO_NODE
On 2022/5/7 1:23, Bjorn Helgaas wrote: > Subject line convention looks like "numa: ..." > > On Fri, May 06, 2022 at 01:58:00AM +0000, Peng Liu wrote: >> Lots of code dose > does > >> node != NUMA_NO_NODE && !node_online(node) >> or >> node == NUMA_NO_NODE || node_online(node) >> so create node_available to do this to simplify code. > node_available() Thanks. > I'm not really sure what meaning "node_available" conveys, though. > Probably just because I don't understand NUMA. > > Should the test for NUMA_NO_NODE be folded into node_state() or > node_online() directly instead of adding a new node_available() > interface? > > NUMA_NO_NODE is -1. It's not clear to me that node_state()/ > node_isset()/test_bit() would do the right thing given -1. I doubt > all node_online() callers ensure they don't pass NUMA_NO_NODE. I have tested node_online(NUMA_NO_NODE) on x86_64, arm64, arm32, the results are: arch node_online(NUMA_NO_NODE) ------------------------------------------ x86_64 0 arm64 1 arm32 0 . Hence, I think the behavior of node_state(NUMA_NO_NODE) is undefined which is due to test_bit(NUMA_NO_NODE) is different between different arches. For many times, callers could ensure they don't pass NUMA_NO_NODE, for example "nid < 0" is checked before node_online() or some code only used in NUMA has node. So, it is not suitable to test NUMA_NO_NODE in node_state() or node_online(). Unfortunately, there are actually some cases that test for NUMA_NO_NODE is needed but ignored. Related issue: ab31c7fd2d37 ("sched/numa: Fix boot crash on arm64 systems") >> --- a/include/linux/nodemask.h >> +++ b/include/linux/nodemask.h >> @@ -70,6 +70,7 @@ >> * >> * int node_online(node) Is some node online? >> * int node_possible(node) Is some node possible? >> + * int node_available(node) Is some node available(online or NUMA_NO_NODE)? > Existing file generally fits in 80 columns; follow that lead unless > you have a really good reason. E.g., maybe this? > > + * int node_available(node) Node online or NUMA_NO_NODE > . Thank you for your suggestion, I will send a revised patch with modified comments later.
diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c index 8ad6946521d8..da3a010bb5fb 100644 --- a/arch/ia64/hp/common/sba_iommu.c +++ b/arch/ia64/hp/common/sba_iommu.c @@ -1969,7 +1969,7 @@ sba_map_ioc_to_node(struct ioc *ioc, acpi_handle handle) unsigned int node; node = acpi_get_node(handle); - if (node != NUMA_NO_NODE && !node_online(node)) + if (!node_available(node)) node = NUMA_NO_NODE; ioc->node = node; diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c index 052f1d78a562..d4909daa44d9 100644 --- a/arch/x86/pci/acpi.c +++ b/arch/x86/pci/acpi.c @@ -254,7 +254,7 @@ static int pci_acpi_root_get_node(struct acpi_pci_root *root) dev_info(&device->dev, FW_BUG "no _PXM; falling back to node %d from hardware (may be inconsistent with ACPI node numbers)\n", node); } - if (node != NUMA_NO_NODE && !node_online(node)) + if (!node_available(node)) node = NUMA_NO_NODE; return node; diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index f2f8f05662de..bdf690010b97 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -1251,7 +1251,7 @@ static int __init arm_smmu_v3_set_proximity(struct device *dev, if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) { int dev_node = pxm_to_node(smmu->pxm); - if (dev_node != NUMA_NO_NODE && !node_online(dev_node)) + if (!node_available(dev_node)) return -EINVAL; set_dev_node(dev, dev_node); diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index c263ffc5884a..502490d26c1d 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -344,7 +344,7 @@ static ssize_t numa_node_store(struct device *dev, if ((node < 0 && node != NUMA_NO_NODE) || node >= MAX_NUMNODES) return -EINVAL; - if (node != NUMA_NO_NODE && !node_online(node)) + if (!node_available(node)) return -EINVAL; add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h index 567c3ddba2c4..591201a1e646 100644 --- a/include/linux/nodemask.h +++ b/include/linux/nodemask.h @@ -70,6 +70,7 @@ * * int node_online(node) Is some node online? * int node_possible(node) Is some node possible? + * int node_available(node) Is some node available(online or NUMA_NO_NODE)? * * node_set_online(node) set bit 'node' in node_online_map * node_set_offline(node) clear bit 'node' in node_online_map @@ -510,6 +511,8 @@ static inline int node_random(const nodemask_t *mask) #define num_possible_nodes() num_node_state(N_POSSIBLE) #define node_online(node) node_state((node), N_ONLINE) #define node_possible(node) node_state((node), N_POSSIBLE) +#define node_available(node) \ + ((node) == NUMA_NO_NODE || node_state((node), N_ONLINE)) #define for_each_node(node) for_each_node_state(node, N_POSSIBLE) #define for_each_online_node(node) for_each_node_state(node, N_ONLINE) diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 8c74107a2b15..7a31b006c5e8 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -141,7 +141,7 @@ int numa_map_to_online_node(int node) { int min_dist = INT_MAX, dist, n, min_node; - if (node == NUMA_NO_NODE || node_online(node)) + if (node_available(node)) return node; min_node = node;
Lots of code dose node != NUMA_NO_NODE && !node_online(node) or node == NUMA_NO_NODE || node_online(node) so create node_available to do this to simplify code. Signed-off-by: Peng Liu <liupeng256@huawei.com> --- arch/ia64/hp/common/sba_iommu.c | 2 +- arch/x86/pci/acpi.c | 2 +- drivers/acpi/arm64/iort.c | 2 +- drivers/pci/pci-sysfs.c | 2 +- include/linux/nodemask.h | 3 +++ mm/mempolicy.c | 2 +- 6 files changed, 8 insertions(+), 5 deletions(-)