On 09/11/2012 10:51 AM, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingf...@linux.vnet.ibm.com>
>
> Without biglock, we try to protect the mr by increase refcnt.
> If we can inc refcnt, go backward and resort to biglock.
>
> Another point is memory radix-tree can be flushed by another
> thread, so we should get the copy of terminal mr to survive
> from such issue.
>
>
> +static QemuMutex mem_map_lock;
> +
> static void core_begin(MemoryListener *listener)
> {
> + qemu_mutex_lock(&mem_map_lock);
Since we have an unbalanced lock here, please add a comment that
explains the lock holding region.
> destroy_all_mappings();
> phys_sections_clear();
> phys_map.ptr = PHYS_MAP_NODE_NIL;
> @@ -3184,17 +3187,32 @@ static void core_commit(MemoryListener *listener)
> for(env = first_cpu; env != NULL; env = env->next_cpu) {
> tlb_flush(env, 1);
> }
> + qemu_mutex_unlock(&mem_map_lock);
> }
>
> static void core_region_add(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> + MemoryRegion *mr = section->mr;
> +
> + if (mr->ops != NULL) {
> + if (mr->ops->ref != NULL) {
> + mr->ops->ref(mr);
> + }
> + }
Can drop '!= NULL'.
> cpu_register_physical_memory_log(section, section->readonly);
> }
>
> static void core_region_del(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> + MemoryRegion *mr = section->mr;
> +
> + if (mr->ops != NULL) {
> + if (mr->ops->unref != NULL) {
> + mr->ops->unref(mr);
> + }
> + }
> }
Here too.
> @@ -3406,6 +3426,52 @@ int cpu_memory_rw_debug(CPUArchState *env,
> target_ulong addr,
> }
>
> #else
> +
> +static MemoryRegionSection *subpage_get_terminal(subpage_t *mmio,
> + target_phys_addr_t addr)
> +{
> + MemoryRegionSection *section;
> + unsigned int idx = SUBPAGE_IDX(addr);
> +
> + section = &phys_sections[mmio->sub_section[idx]];
> + return section;
> +}
> +
> +static MemoryRegionSection mrs_get_terminal(MemoryRegionSection *mrs,
> + target_phys_addr_t addr)
> +{
> + if (mrs->mr->subpage) {
> + mrs = subpage_get_terminal(mrs->mr->opaque, addr);
> + }
> + return *mrs;
> +}
> +
Please spell out memory_region_section_ in function names, like
elsewhere in the file.
> +static int mrs_ref(MemoryRegionSection *mrs)
> +{
> + MemoryRegion *mr;
> + int ret;
> +
> + mr = mrs->mr;
> + if (mr->ops != NULL) {
> + if (mr->ops->ref != NULL) {
> + ret = mr->ops->ref(mr);
> + }
> + }
> + return ret;
> +}
What could 'ret' possibly be?
> +
> +static void mrs_unref(MemoryRegionSection *mrs)
> +{
> + MemoryRegion *mr;
> +
> + mr = mrs->mr;
> + if (mr->ops != NULL) {
> + if (mr->ops->unref != NULL) {
> + mr->ops->unref(mr);
> + }
> + }
> +}
> +
> void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
> int len, int is_write)
> {
> @@ -3413,14 +3479,35 @@ void cpu_physical_memory_rw(target_phys_addr_t addr,
> uint8_t *buf,
> uint8_t *ptr;
> uint32_t val;
> target_phys_addr_t page;
> - MemoryRegionSection *section;
> + MemoryRegionSection *section, obj_mrs;
> + int ret;
> + int need_biglock = 0;
>
> + /* Will finally removed after all mr->ops implement ref/unref() */
> +try_big_lock:
> + if (need_biglock == 1) {
> + qemu_mutex_lock_iothread();
> + }
> while (len > 0) {
> page = addr & TARGET_PAGE_MASK;
> l = (page + TARGET_PAGE_SIZE) - addr;
> if (l > len)
> l = len;
> +
> + qemu_mutex_lock(&mem_map_lock);
> section = phys_page_find(page >> TARGET_PAGE_BITS);
> + if (need_biglock == 0) {
> + obj_mrs = mrs_get_terminal(section, addr);
> + ret = mrs_ref(&obj_mrs);
> + if (ret <= 0) {
> + need_biglock = 1;
> + qemu_mutex_unlock(&mem_map_lock);
> + goto try_big_lock;
> + }
> + /* rely on local variable */
> + section = &obj_mrs;
> + }
> + qemu_mutex_unlock(&mem_map_lock);
Please split out this code to a separate function (can return
MemoryRegionSection by value, with either the big lock held or the
object referenced).
>
> if (is_write) {
> if (!memory_region_is_ram(section->mr)) {
> @@ -3491,10 +3578,16 @@ void cpu_physical_memory_rw(target_phys_addr_t addr,
> uint8_t *buf,
> qemu_put_ram_ptr(ptr);
> }
> }
> +
> + mrs_unref(&obj_mrs);
> len -= l;
> buf += l;
> addr += l;
> }`
> +
> + if (need_biglock == 1) {
> + qemu_mutex_unlock_iothread();
> + }
Similarly this should be in a separate function, either unrefing or
dropping the big lock as appropriate.
> }
>
> /* used for ROM loading : can write in RAM and ROM */
>
I see other calls to phys_page_find(), they all need to be protected.
--
error compiling committee.c: too many arguments to function
