Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1483885 > unrolled thread

Re: [PATCH 2/2 v2] x86/e820: Use much less memory for e820/e820_saved, save up to 120k

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2 v2] x86/e820: Use much less memory for  e820/e820_saved, save up to 120k Ingo Molnar <mingo@kernel.org> - 2016-09-15 09:10 +0200
    Re: [PATCH 2/2 v2] x86/e820: Use much less memory for  e820/e820_saved, save up to 120k Denys Vlasenko <dvlasenk@redhat.com> - 2016-09-17 23:10 +0200
      Re: [PATCH 2/2 v2] x86/e820: Use much less memory for  e820/e820_saved, save up to 120k Ingo Molnar <mingo@kernel.org> - 2016-09-18 10:40 +0200
        Re: [PATCH 2/2 v2] x86/e820: Use much less memory for  e820/e820_saved, save up to 120k Denys Vlasenko <dvlasenk@redhat.com> - 2016-09-18 20:20 +0200
          Re: [PATCH 2/2 v2] x86/e820: Use much less memory for  e820/e820_saved, save up to 120k Andy Lutomirski <luto@amacapital.net> - 2016-09-18 22:00 +0200

#1483885 — Re: [PATCH 2/2 v2] x86/e820: Use much less memory for e820/e820_saved, save up to 120k

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> The maximum size of e820 map array for EFI systems is defined as
> E820_X_MAX (E820MAX + 3 * MAX_NUMNODES).
> 
> In x86_64 defconfig, this ends up with E820_X_MAX = 320, e820 and e820_saved
> are 6404 bytes each.
> 
> With larger configs, for example Fedora kernels, E820_X_MAX = 3200, e820 and e820_saved
> are 64004 bytes each. Most of this space is wasted. Typical machines have some 20-30
> e820 areas at most.
> 
> This patch turns e820 and e820_saved to pointers which initially point to __initdata
> tables, of the same size as before.
> 
> At the very end of setup_arch(), when we are done fiddling with these maps,
> allocate smaller alloc_bootmem blocks, copy maps there, and change pointers.
> 
> Run-tested.

> +/*
> + * Initial e820 and e820_saved are largish __initdata arrays.
> + * Copy them to (usually much smaller) dynamically allocated area.
> + * This is done after all tweaks we ever do to them.
> + */
> +__init void e820_reallocate_tables(void)
> +{
> +	struct e820map *n;
> +	int size;
> +
> +	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820->nr_map;
> +	n = alloc_bootmem(size);
> +	memcpy(n, e820, size);
> +	e820 = n;
> +
> +	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820_saved->nr_map;
> +	n = alloc_bootmem(size);
> +	memcpy(n, e820_saved, size);
> +	e820_saved = n;
> +}

Ok, this makes me quite nervous, could you please split this into two patches so 
that any fails can be nicely bisected to?

First patch only does the pointerization changes with a trivial placeholder 
structure (full size, static allocated), second patch does all the dangerous bits 
such as changing it to __initdata, allocating and copying over bits.

Also, could we please also add some minimal debugging facility to make sure the 
memory table does not get extended after it's been reallocated?

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1485609

On 09/15/2016 09:04 AM, Ingo Molnar wrote:
>
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
>> The maximum size of e820 map array for EFI systems is defined as
>> E820_X_MAX (E820MAX + 3 * MAX_NUMNODES).
>>
>> In x86_64 defconfig, this ends up with E820_X_MAX = 320, e820 and e820_saved
>> are 6404 bytes each.
>>
>> With larger configs, for example Fedora kernels, E820_X_MAX = 3200, e820 and e820_saved
>> are 64004 bytes each. Most of this space is wasted. Typical machines have some 20-30
>> e820 areas at most.
>>
>> This patch turns e820 and e820_saved to pointers which initially point to __initdata
>> tables, of the same size as before.
>>
>> At the very end of setup_arch(), when we are done fiddling with these maps,
>> allocate smaller alloc_bootmem blocks, copy maps there, and change pointers.
>>
>> Run-tested.
>
>> +/*
>> + * Initial e820 and e820_saved are largish __initdata arrays.
>> + * Copy them to (usually much smaller) dynamically allocated area.
>> + * This is done after all tweaks we ever do to them.
>> + */
>> +__init void e820_reallocate_tables(void)
>> +{
>> +	struct e820map *n;
>> +	int size;
>> +
>> +	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820->nr_map;
>> +	n = alloc_bootmem(size);
>> +	memcpy(n, e820, size);
>> +	e820 = n;
>> +
>> +	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820_saved->nr_map;
>> +	n = alloc_bootmem(size);
>> +	memcpy(n, e820_saved, size);
>> +	e820_saved = n;
>> +}
>
> Ok, this makes me quite nervous, could you please split this into two patches so
> that any fails can be nicely bisected to?

