Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[John Hubbard <jhubbard@nvidia.com>]

On 6/4/19 9:55 AM, Ira Weiny wrote:
> On Tue, Jun 04, 2019 at 12:08:08AM -0700, Christoph Hellwig wrote:
>> On Mon, Jun 03, 2019 at 04:56:10PM -0700, Ira Weiny wrote:
>>> On Mon, Jun 03, 2019 at 09:42:06AM -0700, Christoph Hellwig wrote:
>>>>> +#if defined(CONFIG_CMA)
>>>>
>>>> You can just use #ifdef here.
>>>>
>>>>> +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
>>>>> +	struct page **pages)
>>>>
>>>> Please use two instead of one tab to indent the continuing line of
>>>> a function declaration.
>>>>
>>>>> +{
>>>>> +	if (unlikely(gup_flags & FOLL_LONGTERM)) {
>>>>
>>>> IMHO it would be a little nicer if we could move this into the caller.
>>>
>>> FWIW we already had this discussion and thought it better to put this here.
>>>
>>> https://lkml.org/lkml/2019/5/30/1565
>>
>> I don't see any discussion like this.  FYI, this is what I mean,
>> code might be easier than words:
> 
> Indeed that is more clear.  My apologies.
> 
> Ira
> 
>>
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index ddde097cf9e4..62d770b18e2c 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2197,6 +2197,27 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_CMA
>> +static int reject_cma_pages(struct page **pages, int nr_pinned)
>> +{
>> +	int i = 0;
>> +
>> +	for (i = 0; i < nr_pinned; i++)
>> +		if (is_migrate_cma_page(pages[i])) {
>> +			put_user_pages(pages + i, nr_pinned - i);
>> +			return i;
>> +		}
>> +	}
>> +
>> +	return nr_pinned;
>> +}
>> +#else
>> +static inline int reject_cma_pages(struct page **pages, int nr_pinned)
>> +{
>> +	return nr_pinned;
>> +}
>> +#endif /* CONFIG_CMA */
>> +
>>  /**
>>   * get_user_pages_fast() - pin user pages in memory
>>   * @start:	starting user address
>> @@ -2237,6 +2258,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>>  		ret = nr;
>>  	}
>>  
>> +	if (nr && unlikely(gup_flags & FOLL_LONGTERM))
>> +		nr = reject_cma_pages(pages, nr);
>> +

Yes, now I see what you meant, and agree that that is cleaner.

thanks,
-- 
John Hubbard
NVIDIA

>>  	if (nr < nr_pages) {
>>  		/* Try to get the remaining pages with get_user_pages */
>>  		start += nr << PAGE_SHIFT;
>>