From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6824F34571 for ; Wed, 29 Nov 2023 14:58:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=none Received: by smtp.kernel.org (Postfix) with ESMTPSA id 30A6BC433C8; Wed, 29 Nov 2023 14:58:04 +0000 (UTC) Date: Wed, 29 Nov 2023 09:58:26 -0500 From: Steven Rostedt To: Petr Pavlu Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, zhengyejian1@huawei.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] tracing: Simplify and fix "buffered event" synchronization Message-ID: <20231129095826.1aec6381@gandalf.local.home> In-Reply-To: References: <20231127151248.7232-1-petr.pavlu@suse.com> <20231127151248.7232-2-petr.pavlu@suse.com> <20231127124130.1041ffd4@gandalf.local.home> <77037ca1-8116-4bc6-b286-67059db0848e@suse.com> <20231128102748.23328618@gandalf.local.home> X-Mailer: Claws Mail 3.19.1 (GTK+ 2.24.33; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-trace-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Wed, 29 Nov 2023 10:22:23 +0100 Petr Pavlu wrote: > Yes, I believe this should address this potential race condition. > > An alternative would be instead to update > trace_event_buffer_lock_reserve() as follows: > > if (this_cpu_inc_return(trace_buffered_event_cnt) == 1) { > smp_rmb(); This is the problem I have with your approach. That smp_rmb() is in the highly critical path. On some architectures, this has a significant impact on the overhead of this code. This path is called during code execution and increases the overhead of the tracing infrastructure. If I'm given two solutions where one adds a smp_rmb() to the critical path and the other just slows down the non-critical path more, I'll take the slow down of non-critical path every time. > if ((entry = __this_cpu_read(trace_buffered_event))) { > [...] > > That saves the synchronize_rcu() call but additionally modifies > trace_buffered_event_cnt even if trace_buffered_event is currently NULL. > > Another alternative is the approach taken by my patch which avoids more > RCU work and unnecessary memory modifications. > > I'd be interested if you could have a look again at what I'm proposing > in my patch. I think it simplifies the code while addressing these > problems as well. However, if you have reservations about that approach > then it is ok, I can fix the found problems individually as discussed. Fix this without adding any memory barriers to the critical path, then I'll take another look. FYI, this code was designed in the first place to avoid adding memory barriers in the critical path. Thanks! -- Steve