From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755188AbcB2MoY (ORCPT ); Mon, 29 Feb 2016 07:44:24 -0500 Received: from casper.infradead.org ([85.118.1.10]:58762 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755190AbcB2MnW (ORCPT ); Mon, 29 Feb 2016 07:43:22 -0500 Date: Mon, 29 Feb 2016 13:43:18 +0100 From: Peter Zijlstra To: Boqun Feng Cc: "Paul E. McKenney" , linux-kernel@vger.kernel.org, Ingo Molnar , Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan , sasha.levin@oracle.com Subject: Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical sections Message-ID: <20160229124318.GM6356@twins.programming.kicks-ass.net> References: <1455602265-16490-1-git-send-email-boqun.feng@gmail.com> <20160225143243.GP6357@twins.programming.kicks-ass.net> <20160225153724.GL3522@linux.vnet.ibm.com> <20160226030627.GA2431@fixme-laptop.cn.ibm.com> <20160226112521.GZ6356@twins.programming.kicks-ass.net> <20160229011220.GA1583@fixme-laptop.cn.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160229011220.GA1583@fixme-laptop.cn.ibm.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 29, 2016 at 09:12:20AM +0800, Boqun Feng wrote: > The problem is: > > Currently there is no way to know which rcu_dereference() and its > friends a rcu_read_lock() or one of its friends is protecting. And > the lack of such information is a pain for kernel developers and > reviewers to understand the purpose of a certain rcu_read_lock(). If I have to go run a kernel and look at /proc data in order to complete a review the patch will fail review, end of story. > > Secondly, I'm not sure a random list of data is going to help anybody. > > > > One possible usage might be: > > If someone is going to refactor a piece of code which originally uses > RCU for synchronization, and now he wants to switch to other > synchronization mechanism for some data because of some reason(for > consistency or writer side latency guarantee, etc.). Before he makes the > change, he needs to know if this rcu_read_lock() also protects other > data, and this is not an easy job and I don't think we have anything > helpful for this. With RCU_LOCKED_ACCESS, he can build the kernel with > RCU_LOCKED_ACCESS enabled, run some workloads and get the information > from a /proc file. In this way, RCU_LOCKED_ACCESS is helpful to > developers who want to know the information of the associations between > rcu_read_lock() and rcu_dereference(). So 'possible', but will anyone actually do this? ` > > The whole point of lockdep is that it tells you if you do broken. This > > doesn't, and cannot, because it lacks information to verify against. > > > > Yes.. RCU_LOCKED_ACCESS is not a validator, so it cannot be as useful as > lockdep for finding bug. I would try really _really_ hard to make it a validator, those are so much more useful. One could for example allow something like: rcu_read_lock(); rcu_annotate(&var->field); foo(); rcu_read_unlock(); As an alternative to the syntax suggested by Ingo. This would allow keeping the existing rcu_read_lock() signature so you don't have to force update the entire kernel at once, while also (easily) allowing multiple variables. Like: rcu_read_lock(); rcu_annotate(&var->field); rcu_annotate(&var2->field2); You can then have a special rule that if a particular RCU section has an annotation, any rcu_dereference() not matched will field a warning. If the annotation section is empty, nothing. > > So I'm still not sure this is useful. Also, I would argue your code has > > problems if you cannot even find your rcu_read_lock(). > > > > I think what you mean here is, for example, the case where we use > preempt_disable() instead of rcu_read_lock_sched() to pair with > synchronize_sched(), right? No, I was more like: rcu_read_lock(); foo() bar() var->func(); obj->func(); whatever(); and you're looking at a change to whatever() and wonder where the heck the corresponding rcu_read_lock() lives and if we're having it held at all. > If so, sure, this series cannot do anything about this, however, this Why not? Can't you stick whatever you need into preempt_disable() ? kernel/sched/core.c:preempt_count_{add,sub}() are there for a (debug) reason. > If you think this series deserves a reconsideration I will send out a > new version with more text about what's the problem and why this is > useful. So please make it a validator. That's so much more useful.