[igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Chris Wilson]

As we already have the previous portion of the mmap mlocked, we only
need to mlock() the fresh portion for testing available memory.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Caz Yokoyama <caz@caz-nuc.ra.intel.com>
---
 lib/intel_os.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/lib/intel_os.c b/lib/intel_os.c
index e1e31e230..961442a0d 100644
--- a/lib/intel_os.c
+++ b/lib/intel_os.c
@@ -250,22 +250,24 @@ intel_get_total_pinnable_mem(void)
 	}
 
 	for (uint64_t inc = 1024 << 20; inc >= 4 << 10; inc >>= 2) {
-		igt_debug("Testing mlock %'"PRIu64" B (%'"PRIu64" MiB)\n",
-			  *can_mlock, *can_mlock >> 20);
+		uint64_t locked = *can_mlock;
+
+		igt_debug("Testing mlock %'"PRIu64"B (%'"PRIu64"MiB) + %'"PRIu64"B\n",
+			  locked, locked >> 20, inc);
 
 		igt_fork(child, 1) {
-			for (uint64_t bytes = *can_mlock;
-			     bytes <= pin;
-			     bytes += inc) {
-				if (mlock(can_mlock, bytes))
+			uint64_t bytes = *can_mlock;
+
+			while (bytes <= pin) {
+				if (mlock(can_mlock + bytes, inc))
 					break;
 
-				*can_mlock = bytes;
+				*can_mlock = bytes += inc;
 				__sync_synchronize();
 			}
 		}
 		__igt_waitchildren();
-		igt_assert(!mlock(can_mlock, *can_mlock));
+		igt_assert(!mlock(can_mlock + locked, *can_mlock - locked));
 	}
 
 out:
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Ashutosh Dixit]

On Tue, Feb 19 2019 at 03:21:10 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> As we already have the previous portion of the mmap mlocked, we only
> need to mlock() the fresh portion for testing available memory.

Still looking at what is wrong with the patch but with the patch the
test is unable to lock as much memory as before and invoke the
shrinker. Here are the outputs before and after the patch on a system
with 16 GB:

--------BEFORE---------

# ./i915_suspend --run-subtest shrink
IGT-Version: 1.23-g7802324e (x86_64) (Linux: 5.0.0-rc5+ x86_64)
Starting subtest: shrink
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
Locked 16,165,109,760 B (15,416 MiB)
[cmd] rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Nov 25 06:02:55 1998
Subtest shrink: SUCCESS (35.917s)

--------AFTER---------

# ./i915_suspend --run-subtest shrink
IGT-Version: 1.23-gdad68484 (x86_64) (Linux: 5.0.0-rc5+ x86_64)
Starting subtest: shrink
Locked 12,157,976,576 B (11,594 MiB)
[cmd] rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Nov 25 06:04:02 1998
Subtest shrink: SUCCESS (10.247s)
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Ashutosh Dixit]

On Tue, Feb 19 2019 at 09:26:56 PM, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
> On Tue, Feb 19 2019 at 03:21:10 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> As we already have the previous portion of the mmap mlocked, we only
>> need to mlock() the fresh portion for testing available memory.
>
> Still looking at what is wrong with the patch but with the patch the
> test is unable to lock as much memory as before and invoke the
> shrinker. Here are the outputs before and after the patch on a system
> with 16 GB:

OK, found the bug. The bug is that can_mlock is u64*. Therefore
can_mlock must be cast to u8* on these two lines:


> +     if (mlock(can_mlock + bytes, inc))

and 

> +	igt_assert(!mlock(can_mlock + locked, *can_mlock - locked));

However when we do this, the parent process invariably gets killed by
the OOM killer, something which was not happening previously:

# ./i915_suspend --run-subtest shrink
IGT-Version: 1.23-g9ef791ce (x86_64) (Linux: 5.0.0-rc5+ x86_64)
Starting subtest: shrink
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
Killed
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Caz Yokoyama]

Yes, it is a bug.

With the bug, mlocking is sparsely distributed, i.e. there are holes
between mlocked memories. Therefore, process is not killed.

