linux-ext4 - Re: [PATCH v2 2/3] mm/gup.c: Migrate device coherent pages when pinning instead of failing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0c7b50d3-9bca-9462-05ca-b4ef846cc49a@nvidia.com>
Date:   Fri, 11 Feb 2022 19:04:18 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Alistair Popple <apopple@...dia.com>, akpm@...ux-foundation.org,
        linux-mm@...ck.org
Cc:     Felix.Kuehling@....com, rcampbell@...dia.com,
        linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
        amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        hch@....de, jgg@...dia.com, jglisse@...hat.com,
        willy@...radead.org, alex.sierra@....com
Subject: Re: [PATCH v2 2/3] mm/gup.c: Migrate device coherent pages when
 pinning instead of failing

On 2/11/22 18:51, Alistair Popple wrote:
...
>>> @@ -1888,15 +1942,40 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>>>    			continue;
>>>    		prev_head = head;
>>>    		/*
>>> -		 * If we get a movable page, since we are going to be pinning
>>> -		 * these entries, try to move them out if possible.
>>> +		 * Device coherent pages are managed by a driver and should not
>>> +		 * be pinned indefinitely as it prevents the driver moving the
>>> +		 * page. So when trying to pin with FOLL_LONGTERM instead try
>>> +		 * migrating page out of device memory.
>>>    		 */
>>>    		if (is_dev_private_or_coherent_page(head)) {
>>> +			/*
>>> +			 * device private pages will get faulted in during gup
>>> +			 * so it shouldn't be possible to see one here.
>>> +			 */
>>>    			WARN_ON_ONCE(is_device_private_page(head));
>>> -			ret = -EFAULT;
>>> -			goto unpin_pages;
>>> +			WARN_ON_ONCE(PageCompound(head));
>>> +
>>> +			/*
>>> +			 * migration will fail if the page is pinned, so convert
>>> +			 * the pin on the source page to a normal reference.
>>> +			 */
>>> +			if (gup_flags & FOLL_PIN) {
>>> +				get_page(head);
>>> +				unpin_user_page(head);
>>
>> OK...but now gup_flags can no longer be used as a guide for how to
>> release these pages, right? In other words, up until this point,
>> FOLL_PIN meant "call unpin_user_page() in order to release". However,
>> now this page must be released via put_page().
> 
> This is the source page (head). We are unpinning it because we can't migrate a
> pinned page, however we still need a reference on it for migrate_vma hence the
> get_page followed by unpin. In the non-FOLL_PIN case we already have a
> reference from gup.
> 
>> See below...
>>
>>> +			}
>>> +
>>> +			pages[i] = migrate_device_page(head, gup_flags);
> 
> migrate_device_page() will return a new page that has been correctly pinned
> with gup_flags by try_grab_page(). Therefore this page can still be released
> with unpin_user_page() or put_page() as appropriate for the given gup_flags.
> 
> The reference we had on the source page (head) always gets dropped in
> migrate_vma_finalize().

OK. Good.

The above would be good to have in a comment, right around here, imho.
Because we have this marvelous mix of references for migration (get_page())
and other, and it's a bit hard to see that it's all correct without a
hint or two.

...
> 
> Which unless I've missed something is still the correct thing to do.
> 
>> This reminds me: out of the many things to monitor, the FOLL_PIN counts
>> in /proc/vmstat are especially helpful, whenever making changes to code
>> that deals with this:
>>
>> 	nr_foll_pin_acquired
>> 	nr_foll_pin_released
>>
>> ...and those should normally be equal to each other when "at rest".
>>

I hope this is/was run, just to be sure?


thanks,
-- 
John Hubbard
NVIDIA