No problem.

> First patch only does the pointerization changes with a trivial placeholder
> structure (full size, static allocated), second patch does all the dangerous bits
> such as changing it to __initdata, allocating and copying over bits.
>
> Also, could we please also add some minimal debugging facility to make sure the
> memory table does not get extended after it's been reallocated?

I have another idea: run e820_reallocate_tables() later, just before
we free __init and __initdata. Then e820 tables _can't_ be_ changed -
all functions which do that are __init functions.

Will test this now, and send a patchset.

[toc] | [prev] | [next] | [standalone]

#1485732

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> On 09/15/2016 09:04 AM, Ingo Molnar wrote:
> >
> >* Denys Vlasenko <dvlasenk@redhat.com> wrote:
> >
> >>The maximum size of e820 map array for EFI systems is defined as
> >>E820_X_MAX (E820MAX + 3 * MAX_NUMNODES).
> >>
> >>In x86_64 defconfig, this ends up with E820_X_MAX = 320, e820 and e820_saved
> >>are 6404 bytes each.
> >>
> >>With larger configs, for example Fedora kernels, E820_X_MAX = 3200, e820 and e820_saved
> >>are 64004 bytes each. Most of this space is wasted. Typical machines have some 20-30
> >>e820 areas at most.
> >>
> >>This patch turns e820 and e820_saved to pointers which initially point to __initdata
> >>tables, of the same size as before.
> >>
> >>At the very end of setup_arch(), when we are done fiddling with these maps,
> >>allocate smaller alloc_bootmem blocks, copy maps there, and change pointers.
> >>
> >>Run-tested.
> >
> >>+/*
> >>+ * Initial e820 and e820_saved are largish __initdata arrays.
> >>+ * Copy them to (usually much smaller) dynamically allocated area.
> >>+ * This is done after all tweaks we ever do to them.
> >>+ */
> >>+__init void e820_reallocate_tables(void)
> >>+{
> >>+	struct e820map *n;
> >>+	int size;
> >>+
> >>+	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820->nr_map;
> >>+	n = alloc_bootmem(size);
> >>+	memcpy(n, e820, size);
> >>+	e820 = n;
> >>+
> >>+	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820_saved->nr_map;
> >>+	n = alloc_bootmem(size);
> >>+	memcpy(n, e820_saved, size);
> >>+	e820_saved = n;
> >>+}
> >
> >Ok, this makes me quite nervous, could you please split this into two patches so
> >that any fails can be nicely bisected to?
> 
> No problem.
> 
> >First patch only does the pointerization changes with a trivial placeholder
> >structure (full size, static allocated), second patch does all the dangerous bits
> >such as changing it to __initdata, allocating and copying over bits.
> >
> >Also, could we please also add some minimal debugging facility to make sure the
> >memory table does not get extended after it's been reallocated?
> 
> I have another idea: run e820_reallocate_tables() later, just before
> we free __init and __initdata. Then e820 tables _can't_ be_ changed -
> all functions which do that are __init functions.
> 
> Will test this now, and send a patchset.

Could we also mark it __ro_after_init?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1485896

On 09/18/2016 10:31 AM, Ingo Molnar wrote:
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>> On 09/15/2016 09:04 AM, Ingo Molnar wrote:
>>> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>>
>>>> The maximum size of e820 map array for EFI systems is defined as
>>>> E820_X_MAX (E820MAX + 3 * MAX_NUMNODES).
>>>>
>>>> In x86_64 defconfig, this ends up with E820_X_MAX = 320, e820 and e820_saved
>>>> are 6404 bytes each.
>>>>
>>>> With larger configs, for example Fedora kernels, E820_X_MAX = 3200, e820 and e820_saved
>>>> are 64004 bytes each. Most of this space is wasted. Typical machines have some 20-30
>>>> e820 areas at most.
>>>>
>>>> This patch turns e820 and e820_saved to pointers which initially point to __initdata
>>>> tables, of the same size as before.
>>>>
>>>> At the very end of setup_arch(), when we are done fiddling with these maps,
>>>> allocate smaller alloc_bootmem blocks, copy maps there, and change pointers.
>>>>
>>>> Run-tested.
>>>
>>>> +/*
>>>> + * Initial e820 and e820_saved are largish __initdata arrays.
>>>> + * Copy them to (usually much smaller) dynamically allocated area.
>>>> + * This is done after all tweaks we ever do to them.
>>>> + */
>>>> +__init void e820_reallocate_tables(void)
>>>> +{
>>>> +	struct e820map *n;
>>>> +	int size;
>>>> +
>>>> +	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820->nr_map;
>>>> +	n = alloc_bootmem(size);
>>>> +	memcpy(n, e820, size);
>>>> +	e820 = n;
>>>> +
>>>> +	size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820_saved->nr_map;
>>>> +	n = alloc_bootmem(size);
>>>> +	memcpy(n, e820_saved, size);
>>>> +	e820_saved = n;
>>>> +}
>>>
>>> Ok, this makes me quite nervous, could you please split this into two patches so
>>> that any fails can be nicely bisected to?
>>
>> No problem.
>>
>>> First patch only does the pointerization changes with a trivial placeholder
>>> structure (full size, static allocated), second patch does all the dangerous bits
>>> such as changing it to __initdata, allocating and copying over bits.
>>>
>>> Also, could we please also add some minimal debugging facility to make sure the
>>> memory table does not get extended after it's been reallocated?
>>
>> I have another idea: run e820_reallocate_tables() later, just before
>> we free __init and __initdata. Then e820 tables _can't_ be_ changed -
>> all functions which do that are __init functions.
>>
>> Will test this now, and send a patchset.
>
> Could we also mark it __ro_after_init?

