lkml.org 
[lkml]   [2015]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] selinux: reduce locking overhead in inode_free_security()
    On 06/12/2015 08:31 AM, Stephen Smalley wrote:
    > On 06/12/2015 02:26 AM, Raghavendra K T wrote:
    >> On 06/12/2015 03:01 AM, Waiman Long wrote:
    >>> The inode_free_security() function just took the superblock's isec_lock
    >>> before checking and trying to remove the inode security struct from the
    >>> linked list. In many cases, the list was empty and so the lock taking
    >>> is wasteful as no useful work is done. On multi-socket systems with
    >>> a large number of CPUs, there can also be a fair amount of spinlock
    >>> contention on the isec_lock if many tasks are exiting at the same time.
    >>>
    >>> This patch changes the code to check the state of the list first
    >>> before taking the lock and attempting to dequeue it. As this function
    >>> is called indirectly from __destroy_inode(), there can't be another
    >>> instance of inode_free_security() running on the same inode.
    >>>
    >>> Signed-off-by: Waiman Long<Waiman.Long@hp.com>
    >>> ---
    >>> security/selinux/hooks.c | 15 ++++++++++++---
    >>> 1 files changed, 12 insertions(+), 3 deletions(-)
    >>>
    >>> v1->v2:
    >>> - Take out the second list_empty() test inside the lock.
    >>>
    >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
    >>> index 7dade28..e5cdad7 100644
    >>> --- a/security/selinux/hooks.c
    >>> +++ b/security/selinux/hooks.c
    >>> @@ -254,10 +254,19 @@ static void inode_free_security(struct inode
    >>> *inode)
    >>> struct inode_security_struct *isec = inode->i_security;
    >>> struct superblock_security_struct *sbsec = inode->i_sb->s_security;
    >>>
    >>> - spin_lock(&sbsec->isec_lock);
    >>> - if (!list_empty(&isec->list))
    >>> + /*
    >>> + * As not all inode security structures are in a list, we check for
    >>> + * empty list outside of the lock to make sure that we won't waste
    >>> + * time taking a lock doing nothing. As inode_free_security() is
    >>> + * being called indirectly from __destroy_inode(), there is no way
    >>> + * there can be two or more concurrent calls. So doing the
    >>> list_empty()
    >>> + * test outside the loop should be safe.
    >>> + */
    >>> + if (!list_empty(&isec->list)) {
    >>> + spin_lock(&sbsec->isec_lock);
    >>> list_del_init(&isec->list);
    >> Stupid question,
    >>
    >> I need to take a look at list_del_init() code, but it can so happen that
    >> if !list_empty() check could happen simultaneously, then serially two
    >> list_del_init() can happen.
    >>
    >> is that not a problem()?
    > Hmm...I suppose that's possible (sb_finish_set_opts and
    > inode_free_security could both perform the list_del_init). Ok, we'll
    > stay with the first version.
    >

    Actually, list_del_init() can be applied twice with no harm being done.
    The first list_del_init() will set list-> next = list->prev = list. The
    second one will do the same thing and so it should be safe.

    Cheers,
    Longman


    \
     
     \ /
      Last update: 2015-06-13 01:01    [W:4.283 / U:0.204 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site