By fixing the bug, continuously mlocked as planned and killed.

My test shows OOM kills a process when a process mlocks 128mb less than
pinnable memory while not when less than 256mb.

I'll send a patch where
- intel_get_total_pinnable_mem() reports pinnable memory
- test_shrink() mlocks X(>128, <256)mb
-caz

On Tue, 2019-02-19 at 21:39 -0800, Ashutosh Dixit wrote:
> On Tue, Feb 19 2019 at 09:26:56 PM, Ashutosh Dixit <
> ashutosh.dixit@intel.com> wrote:
> > On Tue, Feb 19 2019 at 03:21:10 PM, Chris Wilson <
> > chris@chris-wilson.co.uk> wrote:
> > > As we already have the previous portion of the mmap mlocked, we
> > > only
> > > need to mlock() the fresh portion for testing available memory.
> > 
> > Still looking at what is wrong with the patch but with the patch
> > the
> > test is unable to lock as much memory as before and invoke the
> > shrinker. Here are the outputs before and after the patch on a
> > system
> > with 16 GB:
> 
> OK, found the bug. The bug is that can_mlock is u64*. Therefore
> can_mlock must be cast to u8* on these two lines:
> 
> 
> > +     if (mlock(can_mlock + bytes, inc))
> 
> and 
> 
> > +	igt_assert(!mlock(can_mlock + locked, *can_mlock - locked));
> 
> However when we do this, the parent process invariably gets killed by
> the OOM killer, something which was not happening previously:
> 
> # ./i915_suspend --run-subtest shrink
> IGT-Version: 1.23-g9ef791ce (x86_64) (Linux: 5.0.0-rc5+ x86_64)
> Starting subtest: shrink
> child 0 died with signal 9, Killed
> child 0 died with signal 9, Killed
> child 0 died with signal 9, Killed
> child 0 died with signal 9, Killed
> child 0 died with signal 9, Killed
> child 0 died with signal 9, Killed
> Killed
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Chris Wilson]

As we already have the previous portion of the mmap mlocked, we only
need to mlock() the fresh portion for testing available memory.

v2: Fixup the uint64_t pointer arithmetric and only use a single mmap to
avoid subsequent mlock fail (for reasons unknown, but bet on mm/).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Caz Yokoyama <Caz.Yokoyama@intel.com>
---
 lib/igt_aux.h             |  2 +-
 lib/intel_os.c            | 40 +++++++++++++++++++++------------------
 tests/eviction_common.c   | 17 +++++++++--------
 tests/i915/i915_suspend.c | 17 +++--------------
 4 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index fb02c026a..09b6246bf 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -194,7 +194,7 @@ void intel_purge_vm_caches(int fd);
 uint64_t intel_get_avail_ram_mb(void);
 uint64_t intel_get_total_ram_mb(void);
 uint64_t intel_get_total_swap_mb(void);
-size_t intel_get_total_pinnable_mem(void);
+void *intel_get_total_pinnable_mem(size_t *pinned);
 
 int __intel_check_memory(uint64_t count, uint64_t size, unsigned mode,
 			 uint64_t *out_required, uint64_t *out_total);
diff --git a/lib/intel_os.c b/lib/intel_os.c
index e1e31e230..dd93bea1a 100644
--- a/lib/intel_os.c
+++ b/lib/intel_os.c
@@ -227,11 +227,9 @@ intel_get_total_swap_mb(void)
  *
  * Returns: Amount of memory that can be safely pinned, in bytes.
  */
