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;
Message-ID: <5dde66a7-2236-a890-aaeb-a45876057bec@intel.com>
Date:   Mon, 15 Aug 2022 16:50:51 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.12.0
Subject: Re: [PATCH 1/8] memblock tests: update tests to check if
 memblock_alloc zeroed memory
Content-Language: en-US
To:     Rebecca Mckeever <remckee0@gmail.com>,
        Mike Rapoport <rppt@kernel.org>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>
CC:     David Hildenbrand <david@redhat.com>
References: <cover.1660454730.git.remckee0@gmail.com>
 <02d514d86f212c79e52792b8ecd919156d7a9c20.1660454730.git.remckee0@gmail.com>
From:   "Huang, Shaoqin" <shaoqin.huang@intel.com>
In-Reply-To: <02d514d86f212c79e52792b8ecd919156d7a9c20.1660454730.git.remckee0@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dU1kVnJRQ05qSFNmK1dxOWFyVUVIbUs1YmlYWndnNEdwUlorTUVTQTZJMFRD?=
 =?utf-8?B?Q05WUXVFRUxmNWJwRUExVDJJOWVmM25BRHNyakxVV1lhZTdlT21NNThyalVy?=
 =?utf-8?B?OFRtM3lydjRpeWtWT3hGKzBTeGZWWVJJNVBLZUZIemlOUzB6RFZ6NkFxdm5M?=
 =?utf-8?B?RlRNajFQK0Vxeld5RlBXNWN6dXFHNEJPUWMyQS9oZjdCcHptQmJMV0VJdXlQ?=
 =?utf-8?B?UlZvOVlsZXFhSU9LSi9hamZNRFg3cElEVXllUkdXSm5HWHo1UlE3eENsWi9B?=
 =?utf-8?B?UzRZN0tJbzZEM0hTVmUxTHJoVVIrWnAvRHZnMFpPMkRJRHY1U2VlVkRFbUwr?=
 =?utf-8?B?WkttakZnd01VeEFEMGdpd01lRnBzd3JWUlhnc05rY0Z1bUY4U2FyYWdkVHVi?=
 =?utf-8?B?M0VaNStDVnZXWjQybWJoV2owUStWUlloRUVtNUlGdUVRODJwdXdxaFFPOFRK?=
 =?utf-8?B?QVdJR0NWenU2SittcGU2VzFxcVo3eFJiQmt6RDF3WkdyamtJOC9vSE4wUGVC?=
 =?utf-8?B?RGJzVWNOaTdoL09jT3M5b3pNTXJXMHZqUDhOZ3ZXSzVvbUUxUnNSbHAybTBa?=
 =?utf-8?B?WWtBaXpad2l3MW0wUFVYSWtCZSs1SnVrUm9ySmRLcjRtM3BCMGxuSlkvdXIy?=
 =?utf-8?B?UVFBQkVlZkFscVZqQkpPZ2llbG1jZEZON253QjZCS1RaNldDYVVZN0J1Qytt?=
 =?utf-8?B?VVFtanB1QWlOK21lS3B0YUFLQ05GMXd1Y3RWazNENmQrMWI0ZjlVWVJuK1JS?=
 =?utf-8?B?MVRFRHJrQWp5cFE3aEZ5L1RtL0w4UmMxUG1xaWFIb1JxNEp4eTlOOTRjTGZp?=
 =?utf-8?B?R01zVWpicGg1Szg3Qmt2MEJMZjVNb0NSV3Znc3A0dWozV25hZEdwa2QxQVpO?=
 =?utf-8?B?dlJLK2dUN0lKb1NrQ3VxWUJ5Y3RyV2Y0NlhZWlUzNVF1M3N4ZWs0K29MYWYy?=
 =?utf-8?B?MjlmZTJndUVtVzl2NmVVQkZrbWw0WFB1UFRYWVE2WGlsRjNIT2QrZnIyMVNO?=
 =?utf-8?B?MW5GQUpOSnAvWWJseE1DS2Exbm5BdldqdTdkakdmN3pSSjUyNVpQci9LTjZD?=
 =?utf-8?B?aHJHYmM2b3RaUVBGNTkrWno0cHFQclBLaU0rR3lQaWZYc21WKy81RXhIME1C?=
 =?utf-8?B?aUNzV1FuNnhQVVRXZzczbzFoa2hhN29wcWh5dWYwei9rSy93b2RrS2YvajBi?=
 =?utf-8?B?RDlCM2RscXdmck5CZEhWa21tdnYzRk5TMWlOa1JGdktDRVJRa05nVmRvTHhT?=
 =?utf-8?B?Q0NrU3pvM2tNd0I2YUVwUWNpV0tKWjk1dFRxbE1qcE5QV3pudGJ4YWxVbk05?=
 =?utf-8?B?NG8vSzh4cTZtb3gxcTNRTTVFNFlMd0hwbE9mSmlpcnhXYm9MN1dOcTdIVkZw?=
 =?utf-8?B?aWtib3lPbnBrSFYrc01nRHB2cDZVSVBmbWhBL1FsQ1ZlOHovbm9XcDJ2WUpt?=
 =?utf-8?B?dFN1RHFPTUFjbTdDbFdFNUNydGhzcGhSVDVoYlluTGFVb0V3NHJWUG44TnJh?=
 =?utf-8?B?UG5FU1pFbEdvcHBUaG50a3hiZUZOdzhzTFFRUjNLS0JRZGpaMXVQTEMzY1Fp?=
 =?utf-8?B?K3BlQmlvdVVCU29BWCtZODUwVWNzQlFXWHdUNmc3U2paZm5hbVVXZithays1?=
 =?utf-8?B?dmVVRyt3OGFJRWlleC8xOTN0L0ViRVMzbmw2VTdlL1A3UlhMSkF3UjU1VnFO?=
 =?utf-8?B?d29NYmhFbWlVb0lFWStkMXRYYmVnNEF5eEFWQkRaN3RnYlhtM1c2ZWZ6a3JF?=
 =?utf-8?B?WHh3ZW9yK2NvdGlOZ0pTL3Y2VHZFeGlibFgxTWxmQ2JVdUxLZlpjVktjcktW?=
 =?utf-8?B?eXRlZE1rYk5mTjQ1L0ZlbDh0b2tlRGIrN0s1TkZxNUh1eGg2bmdVbU5ydjBw?=
 =?utf-8?B?SUJBcGE1UHQvNlpQT0ttMGtyc3RaRi9DTjVPYlY5L29UKzRXYjFzVnJRRXM3?=
 =?utf-8?B?dXVqN3JQb2ZmNTdZcWdhcE9RNXlzNU9xYmJBcW95UlhWTjFWQjRiMVlOWURl?=
 =?utf-8?B?RlU4NVE5emFXOG93bUNqMEdHZXRSUUpaSTBTMjBVcTJSbzFqT1VtTE5xOHlu?=
 =?utf-8?B?YnJBS01DTDQzNEY2dExxc1BUSElMTjQxV2UxTE9xUDlMaDNPTThIOTZJMmlI?=
 =?utf-8?B?VzRaSU5WVUk0SDlGNS9YTTRtbDEva0J2MnRXSVA3dndQemowTXBpZXhJL1d4?=
 =?utf-8?B?dXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b246dc20-47cd-4ed5-de96-08da7e9b46f3
