Re: [Qemu-devel] [RFC][PATCH 01/14 v7] Add API to create memory mapping list

[Wen Congyang <wency@cn.fujitsu.com>]

At 03/01/2012 04:33 PM, HATAYAMA Daisuke Wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 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
> 
>