Prev: No SPROM available!
Next: open error path failure...
From: David Howells on 30 Mar 2010 20:00
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> Which it is, as long as the lock is held.

However, in one of the situations I'm thinking of, no lock is held. All that
is being tested is whether the pointer to some RCU-protected data is either
NULL or non-NULL. For example:

@@ -345,7 +346,7 @@ int nfs_inode_return_delegation(struct inode *inode)
struct nfs_delegation *delegation;
int err = 0;

- if (rcu_dereference(nfsi->delegation) != NULL) {
+ if (nfsi->delegation != NULL) {
spin_lock(&clp->cl_lock);
delegation = nfs_detach_delegation_locked(nfsi, NULL);
spin_unlock(&clp->cl_lock);

No lock - RCU or spinlock - is held over the check of nfsi->delegation - which
causes lockdep to complain about an unguarded rcu_dereference().

However, the use of rcu_dereference() here is unnecessary with respect to the
interpolation (where appropriate) of a memory barrier because there is no
second memory access against which to order.

That said, the memory access is repeated inside nfs_detach_delegation_locked(),
which again was wrapped in an rcu_dereference():

static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid)
{
- struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation);
+ struct nfs_delegation *delegation = nfsi->delegation;

if (delegation == NULL)
goto nomatch;

which was not necessary for its memory barrier interpolation properties in this
case because of the spin_lock() the caller now holds.


Your suggestion of using rcu_dereference_check() in both these places would
result in two unnecessary memory barriers on something like an Alpha CPU.


How about:

static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid)
{
struct nfs_delegation *delegation =
rcu_locked_dereference(nfsi->delegation);
...
}

where rcu_locked_dereference() only does the lockdep magic and the dereference,
and does not include a memory barrier. The documentation of such a function
would note this may only be used when the pointer is guarded by an exclusive
lock to prevent it from changing.

And then:

int nfs_inode_return_delegation(struct inode *inode)
{
struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
struct nfs_inode *nfsi = NFS_I(inode);
struct nfs_delegation *delegation;
int err = 0;

if (rcu_pointer_not_null(nfsi->delegation)) {
spin_lock(&clp->cl_lock);
delegation = nfs_detach_delegation_locked(nfsi, NULL);
spin_unlock(&clp->cl_lock);
if (delegation != NULL) {
nfs_msync_inode(inode);
err = __nfs_inode_return_delegation(inode, delegation, 1);
}
}
return err;
}

where rcu_pointer_not_null() simply tests the value of the pointer, casting
away the sparse RCU annotation and not doing the lockdep check and not
including a barrier. It would not return the value of the pointer, thus
preventing you from needing the barrier as a result.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 31 Mar 2010 10:10
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> How about Eric's suggestion of rcu_dereference_protected()? That name
> doesn't imply a lock, which as you say above, isn't always needed to
> keep the structure from changing.

But 'protected' from what or by what?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 31 Mar 2010 13:40
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> Protected by something that the caller did, be it holding the the correct
> lock, operating on it during initialization before other CPUs have access
> to it, operating on it during cleanup after other CPUs' access has been
> revoked, or whatever.

But the point I made very early this morning still stands: What if someone
simply wants to test the pointer, not actually to dereference it?

NFS was using rcu_dereference() for this in a couple of places - which is
overkill. I suggested stripping this off and you countered with the
suggestion that it should be using rcu_dereference_check().

Why do I need anything at all?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 31 Mar 2010 19:00
Eric Dumazet <eric.dumazet(a)gmail.com> wrote:

> If you dont own a lock, and test a pointer, what guarantee do you have
> this pointer doesnt change right after you tested it ?

There are five possibilities:

(1) A pointer points to something when you check, and still points to the
same thing after you've gained the lock.

(2) A pointer points to something when you check, and points to something
else after you've gained the lock.

(3) A pointer points to something when you check, and is NULL after you've
gained the lock.

(4) A pointer points to NULL when you check, and points to something after
you've gained the lock.

(5) A pointer points to NULL when you check, and points to NULL after you've
gained the lock.

However, what if you _know_ that the pointer can only ever be made non-NULL
during initialisation, and may even be left unset? That means possibility (4)
can never happen, and that possibility (5) can be detected by testing before
taking the lock. Now, what if (5) is a common occurrence? It might make
sense to make the test.

And what matter if the pointer _does_ change after you test it. If it was
NULL before, it can only be NULL now - by the semantics defined for that
particular pointer.

> If *something* protects the pointer from being changed, then how can be
> expressed this fact ?
>
> If nothing protects the pointer, why test it then, as result of test is
> unreliable ?

I think you may be misunderstanding the purpose of rcu_dereference(). It is
to make sure the reading and dereferencing of the pointer are correctly
ordered with respect to the setting up of the pointed to record and the
changing of the pointer.

There must be two memory accesses for the barrier implied to be of use. In
nfs_inode_return_delegation() there aren't two memory accesses to order,
therefore the barrier is pointless.

> If NFS was using rcu_dereference(), it probably was for a reason, but if
> nobody can recall it, it was a wrong reason ?

I think it is incorrectly used. Given that the rcu_dereference() in:

if (rcu_dereference(nfsi->delegation) != NULL) {
spin_lock(&clp->cl_lock);
delegation = nfs_detach_delegation_locked(nfsi, NULL);
spin_unlock(&clp->cl_lock);
if (delegation != NULL)
nfs_do_return_delegation(inode, delegation, 0);
}

resolves to:

_________p1 = nfsi->delegation;
smp_read_barrier_depends();
if (_________p1) {
spin_lock(&clp->cl_lock); // implicit LOCK-class barrier
==>nfs_detach_delegation_locked(nfsi, NULL);
[dereference nfsi->delegation]
...
}

do you actually need the smp_read_barrier_depends()? You _have_ a barrier in
the form of the spin_lock(). In fact, the spin_lock() is avowedly sufficient
to protect accesses to and dereferences of nfsi->delegation, which means that:

static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid)
{
struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation);
...
}

has no need of the internal barrier provided by rcu_dereference() either.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 1 Apr 2010 12:40
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> So you have objected to needless memory barriers. How do you feel
> about possibly needless ACCESS_ONCE() calls?

That would work here since it shouldn't emit any excess instructions.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: No SPROM available!
Next: open error path failure...