[igt-dev] [igt PATCH v2 1/1] lib: Incrementally mlock()

[igt-dev] [igt PATCH v2 1/1] lib: Incrementally mlock()

[Caz Yokoyama]

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

Address calculation on mlock is fixed.

When the parent process mlocks, i.e.,
  igt_assert(!mlock(can_mlock, *can_mlock));
most likely killed.

In suspend.c, OOM kills when 64MB less memory is mlocked.

This patch does not make the test faster. It runs 300-900sec.
I recommend followings.

1. Run patched i915_suspend@shrink only on full test(shard test ?)
2. add lighter test such as 1) mlock 3/4 of avail memory, 2) suspend-resume

2 constantly runs less than 5 sec. However, it does not suspend-resume
with exhausting all of system memory.

Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
---
 lib/intel_os.c       | 17 +++++++++--------
 tests/i915/suspend.c |  7 +++++--
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/lib/intel_os.c b/lib/intel_os.c
index e1e31e23..44e852ee 100644
--- a/lib/intel_os.c
+++ b/lib/intel_os.c
@@ -250,22 +250,23 @@ 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((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:
diff --git a/tests/i915/suspend.c b/tests/i915/suspend.c
index 84cb3b49..99ab68be 100644
--- a/tests/i915/suspend.c
+++ b/tests/i915/suspend.c
@@ -160,6 +160,9 @@ test_sysfs_reader(bool hibernate)
 	igt_stop_helper(&reader);
 }
 
+#define MB (1 << 20)
+#define LESS_MEM (252*MB)
+
 static void
 test_shrink(int fd, unsigned int mode)
 {
@@ -170,8 +173,8 @@ test_shrink(int fd, unsigned int mode)
 	intel_purge_vm_caches(fd);
 
 	size = intel_get_total_pinnable_mem();
-	igt_require(size > 64 << 20);
-	size -= 64 << 20;
+	igt_require(size > LESS_MEM);
+	size -= LESS_MEM;
 
 	mem = mmap(NULL, size, PROT_READ, MAP_SHARED | MAP_ANON, -1, 0);
 
-- 
2.17.1

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

Re: [igt-dev] [igt PATCH v2 1/1] lib: Incrementally mlock()

[Chris Wilson]

Quoting Caz Yokoyama (2019-02-22 02:03:57)
> As we already have the previous portion of the mmap mlocked, we only
> need to mlock() the fresh portion for testing available memory.

I liked my v2 much better that avoided the repeated mlock.
 
> Address calculation on mlock is fixed.
> 
> When the parent process mlocks, i.e.,
>   igt_assert(!mlock(can_mlock, *can_mlock));
> most likely killed.
> 
> In suspend.c, OOM kills when 64MB less memory is mlocked.
> 
> This patch does not make the test faster. It runs 300-900sec.
> I recommend followings.
> 
> 1. Run patched i915_suspend@shrink only on full test(shard test ?)
> 2. add lighter test such as 1) mlock 3/4 of avail memory, 2) suspend-resume

That's completely missing the point. The point of the test is what
happens if we hit oom during suspend. If nothing happens during suspend,
it is just yet another suspend.

The complaint for the test was the sheer volume of dmesg spam along with
regularly locking up the system (and if we aren't locking up the system,
the test isn't doing its job...)
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [igt PATCH v2 1/1] lib: Incrementally mlock()

[Ashutosh Dixit]

On Thu, Feb 21 2019 at 07:03:57 PM, Caz Yokoyama <caz.yokoyama@intel.com> wrote:
>  	__igt_waitchildren();
> -	igt_assert(!mlock(can_mlock, *can_mlock));

This is also wrong. The parent process needs to lock the memory for the
calculation of the amount of locked memory to be correct. Also explains
why this tests runs for so long with this patch.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [igt PATCH v2 1/1] lib: Incrementally mlock()

[Caz Yokoyama]

On Fri, 2019-02-22 at 08:12 -0800, Ashutosh Dixit wrote:
> On Thu, Feb 21 2019 at 07:03:57 PM, Caz Yokoyama <
> caz.yokoyama@intel.com> wrote:
> >  	__igt_waitchildren();
> > -	igt_assert(!mlock(can_mlock, *can_mlock));
> 
> This is also wrong. The parent process needs to lock the memory for
> the
> calculation of the amount of locked memory to be correct. Also 
I answered in my commit message as below.

When the parent process mlocks, i.e.,
  igt_assert(!mlock(can_mlock, *can_mlock));
most likely killed.

"most likely" is not correct". "100% of probability" is. 

With the bug, both parent and child mlock 8 times far beyond location.
mlock() may fail because it may beyond mmaped area.

With fixing the bug, the child updates (*can_mlock). Then the parent
mlocks(*can_mlock) and killed by OOM.

Only solution I found is 1) the parent does not mlock(*can_mlock),
2) mlock 252mb less memory.

