From mboxrd@z Thu Jan 1 00:00:00 1970 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 69BCFC531DE for ; Thu, 15 Aug 2024 00:34:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=oemIRTF49tlFLYjQO3CRhjvR9o7AP5/gGFnsJ/rGk3E=; b=yMuFTWpqeZUrwF bVt3FBzZ5YRAe8Y/krcviqTw1j/NxW3H3H0pFbWTDpRp0bi5jV00DUmMlNsAuvGCSkYm6204yjtOL UN0VJayVI/5PvLKKWIAWwD3WufYeSaeRhqovKIT1iJ6BDpZqmzAgbN2NjuBFSgRoLXdqgiiiN8eQc PtdFGb3AobhpWPVR4AAiVdwNDIKCpkgGayHC7n6bjcSt/m63+g2NfKK8MQRCOr0swKQXATS4lCaJb BbuR01qdl8nE8DGlzFgiH0cTHCWCYgmezD6VRlxRnZqOWgLVgkVR9mLXnh5NCSmaC2CG0eGC2C+L2 FBAOjk7hf27VgY6SCxBQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1seORi-00000008eZz-1KWO; Thu, 15 Aug 2024 00:34:30 +0000 Received: from mail-wm1-x329.google.com ([2a00:1450:4864:20::329]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1seORf-00000008eYu-1bDk for linux-riscv@lists.infradead.org; Thu, 15 Aug 2024 00:34:28 +0000 Received: by mail-wm1-x329.google.com with SMTP id 5b1f17b1804b1-4280ca0791bso2117925e9.1 for ; Wed, 14 Aug 2024 17:34:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723682065; x=1724286865; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=GYBBp8MdgJpDzmzKWf0FPivxU2t9lT+77YiFk2ooOxQ=; b=hiDT2bciELcl+ftuFmPJ2sNLIM6FhPcTg5aodSmqo4IuqLqns+VwUp107bgr1rrkDs 6SNYg/bSTYCRfeRepAzUjhV4+wU+ddXM9/EzOsKbEQHL2EAF+Ota3+/7pyW+tJ9CH4yz 3EhSzi8gIox6U/UzMidDQcSUXU9S7n3+bWQsic+yJ6fH+vWoweIUqo6WW5tq0i+yO8ia avttt9fuXNNgeOg3c/mjKrNtm/BZoZUsxF2lxDrEtgpqcOrwOaLZhP72Sy0Spa1mgUZF aBM8iQj4NA7FIuM0qVdsFvo4lyahf/k+lRrqWnIVEu2PFfozQAtaLeKHrhp/eIF5XeMl rMpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723682065; x=1724286865; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=GYBBp8MdgJpDzmzKWf0FPivxU2t9lT+77YiFk2ooOxQ=; b=vYZhaRVGJB8tuZiz/USNYvwb+qf/gIOXR/pgCRYNGMAVspJw8SLaTYqkduto2jY0cJ 7pgcXz2SuU1UyR9CcoDV+3XsmtGcbgNKG3XgUBCswxD+PJMBIaKGXnZwQhRKmuZ6Mxrs 0sc8K02TvVdoL2sEQ/92fPlKAeaHT8GNbcEjVkfW4LcjJbbN7sJxTNnJ2mX63sZorXFD inMlyjvbcDldxdBX0bX5P5mGydI5hvBMkCc2BsNgumBnWlDrRvbO8WBHVjY10gw77+n9 H5O0MKW0RIXW9UhB0HoU9twnZoNIIL7a9HjJEOI7p+Z2+Q3QsWzBY+f7Y6CFOaTCOocA 6I/g== X-Forwarded-Encrypted: i=1; AJvYcCWxg7bTEHqmRxtBmzqrFwkeUw9HZvWH/PzrL6IQm+UGt9lRXN1a925anayQ2QRtlQ7xLmmlOdd8u+4PCx888jxMuEum7qFLTdQGLW+1aBTN X-Gm-Message-State: AOJu0YwwD5lxpkZY4XsatyytzFyTvgs7/l0rSrdPLmjl7O2fG/TLhABh RDrbkZyH9GMdp8l98xtdxN9Ma9JwLzjIUfTJ0pbxgZAZe+SkxY8R X-Google-Smtp-Source: AGHT+IElhjGJFHAjb3pDkCKnDLfu6WM/NR0TgZhQutcTk8dyPGJvZ5qmOb2PW6YXLSFWnrJpHzgUMA== X-Received: by 2002:a05:600c:4f45:b0:428:2502:75b5 with SMTP id 5b1f17b1804b1-429dd238160mr26205455e9.11.1723682064361; Wed, 14 Aug 2024 17:34:24 -0700 (PDT) Received: from andrea ([151.76.20.39]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-429ded19670sm33932685e9.9.2024.08.14.17.34.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Aug 2024 17:34:23 -0700 (PDT) Date: Thu, 15 Aug 2024 02:34:19 +0200 From: Andrea Parri To: Charlie Jenkins Cc: Paul Walmsley , Palmer Dabbelt , Albert Ou , Alexandre Ghiti , Atish Patra , Samuel Holland , Palmer Dabbelt , linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] riscv: Eagerly flush in flush_icache_deferred() Message-ID: References: <20240813-fix_fencei_optimization-v1-0-2aadc2cdde95@rivosinc.com> <20240813-fix_fencei_optimization-v1-2-2aadc2cdde95@rivosinc.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240813-fix_fencei_optimization-v1-2-2aadc2cdde95@rivosinc.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240814_173427_443006_C91A4C40 X-CRM114-Status: GOOD ( 20.65 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org > <---- Thread 1 starts running here on CPU1 > > <---- Thread 2 starts running here with same mm > > T2: prctl(PR_RISCV_SET_ICACHE_FLUSH_CTX, PR_RISCV_CTX_SW_FENCEI_ON, PR_RISCV_SCOPE_PER_PROCESS); > T2: <-- kernel sets current->mm->context.force_icache_flush to true Mmh, TBH, I'm not sure how this patch is supposed to fix the race in question: For once, AFAIU, the operation T2: cpumask_setall(¤t->mm->context.icache_stale_mask) (on CPU2?) you've added with this patch... > T2: > T2: fence.i > > T1: fence.i (to synchronize with other thread, has some logic to > determine when to do this) > T1: <-- thread 1 is preempted > T1: <-- thread 1 is placed onto CPU3 and starts context switch sequence > T1 (kernel): flush_icache_deferred() -> skips flush because switch_to_should_flush_icache() returns true ... does _not_ ensure that T1: flush_icache_deferred() on CPU3 will observe/read from that operation: IOW, T1: cpumask_test_and_clear_cpu(cpu, &mm->context.icache_stale_mask) may still evaluate to FALSE, thus preventing the FENCE.I execution. Moreover, AFAIU, ... > -> thread has migrated and task->mm->context.force_icache_flush is true > > T2: prctl(PR_RISCV_SET_ICACHE_FLUSH_CTX, PR_RISCV_CTX_SW_FENCEI_OFF, PR_RISCV_SCOPE_PER_PROCESS); ... moving the operation(s) T2: set_icache_stale_mask() T2: cpumask_setall(¤t->mm->context.icache_stale_mask) before the following operation... (per patch #1) > T2 (kernel): kernel sets current->mm->context.force_icache_flush = false > > T1 (kernel): switch_to() calls switch_to_should_flush_icache() which now > returns false because task->mm->context.force_icache_flush > is false due to the other thread emitting > PR_RISCV_CTX_SW_FENCEI_OFF. ... does not ensure that T1: switch_to_should_flush_icache() on CPU3 will observe T1: cpumask_test_cpu(, &task->mm->context.icache_stale_mask) == true in fact, T1 may evaluate the latter expression to FALSE while still being able to observe the "later" operation, i.e. T1: task->mm->context.force_icache_flush == false Perhaps a simplified but useful way to look at such scenarios is as follows: - CPUs are just like nodes of a distributed system, and - store are like messages to be exchanged (by the memory subsystem) between CPUs: without some (explicit) synchronization/constraints, messages originating from a given CPU can propagate/be visible to other CPUs at any time and in any order. IAC, can you elaborate on the solution proposed here (maybe by adding some inline comments), keeping the above considerations in mind? what am I missing? > T1 (back in userspace): Instruction cache was never flushed on context > switch to CPU3, and thus may execute incorrect > instructions. Mmh, flushing the I$ (or, as meant here, executing a FENCE.I) seems to be only half of the solution: IIUC, we'd like to ensure that the store operation T2: originated from CPU2 is _visible_ to CPU3 by the time that FENCE.I instruction is executed, cf. [from Zifencei - emphasis mine] A FENCE.I instruction ensures that a subsequent instruction fetch on a RISC-V hart will see any previous data stores _already visible_ to the same RISC-V hart. IOW (but assuming code is in coherent main memory), imagine that the (putative) FENCE.I on CPU3 is replaced by some T1: LOAD reg,0(insts_addr) question is: would such a load be guaranteed to observe the store T2: # STORE new_insts,0(insts_addr) originated from CPU2? can you elaborate? Andrea _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 95C428F62 for ; Thu, 15 Aug 2024 00:34:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723682068; cv=none; b=iQZV4qpO/iwsWe8PxXLbzlg3aofjPPtLqPRUtnXBSm8NRstAtBnjvDPbmo+wv+hkyqTRIRvAeBWhZbUud6cn+zD2MAXzaJBnhY3D5+lNAquEYKXsidrYnTrkSU2YKtSEojK3QlhPXyFuHvbgtdIoRy2RPOMsUkiIATbcnHfFjg8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723682068; c=relaxed/simple; bh=VytSzQob+eJH5p5v4nuk6Tb4UlSJyQPTWnXVa9f7qHA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tF8IhsPs9wxlGtUdGOwBDOsteUaRTmN42zPUZozrF/oVgJnKEDPfwc6deRJax+C8xntXbsPuUvtRYEaY6GCiXW1vZrlPSlSHxD/14FG7OnlbzxqIq55QjkpGLuGJRjN1F20hSaT1t2CrSfKxLoTZRoXaHiqRLyuGH3o8svFS4u8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=L/DnNENI; arc=none smtp.client-ip=209.85.128.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="L/DnNENI" Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-429c4a4c6a8so2188245e9.0 for ; Wed, 14 Aug 2024 17:34:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723682065; x=1724286865; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=GYBBp8MdgJpDzmzKWf0FPivxU2t9lT+77YiFk2ooOxQ=; b=L/DnNENIwaF87Q16E+4ti1rRZRBLBj/NtyLCSvf3wRwKsCQIuaF0RKswlLv4akfjyq fEwf2xvvFR153BkVL8IOcQsFDDCHOU/TUH6DE6mlRuLrDPT2iQKickRVIKoLJBC+b+kK yOTHQHOUlJQ2TqVA9nrgja8/8uJv4jQNnX9pVj4bnDVqd8ELFAsKTocZVPrKOoMg60XL LxbJqIRjrtYFOXtOFSHK+YbnrGhZs7Sva53dYbMyxghXlJHiZ6kOP8JiUpJ2ckMuR9D8 WxyIHnBqNq6Gn9a0PUlr989sRK5KdbVNZVdqjEgMep1B7GgDtMwkZFQTtB51DggmXfDZ 35Aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723682065; x=1724286865; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=GYBBp8MdgJpDzmzKWf0FPivxU2t9lT+77YiFk2ooOxQ=; b=JHDgMpv38szfIAw8yoG740Km1Y/VCh6ywk77T1Fx14sNQcsoqAT1U9ZoWf2IsTUYNc JUvgraWfCKKL4FapqLpTJRPnegF2OaixHHHcByzJhhbFjU8X32dkru9GLH6qzcrgqJFn ccBgK7IkTkHtEa9CG463SbzUgUFpekDFxB+vaiZlUy3JRtPMWNV92EG2iRPV3PoPHD3O oGP39scFyuN1rGSNRVIoZxtBN1xd/SmnJi6fhY3/5a/X/25dAMpWS/8UsacyDgel5GTI Uw+ZXIipROIazu2e00VQTXgArKCCMQvFVofyJH+n5PDvAQ7CdhAUNDEMvvtbzTeFvFyl h03Q== X-Forwarded-Encrypted: i=1; AJvYcCXEyICKWOMRY4me8ZdLzX332i9ehUTJLyQ4wYLEWmCHozy/EiRT856vDYImyuJvTGR9bMsh1DT+s6Wo7YxeNYZxwRCtoVlSMrPyajs5 X-Gm-Message-State: AOJu0Yy6A+7OoaEBAmUcmh2ygE4FHIpQQ6p0ERTJevMEaOjkwuHAVaJ3 yeOeZUjTHSPfxlPKboUQaT3XSFA0xD6Kw4DOpdZHOgXAR4kXYWGD1fnulQ== X-Google-Smtp-Source: AGHT+IElhjGJFHAjb3pDkCKnDLfu6WM/NR0TgZhQutcTk8dyPGJvZ5qmOb2PW6YXLSFWnrJpHzgUMA== X-Received: by 2002:a05:600c:4f45:b0:428:2502:75b5 with SMTP id 5b1f17b1804b1-429dd238160mr26205455e9.11.1723682064361; Wed, 14 Aug 2024 17:34:24 -0700 (PDT) Received: from andrea ([151.76.20.39]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-429ded19670sm33932685e9.9.2024.08.14.17.34.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Aug 2024 17:34:23 -0700 (PDT) Date: Thu, 15 Aug 2024 02:34:19 +0200 From: Andrea Parri To: Charlie Jenkins Cc: Paul Walmsley , Palmer Dabbelt , Albert Ou , Alexandre Ghiti , Atish Patra , Samuel Holland , Palmer Dabbelt , linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] riscv: Eagerly flush in flush_icache_deferred() Message-ID: References: <20240813-fix_fencei_optimization-v1-0-2aadc2cdde95@rivosinc.com> <20240813-fix_fencei_optimization-v1-2-2aadc2cdde95@rivosinc.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240813-fix_fencei_optimization-v1-2-2aadc2cdde95@rivosinc.com> > <---- Thread 1 starts running here on CPU1 > > <---- Thread 2 starts running here with same mm > > T2: prctl(PR_RISCV_SET_ICACHE_FLUSH_CTX, PR_RISCV_CTX_SW_FENCEI_ON, PR_RISCV_SCOPE_PER_PROCESS); > T2: <-- kernel sets current->mm->context.force_icache_flush to true Mmh, TBH, I'm not sure how this patch is supposed to fix the race in question: For once, AFAIU, the operation T2: cpumask_setall(¤t->mm->context.icache_stale_mask) (on CPU2?) you've added with this patch... > T2: > T2: fence.i > > T1: fence.i (to synchronize with other thread, has some logic to > determine when to do this) > T1: <-- thread 1 is preempted > T1: <-- thread 1 is placed onto CPU3 and starts context switch sequence > T1 (kernel): flush_icache_deferred() -> skips flush because switch_to_should_flush_icache() returns true ... does _not_ ensure that T1: flush_icache_deferred() on CPU3 will observe/read from that operation: IOW, T1: cpumask_test_and_clear_cpu(cpu, &mm->context.icache_stale_mask) may still evaluate to FALSE, thus preventing the FENCE.I execution. Moreover, AFAIU, ... > -> thread has migrated and task->mm->context.force_icache_flush is true > > T2: prctl(PR_RISCV_SET_ICACHE_FLUSH_CTX, PR_RISCV_CTX_SW_FENCEI_OFF, PR_RISCV_SCOPE_PER_PROCESS); ... moving the operation(s) T2: set_icache_stale_mask() T2: cpumask_setall(¤t->mm->context.icache_stale_mask) before the following operation... (per patch #1) > T2 (kernel): kernel sets current->mm->context.force_icache_flush = false > > T1 (kernel): switch_to() calls switch_to_should_flush_icache() which now > returns false because task->mm->context.force_icache_flush > is false due to the other thread emitting > PR_RISCV_CTX_SW_FENCEI_OFF. ... does not ensure that T1: switch_to_should_flush_icache() on CPU3 will observe T1: cpumask_test_cpu(, &task->mm->context.icache_stale_mask) == true in fact, T1 may evaluate the latter expression to FALSE while still being able to observe the "later" operation, i.e. T1: task->mm->context.force_icache_flush == false Perhaps a simplified but useful way to look at such scenarios is as follows: - CPUs are just like nodes of a distributed system, and - store are like messages to be exchanged (by the memory subsystem) between CPUs: without some (explicit) synchronization/constraints, messages originating from a given CPU can propagate/be visible to other CPUs at any time and in any order. IAC, can you elaborate on the solution proposed here (maybe by adding some inline comments), keeping the above considerations in mind? what am I missing? > T1 (back in userspace): Instruction cache was never flushed on context > switch to CPU3, and thus may execute incorrect > instructions. Mmh, flushing the I$ (or, as meant here, executing a FENCE.I) seems to be only half of the solution: IIUC, we'd like to ensure that the store operation T2: originated from CPU2 is _visible_ to CPU3 by the time that FENCE.I instruction is executed, cf. [from Zifencei - emphasis mine] A FENCE.I instruction ensures that a subsequent instruction fetch on a RISC-V hart will see any previous data stores _already visible_ to the same RISC-V hart. IOW (but assuming code is in coherent main memory), imagine that the (putative) FENCE.I on CPU3 is replaced by some T1: LOAD reg,0(insts_addr) question is: would such a load be guaranteed to observe the store T2: # STORE new_insts,0(insts_addr) originated from CPU2? can you elaborate? Andrea