| Message ID | 20230629192119.6613-2-eric.devolder@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | crash: Kernel handling of CPU and memory hot un/plug | expand |
On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote: > - the function body of the callback functions are now wrapped with > IS_ENABLED(); as the callback function must exist now that the > attribute is always compiled-in (though not necessarily visible). Why do you need to do this last thing? Is it a code savings goal? Or something else? The file will not be present in the system if the option is not enabled, so it should be safe to not do this unless you feel it's necessary for some reason? Not doing this would make the diff easier to read :) thanks, greg k-h
On 7/3/23 08:05, Greg KH wrote: > On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote: >> - the function body of the callback functions are now wrapped with >> IS_ENABLED(); as the callback function must exist now that the >> attribute is always compiled-in (though not necessarily visible). > > Why do you need to do this last thing? Is it a code savings goal? Or > something else? The file will not be present in the system if the > option is not enabled, so it should be safe to not do this unless you > feel it's necessary for some reason? To accommodate the request, all DEVICE_ATTR() must be unconditionally present in this file. The DEVICE_ATTR() requires the .show() callback. As the callback is referenced from a data structure, the callback has to be present for link. All the callbacks for these attributes are in this file. I have two basic choices for gutting the function body if the config feature is not enabled. I can either use #ifdef or IS_ENABLED(). Thomas has made it clear I need to use IS_ENABLED(). I can certainly use #ifdef (which is what I did in v24). > > Not doing this would make the diff easier to read :) I agree this is messy. I'm not really sure what this request/effort achieves as these attributes are not strongly related (unlike cacheinfo) and the way the file was before results in less code. At any rate, please indicate if you'd rather I use #ifdef. Thanks for your time! eric > > thanks, > > greg k-h
On 7/3/23 11:53, Eric DeVolder wrote: > > > On 7/3/23 08:05, Greg KH wrote: >> On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote: >>> - the function body of the callback functions are now wrapped with >>> IS_ENABLED(); as the callback function must exist now that the >>> attribute is always compiled-in (though not necessarily visible). >> >> Why do you need to do this last thing? Is it a code savings goal? Or >> something else? The file will not be present in the system if the >> option is not enabled, so it should be safe to not do this unless you >> feel it's necessary for some reason? > > To accommodate the request, all DEVICE_ATTR() must be unconditionally present in this file. The > DEVICE_ATTR() requires the .show() callback. As the callback is referenced from a data structure, > the callback has to be present for link. All the callbacks for these attributes are in this file. > > I have two basic choices for gutting the function body if the config feature is not enabled. I can > either use #ifdef or IS_ENABLED(). Thomas has made it clear I need to use IS_ENABLED(). I can > certainly use #ifdef (which is what I did in v24). > >> >> Not doing this would make the diff easier to read :) > > I agree this is messy. I'm not really sure what this request/effort achieves as these attributes are > not strongly related (unlike cacheinfo) and the way the file was before results in less code. > > At any rate, please indicate if you'd rather I use #ifdef. > Thanks for your time! > eric > >> >> thanks, >> >> greg k-h Hi Greg, I was wondering if you might weigh-in so that I can proceed. I think there are three options on the table: - use #ifdef to comment out these function bodies, which keeps the diff much more readable - use IS_ENABLED() as Thomas has requested I do, but makes the diff more difficult to read - remove this refactor altogether, perhaps post-poning until after this crash hotplug series merges, as this refactor is largely unrelated to crash hotplug. Thank you for your time on this topic! eric
On 7/21/23 11:32, Eric DeVolder wrote: > > > On 7/3/23 11:53, Eric DeVolder wrote: >> >> >> On 7/3/23 08:05, Greg KH wrote: >>> On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote: >>>> - the function body of the callback functions are now wrapped with >>>> IS_ENABLED(); as the callback function must exist now that the >>>> attribute is always compiled-in (though not necessarily visible). >>> >>> Why do you need to do this last thing? Is it a code savings goal? Or >>> something else? The file will not be present in the system if the >>> option is not enabled, so it should be safe to not do this unless you >>> feel it's necessary for some reason? >> >> To accommodate the request, all DEVICE_ATTR() must be unconditionally present in this file. The >> DEVICE_ATTR() requires the .show() callback. As the callback is referenced from a data structure, >> the callback has to be present for link. All the callbacks for these attributes are in this file. >> >> I have two basic choices for gutting the function body if the config feature is not enabled. I can >> either use #ifdef or IS_ENABLED(). Thomas has made it clear I need to use IS_ENABLED(). I can >> certainly use #ifdef (which is what I did in v24). >> >>> >>> Not doing this would make the diff easier to read :) >> >> I agree this is messy. I'm not really sure what this request/effort achieves as these attributes >> are not strongly related (unlike cacheinfo) and the way the file was before results in less code. >> >> At any rate, please indicate if you'd rather I use #ifdef. >> Thanks for your time! >> eric >> >>> >>> thanks, >>> >>> greg k-h > > Hi Greg, > I was wondering if you might weigh-in so that I can proceed. > > I think there are three options on the table: > - use #ifdef to comment out these function bodies, which keeps the diff much more readable > - use IS_ENABLED() as Thomas has requested I do, but makes the diff more difficult to read > - remove this refactor altogether, perhaps post-poning until after this crash hotplug series merges, > as this refactor is largely unrelated to crash hotplug. > > Thank you for your time on this topic! > eric Hi Greg, If you have an opinion on how to proceed, please provide. Thanks, eric
On Thu, Aug 03, 2023 at 01:20:28PM -0500, Eric DeVolder wrote: > > > On 7/21/23 11:32, Eric DeVolder wrote: > > > > > > On 7/3/23 11:53, Eric DeVolder wrote: > > > > > > > > > On 7/3/23 08:05, Greg KH wrote: > > > > On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote: > > > > > - the function body of the callback functions are now wrapped with > > > > > IS_ENABLED(); as the callback function must exist now that the > > > > > attribute is always compiled-in (though not necessarily visible). > > > > > > > > Why do you need to do this last thing? Is it a code savings goal? Or > > > > something else? The file will not be present in the system if the > > > > option is not enabled, so it should be safe to not do this unless you > > > > feel it's necessary for some reason? > > > > > > To accommodate the request, all DEVICE_ATTR() must be > > > unconditionally present in this file. The DEVICE_ATTR() requires the > > > .show() callback. As the callback is referenced from a data > > > structure, the callback has to be present for link. All the > > > callbacks for these attributes are in this file. > > > > > > I have two basic choices for gutting the function body if the config > > > feature is not enabled. I can either use #ifdef or IS_ENABLED(). > > > Thomas has made it clear I need to use IS_ENABLED(). I can certainly > > > use #ifdef (which is what I did in v24). > > > > > > > > > > > Not doing this would make the diff easier to read :) > > > > > > I agree this is messy. I'm not really sure what this request/effort > > > achieves as these attributes are not strongly related (unlike > > > cacheinfo) and the way the file was before results in less code. > > > > > > At any rate, please indicate if you'd rather I use #ifdef. > > > Thanks for your time! > > > eric > > > > > > > > > > > thanks, > > > > > > > > greg k-h > > > > Hi Greg, > > I was wondering if you might weigh-in so that I can proceed. > > > > I think there are three options on the table: > > - use #ifdef to comment out these function bodies, which keeps the diff much more readable > > - use IS_ENABLED() as Thomas has requested I do, but makes the diff more difficult to read > > - remove this refactor altogether, perhaps post-poning until after this > > crash hotplug series merges, as this refactor is largely unrelated to > > crash hotplug. > > > > Thank you for your time on this topic! > > eric > > Hi Greg, > If you have an opinion on how to proceed, please provide. Sorry, totally swamped by "stuff". I don't know, use your judgement here and send a new version, don't wait for me to weigh in on design decisions for longer than a week. thanks, greg k-h
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index c1815b9dae68..2455cbcebc87 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -82,24 +82,27 @@ void unregister_cpu(struct cpu *cpu) per_cpu(cpu_sys_devices, logical_cpu) = NULL; return; } +#endif /* CONFIG_HOTPLUG_CPU */ -#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE static ssize_t cpu_probe_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - ssize_t cnt; - int ret; + if (IS_ENABLED(CONFIG_ARCH_CPU_PROBE_RELEASE)) { + ssize_t cnt; + int ret; - ret = lock_device_hotplug_sysfs(); - if (ret) - return ret; + ret = lock_device_hotplug_sysfs(); + if (ret) + return ret; - cnt = arch_cpu_probe(buf, count); + cnt = arch_cpu_probe(buf, count); - unlock_device_hotplug(); - return cnt; + unlock_device_hotplug(); + return cnt; + } + return 0; } static ssize_t cpu_release_store(struct device *dev, @@ -107,23 +110,24 @@ static ssize_t cpu_release_store(struct device *dev, const char *buf, size_t count) { - ssize_t cnt; - int ret; + if (IS_ENABLED(CONFIG_ARCH_CPU_PROBE_RELEASE)) { + ssize_t cnt; + int ret; - ret = lock_device_hotplug_sysfs(); - if (ret) - return ret; + ret = lock_device_hotplug_sysfs(); + if (ret) + return ret; - cnt = arch_cpu_release(buf, count); + cnt = arch_cpu_release(buf, count); - unlock_device_hotplug(); - return cnt; + unlock_device_hotplug(); + return cnt; + } + return 0; } static DEVICE_ATTR(probe, S_IWUSR, NULL, cpu_probe_store); static DEVICE_ATTR(release, S_IWUSR, NULL, cpu_release_store); -#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */ -#endif /* CONFIG_HOTPLUG_CPU */ #ifdef CONFIG_KEXEC #include <linux/kexec.h> @@ -273,14 +277,14 @@ static ssize_t print_cpus_isolated(struct device *dev, } static DEVICE_ATTR(isolated, 0444, print_cpus_isolated, NULL); -#ifdef CONFIG_NO_HZ_FULL static ssize_t print_cpus_nohz_full(struct device *dev, struct device_attribute *attr, char *buf) { - return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(tick_nohz_full_mask)); + if (IS_ENABLED(CONFIG_NO_HZ_FULL)) + return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(tick_nohz_full_mask)); + return 0; } static DEVICE_ATTR(nohz_full, 0444, print_cpus_nohz_full, NULL); -#endif static void cpu_device_release(struct device *dev) { @@ -301,30 +305,32 @@ static void cpu_device_release(struct device *dev) */ } -#ifdef CONFIG_GENERIC_CPU_AUTOPROBE static ssize_t print_cpu_modalias(struct device *dev, struct device_attribute *attr, char *buf) { int len = 0; - u32 i; - - len += sysfs_emit_at(buf, len, - "cpu:type:" CPU_FEATURE_TYPEFMT ":feature:", - CPU_FEATURE_TYPEVAL); - - for (i = 0; i < MAX_CPU_FEATURES; i++) - if (cpu_have_feature(i)) { - if (len + sizeof(",XXXX\n") >= PAGE_SIZE) { - WARN(1, "CPU features overflow page\n"); - break; + if (IS_ENABLED(CONFIG_GENERIC_CPU_AUTOPROBE)) { + u32 i; + + len += sysfs_emit_at(buf, len, + "cpu:type:" CPU_FEATURE_TYPEFMT ":feature:", + CPU_FEATURE_TYPEVAL); + + for (i = 0; i < MAX_CPU_FEATURES; i++) + if (cpu_have_feature(i)) { + if (len + sizeof(",XXXX\n") >= PAGE_SIZE) { + WARN(1, "CPU features overflow page\n"); + break; + } + len += sysfs_emit_at(buf, len, ",%04X", i); } - len += sysfs_emit_at(buf, len, ",%04X", i); - } - len += sysfs_emit_at(buf, len, "\n"); + len += sysfs_emit_at(buf, len, "\n"); + } return len; } +#ifdef CONFIG_GENERIC_CPU_AUTOPROBE static int cpu_uevent(const struct device *dev, struct kobj_uevent_env *env) { char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL); @@ -451,32 +457,61 @@ struct device *cpu_device_create(struct device *parent, void *drvdata, } EXPORT_SYMBOL_GPL(cpu_device_create); -#ifdef CONFIG_GENERIC_CPU_AUTOPROBE static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL); -#endif static struct attribute *cpu_root_attrs[] = { -#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE &dev_attr_probe.attr, &dev_attr_release.attr, -#endif &cpu_attrs[0].attr.attr, &cpu_attrs[1].attr.attr, &cpu_attrs[2].attr.attr, &dev_attr_kernel_max.attr, &dev_attr_offline.attr, &dev_attr_isolated.attr, -#ifdef CONFIG_NO_HZ_FULL &dev_attr_nohz_full.attr, -#endif -#ifdef CONFIG_GENERIC_CPU_AUTOPROBE &dev_attr_modalias.attr, -#endif NULL }; +static umode_t +cpu_root_attr_is_visible(struct kobject *kobj, + struct attribute *attr, int unused) +{ + umode_t mode = attr->mode; + + if (IS_ENABLED(CONFIG_ARCH_CPU_PROBE_RELEASE)) { + if (attr == &dev_attr_probe.attr) + return mode; + if (attr == &dev_attr_release.attr) + return mode; + } + if (attr == &cpu_attrs[0].attr.attr) + return mode; + if (attr == &cpu_attrs[1].attr.attr) + return mode; + if (attr == &cpu_attrs[2].attr.attr) + return mode; + if (attr == &dev_attr_kernel_max.attr) + return mode; + if (attr == &dev_attr_offline.attr) + return mode; + if (attr == &dev_attr_isolated.attr) + return mode; + if (IS_ENABLED(CONFIG_NO_HZ_FULL)) { + if (attr == &dev_attr_nohz_full.attr) + return mode; + } + if (IS_ENABLED(CONFIG_GENERIC_CPU_AUTOPROBE)) { + if (attr == &dev_attr_modalias.attr) + return mode; + } + + return 0; +} + static const struct attribute_group cpu_root_attr_group = { .attrs = cpu_root_attrs, + .is_visible = cpu_root_attr_is_visible, }; static const struct attribute_group *cpu_root_attr_groups[] = { diff --git a/include/linux/tick.h b/include/linux/tick.h index 9459fef5b857..a05fdd4b21f4 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -174,9 +174,9 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } static inline void tick_nohz_idle_stop_tick_protected(void) { } #endif /* !CONFIG_NO_HZ_COMMON */ +extern cpumask_var_t tick_nohz_full_mask; #ifdef CONFIG_NO_HZ_FULL extern bool tick_nohz_full_running; -extern cpumask_var_t tick_nohz_full_mask; static inline bool tick_nohz_full_enabled(void) {
Greg Kroah-Hartman requested that this file use the .is_visible() method instead of #ifdefs for the attributes in cpu.c. static struct attribute *cpu_root_attrs[] = { #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE &dev_attr_probe.attr, &dev_attr_release.attr, #endif &cpu_attrs[0].attr.attr, &cpu_attrs[1].attr.attr, &cpu_attrs[2].attr.attr, &dev_attr_kernel_max.attr, &dev_attr_offline.attr, &dev_attr_isolated.attr, #ifdef CONFIG_NO_HZ_FULL &dev_attr_nohz_full.attr, #endif #ifdef CONFIG_GENERIC_CPU_AUTOPROBE &dev_attr_modalias.attr, #endif NULL }; To that end: - the .is_visible() method is implemented, and IS_ENABLED(), rather than #ifdef, is used to determine the visibility of the attribute. - the DEVICE_ATTR() attributes are moved outside of #ifdefs, so that those structs are always present for the cpu_root_attrs[]. - the function body of the callback functions are now wrapped with IS_ENABLED(); as the callback function must exist now that the attribute is always compiled-in (though not necessarily visible). No functionality change intended. Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> --- drivers/base/cpu.c | 125 +++++++++++++++++++++++++++---------------- include/linux/tick.h | 2 +- 2 files changed, 81 insertions(+), 46 deletions(-)