Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f994d403-df7b-d88e-8324-c29d0ef2034e@amd.com>
Date:   Tue, 2 Aug 2022 11:41:42 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
From:   Ravi Bangoria <ravi.bangoria@amd.com>
Subject: Re: [RFC v2] perf: Rewrite core context handling
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     acme@kernel.org, alexander.shishkin@linux.intel.com,
        jolsa@redhat.com, namhyung@kernel.org, songliubraving@fb.com,
        eranian@google.com, alexey.budankov@linux.intel.com,
        ak@linux.intel.com, mark.rutland@arm.com, megha.dey@intel.com,
        frederic@kernel.org, maddy@linux.ibm.com, irogers@google.com,
        kim.phillips@amd.com, linux-kernel@vger.kernel.org,
        santosh.shukla@amd.com, ravi.bangoria@amd.com
References: <20220113134743.1292-1-ravi.bangoria@amd.com>
 <YqdLH+ZU/sf4n0pa@hirez.programming.kicks-ass.net>
 <YqdL8LsOvxNqhz/v@hirez.programming.kicks-ass.net>
Content-Language: en-US
In-Reply-To: <YqdL8LsOvxNqhz/v@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WFViNXhWN0FBK0JUZ1ZheTBWcE1FREE4REdCUlVvRWZJRituTXdCUW10NHh2?=
 =?utf-8?B?UmswYnZtbUN3NlhZRlJXV2Z4dnlndExpQ09wc2x0OHZBZC9FUHZiTkIvM1M1?=
 =?utf-8?B?a2dHeHJLZXNQNlFTMzZUclNBNGlWcG1wNkpWVU1JVnJiVVdNaGFic0Z0enpR?=
 =?utf-8?B?MS9uRGEweGdwaGtqZCtFZkxmbzYrM2Y3QW85S0dHTm04Ty9ISGM1UnVvV0FG?=
 =?utf-8?B?WGo5K0pmU0VPZzZDc0JnMTZzOXJOcE12TkFTYWw0YjdmRlhBVnpCanZLbVdv?=
 =?utf-8?B?LytwR0g0US9vZjMwTFR2U1Y4MlhxcGFuWVJNRmp6cXV3UlFWby9KUGt6OUE5?=
 =?utf-8?B?TWptZFNtUzB1OGxEMjFRNWI2a1h2c3UxQzh2ZWFXSnJpZXhKdERvUkZZTVV1?=
 =?utf-8?B?QVVuV29PK04vNEQ1ejRVOW9nOEdIL0ZqaysrNzhCVDBibVJTb3FOOWlVNVJ4?=
 =?utf-8?B?aW1Yb0UvUlJGWTZxTGhibmZNbGFIY21CMEQ1d1U1a21kdkJ4RlNsQWNwb1Jw?=
 =?utf-8?B?TkE2YjIyaWw1VE03ajNmL3BiWTEreWZTSlRsYTV4eW9PaGdWREdCQlN3Mzdj?=
 =?utf-8?B?SkJicnVDbnhVem9HZTl4MGlUL0YxRTAvaHJFM09MbDZNaS9OZXFXb21lT0sv?=
 =?utf-8?B?MHZIM0FDRHhJMExiMEVqZjdQclVDWm1weE5vaXdXSzgySEFZUWJuRzE5K1hE?=
 =?utf-8?B?RE1PcE9QbDZTWkNwaTJ0elJRVGhuMjN1cHU0eWdkUWdaVXo0S3RncERaTjk3?=
 =?utf-8?B?RGVScmhnbnR6VVZmdEp5K1VOSkRTMDdadTFuNVB1S0JGaUlhUDc2blYzQThS?=
 =?utf-8?B?Nm9iK29OZDlGaXdRdWpsRk11QTVUcWc5RlducW95aUZ6QlRNUXRaZnRRckIv?=
 =?utf-8?B?dm5mc0lYRXhCdjI4aWZhRnFMZndSaytkWFR5UzUxOXQ3dzExdTNMTHZVQmE1?=
 =?utf-8?B?bUtHVWEveUlWSll4Y3RrTzRMc0hSa2lpeUo0UXdxUTNjRDRoc3JjNjBCQjJB?=
 =?utf-8?B?MnM1VkZIWlNUMVBxT1hDVG5IY01DMHBDZmlHYnppRS9jZUhVRmhTYUtUUFc0?=
 =?utf-8?B?bHM4amZyMjB5RFJCZ1hVSTNONS9CcjN6T1dabkdJbGpMVmswdlAydXJ1M0d1?=
 =?utf-8?B?UlF5NVdveG5kQ0x6M3JuWkFWeUFEdUxqMytPNUZCL3Brb1E5QzZGSnVrZE95?=
 =?utf-8?B?elRiazY5ald2M1VoZitSb1lUZWZ6S3NMT3I3Q2N0WkpoOURLN1M1U1drTlBT?=
 =?utf-8?B?dHhObG14cmlkMFIzLzd4TkFCSTBKS2c0NWFjSFZKSHAxUEFDZFJTcUo5U1BM?=
 =?utf-8?B?THAwWE0zcmtjM284R1ZOV0FFOExpOFBGRjU2aFB1cVE5ZWZ3VG5OMG56T3Bu?=
 =?utf-8?B?dmI0MFNuSlNMMnNReStKbDRkZHBYeC9nbEtVNFZ2ZHRzMHo3SS9MazNnUVVR?=
 =?utf-8?B?UTR3VzJqcWFneCswNUh2WlpRSHhjWks0ZStXVnN6LzBwdCs1VzN0UExHcXhS?=
 =?utf-8?B?UzRRcTYwL0tWOGlQbTM0SnBaVEp1MnRlRHBBYnpwUTd2TlpvTThkK3NRaklw?=
 =?utf-8?B?WEIyTDF0azFac0wvVjFYRzJwYmgyR3J1SUpqZmhrU01GTGZxc2h5ZmpZS3cx?=
 =?utf-8?B?ZVhMQmtCQ05HRVl2YmM3M1ZqZU9IcWEwQnR1eVF5cjVpbnR5OFhkZHNlaDJm?=
 =?utf-8?B?RFA3Nlh2MTB3eC9hL2I5QTFrYXZpOTM4L3U1anRqdERHRGkrNmI5NGEvL2dw?=
 =?utf-8?B?VXllYlk3OHpKTXcvVnU4SVkreWphZGNpblp2alBkejFzdEdPd3Q1WmpFWjdP?=
 =?utf-8?B?ZnJjYS81NHp1emMxV1gvT3RDQ2h4aEtyckNaY1ZrSmtMMGw0a0czaENiL3F0?=
 =?utf-8?B?TDdNMFRUeU1WbEkvTVNYM1dpTEVidVd5WFA3ZHBtMmYyMnZ2eTBLRVVnWE5W?=
 =?utf-8?B?eS9kWTBjbXFvUVErSDkrTldlMldsTUt6RE1wMzE0dVVQbThjeG43VXFQYjl4?=
 =?utf-8?B?b2thQWpmR2dkSWx0Sll1bVJ4VlVXZWNaSWtjcjllNCtrRGJWb25qUnhNcVJy?=
 =?utf-8?B?K0liRjJPNThrMzBqUmZJSzlsSEV5ZTk5OUdpTnlmMTIrczhSY3phcTFmb1Yx?=
 =?utf-8?Q?eg5avQ2PO0bU7CMr5YXH6t3wI?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebc270a4-c9f9-45d3-f68c-08da744de6f8
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6588.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Aug 2022 06:11:56.6475
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jmemrSJTQ39IE3zJKIioXWwE7jr7AACk/xhf6OBx/FXm+TcetlLnZc13ifDhu4LNKD0jFSkHet29g/nSA9fE7w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB3293
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> pulling up the ctx->mutex makes things simpler, but also violates the
> locking order vs exec_update_lock.
> 
> Pull that lock up as well...