-size_t
-intel_get_total_pinnable_mem(void)
+void *intel_get_total_pinnable_mem(size_t *total)
 {
 	uint64_t *can_mlock, pin, avail;
-	size_t ret;
 
 	pin = (intel_get_total_ram_mb() + 1) << 20;
 	avail = (intel_get_avail_ram_mb() + 1) << 20;
@@ -245,34 +243,40 @@ intel_get_total_pinnable_mem(void)
 	 */
 	*can_mlock = (avail >> 1) + (avail >> 2);
 	if (mlock(can_mlock, *can_mlock)) {
-		*can_mlock = 0;
-		goto out;
+		munmap(can_mlock, pin);
+		return MAP_FAILED;
 	}
 
 	for (uint64_t inc = 1024 << 20; inc >= 4 << 10; inc >>= 2) {
-		igt_debug("Testing mlock %'"PRIu64" B (%'"PRIu64" MiB)\n",
-			  *can_mlock, *can_mlock >> 20);
+		uint64_t locked = *can_mlock;
+
+		igt_debug("Testing mlock %'"PRIu64"B (%'"PRIu64"MiB) + %'"PRIu64"B\n",
+			  locked, locked >> 20, inc);
 
 		igt_fork(child, 1) {
-			for (uint64_t bytes = *can_mlock;
-			     bytes <= pin;
-			     bytes += inc) {
-				if (mlock(can_mlock, bytes))
+			uint64_t bytes = *can_mlock;
+
+			while (bytes <= pin) {
+				if (mlock((void *)can_mlock + bytes, inc))
 					break;
 
-				*can_mlock = bytes;
+				*can_mlock = bytes += inc;
 				__sync_synchronize();
 			}
 		}
 		__igt_waitchildren();
-		igt_assert(!mlock(can_mlock, *can_mlock));
-	}
 
-out:
-	ret = *can_mlock;
-	munmap(can_mlock, pin);
+		if (*can_mlock > locked + inc) { /* Weird bit of mm/ lore */
+			*can_mlock -= inc;
+			igt_debug("Claiming mlock %'"PRIu64"B (%'"PRIu64"MiB)\n",
+				  *can_mlock, *can_mlock >> 20);
+			igt_assert(!mlock((void *)can_mlock + locked,
+					  *can_mlock - locked));
+		}
+	}
 
-	return ret;
+	*total = pin;
+	return can_mlock;
 }
 
 static uint64_t vfs_file_max(void)
