Messages in this thread | | | Date | Sun, 7 Apr 2024 12:21:17 -0700 | Subject | Re: [PATCH v1 1/6] x86/resctrl: Move __resctrl_sched_in() out-of-line | From | Reinette Chatre <> |
| |
Hi Peter,
On 4/5/2024 3:04 PM, Peter Newman wrote: > 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.
I expect that each architecture would need architecture specific task switching code and by pointing to rdtgroup from the task_struct every architecture would need to know how to reach the CLOSID/RMID within.
Having architectures reach into the private fs data is not ideal so we should consider to make just a portion of rdtgroup information required to support switching code accessible to the architectures, not the entire rdtgroup and mongroup structures.
..
>>> +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.
I think this needs more thought. rdt_enable_key is x86 specific now and should not be in the fs code. Every architecture will have its own task switch code, with __resctrl_sched_in() belonging to x86 and thus not something to be directly called from the fs code.
Reinette
| |