From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1b6IYm-00069f-UC for mharc-grub-devel@gnu.org; Fri, 27 May 2016 10:16:20 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b6IYh-00064y-LO for grub-devel@gnu.org; Fri, 27 May 2016 10:16:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b6IYc-0007RV-I5 for grub-devel@gnu.org; Fri, 27 May 2016 10:16:15 -0400 Received: from mx2.suse.de ([195.135.220.15]:40151) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b6IYc-0007R5-6I for grub-devel@gnu.org; Fri, 27 May 2016 10:16:10 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 42277AC77; Fri, 27 May 2016 14:16:09 +0000 (UTC) Subject: Re: [PATCH] efi: Free malloc regions on exit To: "Elliott, Robert (Persistent Memory)" , The development of GNU GRUB References: <1463665072-32385-1-git-send-email-agraf@suse.de> <94D0CD8314A33A4D9D801C0FE68B4029639B78D1@G9W0745.americas.hpqcorp.net> Cc: "mchang@suse.com" From: Alexander Graf Message-ID: <574856A8.3030306@suse.de> Date: Fri, 27 May 2016 16:16:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <94D0CD8314A33A4D9D801C0FE68B4029639B78D1@G9W0745.americas.hpqcorp.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] X-Received-From: 195.135.220.15 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 May 2016 14:16:20 -0000 On 05/24/2016 06:56 AM, Elliott, Robert (Persistent Memory) wrote: >> -----Original Message----- >> From: Grub-devel [mailto:grub-devel-bounces+elliott=hpe.com@gnu.org] >> On Behalf Of Alexander Graf >> Sent: Thursday, May 19, 2016 8:38 AM >> Subject: [PATCH] efi: Free malloc regions on exit > ... >> +struct efi_allocation { > If no other file is using this, mark it as static. Good idea. > >> + grub_uint64_t start_addr; > That should be grub_efi_physical_address_t. > >> + grub_uint64_t pages; > That should be grub_efi_uint64_t (the parameter type > for add_memory_regions) or grub_efi_uintn_t (the parameter > type for grub_efi_allocate_pages and grub_efi_free_pages). Yup for the type changes. > >> +} efi_allocated_memory[16]; > Why 16 and not some other number? Consider a #define and making > the comment about 16 in add_memory_regions more generic. I've added some helpful comment above and moved it into a #define. > >> +unsigned int efi_allocated_memory_idx = 0; > The C language definition guarantees that global variables > are initialized to 0. Yes, but multiple other global variables in the file also initialize with 0. Last time I checked gcc just puts those variables into bss either way, so you don't really lose anything by explicitly saying = 0 here. It does increase readability imho though to have it. > > >> @@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t >> *memory_map, >> (void *) ((grub_addr_t) start), >> (unsigned) pages); >> >> + /* Track up to 16 regions that we allocate from */ >> + if (efi_allocated_memory_idx < >> ARRAY_SIZE(efi_allocated_memory)) { >> + efi_allocated_memory[efi_allocated_memory_idx].start_addr = >> start; >> + efi_allocated_memory[efi_allocated_memory_idx].pages = >> pages; >> + efi_allocated_memory_idx++; >> + } >> + > Consider printing if the magic number is exceeded, rather than > silently changing the behavior. Good idea. I don't think anyone really cares enough - there should still be enough ram getting free'd on exit if memory is that heavily segmented. But I suppose firmware authors want to be aware that their memory map is a mess, and this could tell them :). > >> grub_mm_init_region (addr, PAGES_TO_BYTES (pages)); >> >> required_pages -= pages; >> @@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t >> *memory_map, >> grub_fatal ("too little memory"); >> } >> >> +void >> +grub_efi_memory_fini (void) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < efi_allocated_memory_idx; i++) { >> + grub_efi_free_pages (efi_allocated_memory[i].start_addr, >> + efi_allocated_memory[i].pages); >> + } >> +} > Consider clearing efi_allocated_memory_idx after that, since > this function cannot be sure it's the last one to use a > global variable. I'm not sure it's incredibly helpful (since this is a _fini function that basically denotes the end of grub's life), but sure. > > Clearing start_addr might help prevent stale pointer > reference bugs. Pointers that are no longer valid are > dangerous to leave around. That one however I don't agree with. Array elements are only really valid if their counter considers them as valid. If we initialize the counter to 0 that means the whole array should automatically be considered unused. Thanks a lot for the review :). Alex