Mina,
On Wed, May 12, 2021 at 12:42:32PM -0700, Mina Almasry wrote:
> > >> @@ -4868,30 +4869,39 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > >> struct page **pagep)
> > >> {
> > >> bool is_continue = (mode == MCOPY_ATOMIC_CONTINUE);
> > >> - struct address_space *mapping;
> > >> - pgoff_t idx;
> > >> + struct hstate *h = hstate_vma(dst_vma);
> > >> + struct address_space *mapping = dst_vma->vm_file->f_mapping;
> > >> + pgoff_t idx = vma_hugecache_offset(h, dst_vma, dst_addr);
> > >> unsigned long size;
> > >> int vm_shared = dst_vma->vm_flags & VM_SHARED;
> > >> - struct hstate *h = hstate_vma(dst_vma);
> > >> pte_t _dst_pte;
> > >> spinlock_t *ptl;
> > >> - int ret;
> > >> + int ret = -ENOMEM;
> > >> struct page *page;
> > >> int writable;
> > >>
> > >> - mapping = dst_vma->vm_file->f_mapping;
> > >> - idx = vma_hugecache_offset(h, dst_vma, dst_addr);
> > >> + /* Out parameter. */
> > >> + WARN_ON(*pagep);
> > >
> > > I don't think this warning works, because we do set *pagep, in the
> > > copy_huge_page_from_user failure case. In that case, the following
> > > happens:
> > >
> > > 1. We set *pagep, and return immediately.
> > > 2. Our caller notices this particular error, drops mmap_lock, and then
> > > calls us again with *pagep set.
> > >
> > > In this path, we're supposed to just re-use this existing *pagep
> > > instead of allocating a second new page.
> > >
> > > I think this also means we need to keep the "else" case where *pagep
> > > is set below.
> > >
> >
> > +1 to Peter's comment.
> >
>
> Gah, sorry about that. I'll fix in v2.
I have a question regarding v1: how do you guarantee huge_add_to_page_cache()
won't fail again even if checked before page alloc? Say, what if the page
cache got inserted after hugetlbfs_pagecache_present() (which is newly added in
your v1) but before huge_add_to_page_cache()?
I also have a feeling that we've been trying to work around something else, but
I can't tell yet as I'll probably need to read a bit more/better on how hugetlb
does the accounting and also on reservations.
Thanks,
--
Peter Xu
On 5/12/21 1:14 PM, Peter Xu wrote:
> On Wed, May 12, 2021 at 12:42:32PM -0700, Mina Almasry wrote:
>>>>> @@ -4868,30 +4869,39 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>>>>> + WARN_ON(*pagep);
>>>>
>>>> I don't think this warning works, because we do set *pagep, in the
>>>> copy_huge_page_from_user failure case. In that case, the following
>>>> happens:
>>>>
>>>> 1. We set *pagep, and return immediately.
>>>> 2. Our caller notices this particular error, drops mmap_lock, and then
>>>> calls us again with *pagep set.
>>>>
>>>> In this path, we're supposed to just re-use this existing *pagep
>>>> instead of allocating a second new page.
>>>>
>>>> I think this also means we need to keep the "else" case where *pagep
>>>> is set below.
>>>>
>>>
>>> +1 to Peter's comment.
>>>
Apologies to Axel (and Peter) as that comment was from Axel.
>>
>> Gah, sorry about that. I'll fix in v2.
>
> I have a question regarding v1: how do you guarantee huge_add_to_page_cache()
> won't fail again even if checked before page alloc? Say, what if the page
> cache got inserted after hugetlbfs_pagecache_present() (which is newly added in
> your v1) but before huge_add_to_page_cache()?
In the caller (__mcopy_atomic_hugetlb) we obtain the hugetlb fault mutex
before calling this routine. This should prevent changes to the cache
while in the routine.
However, things get complicated in the case where copy_huge_page_from_user
fails. In this case, we will return to the caller which will drop mmap_lock
and the hugetlb fault mutex before doing the copy. After dropping the
mutex, someone could populate the cache. This would result in the same
situation where two reserves are 'temporarily' consumed for the same
mapping offset. By the time we get to the second call to
hugetlb_mcopy_atomic_pte where the previously allocated page is passed
in, it is too late.
--
Mike Kravetz
On Wed, May 12, 2021 at 2:31 PM Mike Kravetz <[email protected]> wrote:
>
> On 5/12/21 1:14 PM, Peter Xu wrote:
> > On Wed, May 12, 2021 at 12:42:32PM -0700, Mina Almasry wrote:
> >>>>> @@ -4868,30 +4869,39 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> >>>>> + WARN_ON(*pagep);
> >>>>
> >>>> I don't think this warning works, because we do set *pagep, in the
> >>>> copy_huge_page_from_user failure case. In that case, the following
> >>>> happens:
> >>>>
> >>>> 1. We set *pagep, and return immediately.
> >>>> 2. Our caller notices this particular error, drops mmap_lock, and then
> >>>> calls us again with *pagep set.
> >>>>
> >>>> In this path, we're supposed to just re-use this existing *pagep
> >>>> instead of allocating a second new page.
> >>>>
> >>>> I think this also means we need to keep the "else" case where *pagep
> >>>> is set below.
> >>>>
> >>>
> >>> +1 to Peter's comment.
> >>>
>
> Apologies to Axel (and Peter) as that comment was from Axel.
>
> >>
> >> Gah, sorry about that. I'll fix in v2.
> >
> > I have a question regarding v1: how do you guarantee huge_add_to_page_cache()
> > won't fail again even if checked before page alloc? Say, what if the page
> > cache got inserted after hugetlbfs_pagecache_present() (which is newly added in
> > your v1) but before huge_add_to_page_cache()?
>
> In the caller (__mcopy_atomic_hugetlb) we obtain the hugetlb fault mutex
> before calling this routine. This should prevent changes to the cache
> while in the routine.
>
> However, things get complicated in the case where copy_huge_page_from_user
> fails. In this case, we will return to the caller which will drop mmap_lock
> and the hugetlb fault mutex before doing the copy. After dropping the
> mutex, someone could populate the cache. This would result in the same
> situation where two reserves are 'temporarily' consumed for the same
> mapping offset. By the time we get to the second call to
> hugetlb_mcopy_atomic_pte where the previously allocated page is passed
> in, it is too late.
>
Thanks. I tried locally to allocate a page, then add it into the
cache, *then* copy its contents (dropping that lock if that fails).
That also has the test passing, but I'm not sure if I'm causing a fire
somewhere else by having a page in the cache that has uninitialized
contents. The only other code that checks the cache seems to be the
hugetlb_fault/hugetlb_cow code. I'm reading that code to try to
understand if I'm breaking that code doing this.
> --
> Mike Kravetz