Re: [Update][PATCH v5 7/9] mm/swap: Add cache for swap slots allocation

[Michal Hocko]

On Tue 17-01-17 10:55:47, Huang, Ying wrote:
[...]
> +int free_swap_slot(swp_entry_t entry)
> +{
> +	struct swap_slots_cache *cache;
> +
> +	BUG_ON(!swap_slot_cache_initialized);
> +
> +	cache = &get_cpu_var(swp_slots);
> +	if (use_swap_slot_cache && cache->slots_ret) {
> +		spin_lock_irq(&cache->free_lock);
> +		/* Swap slots cache may be deactivated before acquiring lock */
> +		if (!use_swap_slot_cache) {
> +			spin_unlock_irq(&cache->free_lock);
> +			goto direct_free;
> +		}
> +		if (cache->n_ret >= SWAP_SLOTS_CACHE_SIZE) {
> +			/*
> +			 * Return slots to global pool.
> +			 * The current swap_map value is SWAP_HAS_CACHE.
> +			 * Set it to 0 to indicate it is available for
> +			 * allocation in global pool
> +			 */
> +			swapcache_free_entries(cache->slots_ret, cache->n_ret);
> +			cache->n_ret = 0;
> +		}
> +		cache->slots_ret[cache->n_ret++] = entry;
> +		spin_unlock_irq(&cache->free_lock);
> +	} else {
> +direct_free:
> +		swapcache_free_entries(&entry, 1);
> +	}
> +	put_cpu_var(swp_slots);
> +
> +	return 0;
> +}
> +
> +swp_entry_t get_swap_page(void)
> +{
> +	swp_entry_t entry, *pentry;
> +	struct swap_slots_cache *cache;
> +
> +	/*
> +	 * Preemption need to be turned on here, because we may sleep
> +	 * in refill_swap_slots_cache().  But it is safe, because
> +	 * accesses to the per-CPU data structure are protected by a
> +	 * mutex.
> +	 */

the comment doesn't really explain why it is safe. THere are other users
which are not using the lock. E.g. just look at free_swap_slot above. 
How can
	cache->slots_ret[cache->n_ret++] = entry;
be safe wrt.
	pentry = &cache->slots[cache->cur++];
	entry = *pentry;

Both of them might touch the same slot, no? Btw. I would rather prefer
this would be a follow up fix with the trace and the detailed
explanation.

> +	cache = raw_cpu_ptr(&swp_slots);
> +
> +	entry.val = 0;
> +	if (check_cache_active()) {
> +		mutex_lock(&cache->alloc_lock);
> +		if (cache->slots) {
> +repeat:
> +			if (cache->nr) {
> +				pentry = &cache->slots[cache->cur++];
> +				entry = *pentry;
> +				pentry->val = 0;
> +				cache->nr--;
> +			} else {
> +				if (refill_swap_slots_cache(cache))
> +					goto repeat;
> +			}
> +		}
> +		mutex_unlock(&cache->alloc_lock);
> +		if (entry.val)
> +			return entry;
> +	}
> +
> +	get_swap_pages(1, &entry);
> +
> +	return entry;
> +}
-- 
Michal Hocko
SUSE Labs