This case is used to migrate pages from device memory, back to system
memory. Device coherent type memory is cache coherent from device and CPU
point of view.
Signed-off-by: Alex Sierra <[email protected]>
Acked-by: Felix Kuehling <[email protected]>
Reviewed-by: Alistair Poppple <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
---
include/linux/migrate.h | 1 +
mm/migrate_device.c | 12 +++++++++---
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 069a89e847f3..b84908debe5c 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -148,6 +148,7 @@ static inline unsigned long migrate_pfn(unsigned long pfn)
enum migrate_vma_direction {
MIGRATE_VMA_SELECT_SYSTEM = 1 << 0,
MIGRATE_VMA_SELECT_DEVICE_PRIVATE = 1 << 1,
+ MIGRATE_VMA_SELECT_DEVICE_COHERENT = 1 << 2,
};
struct migrate_vma {
diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index a4847ad65da3..18bc6483f63a 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -148,15 +148,21 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
if (is_writable_device_private_entry(entry))
mpfn |= MIGRATE_PFN_WRITE;
} else {
- if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
- goto next;
pfn = pte_pfn(pte);
- if (is_zero_pfn(pfn)) {
+ if (is_zero_pfn(pfn) &&
+ (migrate->flags & MIGRATE_VMA_SELECT_SYSTEM)) {
mpfn = MIGRATE_PFN_MIGRATE;
migrate->cpages++;
goto next;
}
page = vm_normal_page(migrate->vma, addr, pte);
+ if (page && !is_zone_device_page(page) &&
+ !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
+ goto next;
+ else if (page && is_device_coherent_page(page) &&
+ (!(migrate->flags & MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
+ page->pgmap->owner != migrate->pgmap_owner))
+ goto next;
mpfn = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
mpfn |= pte_write(pte) ? MIGRATE_PFN_WRITE : 0;
}
--
2.32.0
On 29.06.22 05:54, Alex Sierra wrote:
> This case is used to migrate pages from device memory, back to system
> memory. Device coherent type memory is cache coherent from device and CPU
> point of view.
>
> Signed-off-by: Alex Sierra <[email protected]>
> Acked-by: Felix Kuehling <[email protected]>
> Reviewed-by: Alistair Poppple <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>
I'm not too familiar with this code, please excuse my naive questions:
> @@ -148,15 +148,21 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> if (is_writable_device_private_entry(entry))
> mpfn |= MIGRATE_PFN_WRITE;
> } else {
> - if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> - goto next;
Why not exclude MIGRATE_VMA_SELECT_DEVICE_PRIVATE here? IIRC that would
have happened before this change.
> pfn = pte_pfn(pte);
> - if (is_zero_pfn(pfn)) {
> + if (is_zero_pfn(pfn) &&
> + (migrate->flags & MIGRATE_VMA_SELECT_SYSTEM)) {
> mpfn = MIGRATE_PFN_MIGRATE;
> migrate->cpages++;
> goto next;
> }
> page = vm_normal_page(migrate->vma, addr, pte);
> + if (page && !is_zone_device_page(page) &&
I'm wondering if that check logically belongs into patch #2.
> + !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> + goto next;
> + else if (page && is_device_coherent_page(page) &&
> + (!(migrate->flags & MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> + page->pgmap->owner != migrate->pgmap_owner))
In general LGTM
--
Thanks,
David / dhildenb
David Hildenbrand <[email protected]> writes:
> On 29.06.22 05:54, Alex Sierra wrote:
>> This case is used to migrate pages from device memory, back to system
>> memory. Device coherent type memory is cache coherent from device and CPU
>> point of view.
>>
>> Signed-off-by: Alex Sierra <[email protected]>
>> Acked-by: Felix Kuehling <[email protected]>
>> Reviewed-by: Alistair Poppple <[email protected]>
>> Signed-off-by: Christoph Hellwig <[email protected]>
>
>
> I'm not too familiar with this code, please excuse my naive questions:
>
>> @@ -148,15 +148,21 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>> if (is_writable_device_private_entry(entry))
>> mpfn |= MIGRATE_PFN_WRITE;
>> } else {
>> - if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
>> - goto next;
>
> Why not exclude MIGRATE_VMA_SELECT_DEVICE_PRIVATE here? IIRC that would
> have happened before this change.
I might be missing something as I don't quite follow - this path is for
normal system pages so we only want to skip selecting them if
MIGRATE_VMA_SELECT_SYSTEM or MIGRATE_VMA_SELECT_DEVICE_COHERENT aren't
set.
Note that MIGRATE_VMA_SELECT_DEVICE_PRIVATE doesn't apply here because
we already know it's not a device private page by virtue of
pte_present(pte) == True.
>> pfn = pte_pfn(pte);
>> - if (is_zero_pfn(pfn)) {
>> + if (is_zero_pfn(pfn) &&
>> + (migrate->flags & MIGRATE_VMA_SELECT_SYSTEM)) {
>> mpfn = MIGRATE_PFN_MIGRATE;
>> migrate->cpages++;
>> goto next;
>> }
>> page = vm_normal_page(migrate->vma, addr, pte);
>> + if (page && !is_zone_device_page(page) &&
>
> I'm wondering if that check logically belongs into patch #2.
I don't think so as it would break functionality until the below
conditionals are added - we explicitly don't want to skip
is_zone_device_page(page) == False here because that is the pages we are
trying to select.
You could add in this:
>> + !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
But then in patch 2 we know this can never be true because we've already
checked for !MIGRATE_VMA_SELECT_SYSTEM there.
>> + goto next;
>> + else if (page && is_device_coherent_page(page) &&
>> + (!(migrate->flags & MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
>> + page->pgmap->owner != migrate->pgmap_owner))
>
>
> In general LGTM
On 30.06.22 13:44, Alistair Popple wrote:
>
> David Hildenbrand <[email protected]> writes:
>
>> On 29.06.22 05:54, Alex Sierra wrote:
>>> This case is used to migrate pages from device memory, back to system
>>> memory. Device coherent type memory is cache coherent from device and CPU
>>> point of view.
>>>
>>> Signed-off-by: Alex Sierra <[email protected]>
>>> Acked-by: Felix Kuehling <[email protected]>
>>> Reviewed-by: Alistair Poppple <[email protected]>
>>> Signed-off-by: Christoph Hellwig <[email protected]>
>>
>>
>> I'm not too familiar with this code, please excuse my naive questions:
>>
>>> @@ -148,15 +148,21 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>>> if (is_writable_device_private_entry(entry))
>>> mpfn |= MIGRATE_PFN_WRITE;
>>> } else {
>>> - if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
>>> - goto next;
>>
>> Why not exclude MIGRATE_VMA_SELECT_DEVICE_PRIVATE here? IIRC that would
>> have happened before this change.
>
> I might be missing something as I don't quite follow - this path is for
> normal system pages so we only want to skip selecting them if
> MIGRATE_VMA_SELECT_SYSTEM or MIGRATE_VMA_SELECT_DEVICE_COHERENT aren't
> set.
>
> Note that MIGRATE_VMA_SELECT_DEVICE_PRIVATE doesn't apply here because
> we already know it's not a device private page by virtue of
> pte_present(pte) == True.
Ah, stupid me, pte_present(pte) is the key.
>
>>> pfn = pte_pfn(pte);
>>> - if (is_zero_pfn(pfn)) {
>>> + if (is_zero_pfn(pfn) &&
>>> + (migrate->flags & MIGRATE_VMA_SELECT_SYSTEM)) {
>>> mpfn = MIGRATE_PFN_MIGRATE;
>>> migrate->cpages++;
>>> goto next;
>>> }
>>> page = vm_normal_page(migrate->vma, addr, pte);
>>> + if (page && !is_zone_device_page(page) &&
>>
>> I'm wondering if that check logically belongs into patch #2.
>
> I don't think so as it would break functionality until the below
> conditionals are added - we explicitly don't want to skip
> is_zone_device_page(page) == False here because that is the pages we are
> trying to select.
>
> You could add in this:
>
>>> + !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
>
> But then in patch 2 we know this can never be true because we've already
> checked for !MIGRATE_VMA_SELECT_SYSTEM there.
Ah, okay, thanks
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb