Messages in this thread | | | From | "Kaigai Kohei" <> | Subject | Re: [PATCH]SELinux performance improvement by RCU (Re: RCU issue with SELinux) | Date | Tue, 31 Aug 2004 13:33:33 +0900 |
| |
Hi Stephen, Paul, thanks for your comments.
> > > The attached take-4 patches replace the avc_lock in security/selinux/avc.c > > > by the lock-less read access with RCU. > > > > Thanks. Was there a reason you didn't move the rcu_read_lock call after > > the avc_insert call per the suggestion of Paul McKenney, or was that > > just an oversight? No need to send a new patch, just ack whether or not > > you meant to switch the order there. > > One reason might be because I called it out in the text of my message, > but failed to put it in my patch. :-/ Of course, if there is some reason > why moving the rcu_read_lock() call is bad, I would like to know for > my own education.
In my understanding, the issue is the Paul's suggestion as follows:
> So I do not believe that avc_insert() needs rcu_read_lock(). > Unless I am missing something, the rcu_read_lock() acquired > in avc_has_perm_noaudit() should be moved after the call to > avc_insert().
I don't move the rcu_read_lock() because of the possibility of preemption between the spin_unlock_irqrestore() in avc_insert() and the rcu_read_lock() which may be inserted after avc_insert() in avc_has_perm_noaudit().
When it's returning from avc_insert(), we can't ignore the possibility that execution is preempted in this timing. Therefore, I didn't move rcu_read_lock() in spite of its redundancy.
If rcu_read_lock() was moved after avc_insert() [ in avc_insert() ]---------------------------- : spin_lock_irqsave(&avc_cache.slots_lock[hvalue], flag); list_for_each_entry(pos, &avc_cache.slots[hvalue], list) { : } list_add_rcu(&node->list, &avc_cache.slots[hvalue]); found: spin_unlock_irqrestore(&avc_cache.slots_lock[hvalue], flag); --------- // +--> including preempt_enable() | It has the danger of releasing the 'node'. V } preemption out: is return node; possible } ----------------------------------------------- Because it's legal to hold the rcu_read_lock() twice as Paul says, we should do it for safety. It's the reason that I didn't move rcu_read_lock() at this point, and it might be lack of my explanation, sorry.
Thanks. -------- Kai Gai <kaigai@ak.jp.nec.com>
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |