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

[Caz Yokoyama <Caz.Yokoyama@intel.com>]

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