Messages in this thread | | | From | Peter Newman <> | Date | Fri, 5 Apr 2024 15:04:11 -0700 | Subject | Re: [PATCH v1 1/6] x86/resctrl: Move __resctrl_sched_in() out-of-line |
| |
Hi Reinette,
On Thu, Apr 4, 2024 at 4:09 PM Reinette Chatre <reinette.chatre@intel.com> wrote: > > 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?
No, but it has a lot to do with whether it can de-reference a struct rdtgroup pointer in order to obtain a CLOSID or RMID, as doing so would pull the definitions for struct rdtgroup and struct mongroup into an external header. Before doing so, I would want to make sure implementing __resctrl_sched_in() inline is actually adding value.
> > > > > 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@google.com> > > ... > > > 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.
Will do.
Thanks! -Peter
| |