lkml.org 
[lkml]   [2019]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v10 3/4] mm, thp: introduce FOLL_SPLIT_PMD
    Date


    > On Jul 31, 2019, at 8:18 AM, Oleg Nesterov <oleg@redhat.com> wrote:
    >
    > On 07/30, Song Liu wrote:
    >>
    >>
    >>> On Jul 30, 2019, at 9:11 AM, Oleg Nesterov <oleg@redhat.com> wrote:
    >>>
    >>> So after the next patch we have a single user of FOLL_SPLIT_PMD (uprobes)
    >>> and a single user of FOLL_SPLIT: arch/s390/mm/gmap.c:thp_split_mm().
    >>>
    >>> Hmm.
    >>
    >> I think this is what we want. :)
    >
    > We? I don't ;)
    >
    >> FOLL_SPLIT is the fallback solution for users who cannot handle THP.
    >
    > and again, we have a single user: thp_split_mm(). I do not know if it
    > can use FOLL_SPLIT_PMD or not, may be you can take a look...

    I haven't played with s390, so it gonna take me some time to ramp up.
    I will add it to my to-do list.

    >
    >> With
    >> more THP aware code, there will be fewer users of FOLL_SPLIT.
    >
    > Fewer than 1? Good ;)

    Yes! It will be great if thp_split_mm() can use FOLL_SPLIT_PMD
    instead.

    >
    >>>> @@ -399,7 +399,7 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
    >>>> spin_unlock(ptl);
    >>>> return follow_page_pte(vma, address, pmd, flags, &ctx->pgmap);
    >>>> }
    >>>> - if (flags & FOLL_SPLIT) {
    >>>> + if (flags & (FOLL_SPLIT | FOLL_SPLIT_PMD)) {
    >>>> int ret;
    >>>> page = pmd_page(*pmd);
    >>>> if (is_huge_zero_page(page)) {
    >>>> @@ -408,7 +408,7 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
    >>>> split_huge_pmd(vma, pmd, address);
    >>>> if (pmd_trans_unstable(pmd))
    >>>> ret = -EBUSY;
    >>>> - } else {
    >>>> + } else if (flags & FOLL_SPLIT) {
    >>>> if (unlikely(!try_get_page(page))) {
    >>>> spin_unlock(ptl);
    >>>> return ERR_PTR(-ENOMEM);
    >>>> @@ -420,6 +420,10 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
    >>>> put_page(page);
    >>>> if (pmd_none(*pmd))
    >>>> return no_page_table(vma, flags);
    >>>> + } else { /* flags & FOLL_SPLIT_PMD */
    >>>> + spin_unlock(ptl);
    >>>> + split_huge_pmd(vma, pmd, address);
    >>>> + ret = pte_alloc(mm, pmd);
    >>>
    >>> I fail to understand why this differs from the is_huge_zero_page() case above.
    >>
    >> split_huge_pmd() handles is_huge_zero_page() differently. In this case, we
    >> cannot use the pmd_trans_unstable() check.
    >
    > Please correct me, but iiuc the problem is not that split_huge_pmd() handles
    > is_huge_zero_page() differently, the problem is that __split_huge_pmd_locked()
    > handles the !vma_is_anonymous(vma) differently and returns with pmd_none() = T
    > after pmdp_huge_clear_flush_notify(). This means that pmd_trans_unstable() will
    > fail.

    Agreed.

    >
    > Now, I don't understand why do we need pmd_trans_unstable() after
    > split_huge_pmd(huge-zero-pmd), but whatever reason we have, why can't we
    > unify both cases?
    >
    > IOW, could you explain why the path below is wrong?

    I _think_ the following patch works (haven't fully tested yet). But I am not
    sure whether this is the best. By separating the two cases, we don't duplicate
    much code. And it is clear that the two cases are handled differently.
    Therefore, I would prefer to keep these separate for now.

    Thanks,
    Song

    >
    >
    > --- x/mm/gup.c
    > +++ x/mm/gup.c
    > @@ -399,14 +399,16 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
    > spin_unlock(ptl);
    > return follow_page_pte(vma, address, pmd, flags, &ctx->pgmap);
    > }
    > - if (flags & FOLL_SPLIT) {
    > + if (flags & (FOLL_SPLIT | FOLL_SPLIT_PMD)) {
    > int ret;
    > page = pmd_page(*pmd);
    > - if (is_huge_zero_page(page)) {
    > + if ((flags & FOLL_SPLIT_PMD) || is_huge_zero_page(page)) {
    > spin_unlock(ptl);
    > - ret = 0;
    > split_huge_pmd(vma, pmd, address);
    > - if (pmd_trans_unstable(pmd))
    > + ret = 0;
    > + if (pte_alloc(mm, pmd))
    > + ret = -ENOMEM;
    > + else if (pmd_trans_unstable(pmd))
    > ret = -EBUSY;
    > } else {
    > if (unlikely(!try_get_page(page))) {
    >

    \
     
     \ /
      Last update: 2019-07-31 21:17    [W:3.495 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site