On 2023/7/24 14:11, David Hildenbrand wrote:
> On 24.07.23 07:54, Anshuman Khandual wrote:
>>
>>
>> On 7/24/23 06:55, mawupeng wrote:
>>>
>>> On 2023/7/21 18:36, Will Deacon wrote:
>>>> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote:
>>>>> From: Ma Wupeng <mawupeng1@xxxxxxxxxx>
>>>>>
>>>>> During our test, we found that kernel page table may be unexpectedly
>>>>> cleared with rodata off. The root cause is that the kernel page is
>>>>> initialized with pud size(1G block mapping) while offline is memory
>>>>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added,
>>>>> when offline a memory block, the call trace is shown below,
>
> Is someone adding memory in 2 GiB granularity and then removing parts of it in 128 MiB granularity? That would be against what we support using the add_memory() / offline_and_remove_memory() API and that driver should be fixed instead.
Yes, this kind of situation.
The problem occurs in the following scenarios:
1. use mem=xxG to reserve memory.
2. add_momory to online memory.
3. offline part of the memroy via offline_and_remove_memory.
During my research, ACPI memory removal use memory_subsys_offline to offline memory section and
this will not delete page table entry which do not trigger this kind of problem.
So I understand what you are talking about.
1. 3rd-party driver shouldn't use add_memory/offline_and_remove_memory to online/offline memory.
If it have to use, this can be achieved by driver.
2. memory_subsys_offline is perfered to do such thing.
Should we update the doc to describe this kind of limitation?
>
> Or does this trigger only when a hotplugged memory block falls into the same 2 GiB area as boot memor>
>>>>>
>>>>> offline_and_remove_memory
>>>>> try_remove_memory
>>>>> arch_remove_memory
>>>>> __remove_pgd_mapping
>>>>> unmap_hotplug_range
>>>>> unmap_hotplug_p4d_range
>>>>> unmap_hotplug_pud_range
>>>>> if (pud_sect(pud))
>>>>> pud_clear(pudp);
>
> Which drivers triggers that? In-tree is only virtio-mem and dax/kmem. Both add and remove memory in the same granularity.
It is 3rd-party driver. which use try to offline part of(128M) movable memory and this lead to the problem.
>
> For example, virtio-mem will only call add_memory(memory_block_size()) to then offline_and_remove_memory(memory_block_size()).
>
> Could that trigger it as well?
>
>>>> Sorry, but I'm struggling to understand the problem here. If we're adding
>>>> and removing a 2G memory region, why _wouldn't_ we want to use large 1GiB
>>>> mappings?
>>>
>>>> Or are you saying that only a subset of the memory is removed,
>>>> but we then accidentally unmap the whole thing?
>>> Yes, umap a subset but the whole thing page table entry is removed.
>>>
>
> Can we have some more details about the user and how to trigger it?
>
>>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>>> index 95d360805f8a..44c724ce4f70 100644
>>>>> --- a/arch/arm64/mm/mmu.c
>>>>> +++ b/arch/arm64/mm/mmu.c
>>>>> @@ -44,6 +44,7 @@
>>>>> #define NO_BLOCK_MAPPINGS BIT(0)
>>>>> #define NO_CONT_MAPPINGS BIT(1)
>>>>> #define NO_EXEC_MAPPINGS BIT(2) /* assumes FEAT_HPDS is not used */
>>>>> +#define NO_PUD_MAPPINGS BIT(3)
>>>>> int idmap_t0sz __ro_after_init;
>>>>> @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end,
>>>>> */
>>>>> if (pud_sect_supported() &&
>>>>> ((addr | next | phys) & ~PUD_MASK) == 0 &&
>>>>> - (flags & NO_BLOCK_MAPPINGS) == 0) {
>>>>> + (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) {
>>>>> pud_set_huge(pudp, phys, prot);
>>>>> /*
>>>>> @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void)
>>>>> int arch_add_memory(int nid, u64 start, u64 size,
>>>>> struct mhp_params *params)
>>>>> {
>>>>> - int ret, flags = NO_EXEC_MAPPINGS;
>>>>> + int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS;
>>>> I think we should allow large mappings here and instead prevent partial
>>>> removal of the block, if that's what is causing the issue.
>>> This could solve this problem.
>>> Or we can prevent partial removal? Or rebulid page table entry which is not removed?
>>
>> + David Hildenbrand <david@xxxxxxxxxx>
>>
>> Splitting the block mapping and rebuilding page table entry to reflect non-removed
>> areas will require additional information such as flags and pgtable alloc function
>> as in __create_pgd_mapping(), which need to be passed along, depending on whether
>> it's tearing down vmemmap (would not have PUD block map) or linear mapping. But I
>> am just wondering if we have to go in that direction at all or just prevent partial
>> memory block removal as suggested by Will.
>>
>> - arch_remove_memory() does not have return type, core MM hotremove would not fail
>> because arch_remove_memory() failed or warned
>>
>> - core MM hotremove does check_hotplug_memory_range() which ensures the range and
>> start address are memory_block_size_bytes() aligned
>>
>> - Default memory_block_size_bytes() is dependent on SECTION_SIZE_BITS which on arm64
>> now can be less than PUD_SIZE triggering this problem.
>>
>> #define MIN_MEMORY_BLOCK_SIZE (1UL << SECTION_SIZE_BITS)
>>
>> unsigned long __weak memory_block_size_bytes(void)
>> {
>> return MIN_MEMORY_BLOCK_SIZE;
>> }
>> EXPORT_SYMBOL_GPL(memory_block_size_bytes);
>>
>> - We would need to override memory_block_size_bytes() on arm64 to accommodate such
>> scenarios here
>>
>> Something like this might work (built but not tested)
>>
>> commit 2eb8dc0d08dfe0b2a3bb71df93b12f7bf74a2ca6 (HEAD)
>> Author: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> Date: Mon Jul 24 06:45:34 2023 +0100
>>
>> arm64/mm: Define memory_block_size_bytes()
>> Define memory_block_size_bytes() on arm64 platforms to set minimum hot plug
>> and remove granularity as PUD_SIZE in case where MIN_MEMORY_BLOCK_SIZE just
>> falls below PUD_SIZE. Otherwise a complete PUD block mapping will be teared
>> down while unmapping MIN_MEMORY_BLOCK_SIZE range.
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 95d360805f8a..1918459b3460 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1157,6 +1157,17 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>> }
>> #ifdef CONFIG_MEMORY_HOTPLUG
>> +unsigned long memory_block_size_bytes(void)
>> +{
>> + /*
>> + * Linear mappings might include PUD based block mappings which
>> + * cannot be teared down in part during memory hotremove. Hence
>> + * PUD_SIZE needs to be the minimum granularity, for memory hot
>> + * removal in case MIN_MEMORY_BLOCK_SIZE falls below.
>> + */
>> + return max_t(unsigned long, MIN_MEMORY_BLOCK_SIZE, PUD_SIZE);
>> +}
>> +
>> void vmemmap_free(unsigned long start, unsigned long end,
>> struct vmem_altmap *altmap)
>> {
>>
>
> OH god no. That would seriously degrade memory hotplug capabilities in virtual environments (especially, virtio-mem and DIMMS).
>
> If someone adds memory in 128 MiB chunks and removes memory in 128 MiB chunks, that has to be working.
>
> Removing boot memory is blocked via register_memory_notifier(&prevent_bootmem_remove_nb);
>
