Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gabriel Krisman Bertazi <krisman@collabora.com>
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Shuah Khan <shuah@kernel.org>, kernel@collabora.com,
        david@redhat.com, Will Deacon <will@kernel.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        linux-kselftest@vger.kernel.org
Subject: Re: [PATCH V5 2/2] selftests: vm: Add test for Soft-Dirty PTE bit
Organization: Collabora
References: <20220317103323.94799-1-usama.anjum@collabora.com>
        <20220317103323.94799-2-usama.anjum@collabora.com>
Date:   Fri, 18 Mar 2022 15:23:43 -0400
In-Reply-To: <20220317103323.94799-2-usama.anjum@collabora.com> (Muhammad
        Usama Anjum's message of "Thu, 17 Mar 2022 15:33:22 +0500")
Message-ID: <87ilsbyshs.fsf@collabora.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Muhammad Usama Anjum <usama.anjum@collabora.com> writes:

> new file mode 100644
> index 0000000000000..3153ebac6909b
> --- /dev/null
> +++ b/tools/testing/selftests/vm/soft-dirty.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdbool.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <malloc.h>
> +#include <sys/mman.h>
> +#include "../kselftest.h"
> +#include "vm_util.h"
> +
> +#define PAGEMAP_FILE_PATH "/proc/self/pagemap"
> +#define TEST_ITERATIONS 10000
> +
> +static void test_simple(int pagemap_fd, int pagesize)
> +{
> +	int i;
> +	char *map;
> +
> +	map = aligned_alloc(pagesize, pagesize);
> +	if (!map)
> +		ksft_exit_fail_msg("mmap failed\n");
> +
> +	clear_softdirty();
> +
> +	for (i = 0 ; i < TEST_ITERATIONS; i++) {
> +		if (pagemap_is_softdirty(pagemap_fd, map) == 1) {
> +			ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
> +			break;
> +		}
> +
> +		clear_softdirty();
> +		// Write something to the page to get the dirty bit enabled on the page
> +		map[0] = i % 255;

you don't need this mod at all but at least it should be 256 :).  I think
Either 'map[0] = !map[0]' or keeping the original 'map[0]++' is fine.

> +
> +		if (pagemap_is_softdirty(pagemap_fd, map) == 0) {
> +			ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i);
> +			break;
> +		}
> +
> +		clear_softdirty();
> +	}
> +	free(map);
> +
> +	ksft_test_result(i == TEST_ITERATIONS, "Test %s\n", __func__);
> +}
> +
> +static void test_vma_reuse(int pagemap_fd, int pagesize)
> +{
> +	char *map, *map2;
> +
> +	map = mmap(NULL, pagesize, (PROT_READ | PROT_WRITE), (MAP_PRIVATE | MAP_ANON), -1, 0);
> +	if (map == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap failed");
> +
> +	clear_softdirty();
> +
> +	/* Write to the page before unmapping and map the same size region again to check
> +	 * if same memory region is gotten next time and if dirty bit is preserved across
> +	 * this type of allocations.
> +	 */

This reads weird.  It should *not* be preserved across different
mappings.  Also, we are not testing if the same region is reused, we are
depending on it to test the sd bit.

/* Ensures the soft-dirty bit is reset accross different mappings on the
same address.  */

> +	map[0]++;

This is inconsistent with the other two tests.

> +
> +	munmap(map, pagesize);
> +
> +	map2 = mmap(NULL, pagesize, (PROT_READ | PROT_WRITE), (MAP_PRIVATE | MAP_ANON), -1, 0);
> +	if (map2 == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap failed");
> +
> +	ksft_test_result(map == map2, "Test %s reused memory location\n", __func__);

if map != map2, the test itself is broken, meaning we should skip it, not
fail, i guess.

-- 
Gabriel Krisman Bertazi