I'm not able to apply this patch as is but I get the idea. Few
questions below...

> 
> ---
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12254,13 +12254,29 @@ SYSCALL_DEFINE5(perf_event_open,
>  	if (pmu->task_ctx_nr == perf_sw_context)
>  		event->event_caps |= PERF_EV_CAP_SOFTWARE;
>  
> +	if (task) {
> +		err = down_read_interruptible(&task->signal->exec_update_lock);
> +		if (err)
> +			goto err_alloc;
> +
> +		/*
> +		 * We must hold exec_update_lock across this and any potential
> +		 * perf_install_in_context() call for this new event to
> +		 * serialize against exec() altering our credentials (and the
> +		 * perf_event_exit_task() that could imply).
> +		 */
> +		err = -EACCES;
> +		if (!perf_check_permission(&attr, task))
> +			goto err_cred;
> +	}
> +
>  	/*
>  	 * Get the target context (task or percpu):
>  	 */
>  	ctx = find_get_context(task, event);
>  	if (IS_ERR(ctx)) {
>  		err = PTR_ERR(ctx);
> -		goto err_alloc;
> +		goto err_cred;
>  	}
>  
>  	mutex_lock(&ctx->mutex);
> @@ -12358,58 +12374,14 @@ SYSCALL_DEFINE5(perf_event_open,
>  			goto err_context;
>  	}
>  
> -	event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, f_flags);
> -	if (IS_ERR(event_file)) {
> -		err = PTR_ERR(event_file);
> -		event_file = NULL;
> -		goto err_context;
> -	}
> -
> -	if (task) {
> -		err = down_read_interruptible(&task->signal->exec_update_lock);
> -		if (err)
> -			goto err_file;
> -
> -		/*
> -		 * We must hold exec_update_lock across this and any potential
> -		 * perf_install_in_context() call for this new event to
> -		 * serialize against exec() altering our credentials (and the
> -		 * perf_event_exit_task() that could imply).
> -		 */
> -		err = -EACCES;
> -		if (!perf_check_permission(&attr, task))
> -			goto err_cred;
> -	}
> -
> -	if (ctx->task == TASK_TOMBSTONE) {
> -		err = -ESRCH;
> -		goto err_locked;
> -	}

I think we need to keep (ctx->task == TASK_TOMBSTONE) check?

> -
>  	if (!perf_event_validate_size(event)) {
>  		err = -E2BIG;
> -		goto err_locked;
> -	}
> -
> -	if (!task) {
> -		/*
> -		 * Check if the @cpu we're creating an event for is online.
> -		 *
> -		 * We use the perf_cpu_context::ctx::mutex to serialize against
> -		 * the hotplug notifiers. See perf_event_{init,exit}_cpu().
> -		 */
> -		struct perf_cpu_context *cpuctx =
> -			container_of(ctx, struct perf_cpu_context, ctx);
> -
> -		if (!cpuctx->online) {
> -			err = -ENODEV;
> -			goto err_locked;
> -		}
> +		goto err_context;

Why did you remove this hunk? We should confirm whether cpu is online or not
before creating event. No?

Thanks,
Ravi