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

[Andrew Morton]

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

> From: "Huang, Ying" <ying.huang@xxxxxxxxx>
> 
> 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..