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

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Apr 24, 2024 at 03:56:32PM -0600, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Randhawa, Jagmeet <jagmeet.randhawa@intel.com> 
> Sent: Wednesday, April 24, 2024 2:42 PM
> Cc: igt-dev@lists.freedesktop.org; Cavitt, Jonathan <jonathan.cavitt@intel.com>; Brost, Matthew <matthew.brost@intel.com>; Randhawa, Jagmeet <jagmeet.randhawa@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@intel.com>
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Jagmeet Randhawa <jagmeet.randhawa@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@intel.com>
> > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > Signed-off-by: Jagmeet Randhawa <jagmeet.randhawa@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@intel.com>
> -Jonathan Cavitt

Please don't merge this, I think I've convinced myself this is actually
exposing a KMD bug. I'll have to dig into this on machine a bit to
confirm. It might be a week or so before I can fully dig in.

Matt

> 
> >