BTW, dmesg has

[ 4605.251250]
[   8318]     0  8318   497037   315802  2768896      194          1000
i915_suspend
[ 4605.251252]
[   8320]     0  8320   497037   103550  1056768      194          1000
i915_suspend
[ 4605.251254] Out of memory: Kill process 8318 (i915_suspend) score
1317 or sacrifice child
[ 4605.251267] Killed process 8320 (i915_suspend) total-vm:1988148kB,
anon-rss:0kB, file-rss:0kB, shmem-rss:414200kB
[ 4605.251470] oom_reaper: reaped process 8320 (i915_suspend), now
anon-rss:0kB, file-rss:0kB, shmem-rss:414200kB
[ 4605.251560] Purging GPU memory, 0 pages freed, 2327 pages still
pinned.
[ 4605.251563] 1 and 0 pages still available in the bound and unbound
GPU page lists.
 
> explains
> why this tests runs for so long with this patch.
Correction. It runs 7-9 sec. I did not kill X server.

I'll do no further investigation for this test because this is
disabled.
-caz


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

Re: [igt-dev] [igt PATCH v2 1/1] lib: Incrementally mlock()

[Caz Yokoyama]

As we discussed on the mailng list, please drop my patch.
-caz

On Fri, 2019-02-22 at 02:03 +0000, Caz Yokoyama wrote:
> As we already have the previous portion of the mmap mlocked, we only
> need to mlock() the fresh portion for testing available memory.
> 
> Address calculation on mlock is fixed.
> 
> When the parent process mlocks, i.e.,
>   igt_assert(!mlock(can_mlock, *can_mlock));
> most likely killed.
> 
> In suspend.c, OOM kills when 64MB less memory is mlocked.
> 
> This patch does not make the test faster. It runs 300-900sec.
> I recommend followings.
> 
> 1. Run patched i915_suspend@shrink only on full test(shard test ?)
> 2. add lighter test such as 1) mlock 3/4 of avail memory, 2) suspend-
> resume
> 
> 2 constantly runs less than 5 sec. However, it does not suspend-
> resume
> with exhausting all of system memory.
> 
> Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
> ---
>  lib/intel_os.c       | 17 +++++++++--------
>  tests/i915/suspend.c |  7 +++++--
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/intel_os.c b/lib/intel_os.c
> index e1e31e23..44e852ee 100644
> --- a/lib/intel_os.c
> +++ b/lib/intel_os.c
> @@ -250,22 +250,23 @@ 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((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:
> diff --git a/tests/i915/suspend.c b/tests/i915/suspend.c
> index 84cb3b49..99ab68be 100644
> --- a/tests/i915/suspend.c
> +++ b/tests/i915/suspend.c
> @@ -160,6 +160,9 @@ test_sysfs_reader(bool hibernate)
>  	igt_stop_helper(&reader);
>  }
>  
> +#define MB (1 << 20)
> +#define LESS_MEM (252*MB)
> +
>  static void
>  test_shrink(int fd, unsigned int mode)
>  {
> @@ -170,8 +173,8 @@ test_shrink(int fd, unsigned int mode)
>  	intel_purge_vm_caches(fd);
>  
>  	size = intel_get_total_pinnable_mem();
> -	igt_require(size > 64 << 20);
> -	size -= 64 << 20;
> +	igt_require(size > LESS_MEM);
> +	size -= LESS_MEM;
>  
>  	mem = mmap(NULL, size, PROT_READ, MAP_SHARED | MAP_ANON, -1,
> 0);
>  

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