From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39895) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S31or-0003uC-N6 for qemu-devel@nongnu.org; Thu, 01 Mar 2012 03:57:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S31ol-0003VT-3P for qemu-devel@nongnu.org; Thu, 01 Mar 2012 03:57:01 -0500 Received: from [222.73.24.84] (port=51101 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S31ok-0003Te-GA for qemu-devel@nongnu.org; Thu, 01 Mar 2012 03:56:55 -0500 Message-ID: <4F4F3A18.2040906@cn.fujitsu.com> Date: Thu, 01 Mar 2012 16:58:00 +0800 From: Wen Congyang MIME-Version: 1.0 References: <4F4EE080.9060307@cn.fujitsu.com> <4F4EE167.5000507@cn.fujitsu.com> <20120301.173350.521612479.d.hatayama@jp.fujitsu.com> In-Reply-To: <20120301.173350.521612479.d.hatayama@jp.fujitsu.com> Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [RFC][PATCH 01/14 v7] Add API to create memory mapping list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: HATAYAMA Daisuke Cc: jan.kiszka@siemens.com, anderson@redhat.com, qemu-devel@nongnu.org, eblake@redhat.com, lcapitulino@redhat.com At 03/01/2012 04:33 PM, HATAYAMA Daisuke Wrote: > From: Wen Congyang > Subject: [RFC][PATCH 01/14 v7] Add API to create memory mapping list > Date: Thu, 01 Mar 2012 10:39:35 +0800 > >> +static void memory_mapping_list_add_mapping_sorted(MemoryMappingList *list, >> + MemoryMapping *mapping) > >> +void memory_mapping_list_add_sorted(MemoryMappingList *list, > > These functions not only sort but also merge elements of mapping > list. Is there another name that represents what these are doing more > properly? > >> + target_phys_addr_t phys_addr, >> + target_phys_addr_t virt_addr, >> + ram_addr_t length) >> +{ >> + MemoryMapping *memory_mapping, *last_mapping; >> + >> + if (QTAILQ_EMPTY(&list->head)) { >> + create_new_memory_mapping(list, phys_addr, virt_addr, length); >> + return; >> + } >> + >> + last_mapping = list->last_mapping; >> + if (last_mapping) { >> + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && >> + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { >> + last_mapping->length += length; >> + return; >> + } >> + } >> + >> + QTAILQ_FOREACH(memory_mapping, &list->head, next) { >> + last_mapping = memory_mapping; >> + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && >> + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { >> + last_mapping->length += length; >> + list->last_mapping = last_mapping; >> + return; >> + } > > Please don't use a single variable in multiple meanings in the same > function. It appears to me that the variable last_mapping is used as > n-th entry connected to the list->head within this for loop. So > this_mapping, for example, is reasonable rather than last_mapping. All > use of last_mapping within the for loop can be replaced with > memory_mapping, right? OK > >> + >> + if (phys_addr + length < last_mapping->phys_addr) { >> + /* create a new region before last_mapping */ >> + break; >> + } >> + >> + if (phys_addr >= (last_mapping->phys_addr + last_mapping->length)) { >> + /* last_mapping does not contain this region */ >> + continue; >> + } >> + >> + if ((virt_addr - last_mapping->virt_addr) != >> + (phys_addr - last_mapping->phys_addr)) { >> + /* >> + * last_mapping contains this region, but we cannot merge this >> + * region into last_mapping. Try the next memory mapping. >> + */ >> + continue; >> + } > > Does this condition means the current mapping and the last mapping are > contiguous both phisically and viurtually? If so, it's better to write > the condition so readers can understand that more easily. current mapping and last mapping are always contiguous both phisically and virtually. > >> + >> + /* merge this region into last_mapping */ >> + if (virt_addr < last_mapping->virt_addr) { >> + last_mapping->length += last_mapping->virt_addr - virt_addr; >> + last_mapping->virt_addr = virt_addr; >> + } >> + >> + if ((virt_addr + length) > >> + (last_mapping->virt_addr + last_mapping->length)) { >> + last_mapping->length = virt_addr + length - last_mapping->virt_addr; >> + } >> + >> + list->last_mapping = last_mapping; >> + return; >> + } >> + >> + /* this region can not be merged into any existed memory mapping. */ >> + create_new_memory_mapping(list, phys_addr, virt_addr, length); >> +} > > How about adding helper functions for expressing each conditionals? > Just like below. Then I think the code gets more readable. > > bool mapping_proper_succeor(MemoryMapping *map, > target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > bool mapping_physically_contains(MemoryMapping *map, > target_phys_addr_t phys_addr); > bool mapping_physically_virtually_contiguous(MemoryMapping *map, > target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > void mapping_merge(MemoryMapping *map, target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > > I'm not confident of the naming, these are example, and assuming > define all as static inline functions. Hmm, I think this inline functions make the code more readable. So I will modify my code. Thanks Wen Congyang > > Thanks. > HATAYAMA, Daisuke > >