On Mon, Aug 27, 2018 at 01:56:07PM +0800, Baoquan He wrote:
>On 08/07/18 at 02:50pm, Chao Fan wrote:
>> If 'CONFIG_MEMORY_HOTREMOVE' specified and the account of immovable
>If CONFIG_MEMORY_HOTREMOVE is enabled,
>> memory regions is not zero. Calculate the intersection between memory
>> regions from e820/efi memory table and immovable memory regions.
>> Or go on the old code.
>>
>> Rename process_mem_region to slots_count to match slots_fetch_random,
>> and name new function as process_mem_region.
>>
>> Signed-off-by: Chao Fan <fanc.f...@cn.fujitsu.com>
>> ---
>> arch/x86/boot/compressed/kaslr.c | 66 ++++++++++++++++++++++++++------
>> 1 file changed, 55 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/kaslr.c
>> b/arch/x86/boot/compressed/kaslr.c
>> index 720878f967a3..9c6e24a23a2d 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -635,9 +635,9 @@ static unsigned long slots_fetch_random(void)
>> return 0;
>> }
>>
>> -static void process_mem_region(struct mem_vector *entry,
>> - unsigned long minimum,
>> - unsigned long image_size)
>> +static void slots_count(struct mem_vector *entry,
>> + unsigned long minimum,
>> + unsigned long image_size)
>> {
>> struct mem_vector region, overlap;
>> struct slot_area slot_area;
>> @@ -714,6 +714,56 @@ static void process_mem_region(struct mem_vector *entry,
>> }
>> }
>>
>> +static bool process_mem_region(struct mem_vector *region,
>> + unsigned long long minimum,
>> + unsigned long long image_size)
>> +{
>
>Is it possible to take num_immovable_mem definition out from #ifdef
>CONFIG_MEMORY_HOTREMOVE block and check it here like below? This way,
>one level of indentation can be reduced in the for loop, and code is
>more readable.
>
I think there is a mistake.
The logical is:
if (#ifdef CONFIG_MEMORY_HOTREMOVE) && (num_immovable_mem > 0)
then A;
else
then B;
But below is:
if (num_immovable_mem > 0)
then B;
else if (#ifdef CONFIG_MEMORY_HOTREMOVE)
then A;
else
nothing;
The precondition of the loop is (num_immovable_mem > 0), because
there is only one condition that we need go the A code:
CONFIG_MEMORY_HOTREMOVE is defined, and memory information in srat
found.
But there is many conditions we go the B code:
1. CONFIG_MEMORY_HOTREMOVE is not defined.
2. CONFIG_MEMORY_HOTREMOVE defined, but we didn't get the right acpi tables
3. CONFIG_MEMORY_HOTREMOVE defined, or there is only one node in this machine.
Yes, the code is hard to read, but you have changed the logical, there
is a compromise method, I don't know whether is better:
#ifdef CONFIG_MEMORY_HOTREMOVE
if (num_immovable_mem == 0)
goto B;
for (i = 0; i < num_immovable_mem; i++) {
...
}
#endif
B:
slots_count(region, minimum, image_size);
if (slot_area_index == MAX_SLOT_AREA) {
debug_putstr("Aborted e820/efi memmap scan (slot_areas
full)!\n");
return 1;
}
return 0;
>
>static bool process_mem_region(struct mem_vector *region,
> unsigned long long minimum,
> unsigned long long image_size)
>{
>
> /*
> * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
> * walk all the regions, so use region directely.
> */
> if (num_immovable_mem > 0) {
> slots_count(region, minimum, image_size);
>
> if (slot_area_index == MAX_SLOT_AREA) {
> debug_putstr("Aborted e820/efi memmap scan (slot_areas
> full)!\n");
> return 1;
> }
> return 0;
> }
>
>#ifdef CONFIG_MEMORY_HOTREMOVE
> for (i = 0; i < num_immovable_mem; i++) {
> ...
> }
>#endif
>}
>
