[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC][PATCH 01/14 v7] Add API to create memory mapping
From: |
Wen Congyang |
Subject: |
Re: [Qemu-devel] [RFC][PATCH 01/14 v7] Add API to create memory mapping list |
Date: |
Thu, 01 Mar 2012 16:58:00 +0800 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100413 Fedora/3.0.4-2.fc13 Thunderbird/3.0.4 |
At 03/01/2012 04:33 PM, HATAYAMA Daisuke Wrote:
> From: Wen Congyang <address@hidden>
> 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
>
>