Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/2] mm/hugetlb: grab head page refcount once per group of
 subpages
To:     Joao Martins <joao.m.martins@oracle.com>, linux-mm@kvack.org
Cc:     linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
References: <20210125205744.10203-1-joao.m.martins@oracle.com>
 <20210125205744.10203-2-joao.m.martins@oracle.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
Message-ID: <17c414bd-4262-b2ba-ef5a-85cd86e21e4c@oracle.com>
Date:   Mon, 25 Jan 2021 18:14:23 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
In-Reply-To: <20210125205744.10203-2-joao.m.martins@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [192.168.2.112] (50.38.35.18) by MWHPR1701CA0017.namprd17.prod.outlook.com (2603:10b6:301:14::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3784.12 via Frontend Transport; Tue, 26 Jan 2021 02:14:24 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 6ea80b24-7da4-4edb-00b6-08d8c1a01a07
X-MS-TrafficTypeDiagnostic: CO1PR10MB4516:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <CO1PR10MB45165E79625E0A6CF9887EAFE2BC9@CO1PR10MB4516.namprd10.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:6108;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 3tKBhl3Sni4MAF4zX7wiHzZClMkOI8+fAYyWp6z/bOcGeKoirj2Be60xr2ki7rIaL+mjvtEcN3wJXMm0IAGda7P9Ozk60ZYwizmsx6xUlt6apk+g5ExacVEXow12PKo7mVDLx0dCTsXTQVMZjvPc3l9ZYexAYT2feGIuaVFHumQSsQl6T6n6qKmFhP+MjKhB3Dg45zn/ABRWKh+sQm4YSH6gAxR/dSjoV5Qos/Dfc6fgDyuCOGaCVXVE6U3J/kOpvC2L4IbMg1QeKBRRY1c+IW0KML+Mq8gjoynqP7PZ55p6mXv0JEiMndGMNF84zfe7itIbd/CAfVjarnyMgaVZVoiMDWwo3WkpbTB2M/WyZDjx6VdMX+Q/xswLodEaaBLE7z1ED+ogAILoe31HGQcUpcajujKEDG9msqrXBmHZHQl0jiKYcQhVxfiYzxIxUfK0Q+9In6KqFD//69f5ew+L3dB6ERjvJC2Ibxb3MgBpAQg=
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MWHPR10MB1389.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(366004)(396003)(39860400002)(346002)(376002)(136003)(186003)(16576012)(66946007)(316002)(16526019)(8676002)(53546011)(36756003)(4326008)(26005)(66476007)(31686004)(31696002)(66556008)(5660300002)(2616005)(6486002)(478600001)(8936002)(956004)(2906002)(86362001)(83380400001)(44832011)(52116002)(45980500001)(43740500002);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?VFJtVUoxWDlUcUpGT0dyNzF1VDlXNzNVMFZUR3pDY2JTL1NoaXpIaHpWSnBF?=
 =?utf-8?B?ODdMSSt0VTRCc3M3dmZmYzZwWWdFdVpBMUFCQzR0TVpHOFI0ODhzRHU5QVFy?=
 =?utf-8?B?cG95d3RvRTRhNlovL0FRNWJRNmo5NE04TG5xS0dpN1BKZXVYT2gyNSs4azhJ?=
 =?utf-8?B?OEt0YnZleE15bFZhRVpkMDk4NmdPZjFqeEdlRStwVnEvdG9ESW1OVEd2Rnli?=
 =?utf-8?B?Qmd5bmFCY1dQMzZlTnVUSmpyajIyVkYrMDNCdlJaUnVGaWxjYkpaWFVZVjJX?=
 =?utf-8?B?LzZSUVVDQXM0RWZyVUJsUnlxNGJVcmN1cG54aGdNQzJ4cGFvU3QzU3RkYmFm?=
 =?utf-8?B?WWg3bG9QTWw0UWVEV3gvc3BJYmdmd3RZT1RJeU8vbjU3TVJxQmN6d2krWEx3?=
 =?utf-8?B?dXM4WDRJUnM1VndlRnFHNUUxeU8yYnFBRkR5TGo5bk04ZVh0NnM3MGhoY0pj?=
 =?utf-8?B?bFdZM3E1MDVKeU42YXEySXZ4U3hocWhsclZZdUVHa3VSbDRqYndGYTFyTVBE?=
 =?utf-8?B?R0dxU2NVL1RDOHFqS3hJQU1Xd1dndVJuR0dDSFVxWlRicmFvUWdpSGhXdjJT?=
 =?utf-8?B?WXZBNzdBTUgzOGREZGxTU1N3SGN3UE9YcjlLV1pjbVNjY1pVWnlmRmppMyt0?=
 =?utf-8?B?QXV1VnUvbnVxZHVkMStNZnh2TGRkckhqaGRqM3k1aDA4citxUHVhTTBlQ1Vl?=
 =?utf-8?B?cHBtRmRIWVJ0UHo3MFhtRHJsTlFUMFVoRG9hQndNaEc3aS9xZStEVGorWi92?=
 =?utf-8?B?bDdWQ25yS1RWazRXaHpHcURyaUlyL1JxS1hUU1daVzUrWE1iVC95dXpJVlN1?=
 =?utf-8?B?K1JWdUtYNkdtK3NJQTgyNVJOUlQ4M3JqSjZzUWJhQXdGRmYwMXcycnpJcHQ5?=
 =?utf-8?B?WmJSaTRoN2dSSDFWM29xOXNuNysyb01vSXpNVGpxSEZjUVEyTXFqRHZ0ZVo1?=
 =?utf-8?B?NGxNRThQQnBPdzR5RjlYbU5Nd2FVNWxjNy96MUVQQTJ4Q2NMN1pIR1o5TXNz?=
 =?utf-8?B?MnAxZ1FSNDlsTUNCYkx4NjNWUlRpa29YYTRFd1Z0YTJuUy9LSFptREZMMmpn?=
 =?utf-8?B?b3JtRmk2T2laUm9DS29YbGFQZWprUmNwRWt6VndNUk4wakRBUGZLTTVhUElu?=
 =?utf-8?B?bVdHQTV4dnJzdEdnbURkRGw4SEh0NVBGRHlEK2lvT2x2Mmsva1V0a0dXa2dN?=
 =?utf-8?B?WjhTNy8reFRpc2dMaGVIQlU5TnZPZ3ozWlhJempZVUFrbEZaTElPWEI4UnM0?=
 =?utf-8?B?ek5BVXZTWmo1T20yU2tlWUJuRnZDRDRaQWhBb0dEVnpUd3NSSHVEWDlXNkFX?=
 =?utf-8?B?aUc4NndDTzc2VG1aYzFza29iUUd1K0RLZ01ES0Fjd3RRK3hGczVHNDFVbmFQ?=
 =?utf-8?B?NWdtUVQ1b0F3cjljSGJhWjMzNWxrYkZUNkVySGNLYkZRclFVVWd6RDI3TVVJ?=
 =?utf-8?Q?jo6r0xEm?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6ea80b24-7da4-4edb-00b6-08d8c1a01a07
X-MS-Exchange-CrossTenant-AuthSource: MWHPR10MB1389.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2021 02:14:25.0688
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /5JouT91+hF9Ds5dvCR7cmTwVWSaU8Pl02zY/1KSE98/PRfAjVh7pzqpHUYQXJZWIAGQg8IJfxFV9k0vaVXIDg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4516
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9875 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 spamscore=0 phishscore=0
 adultscore=0 mlxlogscore=999 malwarescore=0 suspectscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2101260007
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9875 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 phishscore=0
 adultscore=0 impostorscore=0 malwarescore=0 lowpriorityscore=0 bulkscore=0
 priorityscore=1501 mlxscore=0 clxscore=1015 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2101260008
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/25/21 12:57 PM, Joao Martins wrote:
> follow_hugetlb_page() once it locks the pmd/pud, it checks all the
> subpages in a huge page and grabs a reference for each one,
> depending on how many pages we can store or the size of va range.
> Similar to gup-fast, have follow_hugetlb_page() grab the head
> page refcount only after counting all its subpages that are part
> of the just faulted huge page.
> 
> Consequently we reduce the number of atomics necessary to pin
> said huge page, which improves non-fast gup() considerably:
> 
>  - 16G with 1G huge page size
>  gup_test -f /mnt/huge/file -m 16384 -r 10 -L -S -n 512 -w
> 
>  PIN_LONGTERM_BENCHMARK: ~87.6k us -> ~11k us
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  include/linux/mm.h |  3 +++
>  mm/gup.c           |  5 ++---
>  mm/hugetlb.c       | 43 ++++++++++++++++++++++++-------------------
>  3 files changed, 29 insertions(+), 22 deletions(-)

Thanks.  Nice straight forward improvement.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a5d618d08506..0d793486822b 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1182,6 +1182,9 @@ static inline void get_page(struct page *page)
>  }
>  
>  bool __must_check try_grab_page(struct page *page, unsigned int flags);
> +__maybe_unused struct page *try_grab_compound_head(struct page *page, int refs,
> +						   unsigned int flags);
> +
>  
>  static inline __must_check bool try_get_page(struct page *page)
>  {
> diff --git a/mm/gup.c b/mm/gup.c
> index 3e086b073624..ecadc80934b2 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -79,9 +79,8 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)
>   * considered failure, and furthermore, a likely bug in the caller, so a warning
>   * is also emitted.
>   */
> -static __maybe_unused struct page *try_grab_compound_head(struct page *page,
> -							  int refs,
> -							  unsigned int flags)
> +__maybe_unused struct page *try_grab_compound_head(struct page *page,
> +						   int refs, unsigned int flags)
>  {
>  	if (flags & FOLL_GET)
>  		return try_get_compound_head(page, refs);
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index a6bad1f686c5..016addc8e413 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4798,7 +4798,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	unsigned long vaddr = *position;
>  	unsigned long remainder = *nr_pages;
>  	struct hstate *h = hstate_vma(vma);
> -	int err = -EFAULT;
> +	int err = -EFAULT, refs;
>  
>  	while (vaddr < vma->vm_end && remainder) {
>  		pte_t *pte;
> @@ -4918,26 +4918,11 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  			continue;
>  		}
>  
> +		refs = 0;
> +
>  same_page:
> -		if (pages) {
> +		if (pages)
>  			pages[i] = mem_map_offset(page, pfn_offset);
> -			/*
> -			 * try_grab_page() should always succeed here, because:
> -			 * a) we hold the ptl lock, and b) we've just checked
> -			 * that the huge page is present in the page tables. If
> -			 * the huge page is present, then the tail pages must
> -			 * also be present. The ptl prevents the head page and
> -			 * tail pages from being rearranged in any way. So this
> -			 * page must be available at this point, unless the page
> -			 * refcount overflowed:
> -			 */
> -			if (WARN_ON_ONCE(!try_grab_page(pages[i], flags))) {
> -				spin_unlock(ptl);
> -				remainder = 0;
> -				err = -ENOMEM;
> -				break;
> -			}
> -		}
>  
>  		if (vmas)
>  			vmas[i] = vma;
> @@ -4946,6 +4931,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  		++pfn_offset;
>  		--remainder;
>  		++i;
> +		refs++;
>  		if (vaddr < vma->vm_end && remainder &&
>  				pfn_offset < pages_per_huge_page(h)) {
>  			/*
> @@ -4953,6 +4939,25 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  			 * of this compound page.
>  			 */
>  			goto same_page;
> +		} else if (pages) {
> +			/*
> +			 * try_grab_compound_head() should always succeed here,
> +			 * because: a) we hold the ptl lock, and b) we've just
> +			 * checked that the huge page is present in the page
> +			 * tables. If the huge page is present, then the tail
> +			 * pages must also be present. The ptl prevents the
> +			 * head page and tail pages from being rearranged in
> +			 * any way. So this page must be available at this
> +			 * point, unless the page refcount overflowed:
> +			 */
> +			if (WARN_ON_ONCE(!try_grab_compound_head(pages[i-1],
> +								 refs,
> +								 flags))) {
> +				spin_unlock(ptl);
> +				remainder = 0;
> +				err = -ENOMEM;
> +				break;
> +			}
>  		}
>  		spin_unlock(ptl);
>  	}
>