From patchwork Thu Sep 21 14:15:12 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sebastian Andrzej Siewior X-Patchwork-Id: 13393952 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 96C51E71072 for ; Thu, 21 Sep 2023 14:15:30 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CE7C110E137; Thu, 21 Sep 2023 14:15:29 +0000 (UTC) Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3E06710E5AF for ; Thu, 21 Sep 2023 14:15:27 +0000 (UTC) From: Sebastian Andrzej Siewior DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1695305725; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FOWN2bP3bL+i8WU1AdRgy2WvLGaYMKjJkqdur8ZMT5U=; b=zORUY5Xkv3wz1yjY4DDKkQzFNInGAaoCeKEqG8+SzR8M7zDT0N1ruCuMNVKDpM/fKlgh73 fCJ9m049d4T1G6He4zIdup4YW9DyLZfJ3UttPGqrGWJ9oE7ZDP7ycRW5rcLKKikjQCRGET jCLgZa1VMJ7KQVKgCsAC3egfUFh3s+VfG4ZqUPCPd3sjB2aTLTTj+gpVxI0tR5NdXqVO5l 3bXUalpS8v/0gDxhZJLRMkBHXbg1kii1QG768G1dMAVf5JAGh1f0A/30F6o1mOJcITctew //OuRxjrxXhzO68wK9aob0OVtekkra0z7oi6HKnN5NN1ekbE6Ih5O43FeqhxxQ== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1695305725; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FOWN2bP3bL+i8WU1AdRgy2WvLGaYMKjJkqdur8ZMT5U=; b=sHscKoKcbk8ILIhokgaU/401GuFMDm+ywMOvfMNuLAGr4ihRWdmUt0LzNlUx4RJMoS+GcD MZ3VQCQlC4Y4mODQ== To: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org Subject: [PATCH 1/5] drm/amd/display: Remove migrate_en/dis from dc_fpu_begin(). Date: Thu, 21 Sep 2023 16:15:12 +0200 Message-Id: <20230921141516.520471-2-bigeasy@linutronix.de> In-Reply-To: <20230921141516.520471-1-bigeasy@linutronix.de> References: <20230921141516.520471-1-bigeasy@linutronix.de> MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Tianci Yin , Leo Li , Sebastian Andrzej Siewior , "Pan, Xinhui" , Rodrigo Siqueira , Peter Zijlstra , Aurabindo Pillai , Alex Deucher , Thomas Gleixner , =?utf-8?q?Christian_K=C3=B6nig?= Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" This is a revert of the commit mentioned below while it is not wrong, as in the kernel will explode, having migrate_disable() here it is complete waste of resources. Additionally commit message is plain wrong the review tag does not make it any better. The migrate_disable() interface has a fat comment describing it and it includes the word "undesired" in the headline which should tickle people to read it before using it. Initially I assumed it is worded too harsh but now I beg to differ. The reviewer of the original commit, even not understanding what migrate_disable() does should ask the following: - migrate_disable() is added only to the CONFIG_X86 block and it claims to protect fpu_recursion_depth. Why are the other the architectures excluded? - migrate_disable() is added after fpu_recursion_depth was modified. Shouldn't it be added before the modification or referencing takes place? Moving on. Disabling preemption DOES prevent CPU migration. A task, that can not be pushed away from the CPU by the scheduler (due to disabled preemption) can not be pushed or migrated to another CPU. Disabling migration DOES NOT ensure consistency of per-CPU variables. It only ensures that the task acts always on the same per-CPU variable. The task remains preemptible meaning multiple tasks can access the same per-CPU variable. This in turn leads to inconsistency for the statement *pcpu -= 1; with two tasks on one CPU and a preemption point during the RMW operation: Task A Task B read pcpu to reg # 0 inc reg # 0 -> 1 read pcpu to reg # 0 inc reg # 0 -> 1 write reg to pcpu # 1 write reg to pcpu # 1 At the end pcpu reads 1 but should read 2 instead. Boom. get_cpu_ptr() already contains a preempt_disable() statement. That means that the per-CPU variable can only be referenced by a single task which is currently running. The only inconsistency that can occur if the variable is additionally accessed from an interrupt. Remove migrate_disable/enable() from dc_fpu_begin/end(). Cc: Tianci Yin Cc: Aurabindo Pillai Fixes: 0c316556d1249 ("drm/amd/display: Disable migration to ensure consistency of per-CPU variable") Signed-off-by: Sebastian Andrzej Siewior --- drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c index 172aa10a8800f..86f4c0e046548 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c @@ -91,7 +91,6 @@ void dc_fpu_begin(const char *function_name, const int line) if (*pcpu == 1) { #if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) - migrate_disable(); kernel_fpu_begin(); #elif defined(CONFIG_PPC64) if (cpu_has_feature(CPU_FTR_VSX_COMP)) { @@ -132,7 +131,6 @@ void dc_fpu_end(const char *function_name, const int line) if (*pcpu <= 0) { #if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) kernel_fpu_end(); - migrate_enable(); #elif defined(CONFIG_PPC64) if (cpu_has_feature(CPU_FTR_VSX_COMP)) { disable_kernel_vsx();