__ro_after_init makes sense only for statically allocated objects.
e820_reallocate_tables() copies e280 maps to kmalloced memory.

[toc] | [prev] | [next] | [standalone]

#1485934

On Sep 18, 2016 8:11 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
>
> On 09/18/2016 10:31 AM, Ingo Molnar wrote:
>>
>> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>>
>>> On 09/15/2016 09:04 AM, Ingo Molnar wrote:
>>>>
>>>> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>>>
>>>>> The maximum size of e820 map array for EFI systems is defined as
>>>>> E820_X_MAX (E820MAX + 3 * MAX_NUMNODES).
>>>>>
>>>>> In x86_64 defconfig, this ends up with E820_X_MAX = 320, e820 and e820_saved
>>>>> are 6404 bytes each.
>>>>>
>>>>> With larger configs, for example Fedora kernels, E820_X_MAX = 3200, e820 and e820_saved
>>>>> are 64004 bytes each. Most of this space is wasted. Typical machines have some 20-30
>>>>> e820 areas at most.
>>>>>
>>>>> This patch turns e820 and e820_saved to pointers which initially point to __initdata
>>>>> tables, of the same size as before.
>>>>>
>>>>> At the very end of setup_arch(), when we are done fiddling with these maps,
>>>>> allocate smaller alloc_bootmem blocks, copy maps there, and change pointers.
>>>>>
>>>>> Run-tested.
>>>>
>>>>
>>>>> +/*
>>>>> + * Initial e820 and e820_saved are largish __initdata arrays.
>>>>> + * Copy them to (usually much smaller) dynamically allocated area.
>>>>> + * This is done after all tweaks we ever do to them.
>>>>> + */
>>>>> +__init void e820_reallocate_tables(void)
>>>>> +{
>>>>> +       struct e820map *n;
>>>>> +       int size;
>>>>> +
>>>>> +       size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820->nr_map;
>>>>> +       n = alloc_bootmem(size);
>>>>> +       memcpy(n, e820, size);
>>>>> +       e820 = n;
>>>>> +
>>>>> +       size = offsetof(struct e820map, map) + sizeof(struct e820entry) * e820_saved->nr_map;
>>>>> +       n = alloc_bootmem(size);
>>>>> +       memcpy(n, e820_saved, size);
>>>>> +       e820_saved = n;
>>>>> +}
>>>>
>>>>
>>>> Ok, this makes me quite nervous, could you please split this into two patches so
>>>> that any fails can be nicely bisected to?
>>>
>>>
>>> No problem.
>>>
>>>> First patch only does the pointerization changes with a trivial placeholder
>>>> structure (full size, static allocated), second patch does all the dangerous bits
>>>> such as changing it to __initdata, allocating and copying over bits.
>>>>
>>>> Also, could we please also add some minimal debugging facility to make sure the
>>>> memory table does not get extended after it's been reallocated?
>>>
>>>
>>> I have another idea: run e820_reallocate_tables() later, just before
>>> we free __init and __initdata. Then e820 tables _can't_ be_ changed -
>>> all functions which do that are __init functions.
>>>
>>> Will test this now, and send a patchset.
>>
>>
>> Could we also mark it __ro_after_init?
>
>
> __ro_after_init makes sense only for statically allocated objects.
> e820_reallocate_tables() copies e280 maps to kmalloced memory.

The pointer can be __ro_after_init, though.  You could also
set_memory_ro() the thing if it's big enough that page-aligning it
wouldn't be a problem.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web