Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 12 Dec 2019 16:55:34 -0500
From:   Joel Fernandes <joel@joelfernandes.org>
To:     "Paul E. McKenney" <paulmck@kernel.org>
Cc:     madhuparnabhowmik04@gmail.com, trond.myklebust@hammerspace.com,
        anna.schumaker@netapp.com, linux-nfs@vger.kernel.org,
        rcu@vger.kernel.org,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error
Message-ID: <20191212215534.GE129023@google.com>
References: <20191206151640.10966-1-madhuparnabhowmik04@gmail.com>
 <20191206160238.GE2889@paulmck-ThinkPad-P72>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191206160238.GE2889@paulmck-ThinkPad-P72>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Fri, Dec 06, 2019 at 08:02:38AM -0800, Paul E. McKenney wrote:

Thanks for fixing these issues and I caught up with all the patches.

> 
> o	Create a list that is safe for bidirectional RCU traversal.
> 	This can use list_head, and would need these functions,
> 	give or take the exact names:

On a related topic, I was trying to reason about how one could come up with
bidirectional traversal without ever getting rid of poisoning.

As you noted in another post, if during traversal, the node is deleted and
poisoned, then the traverser can access a poisoned pointer. If the list is
being traversed in reverse (by following prev), then poisioning could hurt
it.

Even with the below modifications, poisoning would still hurt it. No? Were
you suggesting to remove poisoning for such bidirectional RCU list?

Sorry if I missed something.
thanks,

 - Joel


> 	list_add_tail_rcuprev():  This is like list_add_tail_rcu(),
> 	but also has smp_store_release() for ->prev.  (As in there is
> 	also a __list_add_rcuprev() helper that actually contains the
> 	additional smp_store_release().)
> 
> 	list_del_rcuprev():  This can be exactly __list_del_entry(),
> 	but with the assignment to ->prev in __list_del() becoming
> 	WRITE_ONCE().  And it looks like callers to __list_del_entry()
> 	and __list_del() might need some attention!  And these might
> 	result in additional users of *_rcuprev().
> 
> 	list_prev_rcu() as in your first patch, but with READ_ONCE().
> 	Otherwise DEC Alpha can fail.  And more subtle compiler issues
> 	can appear on other architectures.
> 
> 	Note that list_move_tail() will be OK give or take *_ONCE().
> 	It might be better to define a list_move_tail_rcuprev(), given
> 	the large number of users of list_move_tail() -- some of these
> 	users might not like even the possibility of added overhead due
> 	to volatile accesses.  ;-)
> 
> Or am I missing something subtle here?
> 
> 							Thanx, Paul