linux-kernel - Re: [PATCH v5 2/9] mm/swap: Add cluster lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Jan 2017 15:00:29 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
Cc:     "Huang, Ying" <ying.huang@...el.com>, dave.hansen@...el.com,
        ak@...ux.intel.com, aaron.lu@...el.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Hugh Dickins <hughd@...gle.com>,
        Shaohua Li <shli@...nel.org>, Minchan Kim <minchan@...nel.org>,
        Rik van Riel <riel@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Hillf Danton <hillf.zj@...baba-inc.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v5 2/9] mm/swap: Add cluster lock

On Wed, 11 Jan 2017 09:55:12 -0800 Tim Chen <tim.c.chen@...ux.intel.com> wrote:

> From: "Huang, Ying" <ying.huang@...el.com>
> 
> This patch is to reduce the lock contention of swap_info_struct->lock
> via using a more fine grained lock in swap_cluster_info for some swap
> operations.  swap_info_struct->lock is heavily contended if multiple
> processes reclaim pages simultaneously.  Because there is only one lock
> for each swap device.  While in common configuration, there is only one
> or several swap devices in the system.  The lock protects almost all
> swap related operations.
> 
> In fact, many swap operations only access one element of
> swap_info_struct->swap_map array.  And there is no dependency between
> different elements of swap_info_struct->swap_map.  So a fine grained
> lock can be used to allow parallel access to the different elements of
> swap_info_struct->swap_map.
> 
> In this patch, one bit of swap_cluster_info is used as the bin spinlock
> to protect the elements of swap_info_struct->swap_map in the swap
> cluster and the fields of swap_cluster_info.  This reduced locking
> contention for swap_info_struct->swap_map access greatly.
> 
> To use the bin spinlock, the size of swap_cluster_info needs to increase
> from 4 bytes to 8 bytes on the 64bit system.  This will use 4k more
> memory for every 1G swap space.
> 
> Because the size of swap_cluster_info is much smaller than the size of
> the cache line (8 vs 64 on x86_64 architecture), there may be false
> cache line sharing between swap_cluster_info bit spinlocks.  To avoid
> the false sharing in the first round of the swap cluster allocation, the
> order of the swap clusters in the free clusters list is changed.  So
> that, the swap_cluster_info sharing the same cache line will be placed
> as far as possible.  After the first round of allocation, the order of
> the clusters in free clusters list is expected to be random.  So the
> false sharing should be not noticeable.
> 
> ...
>
> @@ -175,11 +175,16 @@ enum {
>   * protected by swap_info_struct.lock.
>   */
>  struct swap_cluster_info {
> -	unsigned int data:24;
> -	unsigned int flags:8;
> +	unsigned long data;
>  };
>
> ...
>
> +static inline void __lock_cluster(struct swap_cluster_info *ci)
> +{
> +	bit_spin_lock(CLUSTER_FLAG_LOCK_BIT, &ci->data);
> +}

hm, bit_spin_lock() is a nasty thing.  It is slow and it doesn't have
all the lockdep support.

Would the world end if we added a spinlock to swap_cluster_info?  Check
my math: for each 1G of wapspace we have 256k pages, hence 1k of
swap_cluster_infos, hence 4k of memory.  ie, one page of memory for
each 256,000 pages of swap.  Is increasing that 1/256000 to 2/256000 a
big deal?



Also, I note that struct swap_cluster_info is only used in swapfile.c
and as a cleanup we could move its definition into that .c file. 
Perhaps other things could be moved as well..