Received-SPF: pass (google.com: domain of linux-kernel+bounces-133662-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240325172707.73966-1-peternewman@google.com>
 <20240325172707.73966-2-peternewman@google.com> <e2c479b6-43f0-4e2c-9ccd-2e5f322d1974@intel.com>
In-Reply-To: <e2c479b6-43f0-4e2c-9ccd-2e5f322d1974@intel.com>
From: Peter Newman <peternewman@google.com>
Date: Fri, 5 Apr 2024 15:04:11 -0700
Message-ID: <CALPaoCjWDKM9VYXexDt2+zh2srTotuaErFHXkWzH1w0c_KWDjw@mail.gmail.com>
Subject: Re: [PATCH v1 1/6] x86/resctrl: Move __resctrl_sched_in() out-of-line
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>, James Morse <james.morse@arm.com>, 
	Stephane Eranian <eranian@google.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, 
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, 
	"H. Peter Anvin" <hpa@zytor.com>, Peter Zijlstra <peterz@infradead.org>, Juri Lelli <juri.lelli@redhat.com>, 
	Vincent Guittot <vincent.guittot@linaro.org>, Dietmar Eggemann <dietmar.eggemann@arm.com>, 
	Steven Rostedt <rostedt@goodmis.org>, Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>, 
	Daniel Bristot de Oliveira <bristot@redhat.com>, Valentin Schneider <vschneid@redhat.com>, 
	Uros Bizjak <ubizjak@gmail.com>, Mike Rapoport <rppt@kernel.org>, 
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>, 
	Rick Edgecombe <rick.p.edgecombe@intel.com>, Xin Li <xin3.li@intel.com>, 
	Babu Moger <babu.moger@amd.com>, Shaopeng Tan <tan.shaopeng@fujitsu.com>, 
	Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>, Jens Axboe <axboe@kernel.dk>, 
	Christian Brauner <brauner@kernel.org>, Oleg Nesterov <oleg@redhat.com>, 
	Andrew Morton <akpm@linux-foundation.org>, Tycho Andersen <tandersen@netflix.com>, 
	Nicholas Piggin <npiggin@gmail.com>, Beau Belgrave <beaub@linux.microsoft.com>, 
	"Matthew Wilcox (Oracle)" <willy@infradead.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Reinette,

On Thu, Apr 4, 2024 at 4:09=E2=80=AFPM 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 rdt=
group
> 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() onl=
y
> > 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/resctr=
l.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_resour=
ce *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 ID=
s 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 mounte=
d.
> > + *
> > + * 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_R=
ESCTRL
> is not set.

Will do.

Thanks!
-Peter