control_save_page() is for RDMA only, unfold it to make the code more
clear.
In addition:
- Similar to other branches style in ram_save_target_page(), involve RDMA
only if the condition 'migrate_rdma()' is true.
- Further simplify the code by removing the RAM_SAVE_CONTROL_NOT_SUPP.
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V3:
squash previous 2nd, 3th, 4th into one patch
---
migration/ram.c | 34 +++++++---------------------------
migration/rdma.c | 7 ++-----
migration/rdma.h | 3 +--
3 files changed, 10 insertions(+), 34 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 424df6d9f13..c363034c882 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1143,32 +1143,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
return len;
}
-/*
- * @pages: the number of pages written by the control path,
- * < 0 - error
- * > 0 - number of pages written
- *
- * Return true if the pages has been saved, otherwise false is returned.
- */
-static bool control_save_page(PageSearchStatus *pss,
- ram_addr_t offset, int *pages)
-{
- int ret;
-
- ret = rdma_control_save_page(pss->pss_channel, pss->block->offset, offset,
- TARGET_PAGE_SIZE);
- if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
- return false;
- }
-
- if (ret == RAM_SAVE_CONTROL_DELAYED) {
- *pages = 1;
- return true;
- }
- *pages = ret;
- return true;
-}
-
/*
* directly send the page to the stream
*
@@ -1965,7 +1939,13 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
int res;
/* Hand over to RDMA first */
- if (control_save_page(pss, offset, &res)) {
+ if (migrate_rdma()) {
+ res = rdma_control_save_page(pss->pss_channel, pss->block->offset,
+ offset, TARGET_PAGE_SIZE);
+
+ if (res == RAM_SAVE_CONTROL_DELAYED) {
+ res = 1;
+ }
return res;
}
diff --git a/migration/rdma.c b/migration/rdma.c
index e5b4ac599b1..08eb924ffaa 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3284,14 +3284,11 @@ err:
int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size)
{
- if (!migrate_rdma()) {
- return RAM_SAVE_CONTROL_NOT_SUPP;
- }
+ assert(migrate_rdma());
int ret = qemu_rdma_save_page(f, block_offset, offset, size);
- if (ret != RAM_SAVE_CONTROL_DELAYED &&
- ret != RAM_SAVE_CONTROL_NOT_SUPP) {
+ if (ret != RAM_SAVE_CONTROL_DELAYED) {
if (ret < 0) {
qemu_file_set_error(f, ret);
}
diff --git a/migration/rdma.h b/migration/rdma.h
index f55f28bbed1..8eeb0117b91 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -33,7 +33,6 @@ void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
#define RAM_CONTROL_ROUND 1
#define RAM_CONTROL_FINISH 3
-#define RAM_SAVE_CONTROL_NOT_SUPP -1000
#define RAM_SAVE_CONTROL_DELAYED -2000
#ifdef CONFIG_RDMA
@@ -56,7 +55,7 @@ static inline
int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size)
{
- return RAM_SAVE_CONTROL_NOT_SUPP;
+ g_assert_not_reached();
}
#endif
#endif
--
2.44.0On Wed, Feb 26, 2025 at 02:30:42PM +0800, Li Zhijian wrote:
> control_save_page() is for RDMA only, unfold it to make the code more
> clear.
> In addition:
> - Similar to other branches style in ram_save_target_page(), involve RDMA
> only if the condition 'migrate_rdma()' is true.
> - Further simplify the code by removing the RAM_SAVE_CONTROL_NOT_SUPP.
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
[...]
> @@ -56,7 +55,7 @@ static inline
> int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> ram_addr_t offset, size_t size)
> {
> - return RAM_SAVE_CONTROL_NOT_SUPP;
> + g_assert_not_reached();
> }
Not sure if some compiler will be unhappy on the retval not provided, but
anyway we'll see..
Reviewed-by: Peter Xu <peterx@redhat.com>
> #endif
> #endif
> --
> 2.44.0
>
--
Peter Xu
On 26/02/2025 23:51, Peter Xu wrote:
> On Wed, Feb 26, 2025 at 02:30:42PM +0800, Li Zhijian wrote:
>> control_save_page() is for RDMA only, unfold it to make the code more
>> clear.
>> In addition:
>> - Similar to other branches style in ram_save_target_page(), involve RDMA
>> only if the condition 'migrate_rdma()' is true.
>> - Further simplify the code by removing the RAM_SAVE_CONTROL_NOT_SUPP.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>
> [...]
>
>> @@ -56,7 +55,7 @@ static inline
>> int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>> ram_addr_t offset, size_t size)
>> {
>> - return RAM_SAVE_CONTROL_NOT_SUPP;
>> + g_assert_not_reached();
>> }
>
> Not sure if some compiler will be unhappy on the retval not provided, but
> anyway we'll see..
There is no problem in fedora 40(gcc 14.2.1) and ubuntu2204(gcc 11.4.0) with --disable-rdma.
I also noticed we have a few existing same usage:
1708 bool ram_write_tracking_compatible(void)
1709 {
1710 g_assert_not_reached();
1711 }
1712
1713 int ram_write_tracking_start(void)
1714 {
1715 g_assert_not_reached();
1716 }
1717
1718 void ram_write_tracking_stop(void)
1719 {
1720 g_assert_not_reached();
1721 }
I also asked the AI/Deepseek-R1, pasted a piece of his answer
```
3. Why No Warning for Missing return? 🚨
Typical case: A non-void function missing a return triggers -Wreturn-type warnings (enabled by -Wall).
This case: The noreturn annotation ensures no execution path exists beyond g_assert_not_reached(). Because the compiler recognizes this, no warning is necessary.
Conclusion
GCC trusts the noreturn annotation of g_assert_not_reached(), recognizing that the function’s control flow ends there. Thus, no warning is emitted. For code safety, ensure assertions are active or add fallback code if needed.
```
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
>> #endif
>> #endif
>> --
>> 2.44.0
>>
>
On Thu, Feb 27, 2025 at 12:42:30AM +0000, Zhijian Li (Fujitsu) wrote:
>
>
> On 26/02/2025 23:51, Peter Xu wrote:
> > On Wed, Feb 26, 2025 at 02:30:42PM +0800, Li Zhijian wrote:
> >> control_save_page() is for RDMA only, unfold it to make the code more
> >> clear.
> >> In addition:
> >> - Similar to other branches style in ram_save_target_page(), involve RDMA
> >> only if the condition 'migrate_rdma()' is true.
> >> - Further simplify the code by removing the RAM_SAVE_CONTROL_NOT_SUPP.
> >>
> >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >
> > [...]
> >
> >> @@ -56,7 +55,7 @@ static inline
> >> int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> >> ram_addr_t offset, size_t size)
> >> {
> >> - return RAM_SAVE_CONTROL_NOT_SUPP;
> >> + g_assert_not_reached();
> >> }
> >
> > Not sure if some compiler will be unhappy on the retval not provided, but
> > anyway we'll see..
>
> There is no problem in fedora 40(gcc 14.2.1) and ubuntu2204(gcc 11.4.0) with --disable-rdma.
>
> I also noticed we have a few existing same usage:
>
> 1708 bool ram_write_tracking_compatible(void)
> 1709 {
> 1710 g_assert_not_reached();
> 1711 }
> 1712
> 1713 int ram_write_tracking_start(void)
> 1714 {
> 1715 g_assert_not_reached();
> 1716 }
> 1717
> 1718 void ram_write_tracking_stop(void)
> 1719 {
> 1720 g_assert_not_reached();
> 1721 }
Right.
The other question is what about G_DISABLE_ASSERT, then I found this:
osdep.h:
#ifdef G_DISABLE_ASSERT
#error building with G_DISABLE_ASSERT is not supported
#endif
So yeah, we should be good.
>
>
> I also asked the AI/Deepseek-R1, pasted a piece of his answer
>
> ```
> 3. Why No Warning for Missing return? 🚨
> Typical case: A non-void function missing a return triggers -Wreturn-type warnings (enabled by -Wall).
> This case: The noreturn annotation ensures no execution path exists beyond g_assert_not_reached(). Because the compiler recognizes this, no warning is necessary.
>
> Conclusion
> GCC trusts the noreturn annotation of g_assert_not_reached(), recognizing that the function’s control flow ends there. Thus, no warning is emitted. For code safety, ensure assertions are active or add fallback code if needed.
> ```
>
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> >> #endif
> >> #endif
> >> --
> >> 2.44.0
> >>
> >
--
Peter Xu
© 2016 - 2025 Red Hat, Inc.