| Message ID | 1403763216-28508-1-git-send-email-hutao@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 06/26/2014 12:13 AM, Hu Tao wrote: > We already have ROUND_UP but there are similar macros and ROUND_UP-like > lines all around, replace them with ROUND_UP and clean up similar macros. Not a review (yet), but a question - how did you find lines to convert? Is there a particular grep pattern you looked for, or a Coccinelle script, or ...? I ask because it would be nice information in the commit message, so that the actual review can also look to see if you missed any instances that could use the conversion.
On 06/26/2014 12:13 AM, Hu Tao wrote: > We already have ROUND_UP but there are similar macros and ROUND_UP-like > lines all around, replace them with ROUND_UP and clean up similar macros. > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > --- All of these conversions are correct, so: Reviewed-by: Eric Blake <eblake@redhat.com> However, without documenting how you found what to convert, I'm quite sure you missed some conversions. In fact, I found at least: util/oslib-win32.c: memory = (memory + pagesize - 1) & -pagesize; tcg/ppc/tcg-target.c-#define FRAME_SIZE ((TCG_TARGET_CALL_STACK_OFFSET \ tcg/ppc/tcg-target.c- + TCG_STATIC_CALL_ARGS_SIZE \ tcg/ppc/tcg-target.c- + CPU_TEMP_BUF_SIZE \ tcg/ppc/tcg-target.c- + REG_SAVE_SIZE \ tcg/ppc/tcg-target.c- + TCG_TARGET_STACK_ALIGN - 1) \ tcg/ppc/tcg-target.c: & -TCG_TARGET_STACK_ALIGN) block/cow.c: s->cow_sectors_offset = (bitmap_size + 511) & ~511; block/qcow.c: header_size = (header_size + 7) & ~7; before I quit looking for more. (I just manually read through git grep '[^&]& [~-]' which has a lot of noise for non-rounding uses, but definitely found some missed cases)
On Thu, Jun 26, 2014 at 05:52:45AM -0600, Eric Blake wrote: > On 06/26/2014 12:13 AM, Hu Tao wrote: > > We already have ROUND_UP but there are similar macros and ROUND_UP-like > > lines all around, replace them with ROUND_UP and clean up similar macros. > > Not a review (yet), but a question - how did you find lines to convert? > Is there a particular grep pattern you looked for, or a Coccinelle > script, or ...? I ask because it would be nice information in the commit > message, so that the actual review can also look to see if you missed > any instances that could use the conversion. Good point. I used grep but indeed I've missed some instances. Regards, Hu
On Thu, Jun 26, 2014 at 10:55:33AM -0600, Eric Blake wrote: > On 06/26/2014 12:13 AM, Hu Tao wrote: > > We already have ROUND_UP but there are similar macros and ROUND_UP-like > > lines all around, replace them with ROUND_UP and clean up similar macros. > > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > --- > > All of these conversions are correct, so: > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks for review! > > However, without documenting how you found what to convert, I'm quite > sure you missed some conversions. In fact, I found at least: > > util/oslib-win32.c: memory = (memory + pagesize - 1) & -pagesize; The pattern I used is '(.*+.*\-.*\&.*~'. Apparently I forgot the -pagesize case. > > tcg/ppc/tcg-target.c-#define FRAME_SIZE ((TCG_TARGET_CALL_STACK_OFFSET \ > tcg/ppc/tcg-target.c- + TCG_STATIC_CALL_ARGS_SIZE \ > tcg/ppc/tcg-target.c- + CPU_TEMP_BUF_SIZE \ > tcg/ppc/tcg-target.c- + REG_SAVE_SIZE \ > tcg/ppc/tcg-target.c- + TCG_TARGET_STACK_ALIGN - 1) \ > tcg/ppc/tcg-target.c: & -TCG_TARGET_STACK_ALIGN) How did you find this one? Your pattern below just shows the last line. > > block/cow.c: s->cow_sectors_offset = (bitmap_size + 511) & ~511; > > block/qcow.c: header_size = (header_size + 7) & ~7; > > before I quit looking for more. (I just manually read through > git grep '[^&]& [~-]' > which has a lot of noise for non-rounding uses, but definitely found > some missed cases) > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 06/26/2014 09:50 PM, Hu Tao wrote: > On Thu, Jun 26, 2014 at 10:55:33AM -0600, Eric Blake wrote: >> On 06/26/2014 12:13 AM, Hu Tao wrote: >>> We already have ROUND_UP but there are similar macros and ROUND_UP-like >>> lines all around, replace them with ROUND_UP and clean up similar macros. >>> >>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> >>> --- >> >> All of these conversions are correct, so: >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Thanks for review! >> tcg/ppc/tcg-target.c-#define FRAME_SIZE ((TCG_TARGET_CALL_STACK_OFFSET \ >> tcg/ppc/tcg-target.c- + TCG_STATIC_CALL_ARGS_SIZE \ >> tcg/ppc/tcg-target.c- + CPU_TEMP_BUF_SIZE \ >> tcg/ppc/tcg-target.c- + REG_SAVE_SIZE \ >> tcg/ppc/tcg-target.c- + TCG_TARGET_STACK_ALIGN - 1) \ >> tcg/ppc/tcg-target.c: & -TCG_TARGET_STACK_ALIGN) > > How did you find this one? Your pattern below just shows the last line. But the last line had the word ALIGN in it, so I looked at the context and found that it was indeed a rounding operation. >> before I quit looking for more. (I just manually read through >> git grep '[^&]& [~-]' >> which has a lot of noise for non-rounding uses, but definitely found >> some missed cases) Are you going to submit a v2? As this does not appear to be a bug fix, and today is hard freeze, this is probably 2.2 material.
On Tue, Jul 01, 2014 at 02:42:43PM -0600, Eric Blake wrote: > On 06/26/2014 09:50 PM, Hu Tao wrote: > > On Thu, Jun 26, 2014 at 10:55:33AM -0600, Eric Blake wrote: > >> On 06/26/2014 12:13 AM, Hu Tao wrote: > >>> We already have ROUND_UP but there are similar macros and ROUND_UP-like > >>> lines all around, replace them with ROUND_UP and clean up similar macros. > >>> > >>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > >>> --- > >> > >> All of these conversions are correct, so: > >> Reviewed-by: Eric Blake <eblake@redhat.com> > > > > Thanks for review! > > >> tcg/ppc/tcg-target.c-#define FRAME_SIZE ((TCG_TARGET_CALL_STACK_OFFSET \ > >> tcg/ppc/tcg-target.c- + TCG_STATIC_CALL_ARGS_SIZE \ > >> tcg/ppc/tcg-target.c- + CPU_TEMP_BUF_SIZE \ > >> tcg/ppc/tcg-target.c- + REG_SAVE_SIZE \ > >> tcg/ppc/tcg-target.c- + TCG_TARGET_STACK_ALIGN - 1) \ > >> tcg/ppc/tcg-target.c: & -TCG_TARGET_STACK_ALIGN) > > > > How did you find this one? Your pattern below just shows the last line. > > But the last line had the word ALIGN in it, so I looked at the context > and found that it was indeed a rounding operation. > > >> before I quit looking for more. (I just manually read through > >> git grep '[^&]& [~-]' > >> which has a lot of noise for non-rounding uses, but definitely found > >> some missed cases) > > Are you going to submit a v2? As this does not appear to be a bug fix, > and today is hard freeze, this is probably 2.2 material. Yes. I'm fine it gets in after 2.1. Regards, Hu
diff --git a/block/qcow.c b/block/qcow.c index 1f2bac8..4ba7769 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -323,7 +323,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, /* allocate a new l2 entry */ l2_offset = bdrv_getlength(bs->file); /* round to cluster size */ - l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1); + l2_offset = ROUND_UP(l2_offset, s->cluster_size); /* update the L1 entry */ s->l1_table[l1_index] = l2_offset; tmp = cpu_to_be64(l2_offset); diff --git a/block/vdi.c b/block/vdi.c index 01fe22e..d07186d 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -721,7 +721,7 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp) blocks = (bytes + block_size - 1) / block_size; bmap_size = blocks * sizeof(uint32_t); - bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1)); + bmap_size = ROUND_UP(bmap_size, SECTOR_SIZE); memset(&header, 0, sizeof(header)); pstrcpy(header.text, sizeof(header.text), VDI_TEXT); diff --git a/block/vpc.c b/block/vpc.c index 798d854..cc53979 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -280,7 +280,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, } s->free_data_block_offset = - (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511; + ROUND_UP(s->bat_offset + (s->max_table_entries * 4), 512); for (i = 0; i < s->max_table_entries; i++) { be32_to_cpus(&s->pagetable[i]); diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c index 93fd9e4..a552acb 100644 --- a/bsd-user/elfload.c +++ b/bsd-user/elfload.c @@ -777,7 +777,7 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc, k_platform = ELF_PLATFORM; if (k_platform) { size_t len = strlen(k_platform) + 1; - sp -= (len + n - 1) & ~(n - 1); + sp -= ROUND_UP(len, n); u_platform = sp; /* FIXME - check return value of memcpy_to_target() for failure */ memcpy_to_target(sp, k_platform, len); diff --git a/exec.c b/exec.c index c849405..3ca00f9 100644 --- a/exec.c +++ b/exec.c @@ -1064,7 +1064,7 @@ static void *file_ram_alloc(RAMBlock *block, unlink(filename); g_free(filename); - memory = (memory+hpagesize-1) & ~(hpagesize-1); + memory = ROUND_UP(memory, hpagesize); /* * ftruncate is not supported by hugetlbfs in older diff --git a/hw/core/loader.c b/hw/core/loader.c index 2bf6b8f..4770714 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -173,13 +173,12 @@ static void bswap_ahdr(struct exec *e) (N_MAGIC(x) == ZMAGIC ? _N_HDROFF((x)) + sizeof (struct exec) : \ (N_MAGIC(x) == QMAGIC ? 0 : sizeof (struct exec))) #define N_TXTADDR(x, target_page_size) (N_MAGIC(x) == QMAGIC ? target_page_size : 0) -#define _N_SEGMENT_ROUND(x, target_page_size) (((x) + target_page_size - 1) & ~(target_page_size - 1)) #define _N_TXTENDADDR(x, target_page_size) (N_TXTADDR(x, target_page_size)+(x).a_text) #define N_DATADDR(x, target_page_size) \ (N_MAGIC(x)==OMAGIC? (_N_TXTENDADDR(x, target_page_size)) \ - : (_N_SEGMENT_ROUND (_N_TXTENDADDR(x, target_page_size), target_page_size))) + : (ROUND_UP(_N_TXTENDADDR(x, target_page_size), target_page_size))) int load_aout(const char *filename, hwaddr addr, int max_sz, @@ -377,7 +376,7 @@ static void *zalloc(void *x, unsigned items, unsigned size) void *p; size *= items; - size = (size + ZALLOC_ALIGNMENT - 1) & ~(ZALLOC_ALIGNMENT - 1); + size = ROUND_UP(size, ZALLOC_ALIGNMENT); p = g_malloc(size); diff --git a/hw/display/qxl.c b/hw/display/qxl.c index d43aa49..34bf7b7 100644 --- a/hw/display/qxl.c +++ b/hw/display/qxl.c @@ -61,9 +61,6 @@ } \ } -#undef ALIGN -#define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1)) - #define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9" #define QXL_MODE(_x, _y, _b, _o) \ @@ -358,8 +355,8 @@ static void init_qxl_rom(PCIQXLDevice *d) } modes->n_modes = cpu_to_le32(n); - ram_header_size = ALIGN(sizeof(QXLRam), 4096); - surface0_area_size = ALIGN(d->vgamem_size, 4096); + ram_header_size = ROUND_UP(sizeof(QXLRam), 4096); + surface0_area_size = ROUND_UP(d->vgamem_size, 4096); num_pages = d->vga.vram_size; num_pages -= ram_header_size; num_pages -= surface0_area_size; diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 82f183f..98c2efb 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -148,7 +148,7 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi) if (msi) { assert((num == 1) || (num == 2) || (num == 4) || (num == 8) || (num == 16) || (num == 32)); - hint = (spapr->next_irq + num - 1) & ~(num - 1); + hint = ROUND_UP(spapr->next_irq, num); } for (i = 0; i < num; ++i) { diff --git a/kvm-all.c b/kvm-all.c index 3ae30ee..f361094 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -381,8 +381,6 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section, return 0; } -#define ALIGN(x, y) (((x)+(y)-1) & ~((y)-1)) - /** * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space * This function updates qemu's dirty bitmap using @@ -421,8 +419,8 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section) * So for now, let's align to 64 instead of HOST_LONG_BITS here, in * a hope that sizeof(long) wont become >8 any time soon. */ - size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS), - /*HOST_LONG_BITS*/ 64) / 8; + size = ROUND_UP(((mem->memory_size) >> TARGET_PAGE_BITS), + /*HOST_LONG_BITS*/ 64) / 8; if (!d.dirty_bitmap) { d.dirty_bitmap = g_malloc(size); } else if (size > allocated_size) { @@ -945,7 +943,7 @@ void kvm_init_irq_routing(KVMState *s) unsigned int gsi_bits, i; /* Round up so we can search ints using ffs */ - gsi_bits = ALIGN(gsi_count, 32); + gsi_bits = ROUND_UP(gsi_count, 32); s->used_gsi_bitmap = g_malloc0(gsi_bits / 8); s->gsi_count = gsi_count; @@ -1083,7 +1081,7 @@ static void kvm_flush_dynamic_msi_routes(KVMState *s) static int kvm_irqchip_get_virq(KVMState *s) { uint32_t *word = s->used_gsi_bitmap; - int max_words = ALIGN(s->gsi_count, 32) / 32; + int max_words = ROUND_UP(s->gsi_count, 32) / 32; int i, bit; bool retry = true; diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 1248eda..eda763d 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -1498,7 +1498,7 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc, k_platform = ELF_PLATFORM; if (k_platform) { size_t len = strlen(k_platform) + 1; - sp -= (len + n - 1) & ~(n - 1); + sp -= ROUND_UP(len, n); u_platform = sp; /* FIXME - check return value of memcpy_to_target() for failure */ memcpy_to_target(sp, k_platform, len); diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 7d74079..2ccaba7 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7432,7 +7432,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, ret = -TARGET_EINVAL; break; } - mask_size = (arg2 + (sizeof(*mask) - 1)) & ~(sizeof(*mask) - 1); + mask_size = ROUND_UP(arg2, sizeof(*mask)); mask = alloca(mask_size); ret = get_errno(sys_sched_getaffinity(arg1, mask_size, mask)); @@ -7473,7 +7473,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, ret = -TARGET_EINVAL; break; } - mask_size = (arg2 + (sizeof(*mask) - 1)) & ~(sizeof(*mask) - 1); + mask_size = ROUND_UP(arg2, sizeof(*mask)); mask = alloca(mask_size); if (!lock_user_struct(VERIFY_READ, p, arg3, 1)) { diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c index c83fd9f..e56bb9d 100644 --- a/tcg/ppc/tcg-target.c +++ b/tcg/ppc/tcg-target.c @@ -2630,14 +2630,14 @@ void flush_icache_range(uintptr_t start, uintptr_t stop) size_t isize = icache_bsize; start1 = start & ~(dsize - 1); - stop1 = (stop + dsize - 1) & ~(dsize - 1); + stop1 = ROUND_UP(stop, dsize); for (p = start1; p < stop1; p += dsize) { asm volatile ("dcbst 0,%0" : : "r"(p) : "memory"); } asm volatile ("sync" : : : "memory"); start &= start & ~(isize - 1); - stop1 = (stop + isize - 1) & ~(isize - 1); + stop1 = ROUND_UP(stop, isize); for (p = start1; p < stop1; p += isize) { asm volatile ("icbi 0,%0" : : "r"(p) : "memory"); } diff --git a/thunk.c b/thunk.c index 3cca047..de7d969 100644 --- a/thunk.c +++ b/thunk.c @@ -96,14 +96,14 @@ void thunk_register_struct(int id, const char *name, const argtype *types) for(j = 0;j < nb_fields; j++) { size = thunk_type_size(type_ptr, i); align = thunk_type_align(type_ptr, i); - offset = (offset + align - 1) & ~(align - 1); + offset = ROUND_UP(offset, align); se->field_offsets[i][j] = offset; offset += size; if (align > max_align) max_align = align; type_ptr = thunk_type_next(type_ptr); } - offset = (offset + max_align - 1) & ~(max_align - 1); + offset = ROUND_UP(offset, max_align); se->size[i] = offset; se->align[i] = max_align; #ifdef DEBUG diff --git a/ui/vnc-enc-zlib.c b/ui/vnc-enc-zlib.c index d1b97f2..eaf5832 100644 --- a/ui/vnc-enc-zlib.c +++ b/ui/vnc-enc-zlib.c @@ -33,7 +33,7 @@ void *vnc_zlib_zalloc(void *x, unsigned items, unsigned size) void *p; size *= items; - size = (size + ZALLOC_ALIGNMENT - 1) & ~(ZALLOC_ALIGNMENT - 1); + size = ROUND_UP(size, ZALLOC_ALIGNMENT); p = g_malloc0(size); diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 1524ead..3f7b2b6 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -392,7 +392,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory) } /* MAP_POPULATE silently ignores failures */ - memory = (memory + hpagesize - 1) & -hpagesize; + memory = ROUND_UP(memory, hpagesize); for (i = 0; i < (memory/hpagesize); i++) { memset(area + (hpagesize*i), 0, 1); } diff --git a/xen-mapcache.c b/xen-mapcache.c index eda914a..ab89cdc 100644 --- a/xen-mapcache.c +++ b/xen-mapcache.c @@ -133,7 +133,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque) (MCACHE_BUCKET_SHIFT - XC_PAGE_SHIFT)); size = mapcache->nr_buckets * sizeof (MapCacheEntry); - size = (size + XC_PAGE_SIZE - 1) & ~(XC_PAGE_SIZE - 1); + size = ROUND_UP(size, XC_PAGE_SIZE); DPRINTF("%s, nr_buckets = %lx size %lu\n", __func__, mapcache->nr_buckets, size); mapcache->entry = g_malloc0(size);
We already have ROUND_UP but there are similar macros and ROUND_UP-like lines all around, replace them with ROUND_UP and clean up similar macros. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- block/qcow.c | 2 +- block/vdi.c | 2 +- block/vpc.c | 2 +- bsd-user/elfload.c | 2 +- exec.c | 2 +- hw/core/loader.c | 5 ++--- hw/display/qxl.c | 7 ++----- hw/ppc/spapr.c | 2 +- kvm-all.c | 10 ++++------ linux-user/elfload.c | 2 +- linux-user/syscall.c | 4 ++-- tcg/ppc/tcg-target.c | 4 ++-- thunk.c | 4 ++-- ui/vnc-enc-zlib.c | 2 +- util/oslib-posix.c | 2 +- xen-mapcache.c | 2 +- 16 files changed, 24 insertions(+), 30 deletions(-)