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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81734C61D85 for ; Fri, 24 Nov 2023 00:43:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 138618D005C; Thu, 23 Nov 2023 19:43:05 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0E8E58D0002; Thu, 23 Nov 2023 19:43:05 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EF27C8D005C; Thu, 23 Nov 2023 19:43:04 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id DFA028D0002 for ; Thu, 23 Nov 2023 19:43:04 -0500 (EST) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id B7351B6D4B for ; Fri, 24 Nov 2023 00:43:04 +0000 (UTC) X-FDA: 81490998288.27.7654DF2 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.126]) by imf11.hostedemail.com (Postfix) with ESMTP id 9B77C40007 for ; Fri, 24 Nov 2023 00:43:01 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=BmiaO0Eu; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf11.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.126 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1700786582; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=d5x6mIRzoliL7GEORsr4xPNUH/Bw91lX8K4p6mRWWbw=; b=N06JEABl2WfGZT35eEt1HxyMi4XgcnhOREbZ2pKL0lqpxJzDfRdYGc1ARaTefZIhw9RdFE 2jFXB3r1unfHjvYbLSR0ErWDb7Vbz4zy8h9PIuHqAvwAi/LMiFqvTq435XimPUzpQG89b2 7wwH/k7ciThMZN6xyK4iQ5Gr+Y14mSI= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=BmiaO0Eu; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf11.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.126 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1700786582; a=rsa-sha256; cv=none; b=r2OCvbuSdi33Wq0+RCx1+MgN22bTvuZsHXtk0JUP40LcSdITh3e38ndR8vNIFZ/3T6oOBG V7VBokc1Vzqk8DrVjvFcrGW1yhknZxrro2VoW+4kcXbD4EA4Hr/Raw/URr83LM1QXsWYNU O1IY/p1YaqD2Imfkd1CHTJex+dAiMzs= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1700786581; x=1732322581; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=LCijnWUbR8fFTOXQFLktGTa8GhwEgc+K0cu0Fz4sHUE=; b=BmiaO0EuD7pl/aLasNuTvYy8zOTs/dhkg5WAW6OP+LI9qtoFlTG8z/v6 jxjTDtvZWDHpmFAMDFPEHmipTIscOgzr3klgFFJvkPd6FqeJ5aTc29aV2 4pOMDXC+284nee2cGKjZvxk3SDlzkiWoczBGz1VzBZ1yVX4mcApUGSMJr d5On4fg+Mrp+RD5JbcthmiaEOCVCBhC0SOyxp0RdpkAxzTT0B/upipYNZ onjP5oJFxXkmtW2wE4Ps402EyG/frp4vyt1hAY6N22U9b11u912W/wYsc 2MBamsf8GSd+84q8p7P4t+rqk4X2n0XdQBjc263bPYhBQ221q6UW1HE1e A==; X-IronPort-AV: E=McAfee;i="6600,9927,10902"; a="377373485" X-IronPort-AV: E=Sophos;i="6.04,223,1695711600"; d="scan'208";a="377373485" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Nov 2023 16:42:59 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10902"; a="717191678" X-IronPort-AV: E=Sophos;i="6.04,223,1695711600"; d="scan'208";a="717191678" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Nov 2023 16:42:57 -0800 From: "Huang, Ying" To: Kairui Song Cc: linux-mm@kvack.org, Andrew Morton , David Hildenbrand , Hugh Dickins , Johannes Weiner , Matthew Wilcox , Michal Hocko , linux-kernel@vger.kernel.org Subject: Re: [PATCH 16/24] mm/swap: reduce scope of get_swap_device in swapin path In-Reply-To: (Kairui Song's message of "Thu, 23 Nov 2023 19:13:27 +0800") References: <20231119194740.94101-1-ryncsn@gmail.com> <20231119194740.94101-17-ryncsn@gmail.com> <87sf4yaajv.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Fri, 24 Nov 2023 08:40:56 +0800 Message-ID: <875y1s7zl3.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 9B77C40007 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: h4tycbx8qhfy5cjka4ipryoneyezhh31 X-HE-Tag: 1700786581-242408 X-HE-Meta: U2FsdGVkX1+hhpersZpVfCw7Cg1QszIK8spUZjg/QFFmdwae3Di6f36kkXjqh/IZ+EGRKJTBOnw9SDg+rLCbFZ2ueWFKPoJz7WME0vw/Ie/x/ET3Bm+ZOmhNKEXrIENspx1qmo+AyE7f/al95UnoBbZWMd/m2GKyj3250priJS14f9jkHaMK/R7Fwm/cbEcQgCwocc1CTRCs27WDveECB+kixYs7AoVhrdTuuEU50oOPaKHDBHfLWrFkoQJ4V+Y2fYxUlB0ZDKwOQ3280HikrgZuaPspdfy0e5LP9pH1Dcufop6NLr9d1CeA4JSDzMF7KphQ2S3eWmiyeJpgI1kKQQErtd3lnLdYfFms8/iiPXv4E1wlUL/d8eC8FGCfnRbR0dY6JA5hu3eK2cwM9n0ofegyRwXh5ASUhkRwvf+N/Ej/SwgGnaqBOBUmyseEozcrjsK5mc0wH1kOs5owneGzUzW1UgMYnOcIt4dJ+3vXVOIFuzVZA95R+J2aTKZHFXBpVmpCNIuHftAUy236hleVljJqlZgNDSt7sQaWw12T7VIaHzl1aczeLZPywbTWDcDlOosQBaOI9b9uwxxiJXHTpdrrZ8IONr9d7a9eU7u81JpQJkDLe5roWmDIgLxV/rAtMzSGRcI01fE0O2V1l2WbhYDTEvUoX56T0P5QxjTdgw0Sl2nsBSl0laQvJZYT7JjU4RbtjIuBo4E7znzDCfLW+iXC85DQl5wT/oxRXgz2uSZPioyBzDd7ZlgCR/oFEoHFUH7wfPHDXL586LtEXfelyxlwdCPkxWpyhfhe2R+4l1MFOPTP/UHg8xtoAa35/7LMvV9DBav9gVI2PP01uwUhmCjSDA9Ub1/aqeu9A9EFkqjpFdpB3P8HNCahPDcRG+P7BEEo6Gn/8Yfj8QqcVQBjtgRoORkJ8g1xrEh8vC9rSjI0PHRNfiUfEIqEsOyB2mtVaXVCAOr7F9hj48qbO02 2vcrhZhs aYN60eOgLh3utp6Y41p2VfofpABCE5S97YwrVdgr2ghwNkv1dOSb7M8GnwtXsqH2qg6r5C740Z/0a2SebBM/qQBIJJx+dLMibRKqQw5kColjaYOm9uT4NqMVOnFFSXDH9YIzHJvU/VGg7S21Vpi84Ggja3U43Zawe7+e9Z3jqQG3nnj+Hq/Tcxp3CPcucPqIgs5F+US2oOXpD4ZhHL7vTw478+8TmDIVaeTJ8ktnewgAkAPqQQySVLzfFs2foAh9Vu04wZqulXa1l7nAmvCEKcCKxigURzgRUI3Z33i0QUqVtjoqOet6WwmgDoLyRVApO2uMnKegZojbesdUn94GRQLZWdu7wPYHzmFwk 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: List-Subscribe: List-Unsubscribe: Kairui Song writes: > Huang, Ying =E4=BA=8E2023=E5=B9=B411=E6=9C=8822=E6= =97=A5=E5=91=A8=E4=B8=89 08:38=E5=86=99=E9=81=93=EF=BC=9A >> >> Kairui Song writes: >> >> > From: Kairui Song >> > >> > Move get_swap_device into swapin_readahead, simplify the code >> > and prepare for follow up commits. >> >> No. Please don't do this. Please check the get/put_swap_device() usage >> rule in the comments of get_swap_device(). >> >> " >> * When we get a swap entry, if there aren't some other ways to >> * prevent swapoff, such as the folio in swap cache is locked, page >> * table lock is held, etc., the swap entry may become invalid because >> * of swapoff. Then, we need to enclose all swap related functions >> * with get_swap_device() and put_swap_device(), unless the swap >> * functions call get/put_swap_device() by themselves. >> " >> >> This is to simplify the reasoning about swapoff and swap entry. >> >> Why does it bother you? > > Hi Ying, > > This is trying to reduce LOC, avoid a trivial si read, and make error > checking logic easier to refactor in later commits. The race with swapoff isn't considered by many developers usually. So, we should use a simple rule as much as possible. For example, if you get a swap entry, use get/put_swap_device() to enclose all code that operate on the swap entry. This makes code easier to be maintained in the long run. Yes. Sometimes we break the rule a little, but only if we have enough benefit, such as improving performance in some practical use cases. > And besides there is one trivial change I forgot to include in this > commit, get_swap_device can be put after swap_cache_get_folio in > swapin_readahead, since swap_cache_get_folio doesn't need to hold the > swap device, so in cache hit case this get/put_swap_device() can be > saved. swapoff is rare operation, it's OK to delay it a little to make the code easier to be understood. > The comment also mentioned: > > "Then, we need to enclose all swap related functions with > get_swap_device() and put_swap_device(), unless the swap functions > call get/put_swap_device() by themselves" > > So I think it should be OK to do this. No. You should change the code with a good enough reason. Compared with complexity it introduced, the benefit isn't enough for me so far. -- Best Regards, Huang, Ying