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: <cfdf337a-96dd-e0f8-89f9-1339c5a2579b@intel.com>
Date:   Tue, 16 Aug 2022 10:24:42 +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>
CC:     Mike Rapoport <rppt@kernel.org>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>,
        David Hildenbrand <david@redhat.com>
References: <cover.1660454730.git.remckee0@gmail.com>
 <02d514d86f212c79e52792b8ecd919156d7a9c20.1660454730.git.remckee0@gmail.com>
 <5dde66a7-2236-a890-aaeb-a45876057bec@intel.com>
 <20220816021522.GA3590@sophie>
From:   "Huang, Shaoqin" <shaoqin.huang@intel.com>
In-Reply-To: <20220816021522.GA3590@sophie>
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?QnFoc2Q5N0QrSXVTUTRFZmVyRGk0encvSTFHZm9RcVlOc2xVWXJFa3hJYkxT?=
 =?utf-8?B?UkZqaGFyT0M2a0NXYlVSSmovR2RNUzVnUUJYdTl2eVBudmdHYkxORTMrdmVv?=
 =?utf-8?B?MWtlenJkRmtXZHI4bEpmaTZwN2RpZVFRTWh5aDdEVWJGYlg5b0wwUlYzNGFx?=
 =?utf-8?B?dzRNU2xlRnRWT2Q1KzVxbnZCTUpPbk5kVU80VXV2V2lldnNmeTB0M2FUdEFW?=
 =?utf-8?B?VHFIUHFDVkwzNm1UQ1NTbnFoQ3BzMkFvOVkyUzRjSjNWZDVSWVMrckFYc25o?=
 =?utf-8?B?Q3MrYWpvNXRsQjJ6cDdxNmNSRGpyU213QmRiQklocDZmVHNacjMxVFFLUUxm?=
 =?utf-8?B?a1dhNkZPSjlQVjNHV0FmL0hTMWxycUVLVG5ZZXp0b2FFY1F1MVVyalRzd1Zo?=
 =?utf-8?B?bXBJa3JzUHhmeXh2aFBrdTNGY0xkYnExRTQxTm9sc2h2VXBhR09YNktYY1Rk?=
 =?utf-8?B?M21rSTZ6QVhDYzV3M1kzUVMySCs2TkJyVFltSDk0a3BHZ3UweDFwdFM3RDZD?=
 =?utf-8?B?N21iVXAxTHJxTlk3OW44RjhnVDF5RkdqcmdieDV5Q2pjT2owMmtOQnpOSFZG?=
 =?utf-8?B?RDhBNllkbDNkUjQ5UTd1WjZDOEVoN1I0WnY2a09YK0ZqZFVDSENFUUZrZWg5?=
 =?utf-8?B?RnpnM0xwRENMS01nQkh5MVhnL1NzNncrS0NRN1V1Qk1XTkFsSTk0UzYrTHZJ?=
 =?utf-8?B?cDI1c1lsR1YrS2pYZEZ0N3F2T05NRGNtTU80WE81NW1IL1NleEZjeHovQmp4?=
 =?utf-8?B?bjBWUWlTK3J4dUtBZmlXVFhiQzBFQzNGQ0Fqa1pJZmJhemgxSkZaanNrblRK?=
 =?utf-8?B?OU0zRzFMY3FyMng5TVM5OU5uenJEckZsdU0wTHhqd2NoZ0JwbHpLemk3YTBz?=
 =?utf-8?B?NkF3MXN6UFU3WGNtbU54T3BiTGRVSzFDbjJEOGhzNE15QWQ0dzN2M0I2UjZ5?=
 =?utf-8?B?QS9TWjgrckRUVk8wU25aVzRha0huODdBS2wvbXZEai9jZGphenE4WUtHR1BL?=
 =?utf-8?B?dmRMbHcrRXRScmEwMm5qbWl5UVNyVWxWKy9xb3I4bzhlWWs0amxMdFBVMVoz?=
 =?utf-8?B?cXFHWTFaMThlV24wc1ZaQjR6M2FNT2d2WlVOUXR6MXh1OVVBRUdlVXoxb0VT?=
 =?utf-8?B?QlRKUTRVWWZoWThVTml2L1ZnakJYV05STGo1K3VPUXJ3MGIrQ0hqbUpyU2hF?=
 =?utf-8?B?MzdMcFJLVE0wZlgySzJWMFN0S1RkT2cvem9aOXRYVzlMemMvLzVDUHNKN2gr?=
 =?utf-8?B?SGhZR0NBTGU1N1pTYTF2T3BKM01lTG1vTTJEMTJxamxVcE1RUHB1VGdjZ09i?=
 =?utf-8?B?UHg0d2RLL01wZVI0SXN3am81akJYS0hsYU5ORFp0TlFZd2VlVEZYUFM2bFB6?=
 =?utf-8?B?WUJIQW0vT0ZCUG52NnJwS3pla3lXOThXSWwvWDBNWmtVNGRJM1UydWt5K3Vo?=
 =?utf-8?B?bEREQmZUOHc4OEVRVm1DTjlURnlZVEEybkYySloyZjdMdkN1eHlrQi9EcUVX?=
 =?utf-8?B?Z1kxKytnU3dNb0RyTTJTMWF4NWlpT01XWG1kMGtxeVVlbzB1c1hkWkpibFZv?=
 =?utf-8?B?cWc2VGNkdFZSd0NzMFJaL3FSZGIzRHB3UWhCRkpIcnp1ZkNCbzVIanF2M2V4?=
 =?utf-8?B?djVISTVhRWE4NEYvRGZXUndUa1lwL2Q2WVNJajUxL1B1bU80c1hxSVBuQTBR?=
 =?utf-8?B?cEVSbmZ4YWxmeEF0OThYU2Q2V0ZTT1VVNGxhRU8yYkE1UUUxOEVIOEFYcmJB?=
 =?utf-8?B?bTVCaDRNWno5dXNBY0NYd3N6dUdQcW9xNEZBRnBpdndpMm5DUjBWN1lmZDhI?=
 =?utf-8?B?VGh2dkV0aGJZbHN0a3pwcm1YamVTRms2bmh3WnJ0SG5wY1ZxRDl1dnlTaXZE?=
 =?utf-8?B?c3JGTDBkeXhXSmlSbmFTZ3ZRcFhrRXpENCtGOVB6eXRzWlhHbXhrTVdSM1Ez?=
 =?utf-8?B?WHp1NWtPbFhNeUhNRHk3OEVDbU1NUDNSRXVodDRSazFsdGpwYlQ0OE1wWmht?=
 =?utf-8?B?anQvdzVFVjZ4Wlh3ZWs4aFdSQmxocko5VEc3Y2NEQ2kxRlN4dE9KOGZsQUFO?=
 =?utf-8?B?RDRVQmNRV3pWRVRuNVlrM0wxMjNmbEljVkllbGFhUVJhMXVqdEpHbGRLSlU3?=
 =?utf-8?B?ZW0zajNsYmxpQmZMWS9hQ1ptNk5laGtzNnBSZm1jZGMwWmpwTzFHQmFBNkVH?=
 =?utf-8?B?U2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b70fb11-bc3b-4f5c-3857-08da7f2e7fd9
X-MS-Exchange-CrossTenant-AuthSource: MN2PR11MB3870.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Aug 2022 02:24:51.9792
 (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: deawjs2TM1tDOUb5aLUyQ1uPo1vrvsHXGnxD62skNaZosBpGwRIFzPBxEGkyZR32WjumFYpXs4bfb6RdLNFF2g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR11MB6156
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 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_PASS,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


On 8/16/2022 10:15 AM, Rebecca Mckeever wrote:
> On Mon, Aug 15, 2022 at 04:50:51PM +0800, Huang, Shaoqin wrote:
>> 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.
>>
> Do you mean that it's not necessary since the test doesn't run
> verify_mem_content()?
> 

Yes. And the test function name is alloc_no_memory_...(). So I just 
think no memory operation in it may be same with the function name.

>> 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);