* [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-09-30 22:18 ` Alex Goins 0 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-09-30 22:18 UTC (permalink / raw) To: Christian König Cc: Alex Goins, amd-gfx, dri-devel, linux-mm, John Hubbard, Zi Yan Hi Christian, I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would you mind giving some input on this? I noticed that your changes implementing transparent huge page support in TTM are allocating them as non-compound. I understand that using multiorder non-compound pages is common in device drivers, but I think this can cause a problem when these pages are exported to other drivers. It's possible for other drivers to access the DMA-BUF's pages via gem_prime_import_sg_table(), but without context from TTM, it's impossible for the importing driver to make sense of them; they simply appear as individual pages, with only the first page having a non-zero refcount. Making TTM's THP allocations compound puts them more in line with the standard definition of a THP, and allows DMA-BUF-importing drivers to make sense of the pages within. I would like to propose making these allocations compound, but based on patch history, it looks like the decision to make them non-compound was intentional, as there were difficulties figuring out how to map them into CPU page tables. I did some cursory testing with compound THPs, and nothing seems obviously broken. I was also able to map compound THP DMA-BUFs into userspace without issue, and access their contents. Are you aware of any other potential consequences? Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should probably also be reverted if this is applied. Thanks, Alex Alex Goins (1): drm-ttm: Allocate compound transparent huge pages drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-09-30 22:18 ` Alex Goins 0 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-09-30 22:18 UTC (permalink / raw) To: Christian König Cc: Alex Goins, John Hubbard, dri-devel, linux-mm, amd-gfx, Zi Yan Hi Christian, I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would you mind giving some input on this? I noticed that your changes implementing transparent huge page support in TTM are allocating them as non-compound. I understand that using multiorder non-compound pages is common in device drivers, but I think this can cause a problem when these pages are exported to other drivers. It's possible for other drivers to access the DMA-BUF's pages via gem_prime_import_sg_table(), but without context from TTM, it's impossible for the importing driver to make sense of them; they simply appear as individual pages, with only the first page having a non-zero refcount. Making TTM's THP allocations compound puts them more in line with the standard definition of a THP, and allows DMA-BUF-importing drivers to make sense of the pages within. I would like to propose making these allocations compound, but based on patch history, it looks like the decision to make them non-compound was intentional, as there were difficulties figuring out how to map them into CPU page tables. I did some cursory testing with compound THPs, and nothing seems obviously broken. I was also able to map compound THP DMA-BUFs into userspace without issue, and access their contents. Are you aware of any other potential consequences? Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should probably also be reverted if this is applied. Thanks, Alex Alex Goins (1): drm-ttm: Allocate compound transparent huge pages drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-09-30 22:18 ` Alex Goins 0 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-09-30 22:18 UTC (permalink / raw) To: Christian König; +Cc: John Hubbard, dri-devel, linux-mm, amd-gfx, Zi Yan Hi Christian, I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would you mind giving some input on this? I noticed that your changes implementing transparent huge page support in TTM are allocating them as non-compound. I understand that using multiorder non-compound pages is common in device drivers, but I think this can cause a problem when these pages are exported to other drivers. It's possible for other drivers to access the DMA-BUF's pages via gem_prime_import_sg_table(), but without context from TTM, it's impossible for the importing driver to make sense of them; they simply appear as individual pages, with only the first page having a non-zero refcount. Making TTM's THP allocations compound puts them more in line with the standard definition of a THP, and allows DMA-BUF-importing drivers to make sense of the pages within. I would like to propose making these allocations compound, but based on patch history, it looks like the decision to make them non-compound was intentional, as there were difficulties figuring out how to map them into CPU page tables. I did some cursory testing with compound THPs, and nothing seems obviously broken. I was also able to map compound THP DMA-BUFs into userspace without issue, and access their contents. Are you aware of any other potential consequences? Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should probably also be reverted if this is applied. Thanks, Alex Alex Goins (1): drm-ttm: Allocate compound transparent huge pages drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) -- 2.25.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH RFC 1/1] drm-ttm: Allocate transparent huge pages without clearing __GFP_COMP 2020-09-30 22:18 ` Alex Goins (?) @ 2020-09-30 22:18 ` Alex Goins -1 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-09-30 22:18 UTC (permalink / raw) To: Christian König Cc: Alex Goins, amd-gfx, dri-devel, linux-mm, John Hubbard, Zi Yan TTM currently supports allocating pages with GFP_TRANSHUGE_LIGHT, but with the __GFP_COMP flag cleared. Instead of being normal transparent huge pages, these are multiorder non-compound pages that have the same order as THPs. This interferes with drivers that import DMA-BUFs / SGTs backed by pages from TTM, as they don't have the TTM-specific context to know how the pages were allocated. Change the TTM allocator so that it no longer clears the __GFP_COMP flag when allocating THPs. Signed-off-by: Alex Goins <agoins@nvidia.com> --- drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c index 5c8883d7f74a..e72789184398 100644 --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c @@ -920,7 +920,6 @@ static int ttm_get_pages(struct page **pages, unsigned npages, int flags, huge_flags |= GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM; huge_flags &= ~__GFP_MOVABLE; - huge_flags &= ~__GFP_COMP; p = alloc_pages(huge_flags, HPAGE_PMD_ORDER); if (!p) break; @@ -1057,13 +1056,13 @@ int ttm_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages) ttm_page_pool_init_locked(&_manager->wc_pool_huge, (GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM) & - ~(__GFP_MOVABLE | __GFP_COMP), + ~(__GFP_MOVABLE), "wc huge", order); ttm_page_pool_init_locked(&_manager->uc_pool_huge, (GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM) & - ~(__GFP_MOVABLE | __GFP_COMP) + ~(__GFP_MOVABLE) , "uc huge", order); _manager->options.max_size = max_pages; -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH RFC 1/1] drm-ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-09-30 22:18 ` Alex Goins 0 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-09-30 22:18 UTC (permalink / raw) To: Christian König Cc: Alex Goins, John Hubbard, dri-devel, linux-mm, amd-gfx, Zi Yan TTM currently supports allocating pages with GFP_TRANSHUGE_LIGHT, but with the __GFP_COMP flag cleared. Instead of being normal transparent huge pages, these are multiorder non-compound pages that have the same order as THPs. This interferes with drivers that import DMA-BUFs / SGTs backed by pages from TTM, as they don't have the TTM-specific context to know how the pages were allocated. Change the TTM allocator so that it no longer clears the __GFP_COMP flag when allocating THPs. Signed-off-by: Alex Goins <agoins@nvidia.com> --- drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c index 5c8883d7f74a..e72789184398 100644 --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c @@ -920,7 +920,6 @@ static int ttm_get_pages(struct page **pages, unsigned npages, int flags, huge_flags |= GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM; huge_flags &= ~__GFP_MOVABLE; - huge_flags &= ~__GFP_COMP; p = alloc_pages(huge_flags, HPAGE_PMD_ORDER); if (!p) break; @@ -1057,13 +1056,13 @@ int ttm_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages) ttm_page_pool_init_locked(&_manager->wc_pool_huge, (GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM) & - ~(__GFP_MOVABLE | __GFP_COMP), + ~(__GFP_MOVABLE), "wc huge", order); ttm_page_pool_init_locked(&_manager->uc_pool_huge, (GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM) & - ~(__GFP_MOVABLE | __GFP_COMP) + ~(__GFP_MOVABLE) , "uc huge", order); _manager->options.max_size = max_pages; -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH RFC 1/1] drm-ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-09-30 22:18 ` Alex Goins 0 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-09-30 22:18 UTC (permalink / raw) To: Christian König; +Cc: John Hubbard, dri-devel, linux-mm, amd-gfx, Zi Yan TTM currently supports allocating pages with GFP_TRANSHUGE_LIGHT, but with the __GFP_COMP flag cleared. Instead of being normal transparent huge pages, these are multiorder non-compound pages that have the same order as THPs. This interferes with drivers that import DMA-BUFs / SGTs backed by pages from TTM, as they don't have the TTM-specific context to know how the pages were allocated. Change the TTM allocator so that it no longer clears the __GFP_COMP flag when allocating THPs. Signed-off-by: Alex Goins <agoins@nvidia.com> --- drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c index 5c8883d7f74a..e72789184398 100644 --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c @@ -920,7 +920,6 @@ static int ttm_get_pages(struct page **pages, unsigned npages, int flags, huge_flags |= GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM; huge_flags &= ~__GFP_MOVABLE; - huge_flags &= ~__GFP_COMP; p = alloc_pages(huge_flags, HPAGE_PMD_ORDER); if (!p) break; @@ -1057,13 +1056,13 @@ int ttm_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages) ttm_page_pool_init_locked(&_manager->wc_pool_huge, (GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM) & - ~(__GFP_MOVABLE | __GFP_COMP), + ~(__GFP_MOVABLE), "wc huge", order); ttm_page_pool_init_locked(&_manager->uc_pool_huge, (GFP_TRANSHUGE_LIGHT | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM) & - ~(__GFP_MOVABLE | __GFP_COMP) + ~(__GFP_MOVABLE) , "uc huge", order); _manager->options.max_size = max_pages; -- 2.25.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP 2020-09-30 22:18 ` Alex Goins (?) @ 2020-10-01 7:19 ` Christian König -1 siblings, 0 replies; 18+ messages in thread From: Christian König @ 2020-10-01 7:19 UTC (permalink / raw) To: Alex Goins; +Cc: amd-gfx, dri-devel, linux-mm, John Hubbard, Zi Yan Hi Alex, first of all accessing the underlying page of an exported DMA-buf is illegal! So I'm not 100% sure what you're intentions are here, please explain further. Then the reason for TTM not using compound pages is that we can't guarantee that they are mapped as a whole to userspace. The result is that the kernel sometimes tried to de-compound them which created a bunch of problems. So yes this is completely intentional. Regards, Christian. Am 01.10.20 um 00:18 schrieb Alex Goins: > Hi Christian, > > I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > you mind giving some input on this? > > I noticed that your changes implementing transparent huge page support > in TTM are allocating them as non-compound. I understand that using > multiorder non-compound pages is common in device drivers, but I think > this can cause a problem when these pages are exported to other drivers. > > It's possible for other drivers to access the DMA-BUF's pages via > gem_prime_import_sg_table(), but without context from TTM, it's > impossible for the importing driver to make sense of them; they simply > appear as individual pages, with only the first page having a non-zero > refcount. Making TTM's THP allocations compound puts them more in line > with the standard definition of a THP, and allows DMA-BUF-importing > drivers to make sense of the pages within. > > I would like to propose making these allocations compound, but based on > patch history, it looks like the decision to make them non-compound was > intentional, as there were difficulties figuring out how to map them > into CPU page tables. I did some cursory testing with compound THPs, and > nothing seems obviously broken. I was also able to map compound THP > DMA-BUFs into userspace without issue, and access their contents. Are > you aware of any other potential consequences? > > Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > probably also be reverted if this is applied. > > Thanks, > Alex > > Alex Goins (1): > drm-ttm: Allocate compound transparent huge pages > > drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-01 7:19 ` Christian König 0 siblings, 0 replies; 18+ messages in thread From: Christian König @ 2020-10-01 7:19 UTC (permalink / raw) To: Alex Goins; +Cc: linux-mm, Zi Yan, dri-devel, amd-gfx, John Hubbard Hi Alex, first of all accessing the underlying page of an exported DMA-buf is illegal! So I'm not 100% sure what you're intentions are here, please explain further. Then the reason for TTM not using compound pages is that we can't guarantee that they are mapped as a whole to userspace. The result is that the kernel sometimes tried to de-compound them which created a bunch of problems. So yes this is completely intentional. Regards, Christian. Am 01.10.20 um 00:18 schrieb Alex Goins: > Hi Christian, > > I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > you mind giving some input on this? > > I noticed that your changes implementing transparent huge page support > in TTM are allocating them as non-compound. I understand that using > multiorder non-compound pages is common in device drivers, but I think > this can cause a problem when these pages are exported to other drivers. > > It's possible for other drivers to access the DMA-BUF's pages via > gem_prime_import_sg_table(), but without context from TTM, it's > impossible for the importing driver to make sense of them; they simply > appear as individual pages, with only the first page having a non-zero > refcount. Making TTM's THP allocations compound puts them more in line > with the standard definition of a THP, and allows DMA-BUF-importing > drivers to make sense of the pages within. > > I would like to propose making these allocations compound, but based on > patch history, it looks like the decision to make them non-compound was > intentional, as there were difficulties figuring out how to map them > into CPU page tables. I did some cursory testing with compound THPs, and > nothing seems obviously broken. I was also able to map compound THP > DMA-BUFs into userspace without issue, and access their contents. Are > you aware of any other potential consequences? > > Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > probably also be reverted if this is applied. > > Thanks, > Alex > > Alex Goins (1): > drm-ttm: Allocate compound transparent huge pages > > drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-01 7:19 ` Christian König 0 siblings, 0 replies; 18+ messages in thread From: Christian König @ 2020-10-01 7:19 UTC (permalink / raw) To: Alex Goins; +Cc: linux-mm, Zi Yan, dri-devel, amd-gfx, John Hubbard Hi Alex, first of all accessing the underlying page of an exported DMA-buf is illegal! So I'm not 100% sure what you're intentions are here, please explain further. Then the reason for TTM not using compound pages is that we can't guarantee that they are mapped as a whole to userspace. The result is that the kernel sometimes tried to de-compound them which created a bunch of problems. So yes this is completely intentional. Regards, Christian. Am 01.10.20 um 00:18 schrieb Alex Goins: > Hi Christian, > > I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > you mind giving some input on this? > > I noticed that your changes implementing transparent huge page support > in TTM are allocating them as non-compound. I understand that using > multiorder non-compound pages is common in device drivers, but I think > this can cause a problem when these pages are exported to other drivers. > > It's possible for other drivers to access the DMA-BUF's pages via > gem_prime_import_sg_table(), but without context from TTM, it's > impossible for the importing driver to make sense of them; they simply > appear as individual pages, with only the first page having a non-zero > refcount. Making TTM's THP allocations compound puts them more in line > with the standard definition of a THP, and allows DMA-BUF-importing > drivers to make sense of the pages within. > > I would like to propose making these allocations compound, but based on > patch history, it looks like the decision to make them non-compound was > intentional, as there were difficulties figuring out how to map them > into CPU page tables. I did some cursory testing with compound THPs, and > nothing seems obviously broken. I was also able to map compound THP > DMA-BUFs into userspace without issue, and access their contents. Are > you aware of any other potential consequences? > > Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > probably also be reverted if this is applied. > > Thanks, > Alex > > Alex Goins (1): > drm-ttm: Allocate compound transparent huge pages > > drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP 2020-10-01 7:19 ` Christian König (?) @ 2020-10-01 18:45 ` Alex Goins -1 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-10-01 18:45 UTC (permalink / raw) To: Christian König; +Cc: amd-gfx, dri-devel, linux-mm, John Hubbard, Zi Yan [-- Attachment #1: Type: text/plain, Size: 4118 bytes --] Hi Christian, On Thu, 1 Oct 2020, Christian König wrote: > Hi Alex, > > first of all accessing the underlying page of an exported DMA-buf is > illegal! So I'm not 100% sure what you're intentions are here, please > explain further. We have some mapping requirements that I was hoping I could address by mapping these pages manually. Are you sure that it's illegal to access the underlying pages of an exported DMA-BUF? There appears to be quite a few usages of this already. See the usage of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when importing a DMA-BUF from another driver, and the listed drivers then extract the pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages can then be mapped and faulted in. See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After importing the pages from the SGT, vgem can fault them in, taking a refcount with get_page() first. get_page() throws a BUG if the refcount is zero, which it will hit on each of the 'tail' pages from TTM THP allocations. All of this currently works fine with TTM DMA-BUFs when the kernel is built with !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates pages. > Then the reason for TTM not using compound pages is that we can't > guarantee that they are mapped as a whole to userspace. > > The result is that the kernel sometimes tried to de-compound them which > created a bunch of problems. > > So yes this is completely intentional. Understood, I figured something like that was the case, so I wanted to get your input first. Do you know what the problems were, exactly? Practical issues aside, it seems strange to call something a transparent huge page if it's non-compound. Besides making these pages compound, would it be reasonable to split them before sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? That's where it's supposed to make sure that the shared DMA-BUF is accessible by the target device. Thanks, Alex > Regards, > Christian. > > Am 01.10.20 um 00:18 schrieb Alex Goins: > > Hi Christian, > > > > I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > > you mind giving some input on this? > > > > I noticed that your changes implementing transparent huge page support > > in TTM are allocating them as non-compound. I understand that using > > multiorder non-compound pages is common in device drivers, but I think > > this can cause a problem when these pages are exported to other drivers. > > > > It's possible for other drivers to access the DMA-BUF's pages via > > gem_prime_import_sg_table(), but without context from TTM, it's > > impossible for the importing driver to make sense of them; they simply > > appear as individual pages, with only the first page having a non-zero > > refcount. Making TTM's THP allocations compound puts them more in line > > with the standard definition of a THP, and allows DMA-BUF-importing > > drivers to make sense of the pages within. > > > > I would like to propose making these allocations compound, but based on > > patch history, it looks like the decision to make them non-compound was > > intentional, as there were difficulties figuring out how to map them > > into CPU page tables. I did some cursory testing with compound THPs, and > > nothing seems obviously broken. I was also able to map compound THP > > DMA-BUFs into userspace without issue, and access their contents. Are > > you aware of any other potential consequences? > > > > Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > > probably also be reverted if this is applied. > > > > Thanks, > > Alex > > > > Alex Goins (1): > > drm-ttm: Allocate compound transparent huge pages > > > > drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-01 18:45 ` Alex Goins 0 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-10-01 18:45 UTC (permalink / raw) To: Christian König; +Cc: linux-mm, Zi Yan, dri-devel, amd-gfx, John Hubbard [-- Attachment #1: Type: text/plain, Size: 4023 bytes --] Hi Christian, On Thu, 1 Oct 2020, Christian König wrote: > Hi Alex, > > first of all accessing the underlying page of an exported DMA-buf is > illegal! So I'm not 100% sure what you're intentions are here, please > explain further. We have some mapping requirements that I was hoping I could address by mapping these pages manually. Are you sure that it's illegal to access the underlying pages of an exported DMA-BUF? There appears to be quite a few usages of this already. See the usage of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when importing a DMA-BUF from another driver, and the listed drivers then extract the pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages can then be mapped and faulted in. See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After importing the pages from the SGT, vgem can fault them in, taking a refcount with get_page() first. get_page() throws a BUG if the refcount is zero, which it will hit on each of the 'tail' pages from TTM THP allocations. All of this currently works fine with TTM DMA-BUFs when the kernel is built with !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates pages. > Then the reason for TTM not using compound pages is that we can't > guarantee that they are mapped as a whole to userspace. > > The result is that the kernel sometimes tried to de-compound them which > created a bunch of problems. > > So yes this is completely intentional. Understood, I figured something like that was the case, so I wanted to get your input first. Do you know what the problems were, exactly? Practical issues aside, it seems strange to call something a transparent huge page if it's non-compound. Besides making these pages compound, would it be reasonable to split them before sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? That's where it's supposed to make sure that the shared DMA-BUF is accessible by the target device. Thanks, Alex > Regards, > Christian. > > Am 01.10.20 um 00:18 schrieb Alex Goins: > > Hi Christian, > > > > I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > > you mind giving some input on this? > > > > I noticed that your changes implementing transparent huge page support > > in TTM are allocating them as non-compound. I understand that using > > multiorder non-compound pages is common in device drivers, but I think > > this can cause a problem when these pages are exported to other drivers. > > > > It's possible for other drivers to access the DMA-BUF's pages via > > gem_prime_import_sg_table(), but without context from TTM, it's > > impossible for the importing driver to make sense of them; they simply > > appear as individual pages, with only the first page having a non-zero > > refcount. Making TTM's THP allocations compound puts them more in line > > with the standard definition of a THP, and allows DMA-BUF-importing > > drivers to make sense of the pages within. > > > > I would like to propose making these allocations compound, but based on > > patch history, it looks like the decision to make them non-compound was > > intentional, as there were difficulties figuring out how to map them > > into CPU page tables. I did some cursory testing with compound THPs, and > > nothing seems obviously broken. I was also able to map compound THP > > DMA-BUFs into userspace without issue, and access their contents. Are > > you aware of any other potential consequences? > > > > Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > > probably also be reverted if this is applied. > > > > Thanks, > > Alex > > > > Alex Goins (1): > > drm-ttm: Allocate compound transparent huge pages > > > > drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-01 18:45 ` Alex Goins 0 siblings, 0 replies; 18+ messages in thread From: Alex Goins @ 2020-10-01 18:45 UTC (permalink / raw) To: Christian König; +Cc: linux-mm, Zi Yan, dri-devel, amd-gfx, John Hubbard [-- Attachment #1: Type: text/plain, Size: 4023 bytes --] Hi Christian, On Thu, 1 Oct 2020, Christian König wrote: > Hi Alex, > > first of all accessing the underlying page of an exported DMA-buf is > illegal! So I'm not 100% sure what you're intentions are here, please > explain further. We have some mapping requirements that I was hoping I could address by mapping these pages manually. Are you sure that it's illegal to access the underlying pages of an exported DMA-BUF? There appears to be quite a few usages of this already. See the usage of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when importing a DMA-BUF from another driver, and the listed drivers then extract the pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages can then be mapped and faulted in. See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After importing the pages from the SGT, vgem can fault them in, taking a refcount with get_page() first. get_page() throws a BUG if the refcount is zero, which it will hit on each of the 'tail' pages from TTM THP allocations. All of this currently works fine with TTM DMA-BUFs when the kernel is built with !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates pages. > Then the reason for TTM not using compound pages is that we can't > guarantee that they are mapped as a whole to userspace. > > The result is that the kernel sometimes tried to de-compound them which > created a bunch of problems. > > So yes this is completely intentional. Understood, I figured something like that was the case, so I wanted to get your input first. Do you know what the problems were, exactly? Practical issues aside, it seems strange to call something a transparent huge page if it's non-compound. Besides making these pages compound, would it be reasonable to split them before sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? That's where it's supposed to make sure that the shared DMA-BUF is accessible by the target device. Thanks, Alex > Regards, > Christian. > > Am 01.10.20 um 00:18 schrieb Alex Goins: > > Hi Christian, > > > > I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > > you mind giving some input on this? > > > > I noticed that your changes implementing transparent huge page support > > in TTM are allocating them as non-compound. I understand that using > > multiorder non-compound pages is common in device drivers, but I think > > this can cause a problem when these pages are exported to other drivers. > > > > It's possible for other drivers to access the DMA-BUF's pages via > > gem_prime_import_sg_table(), but without context from TTM, it's > > impossible for the importing driver to make sense of them; they simply > > appear as individual pages, with only the first page having a non-zero > > refcount. Making TTM's THP allocations compound puts them more in line > > with the standard definition of a THP, and allows DMA-BUF-importing > > drivers to make sense of the pages within. > > > > I would like to propose making these allocations compound, but based on > > patch history, it looks like the decision to make them non-compound was > > intentional, as there were difficulties figuring out how to map them > > into CPU page tables. I did some cursory testing with compound THPs, and > > nothing seems obviously broken. I was also able to map compound THP > > DMA-BUFs into userspace without issue, and access their contents. Are > > you aware of any other potential consequences? > > > > Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > > probably also be reverted if this is applied. > > > > Thanks, > > Alex > > > > Alex Goins (1): > > drm-ttm: Allocate compound transparent huge pages > > > > drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP 2020-10-01 18:45 ` Alex Goins (?) @ 2020-10-02 6:41 ` Christian König -1 siblings, 0 replies; 18+ messages in thread From: Christian König @ 2020-10-02 6:41 UTC (permalink / raw) To: Alex Goins, Daniel Vetter Cc: amd-gfx, dri-devel, linux-mm, John Hubbard, Zi Yan Hi Alex, adding Daniel as well. Am 01.10.20 um 20:45 schrieb Alex Goins: > Hi Christian, > > On Thu, 1 Oct 2020, Christian König wrote: > >> Hi Alex, >> >> first of all accessing the underlying page of an exported DMA-buf is >> illegal! So I'm not 100% sure what you're intentions are here, please >> explain further. > We have some mapping requirements that I was hoping I could address by mapping > these pages manually. > > Are you sure that it's illegal to access the underlying pages of an exported > DMA-BUF? yes, I'm 100% sure of that. This was discussed multiple times now on the mailing list. > There appears to be quite a few usages of this already. See the usage > of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. > drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when > importing a DMA-BUF from another driver, and the listed drivers then extract the > pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages > can then be mapped and faulted in. No, exactly that doesn't work correctly. You are corrupting internal state in struct page while doing so and risk that userspace is accessing freed up memory. We really need to find a way to fix the few drivers already doing this. > See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After > importing the pages from the SGT, vgem can fault them in, taking a refcount with > get_page() first. get_page() throws a BUG if the refcount is zero, which it will > hit on each of the 'tail' pages from TTM THP allocations. > > All of this currently works fine with TTM DMA-BUFs when the kernel is built with > !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates > pages. You need to redirect the mapping to dma_buf_mmap() instead. Regards, Christian. > >> Then the reason for TTM not using compound pages is that we can't >> guarantee that they are mapped as a whole to userspace. >> >> The result is that the kernel sometimes tried to de-compound them which >> created a bunch of problems. >> >> So yes this is completely intentional. > Understood, I figured something like that was the case, so I wanted to get your > input first. Do you know what the problems were, exactly? Practical issues > aside, it seems strange to call something a transparent huge page if it's > non-compound. > > Besides making these pages compound, would it be reasonable to split them before > sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? > That's where it's supposed to make sure that the shared DMA-BUF is accessible by > the target device. > > Thanks, > Alex > >> Regards, >> Christian. >> >> Am 01.10.20 um 00:18 schrieb Alex Goins: >>> Hi Christian, >>> >>> I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would >>> you mind giving some input on this? >>> >>> I noticed that your changes implementing transparent huge page support >>> in TTM are allocating them as non-compound. I understand that using >>> multiorder non-compound pages is common in device drivers, but I think >>> this can cause a problem when these pages are exported to other drivers. >>> >>> It's possible for other drivers to access the DMA-BUF's pages via >>> gem_prime_import_sg_table(), but without context from TTM, it's >>> impossible for the importing driver to make sense of them; they simply >>> appear as individual pages, with only the first page having a non-zero >>> refcount. Making TTM's THP allocations compound puts them more in line >>> with the standard definition of a THP, and allows DMA-BUF-importing >>> drivers to make sense of the pages within. >>> >>> I would like to propose making these allocations compound, but based on >>> patch history, it looks like the decision to make them non-compound was >>> intentional, as there were difficulties figuring out how to map them >>> into CPU page tables. I did some cursory testing with compound THPs, and >>> nothing seems obviously broken. I was also able to map compound THP >>> DMA-BUFs into userspace without issue, and access their contents. Are >>> you aware of any other potential consequences? >>> >>> Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should >>> probably also be reverted if this is applied. >>> >>> Thanks, >>> Alex >>> >>> Alex Goins (1): >>> drm-ttm: Allocate compound transparent huge pages >>> >>> drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-02 6:41 ` Christian König 0 siblings, 0 replies; 18+ messages in thread From: Christian König @ 2020-10-02 6:41 UTC (permalink / raw) To: Alex Goins, Daniel Vetter Cc: linux-mm, Zi Yan, dri-devel, amd-gfx, John Hubbard Hi Alex, adding Daniel as well. Am 01.10.20 um 20:45 schrieb Alex Goins: > Hi Christian, > > On Thu, 1 Oct 2020, Christian König wrote: > >> Hi Alex, >> >> first of all accessing the underlying page of an exported DMA-buf is >> illegal! So I'm not 100% sure what you're intentions are here, please >> explain further. > We have some mapping requirements that I was hoping I could address by mapping > these pages manually. > > Are you sure that it's illegal to access the underlying pages of an exported > DMA-BUF? yes, I'm 100% sure of that. This was discussed multiple times now on the mailing list. > There appears to be quite a few usages of this already. See the usage > of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. > drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when > importing a DMA-BUF from another driver, and the listed drivers then extract the > pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages > can then be mapped and faulted in. No, exactly that doesn't work correctly. You are corrupting internal state in struct page while doing so and risk that userspace is accessing freed up memory. We really need to find a way to fix the few drivers already doing this. > See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After > importing the pages from the SGT, vgem can fault them in, taking a refcount with > get_page() first. get_page() throws a BUG if the refcount is zero, which it will > hit on each of the 'tail' pages from TTM THP allocations. > > All of this currently works fine with TTM DMA-BUFs when the kernel is built with > !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates > pages. You need to redirect the mapping to dma_buf_mmap() instead. Regards, Christian. > >> Then the reason for TTM not using compound pages is that we can't >> guarantee that they are mapped as a whole to userspace. >> >> The result is that the kernel sometimes tried to de-compound them which >> created a bunch of problems. >> >> So yes this is completely intentional. > Understood, I figured something like that was the case, so I wanted to get your > input first. Do you know what the problems were, exactly? Practical issues > aside, it seems strange to call something a transparent huge page if it's > non-compound. > > Besides making these pages compound, would it be reasonable to split them before > sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? > That's where it's supposed to make sure that the shared DMA-BUF is accessible by > the target device. > > Thanks, > Alex > >> Regards, >> Christian. >> >> Am 01.10.20 um 00:18 schrieb Alex Goins: >>> Hi Christian, >>> >>> I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would >>> you mind giving some input on this? >>> >>> I noticed that your changes implementing transparent huge page support >>> in TTM are allocating them as non-compound. I understand that using >>> multiorder non-compound pages is common in device drivers, but I think >>> this can cause a problem when these pages are exported to other drivers. >>> >>> It's possible for other drivers to access the DMA-BUF's pages via >>> gem_prime_import_sg_table(), but without context from TTM, it's >>> impossible for the importing driver to make sense of them; they simply >>> appear as individual pages, with only the first page having a non-zero >>> refcount. Making TTM's THP allocations compound puts them more in line >>> with the standard definition of a THP, and allows DMA-BUF-importing >>> drivers to make sense of the pages within. >>> >>> I would like to propose making these allocations compound, but based on >>> patch history, it looks like the decision to make them non-compound was >>> intentional, as there were difficulties figuring out how to map them >>> into CPU page tables. I did some cursory testing with compound THPs, and >>> nothing seems obviously broken. I was also able to map compound THP >>> DMA-BUFs into userspace without issue, and access their contents. Are >>> you aware of any other potential consequences? >>> >>> Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should >>> probably also be reverted if this is applied. >>> >>> Thanks, >>> Alex >>> >>> Alex Goins (1): >>> drm-ttm: Allocate compound transparent huge pages >>> >>> drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-02 6:41 ` Christian König 0 siblings, 0 replies; 18+ messages in thread From: Christian König @ 2020-10-02 6:41 UTC (permalink / raw) To: Alex Goins, Daniel Vetter Cc: linux-mm, Zi Yan, dri-devel, amd-gfx, John Hubbard Hi Alex, adding Daniel as well. Am 01.10.20 um 20:45 schrieb Alex Goins: > Hi Christian, > > On Thu, 1 Oct 2020, Christian König wrote: > >> Hi Alex, >> >> first of all accessing the underlying page of an exported DMA-buf is >> illegal! So I'm not 100% sure what you're intentions are here, please >> explain further. > We have some mapping requirements that I was hoping I could address by mapping > these pages manually. > > Are you sure that it's illegal to access the underlying pages of an exported > DMA-BUF? yes, I'm 100% sure of that. This was discussed multiple times now on the mailing list. > There appears to be quite a few usages of this already. See the usage > of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. > drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when > importing a DMA-BUF from another driver, and the listed drivers then extract the > pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages > can then be mapped and faulted in. No, exactly that doesn't work correctly. You are corrupting internal state in struct page while doing so and risk that userspace is accessing freed up memory. We really need to find a way to fix the few drivers already doing this. > See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After > importing the pages from the SGT, vgem can fault them in, taking a refcount with > get_page() first. get_page() throws a BUG if the refcount is zero, which it will > hit on each of the 'tail' pages from TTM THP allocations. > > All of this currently works fine with TTM DMA-BUFs when the kernel is built with > !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates > pages. You need to redirect the mapping to dma_buf_mmap() instead. Regards, Christian. > >> Then the reason for TTM not using compound pages is that we can't >> guarantee that they are mapped as a whole to userspace. >> >> The result is that the kernel sometimes tried to de-compound them which >> created a bunch of problems. >> >> So yes this is completely intentional. > Understood, I figured something like that was the case, so I wanted to get your > input first. Do you know what the problems were, exactly? Practical issues > aside, it seems strange to call something a transparent huge page if it's > non-compound. > > Besides making these pages compound, would it be reasonable to split them before > sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? > That's where it's supposed to make sure that the shared DMA-BUF is accessible by > the target device. > > Thanks, > Alex > >> Regards, >> Christian. >> >> Am 01.10.20 um 00:18 schrieb Alex Goins: >>> Hi Christian, >>> >>> I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would >>> you mind giving some input on this? >>> >>> I noticed that your changes implementing transparent huge page support >>> in TTM are allocating them as non-compound. I understand that using >>> multiorder non-compound pages is common in device drivers, but I think >>> this can cause a problem when these pages are exported to other drivers. >>> >>> It's possible for other drivers to access the DMA-BUF's pages via >>> gem_prime_import_sg_table(), but without context from TTM, it's >>> impossible for the importing driver to make sense of them; they simply >>> appear as individual pages, with only the first page having a non-zero >>> refcount. Making TTM's THP allocations compound puts them more in line >>> with the standard definition of a THP, and allows DMA-BUF-importing >>> drivers to make sense of the pages within. >>> >>> I would like to propose making these allocations compound, but based on >>> patch history, it looks like the decision to make them non-compound was >>> intentional, as there were difficulties figuring out how to map them >>> into CPU page tables. I did some cursory testing with compound THPs, and >>> nothing seems obviously broken. I was also able to map compound THP >>> DMA-BUFs into userspace without issue, and access their contents. Are >>> you aware of any other potential consequences? >>> >>> Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should >>> probably also be reverted if this is applied. >>> >>> Thanks, >>> Alex >>> >>> Alex Goins (1): >>> drm-ttm: Allocate compound transparent huge pages >>> >>> drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP 2020-10-02 6:41 ` Christian König (?) @ 2020-10-02 7:31 ` Daniel Vetter -1 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2020-10-02 7:31 UTC (permalink / raw) To: Christian König Cc: Alex Goins, amd-gfx list, dri-devel, Linux MM, John Hubbard, Zi Yan On Fri, Oct 2, 2020 at 8:41 AM Christian König <christian.koenig@amd.com> wrote: > > Hi Alex, > > adding Daniel as well. > > Am 01.10.20 um 20:45 schrieb Alex Goins: > > Hi Christian, > > > > On Thu, 1 Oct 2020, Christian König wrote: > > > >> Hi Alex, > >> > >> first of all accessing the underlying page of an exported DMA-buf is > >> illegal! So I'm not 100% sure what you're intentions are here, please > >> explain further. > > We have some mapping requirements that I was hoping I could address by mapping > > these pages manually. > > > > Are you sure that it's illegal to access the underlying pages of an exported > > DMA-BUF? > > yes, I'm 100% sure of that. This was discussed multiple times now on the > mailing list. > > > There appears to be quite a few usages of this already. See the usage > > of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. > > drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when > > importing a DMA-BUF from another driver, and the listed drivers then extract the > > pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages > > can then be mapped and faulted in. > > No, exactly that doesn't work correctly. > > You are corrupting internal state in struct page while doing so and risk > that userspace is accessing freed up memory. > > We really need to find a way to fix the few drivers already doing this. Yeah the drivers doing this were merged with everyone aware that it's a bad trick, but 10 years ago we had nothing, not even userspace for multi-gpu, so there needed to be something to get the thing off the ground. But it was a bad idea back then, and it's still a bad idea now (and now we do have the ecosystem off the ground, so there's really not excuse for shortcuts). -Daniel > > See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After > > importing the pages from the SGT, vgem can fault them in, taking a refcount with > > get_page() first. get_page() throws a BUG if the refcount is zero, which it will > > hit on each of the 'tail' pages from TTM THP allocations. > > > > All of this currently works fine with TTM DMA-BUFs when the kernel is built with > > !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > > > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates > > pages. > > You need to redirect the mapping to dma_buf_mmap() instead. > > Regards, > Christian. > > > > >> Then the reason for TTM not using compound pages is that we can't > >> guarantee that they are mapped as a whole to userspace. > >> > >> The result is that the kernel sometimes tried to de-compound them which > >> created a bunch of problems. > >> > >> So yes this is completely intentional. > > Understood, I figured something like that was the case, so I wanted to get your > > input first. Do you know what the problems were, exactly? Practical issues > > aside, it seems strange to call something a transparent huge page if it's > > non-compound. > > > > Besides making these pages compound, would it be reasonable to split them before > > sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? > > That's where it's supposed to make sure that the shared DMA-BUF is accessible by > > the target device. > > > > Thanks, > > Alex > > > >> Regards, > >> Christian. > >> > >> Am 01.10.20 um 00:18 schrieb Alex Goins: > >>> Hi Christian, > >>> > >>> I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > >>> you mind giving some input on this? > >>> > >>> I noticed that your changes implementing transparent huge page support > >>> in TTM are allocating them as non-compound. I understand that using > >>> multiorder non-compound pages is common in device drivers, but I think > >>> this can cause a problem when these pages are exported to other drivers. > >>> > >>> It's possible for other drivers to access the DMA-BUF's pages via > >>> gem_prime_import_sg_table(), but without context from TTM, it's > >>> impossible for the importing driver to make sense of them; they simply > >>> appear as individual pages, with only the first page having a non-zero > >>> refcount. Making TTM's THP allocations compound puts them more in line > >>> with the standard definition of a THP, and allows DMA-BUF-importing > >>> drivers to make sense of the pages within. > >>> > >>> I would like to propose making these allocations compound, but based on > >>> patch history, it looks like the decision to make them non-compound was > >>> intentional, as there were difficulties figuring out how to map them > >>> into CPU page tables. I did some cursory testing with compound THPs, and > >>> nothing seems obviously broken. I was also able to map compound THP > >>> DMA-BUFs into userspace without issue, and access their contents. Are > >>> you aware of any other potential consequences? > >>> > >>> Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > >>> probably also be reverted if this is applied. > >>> > >>> Thanks, > >>> Alex > >>> > >>> Alex Goins (1): > >>> drm-ttm: Allocate compound transparent huge pages > >>> > >>> drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > >>> 1 file changed, 2 insertions(+), 3 deletions(-) > >>> > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-02 7:31 ` Daniel Vetter 0 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2020-10-02 7:31 UTC (permalink / raw) To: Christian König Cc: Alex Goins, John Hubbard, dri-devel, Linux MM, amd-gfx list, Zi Yan On Fri, Oct 2, 2020 at 8:41 AM Christian König <christian.koenig@amd.com> wrote: > > Hi Alex, > > adding Daniel as well. > > Am 01.10.20 um 20:45 schrieb Alex Goins: > > Hi Christian, > > > > On Thu, 1 Oct 2020, Christian König wrote: > > > >> Hi Alex, > >> > >> first of all accessing the underlying page of an exported DMA-buf is > >> illegal! So I'm not 100% sure what you're intentions are here, please > >> explain further. > > We have some mapping requirements that I was hoping I could address by mapping > > these pages manually. > > > > Are you sure that it's illegal to access the underlying pages of an exported > > DMA-BUF? > > yes, I'm 100% sure of that. This was discussed multiple times now on the > mailing list. > > > There appears to be quite a few usages of this already. See the usage > > of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. > > drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when > > importing a DMA-BUF from another driver, and the listed drivers then extract the > > pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages > > can then be mapped and faulted in. > > No, exactly that doesn't work correctly. > > You are corrupting internal state in struct page while doing so and risk > that userspace is accessing freed up memory. > > We really need to find a way to fix the few drivers already doing this. Yeah the drivers doing this were merged with everyone aware that it's a bad trick, but 10 years ago we had nothing, not even userspace for multi-gpu, so there needed to be something to get the thing off the ground. But it was a bad idea back then, and it's still a bad idea now (and now we do have the ecosystem off the ground, so there's really not excuse for shortcuts). -Daniel > > See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After > > importing the pages from the SGT, vgem can fault them in, taking a refcount with > > get_page() first. get_page() throws a BUG if the refcount is zero, which it will > > hit on each of the 'tail' pages from TTM THP allocations. > > > > All of this currently works fine with TTM DMA-BUFs when the kernel is built with > > !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > > > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates > > pages. > > You need to redirect the mapping to dma_buf_mmap() instead. > > Regards, > Christian. > > > > >> Then the reason for TTM not using compound pages is that we can't > >> guarantee that they are mapped as a whole to userspace. > >> > >> The result is that the kernel sometimes tried to de-compound them which > >> created a bunch of problems. > >> > >> So yes this is completely intentional. > > Understood, I figured something like that was the case, so I wanted to get your > > input first. Do you know what the problems were, exactly? Practical issues > > aside, it seems strange to call something a transparent huge page if it's > > non-compound. > > > > Besides making these pages compound, would it be reasonable to split them before > > sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? > > That's where it's supposed to make sure that the shared DMA-BUF is accessible by > > the target device. > > > > Thanks, > > Alex > > > >> Regards, > >> Christian. > >> > >> Am 01.10.20 um 00:18 schrieb Alex Goins: > >>> Hi Christian, > >>> > >>> I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > >>> you mind giving some input on this? > >>> > >>> I noticed that your changes implementing transparent huge page support > >>> in TTM are allocating them as non-compound. I understand that using > >>> multiorder non-compound pages is common in device drivers, but I think > >>> this can cause a problem when these pages are exported to other drivers. > >>> > >>> It's possible for other drivers to access the DMA-BUF's pages via > >>> gem_prime_import_sg_table(), but without context from TTM, it's > >>> impossible for the importing driver to make sense of them; they simply > >>> appear as individual pages, with only the first page having a non-zero > >>> refcount. Making TTM's THP allocations compound puts them more in line > >>> with the standard definition of a THP, and allows DMA-BUF-importing > >>> drivers to make sense of the pages within. > >>> > >>> I would like to propose making these allocations compound, but based on > >>> patch history, it looks like the decision to make them non-compound was > >>> intentional, as there were difficulties figuring out how to map them > >>> into CPU page tables. I did some cursory testing with compound THPs, and > >>> nothing seems obviously broken. I was also able to map compound THP > >>> DMA-BUFs into userspace without issue, and access their contents. Are > >>> you aware of any other potential consequences? > >>> > >>> Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > >>> probably also be reverted if this is applied. > >>> > >>> Thanks, > >>> Alex > >>> > >>> Alex Goins (1): > >>> drm-ttm: Allocate compound transparent huge pages > >>> > >>> drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > >>> 1 file changed, 2 insertions(+), 3 deletions(-) > >>> > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP @ 2020-10-02 7:31 ` Daniel Vetter 0 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2020-10-02 7:31 UTC (permalink / raw) To: Christian König Cc: John Hubbard, dri-devel, Linux MM, amd-gfx list, Zi Yan On Fri, Oct 2, 2020 at 8:41 AM Christian König <christian.koenig@amd.com> wrote: > > Hi Alex, > > adding Daniel as well. > > Am 01.10.20 um 20:45 schrieb Alex Goins: > > Hi Christian, > > > > On Thu, 1 Oct 2020, Christian König wrote: > > > >> Hi Alex, > >> > >> first of all accessing the underlying page of an exported DMA-buf is > >> illegal! So I'm not 100% sure what you're intentions are here, please > >> explain further. > > We have some mapping requirements that I was hoping I could address by mapping > > these pages manually. > > > > Are you sure that it's illegal to access the underlying pages of an exported > > DMA-BUF? > > yes, I'm 100% sure of that. This was discussed multiple times now on the > mailing list. > > > There appears to be quite a few usages of this already. See the usage > > of drm_prime_sg_to_page_addr_arrays() in vgem, vkms, msm, xen, and etnaviv. > > drm_gem_prime_import_dev() uses driver->gem_prime_import_sg_table() when > > importing a DMA-BUF from another driver, and the listed drivers then extract the > > pages from the given SGT using drm_prime_sg_to_page_addr_arrays(). These pages > > can then be mapped and faulted in. > > No, exactly that doesn't work correctly. > > You are corrupting internal state in struct page while doing so and risk > that userspace is accessing freed up memory. > > We really need to find a way to fix the few drivers already doing this. Yeah the drivers doing this were merged with everyone aware that it's a bad trick, but 10 years ago we had nothing, not even userspace for multi-gpu, so there needed to be something to get the thing off the ground. But it was a bad idea back then, and it's still a bad idea now (and now we do have the ecosystem off the ground, so there's really not excuse for shortcuts). -Daniel > > See commit af33a9190d02 ('drm/vgem: Enable dmabuf import interfaces'). After > > importing the pages from the SGT, vgem can fault them in, taking a refcount with > > get_page() first. get_page() throws a BUG if the refcount is zero, which it will > > hit on each of the 'tail' pages from TTM THP allocations. > > > > All of this currently works fine with TTM DMA-BUFs when the kernel is built with > > !CONFIG_TRANSPARENT_HUGEPAGE. However, 'echo never > > > /sys/kernel/mm/transparent_hugepage/enabled' doesn't change how TTM allocates > > pages. > > You need to redirect the mapping to dma_buf_mmap() instead. > > Regards, > Christian. > > > > >> Then the reason for TTM not using compound pages is that we can't > >> guarantee that they are mapped as a whole to userspace. > >> > >> The result is that the kernel sometimes tried to de-compound them which > >> created a bunch of problems. > >> > >> So yes this is completely intentional. > > Understood, I figured something like that was the case, so I wanted to get your > > input first. Do you know what the problems were, exactly? Practical issues > > aside, it seems strange to call something a transparent huge page if it's > > non-compound. > > > > Besides making these pages compound, would it be reasonable to split them before > > sharing them, in e.g. amdgpu_dma_buf_map (and in other drivers that use TTM)? > > That's where it's supposed to make sure that the shared DMA-BUF is accessible by > > the target device. > > > > Thanks, > > Alex > > > >> Regards, > >> Christian. > >> > >> Am 01.10.20 um 00:18 schrieb Alex Goins: > >>> Hi Christian, > >>> > >>> I've been looking into the DMA-BUFs exported from AMDGPU / TTM. Would > >>> you mind giving some input on this? > >>> > >>> I noticed that your changes implementing transparent huge page support > >>> in TTM are allocating them as non-compound. I understand that using > >>> multiorder non-compound pages is common in device drivers, but I think > >>> this can cause a problem when these pages are exported to other drivers. > >>> > >>> It's possible for other drivers to access the DMA-BUF's pages via > >>> gem_prime_import_sg_table(), but without context from TTM, it's > >>> impossible for the importing driver to make sense of them; they simply > >>> appear as individual pages, with only the first page having a non-zero > >>> refcount. Making TTM's THP allocations compound puts them more in line > >>> with the standard definition of a THP, and allows DMA-BUF-importing > >>> drivers to make sense of the pages within. > >>> > >>> I would like to propose making these allocations compound, but based on > >>> patch history, it looks like the decision to make them non-compound was > >>> intentional, as there were difficulties figuring out how to map them > >>> into CPU page tables. I did some cursory testing with compound THPs, and > >>> nothing seems obviously broken. I was also able to map compound THP > >>> DMA-BUFs into userspace without issue, and access their contents. Are > >>> you aware of any other potential consequences? > >>> > >>> Commit 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages") should > >>> probably also be reverted if this is applied. > >>> > >>> Thanks, > >>> Alex > >>> > >>> Alex Goins (1): > >>> drm-ttm: Allocate compound transparent huge pages > >>> > >>> drivers/gpu/drm/ttm/ttm_page_alloc.c | 5 ++--- > >>> 1 file changed, 2 insertions(+), 3 deletions(-) > >>> > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-10-02 7:31 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 22:18 [PATCH RFC 0/1] drm/ttm: Allocate transparent huge pages without clearing __GFP_COMP Alex Goins 2020-09-30 22:18 ` Alex Goins 2020-09-30 22:18 ` Alex Goins 2020-09-30 22:18 ` [PATCH RFC 1/1] drm-ttm: " Alex Goins 2020-09-30 22:18 ` Alex Goins 2020-09-30 22:18 ` Alex Goins 2020-10-01 7:19 ` [PATCH RFC 0/1] drm/ttm: " Christian König 2020-10-01 7:19 ` Christian König 2020-10-01 7:19 ` Christian König 2020-10-01 18:45 ` Alex Goins 2020-10-01 18:45 ` Alex Goins 2020-10-01 18:45 ` Alex Goins 2020-10-02 6:41 ` Christian König 2020-10-02 6:41 ` Christian König 2020-10-02 6:41 ` Christian König 2020-10-02 7:31 ` Daniel Vetter 2020-10-02 7:31 ` Daniel Vetter 2020-10-02 7:31 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.