Re: [PATCH] drivers: base: cacheinfo: Ensure cpu hotplug work is done before Intel RDT

[Rafael J. Wysocki]

On Thu, May 30, 2019 at 6:10 PM James Morse <james.morse@xxxxxxx> wrote:
>
> The cacheinfo structures are alloced/freed by cpu online/offline
> callbacks. Originally these were only used by sysfs to expose the
> cache topology to user space. Without any in-kernel dependencies
> CPUHP_AP_ONLINE_DYN was an appropriate choice.
>
> resctrl has started using these structures to identify CPUs that
> share a cache. It updates its 'domain' structures from cpu
> online/offline callbacks. These depend on the cacheinfo structures
> (resctrl_online_cpu()->domain_add_cpu()->get_cache_id()->
>  get_cpu_cacheinfo()).
> These also run as CPUHP_AP_ONLINE_DYN.
>
> Now that there is an in-kernel dependency, move the cacheinfo
> work earlier so we know its done before resctrl's CPUHP_AP_ONLINE_DYN
> work runs.
>
> Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Cc: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Signed-off-by: James Morse <james.morse@xxxxxxx>

This looks reasonable to me, but I would send the patch to Thomas
Gleixner (with CCs to Tony Luck and Boris Petkov in addition to your
original CC list).

> ---
> I haven't seen any problems because of this. If someone thinks it should
> go to stable:
> Cc: <stable@xxxxxxxxxxxxxxx> #4.10.x
>
> The particular patch that added RDT is:
> Fixes: 2264d9c74dda1 ("x86/intel_rdt: Build structures for each resource based on cache topology")

IMO it is OK to add a Fixes: tag if your patch is needed on top of the
other on for everything to work as expected, regardless of what pieces
of code are touched by each of them.  That information is useful to
people who need to make backporting decisions, for example (if they
decide to backport the original patch, they would probably want to
backport your patch on top of it).

> But as this touches a different set of files, I'm not sure how appropriate
> a fixes tag is.
>
>  drivers/base/cacheinfo.c   | 3 ++-
>  include/linux/cpuhotplug.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index a7359535caf5..b444f89a2041 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -655,7 +655,8 @@ static int cacheinfo_cpu_pre_down(unsigned int cpu)
>
>  static int __init cacheinfo_sysfs_init(void)
>  {
> -       return cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "base/cacheinfo:online",
> +       return cpuhp_setup_state(CPUHP_AP_BASE_CACHEINFO_ONLINE,
> +                                "base/cacheinfo:online",
>                                  cacheinfo_cpu_online, cacheinfo_cpu_pre_down);
>  }
>  device_initcall(cacheinfo_sysfs_init);
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 6a381594608c..50c893f03c21 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -175,6 +175,7 @@ enum cpuhp_state {
>         CPUHP_AP_WATCHDOG_ONLINE,
>         CPUHP_AP_WORKQUEUE_ONLINE,
>         CPUHP_AP_RCUTREE_ONLINE,
> +       CPUHP_AP_BASE_CACHEINFO_ONLINE,
>         CPUHP_AP_ONLINE_DYN,
>         CPUHP_AP_ONLINE_DYN_END         = CPUHP_AP_ONLINE_DYN + 30,
>         CPUHP_AP_X86_HPET_ONLINE,
> --
> 2.20.1
>