| Message ID | 20200602030139.73012-1-song.bao.hua@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | driver core: platform: expose numa_node to users in sysfs | expand |
On Tue, Jun 02, 2020 at 03:01:39PM +1200, Barry Song wrote: > For some platform devices like iommu, particually ARM smmu, users may > care about the numa locality. for example, if threads and drivers run > near iommu, they may get much better performance on dma_unmap_sg. > For other platform devices, users may still want to know the hardware > topology. > > Cc: Prime Zeng <prime.zeng@hisilicon.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > --- > drivers/base/platform.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index b27d0f6c18c9..7794b9a38d82 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -1062,13 +1062,37 @@ static ssize_t driver_override_show(struct device *dev, > } > static DEVICE_ATTR_RW(driver_override); > > +static ssize_t numa_node_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", dev_to_node(dev)); > +} > +static DEVICE_ATTR_RO(numa_node); > + > +static umode_t platform_dev_attrs_visible(struct kobject *kobj, struct attribute *a, > + int n) > +{ > + struct device *dev = container_of(kobj, typeof(*dev), kobj); > + > + if (a == &dev_attr_numa_node.attr && > + dev_to_node(dev) == NUMA_NO_NODE) > + return 0; > + > + return a->mode; > +} > > static struct attribute *platform_dev_attrs[] = { > &dev_attr_modalias.attr, > + &dev_attr_numa_node.attr, > &dev_attr_driver_override.attr, > NULL, > }; > -ATTRIBUTE_GROUPS(platform_dev); > + > +static struct attribute_group platform_dev_group = { > + .attrs = platform_dev_attrs, > + .is_visible = platform_dev_attrs_visible, > +}; > +__ATTRIBUTE_GROUPS(platform_dev); > > static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) > { Platform devices are NUMA? That's crazy, and feels like a total abuse of platform devices and drivers that really should belong on a "real" bus. What drivers in the kernel today have this issue? thanks, greg k-h
On Tue, Jun 02, 2020 at 03:01:39PM +1200, Barry Song wrote: > For some platform devices like iommu, particually ARM smmu, users may > care about the numa locality. for example, if threads and drivers run > near iommu, they may get much better performance on dma_unmap_sg. > For other platform devices, users may still want to know the hardware > topology. > > Cc: Prime Zeng <prime.zeng@hisilicon.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > --- > drivers/base/platform.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index b27d0f6c18c9..7794b9a38d82 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -1062,13 +1062,37 @@ static ssize_t driver_override_show(struct device *dev, > } > static DEVICE_ATTR_RW(driver_override); > > +static ssize_t numa_node_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", dev_to_node(dev)); > +} > +static DEVICE_ATTR_RO(numa_node); > + > +static umode_t platform_dev_attrs_visible(struct kobject *kobj, struct attribute *a, > + int n) > +{ > + struct device *dev = container_of(kobj, typeof(*dev), kobj); > + > + if (a == &dev_attr_numa_node.attr && > + dev_to_node(dev) == NUMA_NO_NODE) > + return 0; > + > + return a->mode; > +} > > static struct attribute *platform_dev_attrs[] = { > &dev_attr_modalias.attr, > + &dev_attr_numa_node.attr, > &dev_attr_driver_override.attr, > NULL, > }; > -ATTRIBUTE_GROUPS(platform_dev); > + > +static struct attribute_group platform_dev_group = { > + .attrs = platform_dev_attrs, > + .is_visible = platform_dev_attrs_visible, > +}; > +__ATTRIBUTE_GROUPS(platform_dev); > > static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) > { Also you forgot a new entry in Documentation/ABI/ :(
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday, June 2, 2020 4:24 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: rafael@kernel.org; iommu@lists.linux-foundation.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Linuxarm > <linuxarm@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Robin > Murphy <robin.murphy@arm.com> > Subject: Re: [PATCH] driver core: platform: expose numa_node to users in sysfs > > On Tue, Jun 02, 2020 at 03:01:39PM +1200, Barry Song wrote: > > For some platform devices like iommu, particually ARM smmu, users may > > care about the numa locality. for example, if threads and drivers run > > near iommu, they may get much better performance on dma_unmap_sg. > > For other platform devices, users may still want to know the hardware > > topology. > > > > Cc: Prime Zeng <prime.zeng@hisilicon.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > > --- > > drivers/base/platform.c | 26 +++++++++++++++++++++++++- > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > index b27d0f6c18c9..7794b9a38d82 100644 > > --- a/drivers/base/platform.c > > +++ b/drivers/base/platform.c > > @@ -1062,13 +1062,37 @@ static ssize_t driver_override_show(struct > device *dev, > > } > > static DEVICE_ATTR_RW(driver_override); > > > > +static ssize_t numa_node_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + return sprintf(buf, "%d\n", dev_to_node(dev)); > > +} > > +static DEVICE_ATTR_RO(numa_node); > > + > > +static umode_t platform_dev_attrs_visible(struct kobject *kobj, struct > attribute *a, > > + int n) > > +{ > > + struct device *dev = container_of(kobj, typeof(*dev), kobj); > > + > > + if (a == &dev_attr_numa_node.attr && > > + dev_to_node(dev) == NUMA_NO_NODE) > > + return 0; > > + > > + return a->mode; > > +} > > > > static struct attribute *platform_dev_attrs[] = { > > &dev_attr_modalias.attr, > > + &dev_attr_numa_node.attr, > > &dev_attr_driver_override.attr, > > NULL, > > }; > > -ATTRIBUTE_GROUPS(platform_dev); > > + > > +static struct attribute_group platform_dev_group = { > > + .attrs = platform_dev_attrs, > > + .is_visible = platform_dev_attrs_visible, > > +}; > > +__ATTRIBUTE_GROUPS(platform_dev); > > > > static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) > > { > > Platform devices are NUMA? That's crazy, and feels like a total abuse > of platform devices and drivers that really should belong on a "real" > bus. I am not sure if it is an abuse of platform device. But smmu is a platform device, drivers/iommu/arm-smmu-v3.c is a platform driver. In a typical ARM server, there are maybe multiple SMMU devices which can support IO virtual address and page tables for other devices on PCI-like busses. Each different SMMU device might be close to different NUMA node. There is really a hardware topology. If you have multiple CPU packages in a NUMA server, some platform devices might Belong to CPU0, some other might belong to CPU1. -barry > > What drivers in the kernel today have this issue? > > thanks, > > greg k-h
> > > > Platform devices are NUMA? That's crazy, and feels like a total abuse > > of platform devices and drivers that really should belong on a "real" > > bus. > > I am not sure if it is an abuse of platform device. But smmu is a platform > device, > drivers/iommu/arm-smmu-v3.c is a platform driver. > In a typical ARM server, there are maybe multiple SMMU devices which can > support > IO virtual address and page tables for other devices on PCI-like busses. > Each different SMMU device might be close to different NUMA node. There is > really a hardware topology. > > If you have multiple CPU packages in a NUMA server, some platform devices > might > Belong to CPU0, some other might belong to CPU1. Those devices are populated by acpi_iort for an ARM server: drivers/acpi/arm64/iort.c: static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { .name = "arm-smmu-v3", .dev_dma_configure = arm_smmu_v3_dma_configure, .dev_count_resources = arm_smmu_v3_count_resources, .dev_init_resources = arm_smmu_v3_init_resources, .dev_set_proximity = arm_smmu_v3_set_proximity, }; void __init acpi_iort_init(void) { acpi_status status; status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table); ... iort_check_id_count_workaround(iort_table); iort_init_platform_devices(); } static void __init iort_init_platform_devices(void) { ... for (i = 0; i < iort->node_count; i++) { if (iort_node >= iort_end) { pr_err("iort node pointer overflows, bad table\n"); return; } iort_enable_acs(iort_node); ops = iort_get_dev_cfg(iort_node); if (ops) { fwnode = acpi_alloc_fwnode_static(); if (!fwnode) return; iort_set_fwnode(iort_node, fwnode); ret = iort_add_platform_device(iort_node, ops); if (ret) { iort_delete_fwnode(iort_node); acpi_free_fwnode_static(fwnode); return; } } ... } ... } NUMA node is got from ACPI: static int __init arm_smmu_v3_set_proximity(struct device *dev, struct acpi_iort_node *node) { struct acpi_iort_smmu_v3 *smmu; smmu = (struct acpi_iort_smmu_v3 *)node->node_data; if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) { int dev_node = acpi_map_pxm_to_node(smmu->pxm); ... set_dev_node(dev, dev_node); ... } return 0; } Barry
On Tue, Jun 02, 2020 at 05:09:57AM +0000, Song Bao Hua (Barry Song) wrote: > > > > > > Platform devices are NUMA? That's crazy, and feels like a total abuse > > > of platform devices and drivers that really should belong on a "real" > > > bus. > > > > I am not sure if it is an abuse of platform device. But smmu is a platform > > device, > > drivers/iommu/arm-smmu-v3.c is a platform driver. > > In a typical ARM server, there are maybe multiple SMMU devices which can > > support > > IO virtual address and page tables for other devices on PCI-like busses. > > Each different SMMU device might be close to different NUMA node. There is > > really a hardware topology. > > > > If you have multiple CPU packages in a NUMA server, some platform devices > > might > > Belong to CPU0, some other might belong to CPU1. > > Those devices are populated by acpi_iort for an ARM server: > > drivers/acpi/arm64/iort.c: > > static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { > .name = "arm-smmu-v3", > .dev_dma_configure = arm_smmu_v3_dma_configure, > .dev_count_resources = arm_smmu_v3_count_resources, > .dev_init_resources = arm_smmu_v3_init_resources, > .dev_set_proximity = arm_smmu_v3_set_proximity, > }; > > void __init acpi_iort_init(void) > { > acpi_status status; > > status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table); > ... > iort_check_id_count_workaround(iort_table); > iort_init_platform_devices(); > } > > static void __init iort_init_platform_devices(void) > { > ... > > for (i = 0; i < iort->node_count; i++) { > if (iort_node >= iort_end) { > pr_err("iort node pointer overflows, bad table\n"); > return; > } > > iort_enable_acs(iort_node); > > ops = iort_get_dev_cfg(iort_node); > if (ops) { > fwnode = acpi_alloc_fwnode_static(); > if (!fwnode) > return; > > iort_set_fwnode(iort_node, fwnode); > > ret = iort_add_platform_device(iort_node, ops); > if (ret) { > iort_delete_fwnode(iort_node); > acpi_free_fwnode_static(fwnode); > return; > } > } > > ... > } > ... > } > > NUMA node is got from ACPI: > > static int __init arm_smmu_v3_set_proximity(struct device *dev, > struct acpi_iort_node *node) > { > struct acpi_iort_smmu_v3 *smmu; > > smmu = (struct acpi_iort_smmu_v3 *)node->node_data; > if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) { > int dev_node = acpi_map_pxm_to_node(smmu->pxm); > > ... > > set_dev_node(dev, dev_node); > ... > } > return 0; > } > > Barry That's fine, but those are "real" devices, not platform devices, right? What platform device has this issue? What one will show up this way with the new patch? thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday, June 2, 2020 6:11 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: rafael@kernel.org; iommu@lists.linux-foundation.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Linuxarm > <linuxarm@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Robin > Murphy <robin.murphy@arm.com> > Subject: Re: [PATCH] driver core: platform: expose numa_node to users in sysfs > > On Tue, Jun 02, 2020 at 05:09:57AM +0000, Song Bao Hua (Barry Song) wrote: > > > > > > > > Platform devices are NUMA? That's crazy, and feels like a total > > > > abuse of platform devices and drivers that really should belong on a > "real" > > > > bus. > > > > > > I am not sure if it is an abuse of platform device. But smmu is a > > > platform device, drivers/iommu/arm-smmu-v3.c is a platform driver. > > > In a typical ARM server, there are maybe multiple SMMU devices which > > > can support IO virtual address and page tables for other devices on > > > PCI-like busses. > > > Each different SMMU device might be close to different NUMA node. > > > There is really a hardware topology. > > > > > > If you have multiple CPU packages in a NUMA server, some platform > > > devices might Belong to CPU0, some other might belong to CPU1. > > > > Those devices are populated by acpi_iort for an ARM server: > > > > drivers/acpi/arm64/iort.c: > > > > static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { > > .name = "arm-smmu-v3", > > .dev_dma_configure = arm_smmu_v3_dma_configure, > > .dev_count_resources = arm_smmu_v3_count_resources, > > .dev_init_resources = arm_smmu_v3_init_resources, > > .dev_set_proximity = arm_smmu_v3_set_proximity, }; > > > > void __init acpi_iort_init(void) > > { > > acpi_status status; > > > > status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table); > > ... > > iort_check_id_count_workaround(iort_table); > > iort_init_platform_devices(); > > } > > > > static void __init iort_init_platform_devices(void) { > > ... > > > > for (i = 0; i < iort->node_count; i++) { > > if (iort_node >= iort_end) { > > pr_err("iort node pointer overflows, bad > table\n"); > > return; > > } > > > > iort_enable_acs(iort_node); > > > > ops = iort_get_dev_cfg(iort_node); > > if (ops) { > > fwnode = acpi_alloc_fwnode_static(); > > if (!fwnode) > > return; > > > > iort_set_fwnode(iort_node, fwnode); > > > > ret = iort_add_platform_device(iort_node, ops); > > if (ret) { > > iort_delete_fwnode(iort_node); > > acpi_free_fwnode_static(fwnode); > > return; > > } > > } > > > > ... > > } > > ... > > } > > > > NUMA node is got from ACPI: > > > > static int __init arm_smmu_v3_set_proximity(struct device *dev, > > struct acpi_iort_node > > *node) { > > struct acpi_iort_smmu_v3 *smmu; > > > > smmu = (struct acpi_iort_smmu_v3 *)node->node_data; > > if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) { > > int dev_node = acpi_map_pxm_to_node(smmu->pxm); > > > > ... > > > > set_dev_node(dev, dev_node); > > ... > > } > > return 0; > > } > > > > Barry > > That's fine, but those are "real" devices, not platform devices, right? > Most platform devices are "real" memory-mapped hardware devices. For an embedded system, almost all "simple-bus" devices are populated from device trees as platform devices. Only a part of platform devices are not "real" hardware. Smmu is a memory-mapped device. It is totally like most other platform devices populated in a memory space mapped in cpu's local space. It uses ioremap to map registers, use readl/writel to read/write its space. > What platform device has this issue? What one will show up this way with > the new patch? if platform device shouldn't be a real hardware, there is no platform device with a hardware topology. But platform devices are "real" hardware at most time. Smmu is a "real" device, but it is a platform device in Linux. > > thanks, > > greg k-h -barry
> > > > On Tue, Jun 02, 2020 at 05:09:57AM +0000, Song Bao Hua (Barry Song) > wrote: > > > > > > > > > > Platform devices are NUMA? That's crazy, and feels like a total > > > > > abuse of platform devices and drivers that really should belong > > > > > on a > > "real" > > > > > bus. > > > > > > > > I am not sure if it is an abuse of platform device. But smmu is a > > > > platform device, drivers/iommu/arm-smmu-v3.c is a platform driver. > > > > In a typical ARM server, there are maybe multiple SMMU devices > > > > which can support IO virtual address and page tables for other > > > > devices on PCI-like busses. > > > > Each different SMMU device might be close to different NUMA node. > > > > There is really a hardware topology. > > > > > > > > If you have multiple CPU packages in a NUMA server, some platform > > > > devices might Belong to CPU0, some other might belong to CPU1. > > > > > > Those devices are populated by acpi_iort for an ARM server: > > > > > > drivers/acpi/arm64/iort.c: > > > > > > static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { > > > .name = "arm-smmu-v3", > > > .dev_dma_configure = arm_smmu_v3_dma_configure, > > > .dev_count_resources = arm_smmu_v3_count_resources, > > > .dev_init_resources = arm_smmu_v3_init_resources, > > > .dev_set_proximity = arm_smmu_v3_set_proximity, }; > > > > > > void __init acpi_iort_init(void) > > > { > > > acpi_status status; > > > > > > status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table); > > > ... > > > iort_check_id_count_workaround(iort_table); > > > iort_init_platform_devices(); } > > > > > > static void __init iort_init_platform_devices(void) { > > > ... > > > > > > for (i = 0; i < iort->node_count; i++) { > > > if (iort_node >= iort_end) { > > > pr_err("iort node pointer overflows, bad > > table\n"); > > > return; > > > } > > > > > > iort_enable_acs(iort_node); > > > > > > ops = iort_get_dev_cfg(iort_node); > > > if (ops) { > > > fwnode = acpi_alloc_fwnode_static(); > > > if (!fwnode) > > > return; > > > > > > iort_set_fwnode(iort_node, fwnode); > > > > > > ret = iort_add_platform_device(iort_node, > ops); > > > if (ret) { > > > iort_delete_fwnode(iort_node); > > > acpi_free_fwnode_static(fwnode); > > > return; > > > } > > > } > > > > > > ... > > > } > > > ... > > > } > > > > > > NUMA node is got from ACPI: > > > > > > static int __init arm_smmu_v3_set_proximity(struct device *dev, > > > struct > acpi_iort_node > > > *node) { > > > struct acpi_iort_smmu_v3 *smmu; > > > > > > smmu = (struct acpi_iort_smmu_v3 *)node->node_data; > > > if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) { > > > int dev_node = acpi_map_pxm_to_node(smmu->pxm); > > > > > > ... > > > > > > set_dev_node(dev, dev_node); > > > ... > > > } > > > return 0; > > > } > > > > > > Barry > > > > That's fine, but those are "real" devices, not platform devices, right? > > > > Most platform devices are "real" memory-mapped hardware devices. For an > embedded system, almost all "simple-bus" > devices are populated from device trees as platform devices. Only a part of > platform devices are not "real" hardware. > > Smmu is a memory-mapped device. It is totally like most other platform > devices populated in a memory space mapped in cpu's local space. It uses > ioremap to map registers, use readl/writel to read/write its space. > > > What platform device has this issue? What one will show up this way > > with the new patch? Meanwhile, this patch also only shows numa_node for those platform devices with "real" hardware and acpi support. For those platform devices which are not "real", numa_node sys entry will not be created as dev_to_node(dev) == NUMA_NO_NODE. For instances, smmu is a platform device with "real" hardware backend, it gets a numa_node like: root@ubuntu:/sys/bus/platform/devices/arm-smmu-v3.0.auto# cat numa_node 0 root@ubuntu:/sys/bus/platform/devices/arm-smmu-v3.7.auto# cat numa_node 2 snd-soc-dummy is a platform device without "real" hardware, then it gets no numa_node: root@ubuntu:/sys/bus/platform/devices/snd-soc-dummy# ls driver driver_override modalias power subsystem uevent -barry > > if platform device shouldn't be a real hardware, there is no platform device > with a hardware topology. > But platform devices are "real" hardware at most time. Smmu is a "real" device, > but it is a platform device in Linux. > > > > > thanks, > > > > greg k-h > > -barry
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index b27d0f6c18c9..7794b9a38d82 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -1062,13 +1062,37 @@ static ssize_t driver_override_show(struct device *dev, } static DEVICE_ATTR_RW(driver_override); +static ssize_t numa_node_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", dev_to_node(dev)); +} +static DEVICE_ATTR_RO(numa_node); + +static umode_t platform_dev_attrs_visible(struct kobject *kobj, struct attribute *a, + int n) +{ + struct device *dev = container_of(kobj, typeof(*dev), kobj); + + if (a == &dev_attr_numa_node.attr && + dev_to_node(dev) == NUMA_NO_NODE) + return 0; + + return a->mode; +} static struct attribute *platform_dev_attrs[] = { &dev_attr_modalias.attr, + &dev_attr_numa_node.attr, &dev_attr_driver_override.attr, NULL, }; -ATTRIBUTE_GROUPS(platform_dev); + +static struct attribute_group platform_dev_group = { + .attrs = platform_dev_attrs, + .is_visible = platform_dev_attrs_visible, +}; +__ATTRIBUTE_GROUPS(platform_dev); static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) {
For some platform devices like iommu, particually ARM smmu, users may care about the numa locality. for example, if threads and drivers run near iommu, they may get much better performance on dma_unmap_sg. For other platform devices, users may still want to know the hardware topology. Cc: Prime Zeng <prime.zeng@hisilicon.com> Cc: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> --- drivers/base/platform.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)