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

[HATAYAMA Daisuke]

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?

> +
> +        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.

> +
> +        /* 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.

Thanks.
HATAYAMA, Daisuke