[Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Peter Maydell]

We have a qemu_fls() function which is just a silly wrapper
around clz32() and which is used in only a handful of places
in the codebase. It turns out that all of those are really
trying to round up or down to a power of 2, which is something
we have utility functions for. This series replaces all
the qemu_fls() calls with pow2ceil() or pow2floor(), and then
removes the now-unused function.

For the case where you really want to do bit counting rather
than just power-of-2 rounding, you should use the clz/clo
functions directly.

(I've set myself a little goal of "try to do one cleanup a
day"; that may be a bit ambitious, so we'll see...)

Peter Maydell (5):
  hw/pci: Use pow2ceil() rather than hand-calculation
  hw/virtio/virtio-pci: Use pow2ceil() rather than hand-calculation
  hw/block/nvme.c: Use pow2ceil() rather than hand-calculation
  exec.c: Use pow2floor() rather than hand-calculation
  Remove unused qemu_fls function

 exec.c                 | 4 +---
 hw/block/nvme.c        | 2 +-
 hw/pci/msix.c          | 4 +---
 hw/pci/pci.c           | 4 +---
 hw/virtio/virtio-pci.c | 4 +---
 include/qemu-common.h  | 1 -
 util/cutils.c          | 5 -----
 7 files changed, 5 insertions(+), 19 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] hw/pci: Use pow2ceil() rather than hand-calculation

[Peter Maydell]

A couple of places in hw/pci use an inline calculation to round a
size up to the next largest power of 2. We have a utility routine
for this, so use it.

(The behaviour of the old code is different if the size value
is 0 -- it would leave it as 0 rather than rounding up to 1,
but in both cases we know the size can't be 0.
In the case where the size value had bit 31 set, the old code
would invoke undefined behaviour; the new code will give a
result of 0. Presumably that could never happen either.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pci/msix.c | 4 +---
 hw/pci/pci.c  | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 7716bf3..2fdada4 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -314,9 +314,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
         bar_size = bar_pba_offset + bar_pba_size;
     }
 
-    if (bar_size & (bar_size - 1)) {
-        bar_size = 1 << qemu_fls(bar_size);
-    }
+    bar_size = pow2ceil(bar_size);
 
     name = g_strdup_printf("%s-msix", dev->name);
     memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a017614..502da8d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2065,9 +2065,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
         g_free(path);
         return;
     }
-    if (size & (size - 1)) {
-        size = 1 << qemu_fls(size);
-    }
+    size = pow2ceil(size);
 
     vmsd = qdev_get_vmsd(DEVICE(pdev));
 
-- 
1.9.1

[Qemu-devel] [PATCH 2/5] hw/virtio/virtio-pci: Use pow2ceil() rather than hand-calculation

[Peter Maydell]

Use the utility function pow2ceil() for rounding up to the next
largest power of 2, rather than inline calculation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/virtio/virtio-pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 283401a..845f52f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1497,9 +1497,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
     if (legacy) {
         size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
             + virtio_bus_get_vdev_config_len(bus);
-        if (size & (size - 1)) {
-            size = 1 << qemu_fls(size);
-        }
+        size = pow2ceil(size);
 
         memory_region_init_io(&proxy->bar, OBJECT(proxy),
                               &virtio_pci_config_ops,
-- 
1.9.1

[Qemu-devel] [PATCH 3/5] hw/block/nvme.c: Use pow2ceil() rather than hand-calculation

[Peter Maydell]

Use pow2ceil() to round up to the next power of 2, rather
than an inline calculation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 40d4880..5da41b2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -805,7 +805,7 @@ static int nvme_init(PCIDevice *pci_dev)
 
     n->num_namespaces = 1;
     n->num_queues = 64;
-    n->reg_size = 1 << qemu_fls(0x1004 + 2 * (n->num_queues + 1) * 4);
+    n->reg_size = pow2ceil(0x1004 + 2 * (n->num_queues + 1) * 4);
     n->ns_size = bs_size / (uint64_t)n->num_namespaces;
 
     n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
-- 
1.9.1

[Qemu-devel] [PATCH 4/5] exec.c: Use pow2floor() rather than hand-calculation

[Peter Maydell]

Use pow2floor() to round down to the nearest power of 2,
rather than an inline calculation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/exec.c b/exec.c
index 7d60e15..4710e2d 100644
--- a/exec.c
+++ b/exec.c
@@ -2371,9 +2371,7 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
     if (l > access_size_max) {
         l = access_size_max;
     }
-    if (l & (l - 1)) {
-        l = 1 << (qemu_fls(l) - 1);
-    }
+    l = pow2floor(l);
 
     return l;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 5/5] Remove unused qemu_fls function

[Peter Maydell]

Nothing uses qemu_fls() any more, so delete it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu-common.h | 1 -
 util/cutils.c         | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 237d654..bc6f8f8 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -157,7 +157,6 @@ int stristart(const char *str, const char *val, const char **ptr);
 int qemu_strnlen(const char *s, int max_len);
 char *qemu_strsep(char **input, const char *delim);
 time_t mktimegm(struct tm *tm);
-int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
 int qemu_parse_fd(const char *param);
diff --git a/util/cutils.c b/util/cutils.c
index 5d1c9eb..43aafde 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -145,11 +145,6 @@ time_t mktimegm(struct tm *tm)
     return t;
 }
 
