Re: [PATCH v2 2/4] mm: hugetlb: Fix out_put_pages subpool reserve calculation

[Ackerley Tng]

Joshua Hahn <joshua.hahnjy@xxxxxxxxx> writes:

>
> [...snip...]
>
>> @@ -6821,26 +6823,21 @@ long hugetlb_reserve_pages(struct inode *inode,
>>  	}
>>  	return chg;
>>
>> -out_put_pages:
>> -	spool_resv = chg - gbl_reserve;
>> -	if (spool_resv) {
>> -		/* put sub pool's reservation back, chg - gbl_reserve */
>> -		gbl_resv = hugepage_subpool_put_pages(spool, spool_resv);
>> -		/*
>> -		 * subpool's reserved pages can not be put back due to race,
>> -		 * return to hstate.
>> -		 */
>> -		hugetlb_acct_memory(h, -gbl_resv);
>> -	}
>> -	/* Restore used_hpages for pages that failed global reservation */
>> -	if (gbl_reserve && spool) {
>> -		unsigned long flags;
>> + out_put_pages:
>
> I'm not sure if this section warrants these two comment blocks anymore.
> The logic is quite straightforward now that we don't have to worry about
> managing the global / subpool reservations separately and rather just
> simply do the subtraction to account the memory.
>
>> +	/*
>> +	 * Return all that was requested from the subpool, let subpool
>> +	 * tell us the new number of reservations that need to be
>> +	 * returned to the global pool.
>> +	 */

I think this could be removed if you think it's obvious enough.

>> +	gbl_resv_put = hugepage_subpool_put_pages(spool, chg);
>> +	/*
>> +	 * There may be a difference between the number of
>> +	 * reservations to consume and the number to restore now if
>> +	 * there are multiple threads interacting with the subpool -
>> +	 * restore the difference.
>> +	 */

However, this other thing about threads racing needs to be emphasized
imo. Both subpool and hstate tracking are global. I believe pretty much
anything that updates global state based on local variables only in 1
thread is going to mess up the global state.

>> +	hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put);
>>
>> -		spin_lock_irqsave(&spool->lock, flags);
>> -		if (spool->max_hpages != -1)
>> -			spool->used_hpages -= gbl_reserve;
>> -		unlock_or_release_subpool(spool, flags);
>> -	}
>>  out_uncharge_cgroup:
>>  	hugetlb_cgroup_uncharge_cgroup_rsvd(hstate_index(h),
>>  					    chg * pages_per_huge_page(h), h_cg);
>>
>> --
>> 2.55.0.1007.g17ff1f9808-goog