On Tue, Nov 12, 2019 at 3:08 PM John Hubbard jhubbard@nvidia.com wrote:

On 11/12/19 2:43 PM, Dan Williams wrote: ...

...

Ah, sorry. This was the first time I had looked at this series and jumped in without reading the background.

Your patch as is looks ok, I assume you've removed the FOLL_LONGTERM warning in get_user_pages_remote in another patch?

Actually, I haven't gone quite that far. Actually this patch is the last change to that function. Therefore, at the end of this patchset, get_user_pages_remote() ends up with this check in it which is a less-restrictive version of the warning:

    /*
     * Current FOLL_LONGTERM behavior is incompatible with
     * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on
     * vmas. However, this only comes up if locked is set, and there are
     * callers that do request FOLL_LONGTERM, but do not set locked. So,
     * allow what we can.
     */
    if (gup_flags & FOLL_LONGTERM) {
            if (WARN_ON_ONCE(locked))
                    return -EINVAL;
    }

Is that OK, or did you want to go further (possibly in a follow-up patchset, as I'm hoping to get this one in soon)?

That looks ok. Something to maybe push down into the core in a future cleanup, but not something that needs to be done now.

...

...

...

...

I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) && vma_is_fsdax()) check and that would also remove the need for __gup_longterm_locked.

Good idea, but there is still the call to check_and_migrate_cma_pages(), inside __gup_longterm_locked(). So it's a little more involved and we can't trivially delete __gup_longterm_locked() yet, right?

[ add Aneesh ]

Yes, you're right. I had overlooked that had snuck in there. That to me similarly needs to be pushed down into the core with its own FOLL flag, or it needs to be an explicit fixup that each caller does after get_user_pages. The fact that migration silently happens as a side effect of gup is too magical for my taste.

Yes. It's an intrusive side effect that is surprising, and not in a "happy surprise" way. :) . Fixing up the CMA pages by splitting that functionality into separate function calls sounds like an improvement worth exploring.

Right, future work.