| Message ID | 1428670569-23089-1-git-send-email-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 10, 2015 at 5:56 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > This changes the allocation for the ASCII-converted command > line to use an ordinary memory pool rather than a separate > page based allocation. > > Pool allocations are generally preferred over page based > allocations due to the fact that they cause less fragmentation, > but in the particular case of arm64, where page allocations are > rounded up to 64 KB and where this allocation happens to be the > only explicit low allocation, it results in the lowest 64 KB of > memory to always be taken up by this particular allocation. > > So allocate from the EFI_LOADER_DATA pool instead. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/firmware/efi/libstub/efi-stub-helper.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index f07d4a67fa76..c95a567ca132 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -684,7 +684,8 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg, > > options_bytes++; /* NUL termination */ > > - status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr); > + status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > + options_bytes, (void **)&cmdline_addr); > if (status != EFI_SUCCESS) > return NULL; > > -- > 1.8.3.2 > Hi Ard, We can't do this without also changing the frees on the error paths in arm-stub.c (line 289) and eboot.c (line 1148) to be a pool free as well. Also, for x86/x86_64 the address of the command line is put into a boot_params structure, which only has a __u32 field for the address of the command line, so we could get an address that won't fit if we use a pool allocation. Looking at that bit of if it, I don't think we handle the case of no 32 bit addressable memory existing at the time of the efi_low_alloc() for x86_64 systems, so if a >32 bit address is returned here, this won't be detected as a failure, and the cmdline address will be the 32 bit truncated address. I don't think that there is a way to control the address range returned by pool allocations, so I think we are stuck with the page based allocations if we have address restrictions. Roy
On 10 April 2015 at 18:43, Roy Franz <roy.franz@linaro.org> wrote: > On Fri, Apr 10, 2015 at 5:56 AM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> This changes the allocation for the ASCII-converted command >> line to use an ordinary memory pool rather than a separate >> page based allocation. >> >> Pool allocations are generally preferred over page based >> allocations due to the fact that they cause less fragmentation, >> but in the particular case of arm64, where page allocations are >> rounded up to 64 KB and where this allocation happens to be the >> only explicit low allocation, it results in the lowest 64 KB of >> memory to always be taken up by this particular allocation. >> >> So allocate from the EFI_LOADER_DATA pool instead. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> drivers/firmware/efi/libstub/efi-stub-helper.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c >> index f07d4a67fa76..c95a567ca132 100644 >> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c >> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c >> @@ -684,7 +684,8 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg, >> >> options_bytes++; /* NUL termination */ >> >> - status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr); >> + status = efi_call_early(allocate_pool, EFI_LOADER_DATA, >> + options_bytes, (void **)&cmdline_addr); >> if (status != EFI_SUCCESS) >> return NULL; >> >> -- >> 1.8.3.2 >> > > Hi Ard, > > We can't do this without also changing the frees on the error paths in > arm-stub.c (line 289) > and eboot.c (line 1148) to be a pool free as well. > Also, for x86/x86_64 the address of the command line is put into a > boot_params structure, > which only has a __u32 field for the address of the command line, so > we could get an address > that won't fit if we use a pool allocation. > Looking at that bit of if it, I don't think we handle the case of no > 32 bit addressable memory > existing at the time of the efi_low_alloc() for x86_64 systems, so if > a >32 bit address is returned here, > this won't be detected as a failure, and the cmdline address will be > the 32 bit truncated address. > > I don't think that there is a way to control the address range > returned by pool allocations, > so I think we are stuck with the page based allocations if we have > address restrictions. > Ah yes, I wondered about the reason for the low_alloc(). I guess I could have looked a bit further myself :-) It is not such a big deal: the memory is reclaimed anyway, I was just trying to reduce the fragmentation a bit, and trying to avoid efi_xxx_alloc() which are substantially heavier than calling allocate_pool() or allocate_pages() directly. I'll drop this patch then. It's not really worth the effort as its primarily cosmetics anyway.
On Fri, Apr 10, 2015 at 10:14 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 10 April 2015 at 18:43, Roy Franz <roy.franz@linaro.org> wrote: >> On Fri, Apr 10, 2015 at 5:56 AM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> This changes the allocation for the ASCII-converted command >>> line to use an ordinary memory pool rather than a separate >>> page based allocation. >>> >>> Pool allocations are generally preferred over page based >>> allocations due to the fact that they cause less fragmentation, >>> but in the particular case of arm64, where page allocations are >>> rounded up to 64 KB and where this allocation happens to be the >>> only explicit low allocation, it results in the lowest 64 KB of >>> memory to always be taken up by this particular allocation. >>> >>> So allocate from the EFI_LOADER_DATA pool instead. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> drivers/firmware/efi/libstub/efi-stub-helper.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c >>> index f07d4a67fa76..c95a567ca132 100644 >>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c >>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c >>> @@ -684,7 +684,8 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg, >>> >>> options_bytes++; /* NUL termination */ >>> >>> - status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr); >>> + status = efi_call_early(allocate_pool, EFI_LOADER_DATA, >>> + options_bytes, (void **)&cmdline_addr); >>> if (status != EFI_SUCCESS) >>> return NULL; >>> >>> -- >>> 1.8.3.2 >>> >> >> Hi Ard, >> >> We can't do this without also changing the frees on the error paths in >> arm-stub.c (line 289) >> and eboot.c (line 1148) to be a pool free as well. >> Also, for x86/x86_64 the address of the command line is put into a >> boot_params structure, >> which only has a __u32 field for the address of the command line, so >> we could get an address >> that won't fit if we use a pool allocation. >> Looking at that bit of if it, I don't think we handle the case of no >> 32 bit addressable memory >> existing at the time of the efi_low_alloc() for x86_64 systems, so if >> a >32 bit address is returned here, >> this won't be detected as a failure, and the cmdline address will be >> the 32 bit truncated address. I'm working on a patch for this, and it also looks like efi_low_alloc() is not correct for 32 bit builds, as internally it can allocate a >32 bit address, and then will truncate that to return an 'unsigned long'. I should have something out next week. >> >> I don't think that there is a way to control the address range >> returned by pool allocations, >> so I think we are stuck with the page based allocations if we have >> address restrictions. >> > > Ah yes, I wondered about the reason for the low_alloc(). I guess I > could have looked a bit further myself :-) > > It is not such a big deal: the memory is reclaimed anyway, I was just > trying to reduce the fragmentation a bit, and trying to avoid > efi_xxx_alloc() which are substantially heavier than calling > allocate_pool() or allocate_pages() directly. > > I'll drop this patch then. It's not really worth the effort as its > primarily cosmetics anyway. > > -- > Ard.
On Fri, 10 Apr, at 07:14:34PM, Ard Biesheuvel wrote: > > It is not such a big deal: the memory is reclaimed anyway, I was just > trying to reduce the fragmentation a bit, and trying to avoid > efi_xxx_alloc() which are substantially heavier than calling > allocate_pool() or allocate_pages() directly. Out of curiosity, do you have any runtime numbers to backup the claim that allocate_*() is substantially more lightweight?
On 15 April 2015 at 11:55, Matt Fleming <matt@codeblueprint.co.uk> wrote: > On Fri, 10 Apr, at 07:14:34PM, Ard Biesheuvel wrote: >> >> It is not such a big deal: the memory is reclaimed anyway, I was just >> trying to reduce the fragmentation a bit, and trying to avoid >> efi_xxx_alloc() which are substantially heavier than calling >> allocate_pool() or allocate_pages() directly. > > Out of curiosity, do you have any runtime numbers to backup the claim > that allocate_*() is substantially more lightweight? > No, I do not. But since efi_[high|low]_alloc() call efi_get_memory_map() internally, which itself calls allocate_pool() at least twice [typically], and then iterate over the entire memory map to select a suitable slot which gets allocated using allocate_pages(), calling either of allocate_[pool|pages] directly is arguably more lightweight. But claiming they are 'substantially heavier' is unsubstantiated.
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index f07d4a67fa76..c95a567ca132 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -684,7 +684,8 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg, options_bytes++; /* NUL termination */ - status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr); + status = efi_call_early(allocate_pool, EFI_LOADER_DATA, + options_bytes, (void **)&cmdline_addr); if (status != EFI_SUCCESS) return NULL;
This changes the allocation for the ASCII-converted command line to use an ordinary memory pool rather than a separate page based allocation. Pool allocations are generally preferred over page based allocations due to the fact that they cause less fragmentation, but in the particular case of arm64, where page allocations are rounded up to 64 KB and where this allocation happens to be the only explicit low allocation, it results in the lowest 64 KB of memory to always be taken up by this particular allocation. So allocate from the EFI_LOADER_DATA pool instead. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/firmware/efi/libstub/efi-stub-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)