Re: [PATCH v1 1/6] x86/resctrl: Move __resctrl_sched_in() out-of-line

[Reinette Chatre]

Hi Peter,

On 3/25/2024 10:27 AM, Peter Newman wrote:
> __resctrl_sched_in() is unable to dereference a struct rdtgroup pointer
> when defined inline because rdtgroup is a private structure defined in
> internal.h.

Being inline has nothing to do with whether it can reference a struct rdtgroup
pointer, no?

> 
> This function is defined inline to avoid impacting context switch
> performance for the majority of users who aren't using resctrl at all.
> These benefits can already be realized without access to internal
> resctrl data structures.
> 
> The logic of performing an out-of-line call to __resctrl_sched_in() only
> when resctrl is mounted is architecture-independent, so the inline
> definition of resctrl_sched_in() can be moved into linux/resctrl.h.
> 
> Signed-off-by: Peter Newman <peternewman@xxxxxxxxxx>

..

> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 0917c7f25720..8f92a87d381d 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -38,6 +38,7 @@
>  #include <linux/io.h>
>  #include <linux/kdebug.h>
>  #include <linux/syscalls.h>
> +#include <linux/resctrl.h>
>  
>  #include <asm/ldt.h>
>  #include <asm/processor.h>
> @@ -51,7 +52,6 @@
>  #include <asm/debugreg.h>
>  #include <asm/switch_to.h>
>  #include <asm/vm86.h>
> -#include <asm/resctrl.h>
>  #include <asm/proto.h>
>  
>  #include "process.h"
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 7062b84dd467..d442269bb25b 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -40,6 +40,7 @@
>  #include <linux/ftrace.h>
>  #include <linux/syscalls.h>
>  #include <linux/iommu.h>
> +#include <linux/resctrl.h>
>  
>  #include <asm/processor.h>
>  #include <asm/pkru.h>

With a change like this we should be very careful about what is included when
the kernel is not built with resctrl and in its current form linux/resctrl.h is
not ready for this.

If CONFIG_X86_CPU_RESCTRL is not set linux/resctrl.h should have the bare minimum,
just like asm/resctrl.h has.

> @@ -53,7 +54,6 @@
>  #include <asm/switch_to.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/vdso.h>
> -#include <asm/resctrl.h>
>  #include <asm/unistd.h>
>  #include <asm/fsgsbase.h>
>  #include <asm/fred.h>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index a365f67131ec..62d607939a73 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -304,4 +304,25 @@ void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_domain *d);
>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  
> +DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
> +
> +void __resctrl_sched_in(struct task_struct *tsk);
> +
> +/*
> + * resctrl_sched_in() - Assigns the incoming task's control/monitor IDs to the
> + *			current CPU
> + *
> + * To minimize impact to the scheduler hot path, this will stay as no-op unless
> + * running on a system supporting resctrl and the filesystem is mounted.
> + *
> + * Must be called with preemption disabled.
> + */
> +static inline void resctrl_sched_in(struct task_struct *tsk)
> +{
> +#ifdef CONFIG_X86_CPU_RESCTRL
> +	if (static_branch_likely(&rdt_enable_key))
> +		__resctrl_sched_in(tsk);
> +#endif
> +}
> +

include/linux/resctrl.h should rather be divided to accommodate code
as below:

#ifdef CONFIG_X86_CPU_RESCTRL

static inline void resctrl_sched_in(struct task_struct *tsk)
{
	if (static_branch_likely(&rdt_enable_key))
		__resctrl_sched_in(tsk);
}

#else

static inline void resctrl_sched_in(struct task_struct *tsk) {}

#endif

so that core code does not get anything unnecessary when CONFIG_X86_CPU_RESCTRL
is not set.

Reinette