[PATCH i-g-t, v2] tests/intel/xe_vm: Always have at least 2 pages bound in mmap hammer sections

[Cavitt, Jonathan]

-----Original Message-----
From: Randhawa, Jagmeet <jagmeet.randhawa at intel.com> 
Sent: Wednesday, April 24, 2024 2:42 PM
Cc: igt-dev at lists.freedesktop.org; Cavitt, Jonathan <jonathan.cavitt at intel.com>; Brost, Matthew <matthew.brost at intel.com>; Randhawa, Jagmeet <jagmeet.randhawa at intel.com>
Subject: [PATCH i-g-t, v2] tests/intel/xe_vm: Always have at least 2 pages bound in mmap hammer sections
> 
> Fixes: ee0a9c8e30f6 ("tests/intel/xe_vm: Always
> have at least 2 pages bound in munmap hammer sections")

This goes above the Cc: Jonathan Cavitt line.  I.E.:

Fixes: ee0a9c8e30f6 ("tests/intel/xe_vm: Always have at least 2 pages bound in munmap hammer sections")
Cc: Jonathan Cavitt <jonathan.cavitt at intel.com>
Suggested-by: Matthew Brost <matthew.brost at intel.com>
Signed-off-by: Jagmeet Randhawa <jagmeet.randhawa at intel.com>

Also, and I don't know if this matters, but the newline added automatically by vim should probably be removed.

> 
> This patch extends the recent improvements made
> to the munmap hammer sections by applying a similar
> fix to the mmap hammer sections.
> 
> If the prefetch buffer is 2k or larger the munmap hammer sections will
> prefetch the next page, if the page is unbound a fault will occur.
> Account for larger prefetchs by always having at least 2 pages bound for
> the hammer thread.

I'm not certain copying the commit message verbatim here is preferred.  If you want
to denote that the two patches are related, the Fixes tag should do that, but to be more
direct, you could say something like...

"""
Extend ee0a9c8e30f6 ("tests/intel/xe_vm: Always have at least 2 pages
bound in munmap hammer sections") to apply a similar fix to the mmap
hammer sections.
"""

Just something to consider.  I won't block on it.

> 
> Cc: Jonathan Cavitt <jonathan.cavitt at intel.com>
> Suggested-by: Matthew Brost <matthew.brost at intel.com>
> Signed-off-by: Jagmeet Randhawa <jagmeet.randhawa at intel.com>
> ---
>  tests/intel/xe_vm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/intel/xe_vm.c b/tests/intel/xe_vm.c
> index de2b1a813..9872df832 100644
> --- a/tests/intel/xe_vm.c
> +++ b/tests/intel/xe_vm.c

The original patch (tests/intel/xe_vm: Always have at least 2 pages bound in munmap hammer sections)
also had the following change applied:

"""
@@ -1263,6 +1263,11 @@ test_munmap_style_unbind(int fd, struct drm_xe_engine_class_instance *eci,
 	int exit = 0;
 	int n_page_per_2mb = 0x200000 / xe_get_default_alignment(fd);
 
+	/* Ensure prefetch will not fetch an unmapped page */
+	if (flags & MAP_FLAG_HAMMER_FIRST_PAGE)
+		igt_assert(unbind_n_page_offset * 0x1000 >
+			   xe_cs_prefetch_size(fd));
+
 	if (flags & MAP_FLAG_LARGE_PAGE) {
 		bo_n_pages *= n_page_per_2mb;
 		unbind_n_pages *= n_page_per_2mb;
"""

I think something similar needs to be added to test_mmap_style_bind (for consistency, if nothing else):

"""
@@ -1568,6 +1568,11 @@ test_mmap_style_bind(int fd, struct drm_xe_engine_class_instance *eci,
        int exit = 0;
        int n_page_per_2mb = 0x200000 / xe_get_default_alignment(fd);

+       /* Ensure prefetch will not fetch an unmapped page */
+       if (flags & MAP_FLAG_HAMMER_FIRST_PAGE)
+               igt_assert(unbind_n_page_offset * 0x1000 >
+                          xe_cs_prefetch_size(fd));
+
        if (flags & MAP_FLAG_LARGE_PAGE) {
                bo_n_pages *= n_page_per_2mb;
                unbind_n_pages *= n_page_per_2mb;
"""

> @@ -1913,12 +1913,12 @@ igt_main
>  		{ "one-partial", 4, 1, 1, 2, 0 },
>  		{ "either-side-partial", 4, 2, 1, 2, 0 },
>  		{ "either-side-full", 4, 4, 1, 2, 0 },
> -		{ "either-side-partial-hammer", 4, 2, 1, 2,
> +		{ "either-side-partial-hammer", 6, 2, 2, 2,
>  			MAP_FLAG_HAMMER_FIRST_PAGE },
> -		{ "either-side-partial-split-page-hammer", 4, 2, 1, 2,
> +		{ "either-side-partial-split-page-hammer", 6, 2, 2, 2,
>  			MAP_FLAG_HAMMER_FIRST_PAGE |
>  			MAP_FLAG_LARGE_PAGE },
> -		{ "either-side-partial-large-page-hammer", 4, 2, 1, 2,
> +		{ "either-side-partial-large-page-hammer", 6, 2, 2, 2,
>  			MAP_FLAG_HAMMER_FIRST_PAGE |
>  			MAP_FLAG_LARGE_PAGE |
>  			MAP_FLAG_LARGE_PAGE_NO_SPLIT },
> @@ -1926,7 +1926,7 @@ igt_main
>  		{ "front", 4, 2, 1, 3, 0 },
>  		{ "many-all", 4 * 8, 2 * 8, 0 * 8, 4 * 8, 0 },
>  		{ "many-either-side-partial", 4 * 8, 2 * 8, 1, 4 * 8 - 2, 0 },
> -		{ "many-either-side-partial-hammer", 4 * 8, 2 * 8, 1, 4 * 8 - 2,
> +		{ "many-either-side-partial-hammer", 4 * 8, 2 * 8, 2, 4 * 8 - 4,
>  			MAP_FLAG_HAMMER_FIRST_PAGE },
>  		{ "userptr-all", 4, 2, 0, 4, MAP_FLAG_USERPTR },
>  		{ "userptr-one-partial", 4, 1, 1, 2, MAP_FLAG_USERPTR },
> -- 
> 2.25.1
> 

Move the Fixes tag, and add the missing change to test_mmap_style_bind, and this is
Acked-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

>