Am 25.03.21 um 17:28 schrieb Felix Kuehling:
Am 2021-03-25 um 12:22 p.m. schrieb Christian König:
My idea is to change the amdgpu_vm_update_mapping interface to use
some
high-bit in the pages_addr array to indicate the page type. For the
default page type (0) nothing really changes for the callers. The
"flags" parameter needs to become a pointer to an array that gets
indexed by the high bits from the pages_addr array. For existing
callers
it's as easy as changing flags to &flags (array of size 1). For
HMM we
would pass a pointer to a real array.
Yeah, I've thought about stuff like that as well for a while.
Problem is that this won't work that easily. We assume at many places
that the flags doesn't change for the range in question.
I think some lower level functions assume that the flags stay the same
for physically contiguous ranges. But if you use the high-bits to
encode
the page type, the ranges won't be contiguous any more. So you can
change page flags for different contiguous ranges.
Yeah, but then you also get absolutely zero THP and fragment flags
support.
As long as you have a contiguous 2MB page with the same page type, I
think you can still get a THP mapping in the GPU page table. But if one
page in the middle of a 2MB page has a different page type, that will
break the THP mapping, as it should.
Yeah, but currently we detect that before we call down into the
functions to update the tables.
When you give a mixed list the chance for that is just completely gone.
Currently the detection of contiguous ranges is in amdgpu_vm_update_mapping:
if (num_entries > AMDGPU_GPU_PAGES_IN_CPU_PAGE) { uint64_t pfn =
cursor.start >> PAGE_SHIFT; uint64_t count; contiguous =
pages_addr[pfn + 1] == pages_addr[pfn] + PAGE_SIZE; tmp = num_entries
/ AMDGPU_GPU_PAGES_IN_CPU_PAGE; for (count = 2; count < tmp; ++count)
{ uint64_t idx = pfn + count; if (contiguous != (pages_addr[idx] ==
pages_addr[idx - 1] + PAGE_SIZE)) break; } num_entries = count *
AMDGPU_GPU_PAGES_IN_CPU_PAGE; }
If a page type changes from one page to the next, it will end a
contiguous range and call into the lower level (amdgpu_vm_update_ptes).
I don't think anything needs to change in amdgpu_vm_update_ptes, because
all pages contiguous passed to it use the same page type, so the same
flags. And the existing code in amdgpu_vm_update_mapping already does
the right thing. I honestly don't see the problem.
But then you could also just call amdgpu_vm_update_mapping() multiple
times feeding it from whatever data structure you use in the HMM code.
Using the page array sounds like an intermediate data structure to me
you only created to feed into amdgpu_vm_update_mapping().
Regards,
Christian.
Regards,
Felix
Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
