Let's use QEMU_ALIGN_DOWN() and friends to make the code a bit easier to
read.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
migration/migration.c | 6 +++---
migration/postcopy-ram.c | 9 ++++-----
migration/ram.c | 2 +-
3 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index bb909781b7..ae97c2c461 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -391,7 +391,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
int migrate_send_rp_req_pages(MigrationIncomingState *mis,
RAMBlock *rb, ram_addr_t start, uint64_t haddr)
{
- void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb)));
+ void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb));
bool received = false;
WITH_QEMU_LOCK_GUARD(&mis->page_request_mutex) {
@@ -2619,8 +2619,8 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
* Since we currently insist on matching page sizes, just sanity check
* we're being asked for whole host pages.
*/
- if (start & (our_host_ps - 1) ||
- (len & (our_host_ps - 1))) {
+ if (!QEMU_IS_ALIGNED(start, our_host_ps) ||
+ !QEMU_IS_ALIGNED(len, our_host_ps)) {
error_report("%s: Misaligned page request, start: " RAM_ADDR_FMT
" len: %zd", __func__, start, len);
mark_source_rp_bad(ms);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 39e3e057b4..3f0a1f7aa6 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -402,7 +402,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
strerror(errno));
goto out;
}
- g_assert(((size_t)testarea & (pagesize - 1)) == 0);
+ g_assert(QEMU_PTR_IS_ALIGNED(testarea, pagesize));
reg_struct.range.start = (uintptr_t)testarea;
reg_struct.range.len = pagesize;
@@ -660,7 +660,7 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
struct uffdio_range range;
int ret;
trace_postcopy_wake_shared(client_addr, qemu_ram_get_idstr(rb));
- range.start = client_addr & ~(pagesize - 1);
+ range.start = QEMU_ALIGN_DOWN(client_addr, pagesize);
range.len = pagesize;
ret = ioctl(pcfd->fd, UFFDIO_WAKE, &range);
if (ret) {
@@ -702,8 +702,7 @@ static int postcopy_request_page(MigrationIncomingState *mis, RAMBlock *rb,
int postcopy_request_shared_page(struct PostCopyFD *pcfd, RAMBlock *rb,
uint64_t client_addr, uint64_t rb_offset)
{
- size_t pagesize = qemu_ram_pagesize(rb);
- uint64_t aligned_rbo = rb_offset & ~(pagesize - 1);
+ uint64_t aligned_rbo = QEMU_ALIGN_DOWN(rb_offset, qemu_ram_pagesize(rb));
MigrationIncomingState *mis = migration_incoming_get_current();
trace_postcopy_request_shared_page(pcfd->idstr, qemu_ram_get_idstr(rb),
@@ -993,7 +992,7 @@ static void *postcopy_ram_fault_thread(void *opaque)
break;
}
- rb_offset &= ~(qemu_ram_pagesize(rb) - 1);
+ rb_offset = QEMU_ALIGN_DOWN(rb_offset, qemu_ram_pagesize(rb));
trace_postcopy_ram_fault_thread_request(msg.arg.pagefault.address,
qemu_ram_get_idstr(rb),
rb_offset,
diff --git a/migration/ram.c b/migration/ram.c
index e8abe10ddb..e1c158dc92 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -811,7 +811,7 @@ static void migration_clear_memory_region_dirty_bitmap(RAMBlock *rb,
assert(shift >= 6);
size = 1ULL << (TARGET_PAGE_BITS + shift);
- start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size);
+ start = QEMU_ALIGN_DOWN((ram_addr_t)page << TARGET_PAGE_BITS, size);
trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page);
memory_region_clear_dirty_bitmap(rb->mr, start, size);
}
--
2.31.1
On Thu, Sep 02, 2021 at 03:14:30PM +0200, David Hildenbrand wrote:
> diff --git a/migration/migration.c b/migration/migration.c
> index bb909781b7..ae97c2c461 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -391,7 +391,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
> int migrate_send_rp_req_pages(MigrationIncomingState *mis,
> RAMBlock *rb, ram_addr_t start, uint64_t haddr)
> {
> - void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb)));
> + void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb));
Is uintptr_t still needed? I thought it would generate a warning otherwise but
not sure.
Also, maybe ROUND_DOWN() is better? QEMU_ALIGN_DOWN is the slow version for
arbitrary numbers.
--
Peter Xu
On 03.09.21 00:32, Peter Xu wrote:
> On Thu, Sep 02, 2021 at 03:14:30PM +0200, David Hildenbrand wrote:
>> diff --git a/migration/migration.c b/migration/migration.c
>> index bb909781b7..ae97c2c461 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -391,7 +391,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
>> int migrate_send_rp_req_pages(MigrationIncomingState *mis,
>> RAMBlock *rb, ram_addr_t start, uint64_t haddr)
>> {
>> - void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb)));
>> + void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb));
>
> Is uintptr_t still needed? I thought it would generate a warning otherwise but
> not sure.
It doesn't in my setup, but maybe it will on 32bit archs ...
I discussed this with Phil in
https://lkml.kernel.org/r/2c8d80ad-f171-7d5f-3235-92f02fa174b3@redhat.com
Maybe
QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb)));
Is really what we want.
>
> Also, maybe ROUND_DOWN() is better? QEMU_ALIGN_DOWN is the slow version for
> arbitrary numbers.
We do have exactly 2 direct users of ROUND_DOWN() in the tree (well, we
do have some more for ROUND_UP) :)
QEMU_ALIGN_DOWN vs. QEMU_ALIGN_DOWN is much easier to map and understand
IMHO, and there is usually little need to optimize.
I actually do wonder how much of a difference it actually makes on
modern CPUs ...
--
Thanks,
David / dhildenb
On 03.09.21 10:47, David Hildenbrand wrote:
> On 03.09.21 00:32, Peter Xu wrote:
>> On Thu, Sep 02, 2021 at 03:14:30PM +0200, David Hildenbrand wrote:
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index bb909781b7..ae97c2c461 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -391,7 +391,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
>>> int migrate_send_rp_req_pages(MigrationIncomingState *mis,
>>> RAMBlock *rb, ram_addr_t start, uint64_t haddr)
>>> {
>>> - void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb)));
>>> + void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb));
>>
>> Is uintptr_t still needed? I thought it would generate a warning otherwise but
>> not sure.
>
> It doesn't in my setup, but maybe it will on 32bit archs ...
>
> I discussed this with Phil in
>
> https://lkml.kernel.org/r/2c8d80ad-f171-7d5f-3235-92f02fa174b3@redhat.com
>
> Maybe
>
> QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb)));
>
> Is really what we want.
... but it would suffer the same issue I think. I just ran it trough the
gitlab pipeline, including "i386-fedora-cross-compile" ... and it seems
to compile just fine, which is weird, because I'd also expect
"warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]"
We most certainly need the "(void *)(uintptr_t)" to convert from u64 to
a pointer.
Let's just do it cleanly:
void *unaligned = (void *)(uintptr_t)haddr;
void *aligned = QEMU_ALIGN_PTR_DOWN(unaligned, qemu_ram_pagesize(rb));
Thoughts?
--
Thanks,
David / dhildenb
On 03.09.21 12:07, David Hildenbrand wrote:
> On 03.09.21 10:47, David Hildenbrand wrote:
>> On 03.09.21 00:32, Peter Xu wrote:
>>> On Thu, Sep 02, 2021 at 03:14:30PM +0200, David Hildenbrand wrote:
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index bb909781b7..ae97c2c461 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -391,7 +391,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
>>>> int migrate_send_rp_req_pages(MigrationIncomingState *mis,
>>>> RAMBlock *rb, ram_addr_t start, uint64_t haddr)
>>>> {
>>>> - void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb)));
>>>> + void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb));
>>>
>>> Is uintptr_t still needed? I thought it would generate a warning otherwise but
>>> not sure.
>>
>> It doesn't in my setup, but maybe it will on 32bit archs ...
>>
>> I discussed this with Phil in
>>
>> https://lkml.kernel.org/r/2c8d80ad-f171-7d5f-3235-92f02fa174b3@redhat.com
>>
>> Maybe
>>
>> QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb)));
>>
>> Is really what we want.
>
> ... but it would suffer the same issue I think. I just ran it trough the
> gitlab pipeline, including "i386-fedora-cross-compile" ... and it seems
> to compile just fine, which is weird, because I'd also expect
[I know, talking to my self] Some 32bit tests actually did fail later,
so the CI is able to catch this properly.
--
Thanks,
David / dhildenb
On Fri, Sep 03, 2021 at 12:07:20PM +0200, David Hildenbrand wrote:
> On 03.09.21 10:47, David Hildenbrand wrote:
> > On 03.09.21 00:32, Peter Xu wrote:
> > > On Thu, Sep 02, 2021 at 03:14:30PM +0200, David Hildenbrand wrote:
> > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > index bb909781b7..ae97c2c461 100644
> > > > --- a/migration/migration.c
> > > > +++ b/migration/migration.c
> > > > @@ -391,7 +391,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
> > > > int migrate_send_rp_req_pages(MigrationIncomingState *mis,
> > > > RAMBlock *rb, ram_addr_t start, uint64_t haddr)
> > > > {
> > > > - void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb)));
> > > > + void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb));
> > >
> > > Is uintptr_t still needed? I thought it would generate a warning otherwise but
> > > not sure.
> >
> > It doesn't in my setup, but maybe it will on 32bit archs ...
> >
> > I discussed this with Phil in
> >
> > https://lkml.kernel.org/r/2c8d80ad-f171-7d5f-3235-92f02fa174b3@redhat.com
> >
> > Maybe
> >
> > QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb)));
> >
> > Is really what we want.
>
> ... but it would suffer the same issue I think. I just ran it trough the
> gitlab pipeline, including "i386-fedora-cross-compile" ... and it seems to
> compile just fine, which is weird, because I'd also expect
>
> "warning: cast to pointer from integer of different size
> [-Wint-to-pointer-cast]"
>
> We most certainly need the "(void *)(uintptr_t)" to convert from u64 to a
> pointer.
>
> Let's just do it cleanly:
>
> void *unaligned = (void *)(uintptr_t)haddr;
> void *aligned = QEMU_ALIGN_PTR_DOWN(unaligned, qemu_ram_pagesize(rb));
>
> Thoughts?
---8<---
$ cat a.c
#include <stdio.h>
#include <time.h>
#include <assert.h>
#define ROUND_DOWN(n, d) ((n) & -(0 ? (n) : (d)))
#define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
unsigned long getns(void)
{
struct timespec tp;
clock_gettime(CLOCK_MONOTONIC, &tp);
return tp.tv_sec * 1000000000 + tp.tv_nsec;
}
void main(void)
{
int i;
unsigned long start, end, v1 = 0x1234567890, v2 = 0x1000;
start = getns();
for (i = 0; i < 1000000; i++) {
v1 = ROUND_DOWN(v1, v2);
}
end = getns();
printf("ROUND_DOWN took: \t%ld (us)\n", (end - start) / 1000);
start = getns();
for (i = 0; i < 1000000; i++) {
v1 = QEMU_ALIGN_DOWN(v1, v2);
}
end = getns();
printf("QEMU_ALIGN_DOWN took: \t%ld (us)\n", (end - start) / 1000);
}
$ make a
$ ./a
ROUND_DOWN took: 1445 (us)
QEMU_ALIGN_DOWN took: 9684 (us)
---8<---
So it's ~5 times slower here on the laptop, even if not very stable. Agree
it's not a big deal. :)
It's just that since we know it's still faster, I then second:
(uinptr_t)ROUND_DOWN(...);
Thanks,
--
Peter Xu
On 03.09.21 21:14, Peter Xu wrote:
> On Fri, Sep 03, 2021 at 12:07:20PM +0200, David Hildenbrand wrote:
>> On 03.09.21 10:47, David Hildenbrand wrote:
>>> On 03.09.21 00:32, Peter Xu wrote:
>>>> On Thu, Sep 02, 2021 at 03:14:30PM +0200, David Hildenbrand wrote:
>>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>>> index bb909781b7..ae97c2c461 100644
>>>>> --- a/migration/migration.c
>>>>> +++ b/migration/migration.c
>>>>> @@ -391,7 +391,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
>>>>> int migrate_send_rp_req_pages(MigrationIncomingState *mis,
>>>>> RAMBlock *rb, ram_addr_t start, uint64_t haddr)
>>>>> {
>>>>> - void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb)));
>>>>> + void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb));
>>>>
>>>> Is uintptr_t still needed? I thought it would generate a warning otherwise but
>>>> not sure.
>>>
>>> It doesn't in my setup, but maybe it will on 32bit archs ...
>>>
>>> I discussed this with Phil in
>>>
>>> https://lkml.kernel.org/r/2c8d80ad-f171-7d5f-3235-92f02fa174b3@redhat.com
>>>
>>> Maybe
>>>
>>> QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb)));
>>>
>>> Is really what we want.
>>
>> ... but it would suffer the same issue I think. I just ran it trough the
>> gitlab pipeline, including "i386-fedora-cross-compile" ... and it seems to
>> compile just fine, which is weird, because I'd also expect
>>
>> "warning: cast to pointer from integer of different size
>> [-Wint-to-pointer-cast]"
>>
>> We most certainly need the "(void *)(uintptr_t)" to convert from u64 to a
>> pointer.
>>
>> Let's just do it cleanly:
>>
>> void *unaligned = (void *)(uintptr_t)haddr;
>> void *aligned = QEMU_ALIGN_PTR_DOWN(unaligned, qemu_ram_pagesize(rb));
>>
>> Thoughts?
>
> ---8<---
> $ cat a.c
> #include <stdio.h>
> #include <time.h>
> #include <assert.h>
>
> #define ROUND_DOWN(n, d) ((n) & -(0 ? (n) : (d)))
> #define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
>
> unsigned long getns(void)
> {
> struct timespec tp;
>
> clock_gettime(CLOCK_MONOTONIC, &tp);
> return tp.tv_sec * 1000000000 + tp.tv_nsec;
> }
>
> void main(void)
> {
> int i;
> unsigned long start, end, v1 = 0x1234567890, v2 = 0x1000;
>
> start = getns();
> for (i = 0; i < 1000000; i++) {
> v1 = ROUND_DOWN(v1, v2);
> }
> end = getns();
> printf("ROUND_DOWN took: \t%ld (us)\n", (end - start) / 1000);
>
> start = getns();
> for (i = 0; i < 1000000; i++) {
> v1 = QEMU_ALIGN_DOWN(v1, v2);
> }
> end = getns();
> printf("QEMU_ALIGN_DOWN took: \t%ld (us)\n", (end - start) / 1000);
> }
> $ make a
> $ ./a
> ROUND_DOWN took: 1445 (us)
> QEMU_ALIGN_DOWN took: 9684 (us)
> ---8<---
>
> So it's ~5 times slower here on the laptop, even if not very stable. Agree
> it's not a big deal. :)
Same results for me, especially even if I turn v1 and v2 into global volatiles,
make sure the results won't get optimized out and compile with -03.
>
> It's just that since we know it's still faster, I then second:
>
> (uinptr_t)ROUND_DOWN(...);
Well okay then,
void *aligned = (void *)(uintptr_t)ROUND_DOWN(haddr, qemu_ram_pagesize(rb));
fits precisely into a single line :)
--
Thanks,
David / dhildenb
© 2016 - 2026 Red Hat, Inc.