X-MS-Exchange-CrossTenant-AuthSource: MN2PR11MB3870.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Aug 2022 08:51:00.5358
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Cy3/SyHQauiawVn604QKK3Hr26rzVWpbAnsb78GChshfRcZrr8qVD1MUa6SRaL1Xy3txpDxSnEdx8G74D3eJKA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB5713
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi, Rebecca.

On 8/14/2022 1:53 PM, Rebecca Mckeever wrote:
> Add an assert in memblock_alloc() tests where allocation is expected to
> occur. The assert checks whether the entire chunk of allocated memory is
> cleared.
> 
> The current memblock_alloc() tests do not check whether the allocated
> memory was zeroed. memblock_alloc() should zero the allocated memory since
> it is a wrapper for memblock_alloc_try_nid().
> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>   tools/testing/memblock/tests/alloc_api.c | 24 ++++++++++++++++++++++++
>   tools/testing/memblock/tests/common.c    |  7 +++++++
>   tools/testing/memblock/tests/common.h    | 13 +++++++++++++
>   3 files changed, 44 insertions(+)
> 
> diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
> index a14f38eb8a89..71c89cb9b2a8 100644
> --- a/tools/testing/memblock/tests/alloc_api.c
> +++ b/tools/testing/memblock/tests/alloc_api.c
> @@ -22,6 +22,8 @@ static int alloc_top_down_simple_check(void)
>   	allocated_ptr = memblock_alloc(size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, size);
> +
>   	ASSERT_EQ(rgn->size, size);
>   	ASSERT_EQ(rgn->base, expected_start);
>   
> @@ -80,6 +82,8 @@ static int alloc_top_down_disjoint_check(void)
>   	allocated_ptr = memblock_alloc(r2_size, alignment);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r2_size);
> +
>   	ASSERT_EQ(rgn1->size, r1.size);
>   	ASSERT_EQ(rgn1->base, r1.base);
>   
> @@ -125,6 +129,8 @@ static int alloc_top_down_before_check(void)
>   	allocated_ptr = memblock_alloc(r2_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r2_size);
> +
>   	ASSERT_EQ(rgn->size, total_size);
>   	ASSERT_EQ(rgn->base, memblock_end_of_DRAM() - total_size);
>   
> @@ -173,6 +179,8 @@ static int alloc_top_down_after_check(void)
>   	allocated_ptr = memblock_alloc(r2_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r2_size);
> +
>   	ASSERT_EQ(rgn->size, total_size);
>   	ASSERT_EQ(rgn->base, r1.base - r2_size);
>   
> @@ -223,6 +231,8 @@ static int alloc_top_down_second_fit_check(void)
>   	allocated_ptr = memblock_alloc(r3_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r3_size);
> +
>   	ASSERT_EQ(rgn->size, r2.size + r3_size);
>   	ASSERT_EQ(rgn->base, r2.base - r3_size);
>   
> @@ -277,6 +287,8 @@ static int alloc_in_between_generic_check(void)
>   	allocated_ptr = memblock_alloc(r3_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r3_size);
> +
>   	ASSERT_EQ(rgn->size, total_size);
>   	ASSERT_EQ(rgn->base, r1.base - r2.size - r3_size);
>   
> @@ -418,6 +430,8 @@ static int alloc_limited_space_generic_check(void)
>   	allocated_ptr = memblock_alloc(available_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, available_size);
> +
>   	ASSERT_EQ(rgn->size, MEM_SIZE);
>   	ASSERT_EQ(rgn->base, memblock_start_of_DRAM());
>   
> @@ -442,6 +456,7 @@ static int alloc_no_memory_generic_check(void)
>   	PREFIX_PUSH();
>   
>   	reset_memblock_regions();
> +	fill_memblock();

