From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AE7F0C433B4 for ; Fri, 9 Apr 2021 08:42:35 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5586B6103E for ; Fri, 9 Apr 2021 08:42:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5586B6103E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id CDD4A6B006C; Fri, 9 Apr 2021 04:42:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C8E756B006E; Fri, 9 Apr 2021 04:42:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B06E06B0070; Fri, 9 Apr 2021 04:42:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0253.hostedemail.com [216.40.44.253]) by kanga.kvack.org (Postfix) with ESMTP id 90D456B006C for ; Fri, 9 Apr 2021 04:42:34 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 427D52470 for ; Fri, 9 Apr 2021 08:42:34 +0000 (UTC) X-FDA: 78012187428.16.F5DBB51 Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by imf30.hostedemail.com (Postfix) with ESMTP id 3B933E00011B for ; Fri, 9 Apr 2021 08:42:26 +0000 (UTC) Received: from DGGEMS410-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FGs5Y3kT1z19KrK; Fri, 9 Apr 2021 16:40:17 +0800 (CST) Received: from [10.174.179.9] (10.174.179.9) by DGGEMS410-HUB.china.huawei.com (10.3.19.210) with Microsoft SMTP Server id 14.3.498.0; Fri, 9 Apr 2021 16:42:25 +0800 Subject: Re: [PATCH 2/5] swap: fix do_swap_page() race with swapoff To: Tim Chen , CC: , , , , , , , , , , , References: <20210408130820.48233-1-linmiaohe@huawei.com> <20210408130820.48233-3-linmiaohe@huawei.com> <7684b3de-2824-9b1f-f033-d4bc14f9e195@linux.intel.com> From: Miaohe Lin Message-ID: <50d34b02-c155-bad7-da1f-03807ad31275@huawei.com> Date: Fri, 9 Apr 2021 16:42:24 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <7684b3de-2824-9b1f-f033-d4bc14f9e195@linux.intel.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.179.9] X-CFilter-Loop: Reflected X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 3B933E00011B X-Stat-Signature: haomknfz6wcb8fh4iieijjsjf4865aga Received-SPF: none (huawei.com>: No applicable sender policy available) receiver=imf30; identity=mailfrom; envelope-from=""; helo=szxga04-in.huawei.com; client-ip=45.249.212.190 X-HE-DKIM-Result: none/none X-HE-Tag: 1617957746-566564 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 2021/4/9 5:34, Tim Chen wrote: > > > On 4/8/21 6:08 AM, Miaohe Lin wrote: >> When I was investigating the swap code, I found the below possible race >> window: >> >> CPU 1 CPU 2 >> ----- ----- >> do_swap_page >> synchronous swap_readpage >> alloc_page_vma >> swapoff >> release swap_file, bdev, or ... > Many thanks for quick review and reply! > Perhaps I'm missing something. The release of swap_file, bdev etc > happens after we have cleared the SWP_VALID bit in si->flags in destroy_swap_extents > if I read the swapoff code correctly. Agree. Let's look this more close: CPU1 CPU2 ----- ----- swap_readpage if (data_race(sis->flags & SWP_FS_OPS)) { swapoff p->swap_file = NULL; struct file *swap_file = sis->swap_file; struct address_space *mapping = swap_file->f_mapping;[oops!] ... p->flags = 0; ... Does this make sense for you? > > >> swap_readpage >> check sis->flags is ok >> access swap_file, bdev...[oops!] >> si->flags = 0 > > This happens after we clear the si->flags > synchronize_rcu() > release swap_file, bdev, in destroy_swap_extents() > > So I think if we have get_swap_device/put_swap_device in do_swap_page, > it should fix the race you've pointed out here. > Then synchronize_rcu() will wait till we have completed do_swap_page and > call put_swap_device. Right, get_swap_device/put_swap_device could fix this race. __But__ rcu_read_lock() in get_swap_device() could disable preempt and do_swap_page() may take a really long time because it involves I/O. It may not be acceptable to disable preempt for such a long time. :( > >> >> Using current get/put_swap_device() to guard against concurrent swapoff for >> swap_readpage() looks terrible because swap_readpage() may take really long >> time. And this race may not be really pernicious because swapoff is usually >> done when system shutdown only. To reduce the performance overhead on the >> hot-path as much as possible, it appears we can use the percpu_ref to close >> this race window(as suggested by Huang, Ying). > > I think it is better to break this patch into two. > > One patch is to fix the race in do_swap_page and swapoff > by adding get_swap_device/put_swap_device in do_swap_page. > > The second patch is to modify get_swap_device and put_swap_device > with percpu_ref. But swapoff is a relatively rare events. Sounds reasonable. Will do it. > > I am not sure making percpu_ref change for performance is really beneficial. > Did you encounter a real use case where you see a problem with swapoff? > The delay in swapoff is primarily in try_to_unuse to bring all > the swapped off pages back into memory. Synchronizing with other > CPU for paging in probably is a small component in overall scheme > of things. > I can't find a more simple and stable way to fix this potential and *theoretical* issue. This could happen in real word but the race window should be very small. While swapoff is usually done when system shutdown only, I'am not really sure if this effort is worth. But IMO, we should eliminate any potential trouble. :) > Thanks. > Thanks again. > Tim > > . >