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] hugetlbfs: remove meaningless variable avoid_reserve
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
References: <20210116092644.42697-1-linmiaohe@huawei.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
Message-ID: <2900fea0-e77c-8c6a-1529-c95ded5319e6@oracle.com>
Date:   Tue, 19 Jan 2021 10:41:58 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
In-Reply-To: <20210116092644.42697-1-linmiaohe@huawei.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 MW4PR03CA0420.namprd03.prod.outlook.com (2603:10b6:303:115::35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3763.12 via Frontend Transport; Tue, 19 Jan 2021 18:41:59 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 177a0edd-9d07-40d7-7b3e-08d8bca9e7a5
X-MS-TrafficTypeDiagnostic: CO1PR10MB4435:
X-Microsoft-Antispam-PRVS: <CO1PR10MB44359991D852D632CA5254E5E2A30@CO1PR10MB4435.namprd10.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:10000;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: RU5fNGNTb01tD+IDA+e200V3T9H7TnuB52QTZ/ou6edOGkT5PFhPNvlh3KXjxvRxR2JFKNi4VQHBVVenyHDGDdfuiwB14EF6IgzfYjCxvnT/DmL3enqlt9TWi0y1i+62kBbNjE2hlvJ1tyI5RKLrQgZhdS52ss6tns48of0B+C3tXU5BrEHrNDIdfqyDb/+1WsqqO1iIjG2utIdD8/odxHw5sPdFLpjCPG80IYPZUHoBCrzw6iIU8KwjmCAgsQOxW7qCk4uu0YVyWoLWXbdM7BeG37v1hZi3yVqUjtwO2bcuUTMql7JcjKQCPNQOY8Z4s8dnbywryPR9nF/LJ6pwTkMwiEMvcuT1bY+uxss/NjI1NpxFtVnaDWZpM30DpcNH4cvdFZyZDRS3/vHt4Di7boij3brovjcIlsPzaqM6hq3jS/+1OE2cZwxjIq86MQ5Hv11T+wKgxLkhHVLOo2YmrDMQ9PnzfeKlqefJWFu6D2o=
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:(396003)(39860400002)(346002)(366004)(136003)(376002)(16526019)(186003)(478600001)(53546011)(66476007)(36756003)(26005)(44832011)(83380400001)(66946007)(66556008)(6916009)(8936002)(31696002)(31686004)(16576012)(2616005)(6486002)(2906002)(956004)(316002)(5660300002)(86362001)(52116002)(8676002)(4326008)(43740500002)(45980500001);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?QU9xMXZQcFliYlYxdHFlbXkvQ3BjelkzUlJ0RDA1cTNLTVUyUU1Wc2lrYnpC?=
 =?utf-8?B?d2txQ3h3MFJkN3Z5NDVKMmZnUGt1V1dROTB0K29sb1ltbGx6WkUvTWVocXAz?=
 =?utf-8?B?UFpBVHRwQkdGQ0EzTCtZOHBwTVF0ME4zVExjWS9yQ2NPbjd0YjJVRjQvMGQw?=
 =?utf-8?B?SlNJbE5sTnJWT3ZsQzlLbU1HUVZMbmtNSE5nTWZ4enk4Sjk2STVmd2Ztd1Uy?=
 =?utf-8?B?YWJNSEp4TnFCb2ppODZtWFVxV21zeW81WnVjM3ZzakNPZmJ0U3FrWlVqMG9V?=
 =?utf-8?B?MzFpLzIzbUdDcDk0ZktTOHNCYmk5N0x3Qk5LR3dwQ1N4NXk3dS80Sm9TdHY5?=
 =?utf-8?B?cFdWR2VMMk94UDFpdnZUNzE2L3FsUEhCbldXbFhzWnJPazlCNnBKd1lUWkxn?=
 =?utf-8?B?WS9FbmRRTUFFNHM5bStZUWtHUkFlMXpqUndML0dCNFh5dTZ6ZGQreSt6K1Zo?=
 =?utf-8?B?bzhJOWtQLzRURXNJMlBCUTVIc2ZIK1RWOUVBYStmb3F3OXJCZFJjbmswRVdJ?=
 =?utf-8?B?SFpUcWI1c1RRaXFsN1VlVGxWZ1g1dDBOMkxIUEIwbExMdzEzc21tNW5rVjBX?=
 =?utf-8?B?aUZyYVBYNC83RXBucldFU2pha2c3SmRHeGtPUmFveGlJMDFKTlh2dW8vUGdi?=
 =?utf-8?B?WWpMUlgzRE9uYkZSVDR4a1dmT0FGbkZySW1kcDRwK2ZKWU4yUnZXM05XajN4?=
 =?utf-8?B?ejVyMWNyN1gxL0N4WkJjdjllYi90YmVyQ0c4bjY3RFdKdzFacGhPMnVLVDhY?=
 =?utf-8?B?bW81bi9RbkpJSWhaSkZyZjc2YXQ1dlcySEFWSXk0MW8xc2dFUElDVTZFME5s?=
 =?utf-8?B?cmFBdVdOOENVT3ZqYnI4cXRKVHF3NUdiWnhJWldYcjFxVTA0VXd6amY2VjVY?=
 =?utf-8?B?dVlCNzZ4SFFqWkcyc3M5ZWI5UFBYRExHMmx6eW1sbThsRFVUbXgwQUJEbjlj?=
 =?utf-8?B?MUs4TWxnbENuUkhKUU40ZFlnM3pDYVlyb0M1RDdpRTBsVWdPdmFWa0VVemFk?=
 =?utf-8?B?elJONGQ0YThYeFRGOEg0R2NXc0ZYbzFyVndmRDgrZ0ZBbEZ1bFg2NjZBZTkw?=
 =?utf-8?B?ZEZRbDdtNVpMMVpVOVJSdEhLcGJFZG9ITlFpYkRFN1pUZEJQR1ZWK09IV3Js?=
 =?utf-8?B?OUs3RlZnVXZEZkN1OFhFVy9BMEU0cTFSa2dGUnZDRjF1MUx1STRQaWdYOVh1?=
 =?utf-8?B?KzlhbEZFWTROL052ZnpFZW5pVHBvRkJTOXZOK0NHR0hyMmxjQ2o4bzcyTFho?=
 =?utf-8?B?YitxUEFDdWZrM2Y4NXdUbTVlZVlielZSM1dySVhZSS9wV0tjaFdhelRSQnB6?=
 =?utf-8?B?YklJdXFBN0I4cnZRSTc4aWtOUkprQkxZeXJxVkxXcm9VY1YrNmR2ckVKK0Rw?=
 =?utf-8?B?TXVVMkZuQlh2UVdYT2VBQU05ZEtMVzUrTXVoZ2kvZ0dHUm5yMFZZWitoaXpV?=
 =?utf-8?Q?2nzijciS?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 177a0edd-9d07-40d7-7b3e-08d8bca9e7a5
X-MS-Exchange-CrossTenant-AuthSource: MWHPR10MB1389.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2021 18:41:59.8510
 (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: kkbbFXe/pCeZ8tn3R2Yk2Y02AixatwYD972TLnkoWlee7Z50SU56z3VPBq1EKYPgnYkgbgw1SIFkmRZEZrTd5A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4435
X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9869 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 malwarescore=0
 phishscore=0 mlxscore=0 adultscore=0 spamscore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2101190103
X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9869 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0
 malwarescore=0 mlxlogscore=999 bulkscore=0 priorityscore=1501 spamscore=0
 mlxscore=0 impostorscore=0 lowpriorityscore=0 suspectscore=0 clxscore=1015
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2101190103
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Please CC Andrew on hugetlb patches as they need to go through his tree.

On 1/16/21 1:26 AM, Miaohe Lin wrote:
> The variable avoid_reserve is meaningless because we never changed its
> value and just passed it to alloc_huge_page(). So remove it to make code
> more clear that in hugetlbfs_fallocate, we never avoid reserve when alloc
> hugepage yet.

One might argue that using a named variable makes the call to alloc_huge_page
more clear.  I do not disagree with the change,  However, there are some
subtle reasons why alloc_huge_page is called with 'avoid_reserve = 0' from
fallocate.  Therefore, I would prefer that a comment be added above the call
in addition to this change.  See below.

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  fs/hugetlbfs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 88751e35e69d..23ad6ed8b75f 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -680,7 +680,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  		 */
>  		struct page *page;
>  		unsigned long addr;
> -		int avoid_reserve = 0;
>  
>  		cond_resched();
>  
> @@ -717,7 +716,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  		}
>  
>  		/* Allocate page and add to page cache */

Perhaps, change comment to read:

		/*
		 * Allocate page without setting the avoid_reserve argument.
		 * There certainly are no reserves associated with the
		 * pseudo_vma.  However, there could be shared mappings with
		 * reserves for the file at the inode level.  If we fallocate
		 * pages in these areas, we need to consume the reserves
		 * to keep reservation accounting consistent.
		 */

-- 
Mike Kravetz

> -		page = alloc_huge_page(&pseudo_vma, addr, avoid_reserve);
> +		page = alloc_huge_page(&pseudo_vma, addr, 0);
>  		hugetlb_drop_vma_policy(&pseudo_vma);
>  		if (IS_ERR(page)) {
>  			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>