diff --git a/tests/eviction_common.c b/tests/eviction_common.c
index 321772ba7..a3b9e4167 100644
--- a/tests/eviction_common.c
+++ b/tests/eviction_common.c
@@ -133,23 +133,24 @@ static void mlocked_evictions(int fd, struct igt_eviction_test_ops *ops,
 			      uint64_t surface_size,
 			      uint64_t surface_count)
 {
+	uint64_t sz, pin, total;
 	void *mem;
-	uint64_t sz, pin_total;
 
 	intel_require_memory(surface_count, surface_size, CHECK_RAM);
 
-	sz = surface_size*surface_count;
-	pin_total = intel_get_total_pinnable_mem();
-	igt_require(pin_total > sz);
-
-	mem = mmap(NULL, pin_total, PROT_READ, MAP_SHARED | MAP_ANON, -1, 0);
+	mem = intel_get_total_pinnable_mem(&total);
 	igt_assert(mem != MAP_FAILED);
+	pin = *(uint64_t *)mem;
+	igt_assert(!munlock(mem, pin));
+
+	sz = surface_size * surface_count;
+	igt_require(pin > sz);
 
 	igt_fork(child, 1) {
 		uint32_t *bo;
 		uint64_t n;
 		int ret;
-		size_t lock = pin_total - sz;
+		size_t lock = pin - sz;
 
 		bo = malloc(surface_count * sizeof(*bo));
 		igt_assert(bo);
@@ -186,7 +187,7 @@ static void mlocked_evictions(int fd, struct igt_eviction_test_ops *ops,
 	}
 	igt_waitchildren();
 
-	munmap(mem, pin_total);
+	munmap(mem, total);
 }
 
 static int swapping_evictions(int fd, struct igt_eviction_test_ops *ops,
diff --git a/tests/i915/i915_suspend.c b/tests/i915/i915_suspend.c
index 84cb3b490..cd7cf9675 100644
--- a/tests/i915/i915_suspend.c
+++ b/tests/i915/i915_suspend.c
@@ -163,25 +163,14 @@ test_sysfs_reader(bool hibernate)
 static void
 test_shrink(int fd, unsigned int mode)
 {
-	void *mem;
 	size_t size;
+	void *mem;
 
 	gem_quiescent_gpu(fd);
 	intel_purge_vm_caches(fd);
 
-	size = intel_get_total_pinnable_mem();
-	igt_require(size > 64 << 20);
-	size -= 64 << 20;
-
-	mem = mmap(NULL, size, PROT_READ, MAP_SHARED | MAP_ANON, -1, 0);
-
-	intel_purge_vm_caches(fd);
-
-	igt_debug("Locking %'zu B (%'zu MiB)\n",
-		  size, size >> 20);
-	igt_assert(!mlock(mem, size));
-	igt_info("Locked %'zu B (%'zu MiB)\n",
-		 size, size >> 20);
+	mem = intel_get_total_pinnable_mem(&size);
+	igt_assert(mem != MAP_FAILED);
 
 	intel_purge_vm_caches(fd);
 	igt_system_suspend_autoresume(mode, SUSPEND_TEST_NONE);
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for lib: Incrementally mlock()

[Patchwork]

== Series Details ==

Series: lib: Incrementally mlock()
URL   : https://patchwork.freedesktop.org/series/57302/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5666 -> IGTPW_2532
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2532 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2532, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57302/revisions/1/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2532:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_gem:
    - fi-skl-gvtdvm:      PASS -> DMESG-WARN

  
Known issues
------------

  Here are the changes found in IGTPW_2532 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_contexts:
    - fi-icl-u2:          NOTRUN -> DMESG-FAIL [fdo#108569]

  * igt@kms_busy@basic-flip-a:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182]

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       PASS -> WARN [fdo#109380]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
    - fi-kbl-7567u:       PASS -> SKIP [fdo#109271] +33

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +27

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         PASS -> FAIL [fdo#104008]

  * igt@runner@aborted:
    - fi-skl-gvtdvm:      NOTRUN -> FAIL [fdo#104108]

  
#### Possible fixes ####

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         INCOMPLETE [fdo#103927] / [fdo#109720] -> PASS

  * igt@i915_selftest@live_requests:
    - fi-icl-u2:          INCOMPLETE [fdo#109644] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380
  [fdo#109644]: https://bugs.freedesktop.org/show_bug.cgi?id=109644
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720


Participating hosts (44 -> 37)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510 fi-icl-y 


Build changes
-------------

    * IGT: IGT_4860 -> IGTPW_2532

  CI_DRM_5666: 358ab8acaabef3cef2a7ce9e5dd7c4196a0c30fc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2532: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2532/
  IGT_4860: b79007f9c575a538a63ce9301a890ed9e1a45f35 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2532/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Caz Yokoyama]

inline.

v2 patches fixes
- address calculation bug
- not killed
However, the test does not runs faster.
-caz

On Wed, 2019-02-27 at 16:29 +0000, Chris Wilson wrote:
> As we already have the previous portion of the mmap mlocked, we only
> need to mlock() the fresh portion for testing available memory.
> 
> v2: Fixup the uint64_t pointer arithmetric and only use a single mmap
> to
> avoid subsequent mlock fail (for reasons unknown, but bet on mm/).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Caz Yokoyama <Caz.Yokoyama@intel.com>
> ---
>  lib/igt_aux.h             |  2 +-
>  lib/intel_os.c            | 40 +++++++++++++++++++++--------------
> ----
>  tests/eviction_common.c   | 17 +++++++++--------
>  tests/i915/i915_suspend.c | 17 +++--------------
>  4 files changed, 35 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index fb02c026a..09b6246bf 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -194,7 +194,7 @@ void intel_purge_vm_caches(int fd);
>  uint64_t intel_get_avail_ram_mb(void);
>  uint64_t intel_get_total_ram_mb(void);
>  uint64_t intel_get_total_swap_mb(void);
> -size_t intel_get_total_pinnable_mem(void);
> +void *intel_get_total_pinnable_mem(size_t *pinned);
>  
>  int __intel_check_memory(uint64_t count, uint64_t size, unsigned
> mode,
>  			 uint64_t *out_required, uint64_t *out_total);
> diff --git a/lib/intel_os.c b/lib/intel_os.c
> index e1e31e230..dd93bea1a 100644
> --- a/lib/intel_os.c
> +++ b/lib/intel_os.c
> @@ -227,11 +227,9 @@ intel_get_total_swap_mb(void)
>   *
>   * Returns: Amount of memory that can be safely pinned, in bytes.
>   */
> -size_t
> -intel_get_total_pinnable_mem(void)
> +void *intel_get_total_pinnable_mem(size_t *total)
>  {
>  	uint64_t *can_mlock, pin, avail;
> -	size_t ret;
>  
>  	pin = (intel_get_total_ram_mb() + 1) << 20;
>  	avail = (intel_get_avail_ram_mb() + 1) << 20;
> @@ -245,34 +243,40 @@ intel_get_total_pinnable_mem(void)
>  	 */
>  	*can_mlock = (avail >> 1) + (avail >> 2);
>  	if (mlock(can_mlock, *can_mlock)) {
> -		*can_mlock = 0;
> -		goto out;
> +		munmap(can_mlock, pin);
> +		return MAP_FAILED;
>  	}
>  
>  	for (uint64_t inc = 1024 << 20; inc >= 4 << 10; inc >>= 2) {
> -		igt_debug("Testing mlock %'"PRIu64" B (%'"PRIu64"
> MiB)\n",
> -			  *can_mlock, *can_mlock >> 20);
> +		uint64_t locked = *can_mlock;
> +
> +		igt_debug("Testing mlock %'"PRIu64"B (%'"PRIu64"MiB) +
> %'"PRIu64"B\n",
> +			  locked, locked >> 20, inc);
>  
>  		igt_fork(child, 1) {
> -			for (uint64_t bytes = *can_mlock;
> -			     bytes <= pin;
> -			     bytes += inc) {
> -				if (mlock(can_mlock, bytes))
> +			uint64_t bytes = *can_mlock;
> +
> +			while (bytes <= pin) {
> +				if (mlock((void *)can_mlock + bytes,
> inc))
>  					break;
>  
> -				*can_mlock = bytes;
> +				*can_mlock = bytes += inc;
>  				__sync_synchronize();
>  			}
>  		}
>  		__igt_waitchildren();
> -		igt_assert(!mlock(can_mlock, *can_mlock));
> -	}
>  
> -out:
> -	ret = *can_mlock;
> -	munmap(can_mlock, pin);
> +		if (*can_mlock > locked + inc) { /* Weird bit of mm/
> lore */
> +			*can_mlock -= inc;
> 
Are you able to explain this? Is this for when child is killed during
updating *can_mlock?

If the condition is not met, parent does not mlock. Is this OK?

> +			igt_debug("Claiming mlock %'"PRIu64"B
> (%'"PRIu64"MiB)\n",
> +				  *can_mlock, *can_mlock >> 20);
> +			igt_assert(!mlock((void *)can_mlock + locked,
> 
> +					  *can_mlock - locked));
> +		}
> +	}
>  
> -	return ret;
> +	*total = pin;
> 
*total = *can_mlock?

Also you don't unmap. I mean map and unmap are not paired in the same
function. Not elegant. But c'est la vie.
-caz

> +	return can_mlock;
>  }
>  
>  static uint64_t vfs_file_max(void)
> diff --git a/tests/eviction_common.c b/tests/eviction_common.c
> index 321772ba7..a3b9e4167 100644
> --- a/tests/eviction_common.c
> +++ b/tests/eviction_common.c
> @@ -133,23 +133,24 @@ static void mlocked_evictions(int fd, struct
> igt_eviction_test_ops *ops,
>  			      uint64_t surface_size,
>  			      uint64_t surface_count)
>  {
> +	uint64_t sz, pin, total;
>  	void *mem;
> -	uint64_t sz, pin_total;
>  
>  	intel_require_memory(surface_count, surface_size, CHECK_RAM);
>  
> -	sz = surface_size*surface_count;
> -	pin_total = intel_get_total_pinnable_mem();
> -	igt_require(pin_total > sz);
> -
> -	mem = mmap(NULL, pin_total, PROT_READ, MAP_SHARED | MAP_ANON,
> -1, 0);
> +	mem = intel_get_total_pinnable_mem(&total);
>  	igt_assert(mem != MAP_FAILED);
> +	pin = *(uint64_t *)mem;
> +	igt_assert(!munlock(mem, pin));
> +
> +	sz = surface_size * surface_count;
> +	igt_require(pin > sz);
>  
>  	igt_fork(child, 1) {
>  		uint32_t *bo;
>  		uint64_t n;
>  		int ret;
> -		size_t lock = pin_total - sz;
> +		size_t lock = pin - sz;
>  
>  		bo = malloc(surface_count * sizeof(*bo));
>  		igt_assert(bo);
> @@ -186,7 +187,7 @@ static void mlocked_evictions(int fd, struct
> igt_eviction_test_ops *ops,
>  	}
>  	igt_waitchildren();
>  
> -	munmap(mem, pin_total);
> +	munmap(mem, total);
>  }
>  
>  static int swapping_evictions(int fd, struct igt_eviction_test_ops
> *ops,
> diff --git a/tests/i915/i915_suspend.c b/tests/i915/i915_suspend.c
> index 84cb3b490..cd7cf9675 100644
> --- a/tests/i915/i915_suspend.c
> +++ b/tests/i915/i915_suspend.c
> @@ -163,25 +163,14 @@ test_sysfs_reader(bool hibernate)
>  static void
>  test_shrink(int fd, unsigned int mode)
>  {
> -	void *mem;
>  	size_t size;
> +	void *mem;
>  
>  	gem_quiescent_gpu(fd);
>  	intel_purge_vm_caches(fd);
>  
> -	size = intel_get_total_pinnable_mem();
> -	igt_require(size > 64 << 20);
> -	size -= 64 << 20;
> -
> -	mem = mmap(NULL, size, PROT_READ, MAP_SHARED | MAP_ANON, -1,
> 0);
> -
> -	intel_purge_vm_caches(fd);
> -
> -	igt_debug("Locking %'zu B (%'zu MiB)\n",
> -		  size, size >> 20);
> -	igt_assert(!mlock(mem, size));
> -	igt_info("Locked %'zu B (%'zu MiB)\n",
> -		 size, size >> 20);
> +	mem = intel_get_total_pinnable_mem(&size);
> +	igt_assert(mem != MAP_FAILED);
>  
>  	intel_purge_vm_caches(fd);
>  	igt_system_suspend_autoresume(mode, SUSPEND_TEST_NONE);

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Chris Wilson]

Quoting Caz Yokoyama (2019-02-27 17:34:57)
> inline.
> 
> v2 patches fixes
> - address calculation bug
> - not killed
> However, the test does not runs faster.

Fair enough, I expected some improvement from the incremental and
avoiding the second mlock + populate of a large region, but if it's not
significant, then let's just make oomkiller faster ;)
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Incrementally mlock()

[Caz Yokoyama]

Reviewed-by: Caz Yokoyama <caz.yokoyama@intel.com>

Correction of my test report: The patched test runs less than 7 sec on
my nuc. 10 times faster.
-caz

On Wed, 2019-02-27 at 16:29 +0000, Chris Wilson wrote:
> As we already have the previous portion of the mmap mlocked, we only
> need to mlock() the fresh portion for testing available memory.
> 
> v2: Fixup the uint64_t pointer arithmetric and only use a single mmap
> to
> avoid subsequent mlock fail (for reasons unknown, but bet on mm/).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Caz Yokoyama <Caz.Yokoyama@intel.com>
> ---
>  lib/igt_aux.h             |  2 +-
>  lib/intel_os.c            | 40 +++++++++++++++++++++--------------
> ----
>  tests/eviction_common.c   | 17 +++++++++--------
>  tests/i915/i915_suspend.c | 17 +++--------------
>  4 files changed, 35 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index fb02c026a..09b6246bf 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -194,7 +194,7 @@ void intel_purge_vm_caches(int fd);
>  uint64_t intel_get_avail_ram_mb(void);
>  uint64_t intel_get_total_ram_mb(void);
>  uint64_t intel_get_total_swap_mb(void);
> -size_t intel_get_total_pinnable_mem(void);
> +void *intel_get_total_pinnable_mem(size_t *pinned);
>  
>  int __intel_check_memory(uint64_t count, uint64_t size, unsigned
> mode,
>  			 uint64_t *out_required, uint64_t *out_total);
> diff --git a/lib/intel_os.c b/lib/intel_os.c
> index e1e31e230..dd93bea1a 100644
> --- a/lib/intel_os.c
> +++ b/lib/intel_os.c
> @@ -227,11 +227,9 @@ intel_get_total_swap_mb(void)
>   *
>   * Returns: Amount of memory that can be safely pinned, in bytes.
>   */
> -size_t
> -intel_get_total_pinnable_mem(void)
> +void *intel_get_total_pinnable_mem(size_t *total)
>  {
>  	uint64_t *can_mlock, pin, avail;
> -	size_t ret;
>  
>  	pin = (intel_get_total_ram_mb() + 1) << 20;
>  	avail = (intel_get_avail_ram_mb() + 1) << 20;
> @@ -245,34 +243,40 @@ intel_get_total_pinnable_mem(void)
>  	 */
>  	*can_mlock = (avail >> 1) + (avail >> 2);
>  	if (mlock(can_mlock, *can_mlock)) {
> -		*can_mlock = 0;
> -		goto out;
> +		munmap(can_mlock, pin);
> +		return MAP_FAILED;
>  	}
>  
>  	for (uint64_t inc = 1024 << 20; inc >= 4 << 10; inc >>= 2) {
> -		igt_debug("Testing mlock %'"PRIu64" B (%'"PRIu64"
> MiB)\n",
> -			  *can_mlock, *can_mlock >> 20);
> +		uint64_t locked = *can_mlock;
> +
> +		igt_debug("Testing mlock %'"PRIu64"B (%'"PRIu64"MiB) +
> %'"PRIu64"B\n",
> +			  locked, locked >> 20, inc);
>  
>  		igt_fork(child, 1) {
> -			for (uint64_t bytes = *can_mlock;
> -			     bytes <= pin;
> -			     bytes += inc) {
> -				if (mlock(can_mlock, bytes))
> +			uint64_t bytes = *can_mlock;
> +
> +			while (bytes <= pin) {
> +				if (mlock((void *)can_mlock + bytes,
> inc))
>  					break;
>  
> -				*can_mlock = bytes;
> +				*can_mlock = bytes += inc;
>  				__sync_synchronize();
>  			}
>  		}
>  		__igt_waitchildren();
> -		igt_assert(!mlock(can_mlock, *can_mlock));
> -	}
>  
> -out:
> -	ret = *can_mlock;
> -	munmap(can_mlock, pin);
> +		if (*can_mlock > locked + inc) { /* Weird bit of mm/
> lore */
> +			*can_mlock -= inc;
> +			igt_debug("Claiming mlock %'"PRIu64"B
> (%'"PRIu64"MiB)\n",
> +				  *can_mlock, *can_mlock >> 20);
> +			igt_assert(!mlock((void *)can_mlock + locked,
> +					  *can_mlock - locked));
> +		}
> +	}
>  
> -	return ret;
> +	*total = pin;
> +	return can_mlock;
>  }
>  
>  static uint64_t vfs_file_max(void)
> diff --git a/tests/eviction_common.c b/tests/eviction_common.c
> index 321772ba7..a3b9e4167 100644
> --- a/tests/eviction_common.c
> +++ b/tests/eviction_common.c
> @@ -133,23 +133,24 @@ static void mlocked_evictions(int fd, struct
> igt_eviction_test_ops *ops,
>  			      uint64_t surface_size,
>  			      uint64_t surface_count)
>  {
> +	uint64_t sz, pin, total;
>  	void *mem;
> -	uint64_t sz, pin_total;
>  
>  	intel_require_memory(surface_count, surface_size, CHECK_RAM);
>  
> -	sz = surface_size*surface_count;
> -	pin_total = intel_get_total_pinnable_mem();
> -	igt_require(pin_total > sz);
> -
> -	mem = mmap(NULL, pin_total, PROT_READ, MAP_SHARED | MAP_ANON,
> -1, 0);
> +	mem = intel_get_total_pinnable_mem(&total);
>  	igt_assert(mem != MAP_FAILED);
> +	pin = *(uint64_t *)mem;
> +	igt_assert(!munlock(mem, pin));
> +
> +	sz = surface_size * surface_count;
> +	igt_require(pin > sz);
>  
>  	igt_fork(child, 1) {
>  		uint32_t *bo;
>  		uint64_t n;
>  		int ret;
> -		size_t lock = pin_total - sz;
> +		size_t lock = pin - sz;
>  
>  		bo = malloc(surface_count * sizeof(*bo));
>  		igt_assert(bo);
> @@ -186,7 +187,7 @@ static void mlocked_evictions(int fd, struct
> igt_eviction_test_ops *ops,
>  	}
>  	igt_waitchildren();
>  
> -	munmap(mem, pin_total);
> +	munmap(mem, total);
>  }
>  
>  static int swapping_evictions(int fd, struct igt_eviction_test_ops
> *ops,
> diff --git a/tests/i915/i915_suspend.c b/tests/i915/i915_suspend.c
> index 84cb3b490..cd7cf9675 100644
> --- a/tests/i915/i915_suspend.c
> +++ b/tests/i915/i915_suspend.c
> @@ -163,25 +163,14 @@ test_sysfs_reader(bool hibernate)
>  static void
>  test_shrink(int fd, unsigned int mode)
>  {
> -	void *mem;
>  	size_t size;
> +	void *mem;
>  
>  	gem_quiescent_gpu(fd);
>  	intel_purge_vm_caches(fd);
>  
> -	size = intel_get_total_pinnable_mem();
> -	igt_require(size > 64 << 20);
> -	size -= 64 << 20;
> -
> -	mem = mmap(NULL, size, PROT_READ, MAP_SHARED | MAP_ANON, -1,
> 0);
> -
> -	intel_purge_vm_caches(fd);
> -
> -	igt_debug("Locking %'zu B (%'zu MiB)\n",
> -		  size, size >> 20);
> -	igt_assert(!mlock(mem, size));
> -	igt_info("Locked %'zu B (%'zu MiB)\n",
> -		 size, size >> 20);
> +	mem = intel_get_total_pinnable_mem(&size);
> +	igt_assert(mem != MAP_FAILED);
>  
>  	intel_purge_vm_caches(fd);
>  	igt_system_suspend_autoresume(mode, SUSPEND_TEST_NONE);

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] lib: Incrementally mlock()

[Ashutosh Dixit]

On Wed, Feb 27, 2019 at 04:29:50PM +0000, Chris Wilson wrote:
>As we already have the previous portion of the mmap mlocked, we only
>need to mlock() the fresh portion for testing available memory.
>
>v2: Fixup the uint64_t pointer arithmetric and only use a single mmap to
>avoid subsequent mlock fail (for reasons unknown, but bet on mm/).
>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Caz Yokoyama <Caz.Yokoyama@intel.com>

Wondering how this patch was reviewed, tested and merged. The parent
process still dies with 100% certainty rendering the test useless:

# build/tests/i915_suspend --run-subtest shrink
IGT-Version: 1.23-g6be2dc8d (x86_64) (Linux: 5.0.0-rc5+ x86_64)
Starting subtest: shrink
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
child 0 died with signal 9, Killed
Killed

>+		if (*can_mlock > locked + inc) { /* Weird bit of mm/ lore */

What is the meaning of this if check? The parent should mlock the
incremental amount the child has mlocked unconditionally as was done in the
previous version. If the objective was to kludge our way out of the parent
dying that objective doesn't seem to have been met either :(

-Ashutosh
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev