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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E3BCACDB474 for ; Mon, 16 Oct 2023 06:20:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229866AbjJPGUt (ORCPT ); Mon, 16 Oct 2023 02:20:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48684 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229623AbjJPGUp (ORCPT ); Mon, 16 Oct 2023 02:20:45 -0400 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.88]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5827E95 for ; Sun, 15 Oct 2023 23:20:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697437243; x=1728973243; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=6QV46esyzMbwV1pApkXxUKx74MXSyq2TMVxLFgLyDZs=; b=lMMc9MQpgFD7Qu+cs1uzJaflS4KKTtKR0sykwVHVvzxempSK7fnfhD50 IP6nxMhhS2MT3d17DT1qRL7csoOYLTrbmDzr+ivP7IJ4UItOyTt2R2wjR oDYndGL8vvQvyw04CTRxnt/HRuw369dfptdoOHjdXOtaz3aBDFTIfOsL7 BeS31622nSX9Y565ehMZftRyroqQ54sC/BVo12GnXxcDZqvYVJz78eI3s FKqWgRJWkI/9wPsoJ6A5FEQ3Jm/ETimcFTBdqFDUo9cSgu/hBGvjRqMfl lUivOCFfWBl9cHvVywYK6An0CepVJcxX7slYbuyaLHk+VREGveQGJifXg g==; X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="416522614" X-IronPort-AV: E=Sophos;i="6.03,228,1694761200"; d="scan'208";a="416522614" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2023 23:20:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="755571761" X-IronPort-AV: E=Sophos;i="6.03,228,1694761200"; d="scan'208";a="755571761" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2023 23:20:11 -0700 From: "Huang, Ying" To: Ryan Roberts Cc: Andrew Morton , David Hildenbrand , Matthew Wilcox , Gao Xiang , Yu Zhao , Yang Shi , Michal Hocko , , Subject: Re: [RFC PATCH v1 2/2] mm: swap: Swap-out small-sized THP without splitting References: <20231010142111.3997780-1-ryan.roberts@arm.com> <20231010142111.3997780-3-ryan.roberts@arm.com> <87r0m1ftvu.fsf@yhuang6-desk2.ccr.corp.intel.com> <1ccde143-18a7-483b-a9a4-fff07b0edc72@arm.com> Date: Mon, 16 Oct 2023 14:17:43 +0800 In-Reply-To: <1ccde143-18a7-483b-a9a4-fff07b0edc72@arm.com> (Ryan Roberts's message of "Wed, 11 Oct 2023 18:14:37 +0100") Message-ID: <87ttqrm6pk.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ryan Roberts writes: > On 11/10/2023 11:36, Ryan Roberts wrote: >> On 11/10/2023 09:25, Huang, Ying wrote: >>> Ryan Roberts writes: >>> >>>> The upcoming anonymous small-sized THP feature enables performance >>>> improvements by allocating large folios for anonymous memory. However >>>> I've observed that on an arm64 system running a parallel workload (e.g. >>>> kernel compilation) across many cores, under high memory pressure, the >>>> speed regresses. This is due to bottlenecking on the increased number of >>>> TLBIs added due to all the extra folio splitting. >>>> >>>> Therefore, solve this regression by adding support for swapping out >>>> small-sized THP without needing to split the folio, just like is already >>>> done for PMD-sized THP. This change only applies when CONFIG_THP_SWAP is >>>> enabled, and when the swap backing store is a non-rotating block device >>>> - these are the same constraints as for the existing PMD-sized THP >>>> swap-out support. >>>> >>>> Note that no attempt is made to swap-in THP here - this is still done >>>> page-by-page, like for PMD-sized THP. >>>> >>>> The main change here is to improve the swap entry allocator so that it >>>> can allocate any power-of-2 number of contiguous entries between [4, (1 >>>> << PMD_ORDER)]. This is done by allocating a cluster for each distinct >>>> order and allocating sequentially from it until the cluster is full. >>>> This ensures that we don't need to search the map and we get no >>>> fragmentation due to alignment padding for different orders in the >>>> cluster. If there is no current cluster for a given order, we attempt to >>>> allocate a free cluster from the list. If there are no free clusters, we >>>> fail the allocation and the caller falls back to splitting the folio and >>>> allocates individual entries (as per existing PMD-sized THP fallback). >>>> >>>> As far as I can tell, this should not cause any extra fragmentation >>>> concerns, given how similar it is to the existing PMD-sized THP >>>> allocation mechanism. There will be up to (PMD_ORDER-1) clusters in >>>> concurrent use though. In practice, the number of orders in use will be >>>> small though. >>>> >>>> Signed-off-by: Ryan Roberts >>>> --- >>>> include/linux/swap.h | 7 ++++++ >>>> mm/swapfile.c | 60 +++++++++++++++++++++++++++++++++----------- >>>> mm/vmscan.c | 10 +++++--- >>>> 3 files changed, 59 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/include/linux/swap.h b/include/linux/swap.h >>>> index a073366a227c..fc55b760aeff 100644 >>>> --- a/include/linux/swap.h >>>> +++ b/include/linux/swap.h >>>> @@ -320,6 +320,13 @@ struct swap_info_struct { >>>> */ >>>> struct work_struct discard_work; /* discard worker */ >>>> struct swap_cluster_list discard_clusters; /* discard clusters list */ >>>> + unsigned int large_next[PMD_ORDER]; /* >>>> + * next free offset within current >>>> + * allocation cluster for large >>>> + * folios, or UINT_MAX if no current >>>> + * cluster. Index is (order - 1). >>>> + * Only when cluster_info is used. >>>> + */ >>> >>> I think that it is better to make this per-CPU. That is, extend the >>> percpu_cluster mechanism. Otherwise, we may have scalability issue. >> >> Is your concern that the swap_info spinlock will get too contended as its >> currently written? From briefly looking at percpu_cluster, it looks like that >> spinlock is always held when accessing the per-cpu structures - presumably >> that's what's disabling preemption and making sure the thread is not migrated? >> So I'm not sure what the benefit is currently? Surely you want to just disable >> preemption but not hold the lock? I'm sure I've missed something crucial... > > I looked a bit further at how to implement what you are suggesting. > get_swap_pages() is currently taking the swap_info lock which it needs to check > and update some other parts of the swap_info - I'm not sure that part can be > removed. swap_alloc_large() (my new function) is not doing an awful lot of work, > so I'm not convinced that you would save too much by releasing the lock for that > part. In contrast there is a lot more going on in scan_swap_map_slots() so there > is more benefit to releasing the lock and using the percpu stuff - correct me if > I've missunderstood. > > As an alternative approach, perhaps it makes more sense to beef up the caching > layer in swap_slots.c to handle large folios too? Then you avoid taking the > swap_info lock at all most of the time, like you currently do for single entry > allocations. > > What do you think? Sorry for late reply. percpu_cluster is introduced in commit ebc2a1a69111 ("swap: make cluster allocation per-cpu"). Please check the changelog for why it's introduced. Sorry about my incorrect memory about scalability. percpu_cluster is introduced for disk performance mainly instead of scalability. -- Best Regards, Huang, Ying >> >>> >>> And this should be enclosed in CONFIG_THP_SWAP. >> >> Yes, I'll fix this in the next version. >> >> Thanks for the review! >> >>> >>>> struct plist_node avail_lists[]; /* >>>> * entries in swap_avail_heads, one >>>> * entry per node.