Re: [RFC PATCH v2 4/5] hugetlb_cgroup: Add accounting for shared mappings

[Mike Kravetz <mike.kravetz@oracle.com>]

On 8/15/19 4:08 PM, Mina Almasry wrote:
> On Tue, Aug 13, 2019 at 4:54 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>>  mm/hugetlb.c | 208 +++++++++++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 170 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index 235996aef6618..d76e3137110ab 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -242,8 +242,72 @@ struct file_region {
>>>       struct list_head link;
>>>       long from;
>>>       long to;
>>> +#ifdef CONFIG_CGROUP_HUGETLB
>>> +     /*
>>> +      * On shared mappings, each reserved region appears as a struct
>>> +      * file_region in resv_map. These fields hold the info needed to
>>> +      * uncharge each reservation.
>>> +      */
>>> +     struct page_counter *reservation_counter;
>>> +     unsigned long pages_per_hpage;
>>> +#endif
>>>  };
>>>
>>> +/* Must be called with resv->lock held. Calling this with dry_run == true will
>>> + * count the number of pages added but will not modify the linked list.
>>> + */
>>> +static long consume_regions_we_overlap_with(struct file_region *rg,
>>> +             struct list_head *head, long f, long *t,
>>> +             struct hugetlb_cgroup *h_cg,
>>> +             struct hstate *h,
>>> +             bool dry_run)
>>> +{
>>> +     long add = 0;
>>> +     struct file_region *trg = NULL, *nrg = NULL;
>>> +
>>> +     /* Consume any regions we now overlap with. */
>>> +     nrg = rg;
>>> +     list_for_each_entry_safe(rg, trg, rg->link.prev, link) {
>>> +             if (&rg->link == head)
>>> +                     break;
>>> +             if (rg->from > *t)
>>> +                     break;
>>> +
>>> +             /* If this area reaches higher then extend our area to
>>> +              * include it completely.  If this is not the first area
>>> +              * which we intend to reuse, free it.
>>> +              */
>>> +             if (rg->to > *t)
>>> +                     *t = rg->to;
>>> +             if (rg != nrg) {
>>> +                     /* Decrement return value by the deleted range.
>>> +                      * Another range will span this area so that by
>>> +                      * end of routine add will be >= zero
>>> +                      */
>>> +                     add -= (rg->to - rg->from);
>>> +                     if (!dry_run) {
>>> +                             list_del(&rg->link);
>>> +                             kfree(rg);
>>
>> Is it possible that the region struct we are deleting pointed to
>> a reservation_counter?  Perhaps even for another cgroup?
>> Just concerned with the way regions are coalesced that we may be
>> deleting counters.
>>
> 
> Yep, that needs to be handled I think. Thanks for catching!
> 

I believe that we will no longer be able to coalesce reserv_map entries
for shared mappings.  That is because we need to record who is responsible
for creating reservation entries.

-- 
Mike Kravetz