> From David Howells Mon Apr 3 12:08:16 2006
> Suzanne Wood <[email protected]> wrote:
> > Since RCU is applicable in read intensive environments and I
> > look for rcu_dereference() on the read side to be paired with
> > rcu_assign_pointer() on the update side, I wonder if key_put()
> > might be redundant or risky after synchronize_rcu(),
> No, it's not redundant, and neither is it risky, at least, not as far as I can
> tell from the RCU docs.
> > because key_put() opens with if(key)
> That's just to permit key_put(NULL) to avoid going splat.
> > and employs atomic_dec_and_test(&key->usage)) before
> > schedule_work(&key_cleanup_task).
> How else does the cleaner-upper know to dispose of the key? And which key?
> The cleaner scans the entire key tree and weeds out any key that is dead.
> > If the memory referenced by old has already been reclaimed which appears is
> > made possible by synchronize_rcu(), it seems that there is a contention
> > here.
>From P. McKenney's 'What is RCU?': "synchronize_rcu() marks the end of updater
code and the beginning of reclaimer code. It does this by blocking until all
pre-existing RCU read-side critical sections on all CPUs have completed. Note
that synchronize_rcu() will not necessarily wait for any subsequent RCU read-side
critical sections to complete."
It seems that once synchronize_rcu() is called, the rcu-protected memory
can be reclaimed which means available for reuse, so a NULL check wouldn't be
reliable.
> It will not be reclaimed until after its refcount is zero, and once that
> happens, there can't be any other valid links to it.
> > Yes, so I guess you mean to get rid of 'old' altogether and the three
> > lines that refer to it. But I think the original intent was to have
> > the former session keyring persist to some extent.
> Sorry, I'm not sure what you mean.
We do find rcu_dereference(tsk->signal->session_keyring) within
rcu_read_lock()/unlock() pairs (and similarly current->signal->session_keyring
and context->signal->session_keyring) in security/keys/process_keys.c and
request_key.c.
> I can't get rid of "old". I have to exchange the new pointer for the old
> pointer and then release the old object, otherwise there's a resource leak.
> However, I think the code should probably be:
> spin_lock_irqsave(&tsk->sighand->siglock, flags);
> old = tsk->signal->session_keyring;
> rcu_assign_pointer(tsk->signal->session_keyring, keyring);
> spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
> The code then calls synchronize_rcu() to make sure no one can then follow that
> pointer and then releases the reference on it. Anyone who wants the object to
> persist beyond the rcu_read_lock()'d region must have incremented the refcount
> whilst inside that region. But once the synchronize_rcu() completes, it must
> be safe for me to release the reference.
When synchronize_rcu() reclaims memory, it's because the old references to stale
data were retained just for the duration of the rcu readside critical sections.
> It's possible, if not probable, that a memory barrier is required before the
> atomic_dec_and_test() in key_put().
> I don't think one is required after an atomic_inc() between rcu_read_lock()
> and rcu_read_unlock() because I think those need to imply LOCK/UNLOCK barrier
> semantics. Maybe Paul McKenney thinks otherwise, though I think I convinced
> him to agree with me.
> By the way, the use of schedule_work() to dispose of keys is so that the key
> destroyer can be relatively slow; it also keeps the latency of key_get() as
> minimal as possible in the slow case, but that's a lesser consideration. It
> also makes the locking simpler. Note that there is no intention for the key
> to persist any great length of time after its usage count reaches zero.
> Note also that the replacement of the session keyring does not suggest that
> the session keyring will most likely be destroyed; in fact the likelyhood is
> that it won't. It's a _session_ keyring, and is most probably going to be
> shared by quite a few processes.
> So, to sum up, I think there's three potential problems with my code:
> (1) key_put() probably needs smp_mb__before_atomic_dec().
> (2) key_get() may need smp_mb__after_atomic_inc() inside of rcu_read_lock()'d
> sections, but I don't think that's necessary as the mere fact it's inside
> a locked section should obviate that need.
> (3) rcu_dereference() is a waste of processing time inside the spinlocks in
> install_session_keyring(). It should be a direct dereference. The
> semantics of spin_unlock() should obviate the need for the
> smp_read_barrier_depends().
> But thanks for pointing out the potential problems in my code. That's much
> appreciated. The kernel needs a good memory barrier audit.
> David
I'm hoping to develop rules to automate a correct application
of the RCU-API and appreciate your help. And memory barrier issues
specific to different architectures provide complexity.
Thanks.
Suzanne
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
