Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Nov 2022 13:27:41 -0800
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Chen Wandun <chenwandun@huawei.com>
Cc:     <hugh@veritas.com>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>, <wangkefeng.wang@huawei.com>,
        <sunnanyong@huawei.com>, <xialonglong1@huawei.com>,
        Huang Ying <ying.huang@intel.com>
Subject: Re: [RFC PATCH] swapfile: fix soft lockup in scan_swap_map_slots
Message-Id: <20221118132741.aaf6f9081b5a1018cc9a5402@linux-foundation.org>
In-Reply-To: <20221118133850.3360369-1-chenwandun@huawei.com>
References: <20221118133850.3360369-1-chenwandun@huawei.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 18 Nov 2022 21:38:50 +0800 Chen Wandun <chenwandun@huawei.com> wrote:

> A soft lockup occur in scan free swap slot by constructing
> huge memory pressure.
> The test scenario is: 64 CPU cores, 64GB memory, and 28
> zram devices, the disksize of each zram device is 50MB.
> 
> LATENCY_LIMIT is used to prevent soft lockup in function
> scan_swap_map_slots, but the real loop number would more
> than LATENCY_LIMIT because of "goto checks and goto scan"
> repeatly without decrease of latency limit.
> 
> In order to fix it, move decrease latency_ration code in advance.
> 
> There is also a suspicious place that will cause soft lockup in
> function get_swap_pages, in this function, the "goto start_over"
> may result in continuous scanning of swap partition, if there is
> no cond_sched in scan_swap_map_slots, it would cause soft lockup
> (I am not sure about this).
> 
> ...
>

Looks sensible.

> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -972,23 +972,23 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
>  scan:
>  	spin_unlock(&si->lock);
>  	while (++offset <= READ_ONCE(si->highest_bit)) {
> -		if (swap_offset_available_and_locked(si, offset))
> -			goto checks;
>  		if (unlikely(--latency_ration < 0)) {
>  			cond_resched();
>  			latency_ration = LATENCY_LIMIT;
>  			scanned_many = true;
>  		}
> +		if (swap_offset_available_and_locked(si, offset))
> +			goto checks;
>  	}
>  	offset = si->lowest_bit;
>  	while (offset < scan_base) {
> -		if (swap_offset_available_and_locked(si, offset))
> -			goto checks;
>  		if (unlikely(--latency_ration < 0)) {
>  			cond_resched();
>  			latency_ration = LATENCY_LIMIT;
>  			scanned_many = true;
>  		}
> +		if (swap_offset_available_and_locked(si, offset))
> +			goto checks;
>  		offset++;
>  	}
>  	spin_lock(&si->lock);

But this does somewhat alter the `scanned_many' logic.  We'll now set
'scanned_many` earlier.  What are the effects of this?

The ed43af10975eef7e changelog outlines tests which could be performed
to ensure we aren't regressing from this.