Received-SPF: pass (google.com: domain of linux-kernel+bounces-132236-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <e2c479b6-43f0-4e2c-9ccd-2e5f322d1974@intel.com>
Date: Thu, 4 Apr 2024 16:09:23 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 1/6] x86/resctrl: Move __resctrl_sched_in() out-of-line
To: Peter Newman <peternewman@google.com>, Fenghua Yu <fenghua.yu@intel.com>,
	James Morse <james.morse@arm.com>
CC: 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>
References: <20240325172707.73966-1-peternewman@google.com>
 <20240325172707.73966-2-peternewman@google.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240325172707.73966-2-peternewman@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WW5wZ0NXL3pVMFg1WGNMdTRtTFZkR1R5M0hueW9YNUVPNi9DcXMvOTBZYVox?=
 =?utf-8?B?VGlqQnYvSjg0Wm9WYVZsUFZVaDFLK016M3Jsd01DTDJUVkRZdGZZNk5sUkE2?=
 =?utf-8?B?QnZFVFQ1dVh2NVpaNURsQklqSVRXTEJuLytwK05oay9kellmVGUxUkZHTlg3?=
 =?utf-8?B?SjUzRDhSZmVORis3eHZkWXZNS096Rmk0emR2ZXJZSmN1L1AyUUF5Q082OFNO?=
 =?utf-8?B?TVozaGcwM3NnL1NNYW4vTzc5UjJUZzlyRFI0R2hSbWw3ZXBLNmVRaWFnRDF1?=
 =?utf-8?B?TmJYWG5GejF2aHcvR01YNytITEo2SDZWRzJjTjhIZmYrVWsxWTlpUzd3eGQy?=
 =?utf-8?B?NXlUb0YzdHoyZ29wL1NjZ0VpUlVneWM3RnFBRXRPREhqUzJkU3hhQUNqWFpy?=
 =?utf-8?B?UC9LWXZVT1pCYWRzWHJKQWRFSWNkdjFwZmVRbE5EYlJVcjlVM1pVakFhSCt6?=
 =?utf-8?B?Z3BNN2dXZGZ0ZmIyS0FJVDlnQ3hCRlowcTU3SmZQN0t1QjRjeDArYWZWR1R5?=
 =?utf-8?B?NTI3ZmdlM0JiUUdZaEJUS21qcVdJVzVSVk43cTIxVTNQTER4ejJKTlNZZUxD?=
 =?utf-8?B?dWtaRC9qd2ZxdytORzVlRXh4enE2TGNIQnQzUkRkVklUbUpXaVhRQXFaT2JQ?=
 =?utf-8?B?dkMrbjFvZmJMcE5uN2NPTTNYMmJoTkJNVjg3N0JXSVA4THc0ZVVSRUExSWlB?=
 =?utf-8?B?bXFzaGR6RnIvbDBENTVmN2dTYVpJVVo5UEFqcDdUYkk5ckJ5QU1yL1ppRkl4?=
 =?utf-8?B?TnNKaXlSbWErWFB4ZEVJcGpNUVhGSmNYUVQ4VXJKNXFHQzdaQ0lHaXovVlVh?=
 =?utf-8?B?VlArbUJYNUcwa0FmY3d4Y0tzc3RRRXRlcStNY1V5SzhuK2RjazE2eU43YTU0?=
 =?utf-8?B?ZC9TYUVJTDBxQkZ4ZTlZSHJpUzVTdHBsNUlqTE1WUGRsb3VmRzcvRkRhSFQw?=
 =?utf-8?B?bWpFNHd2OGNXK3IzM3p1YStKREZRcVV0TjNXSytTQ1JQdnd5V0xvejBuK2dy?=
 =?utf-8?B?V2UyN2JjT2E3VUl3RXI0MEV5TXFWSGtTcDRYTDdvalMyc1dOL05qN2t0Vmc0?=
 =?utf-8?B?TDhkT1IvTGN2cU83c1RjVFNYTmIvblp0eERIaUlVR2R0My8xWkkwMFRrUHRE?=
 =?utf-8?B?MWp3YmhFS1M4TW1maFhreGxiQXlKKzFXN1g2V1VXQWc2RUNvZFVjVE4rQ2hV?=
 =?utf-8?B?TUhhQkw5dWFxRjBVdk8wTWM1d3BOQ1V1SG4yMWdtYkk1ellTL3lCQngySXdX?=
 =?utf-8?B?MmJTVUE5SVpZdzBNSnQzMkRIeXNoMjB6dXJkMFVxU21HaDJuY0FNdjVmazJn?=
 =?utf-8?B?MTI0ZXZZeVU4ZDcvQ2p0d09SNlpxQjh1T1lOWHpXWTgvZGdZNW5mbm9wU0VH?=
 =?utf-8?B?VDhQd2prMFdWY1lUcjNNak1pZmg3UnlDZUJHZFAvNVdkekpEeldaa3dObFNq?=
 =?utf-8?B?eDJxNXlIWUVYVnlWMjQwWXBTdnVlMFMzSTNCTUpwemZYZk54bUpOcG1qOUlY?=
 =?utf-8?B?bWttUkZ6YzRCOS80b1NzZmlPMmRNUHJVWEIzcVJtUWFRMmFyK0U0MnNqOWlF?=
 =?utf-8?B?VnN2cENkd3hpaHN4U05RUmRCbFN5OUxoL2hRSWFIZ0JkMXBabVErVFZWZkx6?=
 =?utf-8?B?dG5nTGVZMHVOZjJoUWZiV1gzdGszTWp5UFZNTnB3Qkt0TERWWFZZMVhBODg0?=
 =?utf-8?B?L3BGbkpkbCtMUVRZMVc0b1lORE1tK2Z5KzBzMFFwZWdlV2VyNGRUcHJpRWZO?=
 =?utf-8?B?SHduWlcwd0gxcVJPQUk2dW01Ukl5anV3MDlLUnd2WHNjcS9wbk8xMUZoWmJa?=
 =?utf-8?B?U1JMOFl3dWc5WlByMDRCU2NPMFcwMnVKdjFvVUZmYi9wWGNzR2xWOWNQV01G?=
 =?utf-8?B?dHlKbjBCQ3M4blRWclV5QzZVUE4wc0tCZWxWRkxpNkZsL09Rc09XMU9LdFBo?=
 =?utf-8?B?cFlJVFY2a3NkejkxanI0eGV6QXdCdkU0NTNLU2l3YitVTFlyTEhDYWxOMElm?=
 =?utf-8?B?Y2xWV2VlajRCSks0SjhxYndjNy9pb3BPZ1FJYVFTMXpoOXRBakdvdnZKU0Jq?=
 =?utf-8?B?cmg0U1lvWlNNSVJqeUpjZWozWGxuNmIzWTcvKzloN1VzcE1yaUVGSVExOVpv?=
 =?utf-8?B?TEsxQU43UTNPczhIY0JWRW8raGN5Q2VvUm1KNTArWk9vNExWaC9jSVBIdE11?=
 =?utf-8?B?a3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 29082866-c3b7-4284-075d-08dc54fc465f
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2024 23:09:27.0155
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 08pO1rd3sl/00nAYDG0V/kx2BkgqFGmqh4EGyHjCH0ygZsS5vB0OlAxe5+znrZ0av62Mw0UqAu6t8XCU6Y7uB/5nXoCmbha3HQGNbCKESeM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5069
X-OriginatorOrg: intel.com

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@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.

Reinette