On 3/12/21 7:11 PM, Miaohe Lin wrote:
> On 2021/3/13 3:09, Mike Kravetz wrote:
>> On 3/1/21 4:05 AM, Miaohe Lin wrote:
>>> The current implementation of hugetlb_cgroup for shared mappings could have
>>> different behavior. Consider the following two scenarios:
>>>
>>> 1.Assume initial css reference count of hugetlb_cgroup is 1:
>>> 1.1 Call hugetlb_reserve_pages with from = 1, to = 2. So css reference
>>> count is 2 associated with 1 file_region.
>>> 1.2 Call hugetlb_reserve_pages with from = 2, to = 3. So css reference
>>> count is 3 associated with 2 file_region.
>>> 1.3 coalesce_file_region will coalesce these two file_regions into one.
>>> So css reference count is 3 associated with 1 file_region now.
>>>
>>> 2.Assume initial css reference count of hugetlb_cgroup is 1 again:
>>> 2.1 Call hugetlb_reserve_pages with from = 1, to = 3. So css reference
>>> count is 2 associated with 1 file_region.
>>>
>>> Therefore, we might have one file_region while holding one or more css
>>> reference counts. This inconsistency could lead to imbalanced css_get()
>>> and css_put() pair. If we do css_put one by one (i.g. hole punch case),
>>> scenario 2 would put one more css reference. If we do css_put all together
>>> (i.g. truncate case), scenario 1 will leak one css reference.
>>>
>>> The imbalanced css_get() and css_put() pair would result in a non-zero
>>> reference when we try to destroy the hugetlb cgroup. The hugetlb cgroup
>>> directory is removed __but__ associated resource is not freed. This might
>>> result in OOM or can not create a new hugetlb cgroup in a busy workload
>>> ultimately.
>>>
>>> In order to fix this, we have to make sure that one file_region must hold
>>> and only hold one css reference. So in coalesce_file_region case, we should
>>
>> I think this would sound/read better if stated as:
>> ... one file_region must hold exactly one css reference ...
>>
>> This phrase is repeated in comments throughout the patch.
>>
>>> release one css reference before coalescence. Also only put css reference
>>> when the entire file_region is removed.
>>>
>>> The last thing to note is that the caller of region_add() will only hold
>>> one reference to h_cg->css for the whole contiguous reservation region.
>>> But this area might be scattered when there are already some file_regions
>>> reside in it. As a result, many file_regions may share only one h_cg->css
>>> reference. In order to ensure that one file_region must hold and only hold
>>> one h_cg->css reference, we should do css_get() for each file_region and
>>> release the reference held by caller when they are done.
>>
>> Thanks for adding this to the commit message.
>>
>>>
>>> Fixes: 075a61d07a8e ("hugetlb_cgroup: add accounting for shared mappings")
>>> Reported-by: kernel test robot <lkp@xxxxxxxxx> (auto build test ERROR)
>>> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>>> Cc: stable@xxxxxxxxxx
>>> ---
>>> v1->v2:
>>> Fix auto build test ERROR when CGROUP_HUGETLB is disabled.
>>> ---
>>> include/linux/hugetlb_cgroup.h | 15 ++++++++++--
>>> mm/hugetlb.c | 42 ++++++++++++++++++++++++++++++----
>>> mm/hugetlb_cgroup.c | 11 +++++++--
>>> 3 files changed, 60 insertions(+), 8 deletions(-)
>>
>> Just a few minor nits below, all in comments. It is not required, but
>> would be nice to update these. Code looks good.
>>
>
> Many thanks for review! I will fix all this nits. Should I resend this patch or send another
> one to fix this? Just let me know which is the easiest one for you.
>
> Thanks again. :)
>
This is really Andrew's call as it goes through his tree.
I would suggest you just update the comments and send another verion.
In that way, Andrew can do whatever is easiest for him.
--
Mike Kravetz
