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)?
...
>>> 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.
thanks,
--
John Hubbard
NVIDIA