-int qemu_fls(int i)
-{
-    return 32 - clz32(i);
-}
-
 /*
  * Make sure data goes on disk, but if possible do not bother to
  * write out the inode just for timestamp updates.
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Paolo Bonzini]

On 23/07/2015 13:08, Peter Maydell wrote:
> We have a qemu_fls() function which is just a silly wrapper
> around clz32() and which is used in only a handful of places
> in the codebase. It turns out that all of those are really
> trying to round up or down to a power of 2, which is something
> we have utility functions for. This series replaces all
> the qemu_fls() calls with pow2ceil() or pow2floor(), and then
> removes the now-unused function.
> 
> For the case where you really want to do bit counting rather
> than just power-of-2 rounding, you should use the clz/clo
> functions directly.
> 
> (I've set myself a little goal of "try to do one cleanup a
> day"; that may be a bit ambitious, so we'll see...)

If it turns out to be ambitious, even just "record one cleanup a day in
BiteSizedTasks" would be awesome.

Paolo

Re: [Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Paolo Bonzini]

On 23/07/2015 13:08, Peter Maydell wrote:
> We have a qemu_fls() function which is just a silly wrapper
> around clz32() and which is used in only a handful of places
> in the codebase. It turns out that all of those are really
> trying to round up or down to a power of 2, which is something
> we have utility functions for. This series replaces all
> the qemu_fls() calls with pow2ceil() or pow2floor(), and then
> removes the now-unused function.

The series looks good, but I'd prefer (especially for patch 4 which is
in a fast path) if pow2ceil and pow2floor were made inline.

BTW,

    return 1ULL << (64 - nlz);

can be implemented as "rotate_right(1ULL, nlz)" (the latter is (1ULL <<
(64 - nlz)) | (1ULL >> nlz), which only differs in the undefined case
nlz == 0).  However, this is probably something for the compiler to
implement, not really something to do in our sources.

Paolo

> For the case where you really want to do bit counting rather
> than just power-of-2 rounding, you should use the clz/clo
> functions directly.
> 
> (I've set myself a little goal of "try to do one cleanup a
> day"; that may be a bit ambitious, so we'll see...)
> 
> Peter Maydell (5):
>   hw/pci: Use pow2ceil() rather than hand-calculation
>   hw/virtio/virtio-pci: Use pow2ceil() rather than hand-calculation
>   hw/block/nvme.c: Use pow2ceil() rather than hand-calculation
>   exec.c: Use pow2floor() rather than hand-calculation
>   Remove unused qemu_fls function
> 
>  exec.c                 | 4 +---
>  hw/block/nvme.c        | 2 +-
>  hw/pci/msix.c          | 4 +---
>  hw/pci/pci.c           | 4 +---
>  hw/virtio/virtio-pci.c | 4 +---
>  include/qemu-common.h  | 1 -
>  util/cutils.c          | 5 -----
>  7 files changed, 5 insertions(+), 19 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Peter Maydell]

On 23 July 2015 at 17:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 23/07/2015 13:08, Peter Maydell wrote:
>> We have a qemu_fls() function which is just a silly wrapper
>> around clz32() and which is used in only a handful of places
>> in the codebase. It turns out that all of those are really
>> trying to round up or down to a power of 2, which is something
>> we have utility functions for. This series replaces all
>> the qemu_fls() calls with pow2ceil() or pow2floor(), and then
>> removes the now-unused function.
>
> The series looks good, but I'd prefer (especially for patch 4 which is
> in a fast path) if pow2ceil and pow2floor were made inline.

Yeah, I was wondering if I was going to be asked to do that.
Note that qemu_fls() is/was *not* inline, though, so you're
taking an out-of-line function call on these code paths
already.

-- PMM

Re: [Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Paolo Bonzini]

On 23/07/2015 22:10, Peter Maydell wrote:
>> > The series looks good, but I'd prefer (especially for patch 4 which is
>> > in a fast path) if pow2ceil and pow2floor were made inline.
> Yeah, I was wondering if I was going to be asked to do that.
> Note that qemu_fls() is/was *not* inline, though, so you're
> taking an out-of-line function call on these code paths
> already.

But in patch 4 the call is only in the rare case where "l & (l - 1)" is
not zero.

Paolo

Re: [Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Peter Maydell]

On 24 July 2015 at 06:10, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 23/07/2015 22:10, Peter Maydell wrote:
>>> > The series looks good, but I'd prefer (especially for patch 4 which is
>>> > in a fast path) if pow2ceil and pow2floor were made inline.
>> Yeah, I was wondering if I was going to be asked to do that.
>> Note that qemu_fls() is/was *not* inline, though, so you're
>> taking an out-of-line function call on these code paths
>> already.
>
> But in patch 4 the call is only in the rare case where "l & (l - 1)" is
> not zero.

True. Any preferences for which header file to put them in?
qemu-common.h is where the prototype is currently and where
the inline is_power_of_2() is defined...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] replace qemu_fls() with pow2ceil()/pow2floor()

[Paolo Bonzini]

On 24/07/2015 10:39, Peter Maydell wrote:
>> > But in patch 4 the call is only in the rare case where "l & (l - 1)" is
>> > not zero.
> True. Any preferences for which header file to put them in?
> qemu-common.h is where the prototype is currently and where
> the inline is_power_of_2() is defined...

Either there, or host-utils.h.

Paolo