Prev: No SPROM available!
Next: open error path failure...
From: David Howells on 1 Apr 2010 13:10
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> > 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);
> > }
>
> And nfs_detach_delegation_locked() rechecks nfsi->delegation() under
> the lock, so this is a legitimate use.
>
> The pointer is not held constant, but any changes will be accounted
> for and handled correctly. So I would argue that the pointer value is
> in fact protected by the recheck-under-lock algorithm used here.

A legitimate use of 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 6 Apr 2010 05:40
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> And here is the corresponding patch. Seem reasonable?

Okay. Still not keen on the name, but it'll do.

I think the problem is that rcu_dereference() is itself a misnomer: It doesn't
actually dereference.

Anyway:

Acked-by: David Howells <dhowells(a)redhat.com>
--
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 6 Apr 2010 12:20
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.
>
> And here is the corresponding patch. Seem reasonable?

Actually, now I've thought about it some more. No, it's not reasonable.
You've written:

This patch adds a variant of rcu_dereference() that handles situations
where the RCU-protected data structure cannot change, perhaps due to
our holding the update-side lock, or where the RCU-protected pointer is
only to be tested, not dereferenced.

But if we hold the update-side lock, then why should we be forced to use
ACCESS_ONCE()?

In fact, if we don't hold the lock, but we want to test the pointer twice in
succession, why should we be required to use ACCESS_LOCK()?

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 6 Apr 2010 15:40
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> OK, just to make sure I understand you... You are asking for two additional
> RCU API members:
>
> 1. rcu_access_pointer() or some such that includes ACCESS_ONCE(),
> but not smp_read_barrier_depends(), which may be used when
> we are simply examining the value of the RCU-protected pointer
> (as in the NFS case). It could also be used when the
> appropriate update-side lock is held, but for that we have:
>
> 2. rcu_dereference_protected() or some such that includes neither
> ACCESS_ONCE() nor smp_read_barrier_depends(), and that may
> only be used if updates are prevented, for example, by holding
> the appropriate update-side lock.
>
> Does this fit?

Yep. I think so.

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 7 Apr 2010 09:30
Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote:

> +#define rcu_access_pointer(p, c) \

Why is there a need for 'c'?

> +#define rcu_dereference_protect(p, c) \

I'd prefer rcu_dereference_protected(), I think. This macro doesn't protect
anything. Also, again, why the need for 'c'?

For instance, in:

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

what would be the condition? That the spinlock is held? That's a condition
for calling the function.

And in:

void nfs_inode_return_delegation_noreclaim(struct inode *inode)
{
struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
struct nfs_inode *nfsi = NFS_I(inode);
struct nfs_delegation *delegation;

if (rcu_access_pointer(nfsi->delegation, ????) != NULL) {

what would be the condition here? There's no lock to check - that's the whole
point of the macro. I also can't give it nfsi->delegation to check as the
value may change between the two accesses.

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  | 
Pages: 1 2 3
Prev: No SPROM available!
Next: open error path failure...