Maybe we don't need this line, it has no effect at here. Anyway, Others 
Looks Good to me.

Reviewed-by: Shaoqin Huang <shaoqin.huang@intel.com>

>   
>   	allocated_ptr = memblock_alloc(SZ_1K, SMP_CACHE_BYTES);
>   
> @@ -472,6 +487,8 @@ static int alloc_bottom_up_simple_check(void)
>   	allocated_ptr = memblock_alloc(SZ_2, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, SZ_2);
> +
>   	ASSERT_EQ(rgn->size, SZ_2);
>   	ASSERT_EQ(rgn->base, memblock_start_of_DRAM());
>   
> @@ -528,6 +545,7 @@ static int alloc_bottom_up_disjoint_check(void)
>   	allocated_ptr = memblock_alloc(r2_size, alignment);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r2_size);
>   
>   	ASSERT_EQ(rgn1->size, r1.size);
>   	ASSERT_EQ(rgn1->base, r1.base);
> @@ -571,6 +589,8 @@ static int alloc_bottom_up_before_check(void)
>   	allocated_ptr = memblock_alloc(r1_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r1_size);
> +
>   	ASSERT_EQ(rgn->size, total_size);
>   	ASSERT_EQ(rgn->base, memblock_start_of_DRAM());
>   
> @@ -618,6 +638,8 @@ static int alloc_bottom_up_after_check(void)
>   	allocated_ptr = memblock_alloc(r2_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r2_size);
> +
>   	ASSERT_EQ(rgn->size, total_size);
>   	ASSERT_EQ(rgn->base, r1.base);
>   
> @@ -669,6 +691,8 @@ static int alloc_bottom_up_second_fit_check(void)
>   	allocated_ptr = memblock_alloc(r3_size, SMP_CACHE_BYTES);
>   
>   	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, r3_size);
> +
>   	ASSERT_EQ(rgn->size, r2.size + r3_size);
>   	ASSERT_EQ(rgn->base, r2.base);
>   
> diff --git a/tools/testing/memblock/tests/common.c b/tools/testing/memblock/tests/common.c
> index 76a8ad818f3a..0ca26fe12c38 100644
> --- a/tools/testing/memblock/tests/common.c
> +++ b/tools/testing/memblock/tests/common.c
> @@ -60,16 +60,23 @@ void reset_memblock_attributes(void)
>   	memblock.current_limit	= MEMBLOCK_ALLOC_ANYWHERE;
>   }
>   
> +void fill_memblock(void)
> +{
> +	memset(memory_block.base, 1, MEM_SIZE);
> +}
> +
>   void setup_memblock(void)
>   {
>   	reset_memblock_regions();
>   	memblock_add((phys_addr_t)memory_block.base, MEM_SIZE);
> +	fill_memblock();
>   }
>   
>   void dummy_physical_memory_init(void)
>   {
>   	memory_block.base = malloc(MEM_SIZE);
>   	assert(memory_block.base);
> +	fill_memblock();
>   }
>   
>   void dummy_physical_memory_cleanup(void)
> diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h
> index d396e5423a8e..7a16a7dc8f2c 100644
> --- a/tools/testing/memblock/tests/common.h
> +++ b/tools/testing/memblock/tests/common.h
> @@ -51,6 +51,18 @@
>   	assert((_expected) < (_seen)); \
>   } while (0)
>   
> +/**
> + * ASSERT_MEM_EQ():
> + * Check that the first @_size bytes of @_seen are all equal to @_expected.
> + * If false, print failed test message (if running with --verbose) and then
> + * assert.
> + */
> +#define ASSERT_MEM_EQ(_seen, _expected, _size) do { \
> +	for (int _i = 0; _i < (_size); _i++) { \
> +		ASSERT_EQ((_seen)[_i], (_expected)); \
> +	} \
> +} while (0)
> +
>   #define PREFIX_PUSH() prefix_push(__func__)
>   
>   /*
> @@ -70,6 +82,7 @@ struct region {
>   
>   void reset_memblock_regions(void);
>   void reset_memblock_attributes(void);
> +void fill_memblock(void);
>   void setup_memblock(void);
>   void dummy_physical_memory_init(void);
>   void dummy_physical_memory_cleanup(void);