Re: [PATCH] mm: Add error check after call to rmap_walk in the function page_referenced

[nick <xerofoify@gmail.com>]

On 2015-06-26 09:56 AM, Dominik Dingel wrote:
> On Thu, 25 Jun 2015 21:36:37 -0400
> Nicholas Krause <xerofoify@gmail.com> wrote:
> 
>> This adds a return check after the call to the function rmap_walk
>> in the function page_referenced as this function call can fail
>> and thus should signal callers of page_referenced if this happens
>> by returning the SWAP macro return value as returned by rmap_walk
>> here. In addition also check if have locked the page pointer as
>> passed to this particular and unlock it with unlock_page if this
>> page is locked before returning our SWAP marco return code from
>> rmap_walk.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  mm/rmap.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 171b687..e4df848 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -814,7 +814,9 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
>>   * @vm_flags: collect encountered vma->vm_flags who actually referenced the page
>>   *
>>   * Quick test_and_clear_referenced for all mappings to a page,
>> - * returns the number of ptes which referenced the page.
>> + * returns the number of ptes which referenced the page.On
>> + * error returns either zero or the error code returned from
>> + * the failed call to rmap_walk.
>>   */
>>  int page_referenced(struct page *page,
>>  		    int is_locked,
>> @@ -855,7 +857,13 @@ int page_referenced(struct page *page,
>>  		rwc.invalid_vma = invalid_page_referenced_vma;
>>  	}
>>
>> +
> 
> unnecessary empty line
> 
>>  	ret = rmap_walk(page, &rwc);
>> +	if (!ret) {
>> +		if (we_locked)
>> +			unlock_page(page);
>> +		return ret;
>> +	}
> 
> I don't see why the function should propagate the rmap_walk return value.
> rmap_walk will not set pra.referenced, so that both callers just skip.
> 
> What is the purpose of the given patch? Do you have any real case introducing such code,
> which is imho incomplete as all callers need to take care of the changed return value!
> 
There is only one caller that needs to be moved over if this case is put in. Further more 
do we care if executing rmap_walk fails as if it does this means we were unable to execute
the function page_referenced one on the rmap_walk_control structure rwc and this can be
a issue in my option, if not then we can just remove the ret variable and execute rmap_walk
without checking it's return value.
Cheers Nick 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>