diff mbox series

[2/2] migration/xbzrle: cleanup the handling cache miss condition

Message ID 20190606013138.13312-3-richardw.yang@linux.intel.com
State New, archived
Headers show
Series xbzrle: improve readability a little | expand

Commit Message

Wei Yang June 6, 2019, 1:31 a.m. UTC
For cache miss condition not in last_stage, we need to insert data into
cache. When this step succeed, current_data should be updated. While no
matter these checks pass or not, -1 is returned.

Based on this, the logic in cache miss handling could be simplified a
little.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Dr. David Alan Gilbert June 7, 2019, 7:01 p.m. UTC | #1
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> For cache miss condition not in last_stage, we need to insert data into
> cache. When this step succeed, current_data should be updated. While no
> matter these checks pass or not, -1 is returned.
> 
> Based on this, the logic in cache miss handling could be simplified a
> little.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 878cd8de7a..67ba075cc4 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1572,15 +1572,14 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>      if (!cache_is_cached(XBZRLE.cache, current_addr,
>                           ram_counters.dirty_sync_count)) {
>          xbzrle_counters.cache_miss++;
> -        if (!last_stage) {
> -            if (cache_insert(XBZRLE.cache, current_addr, *current_data,
> -                             ram_counters.dirty_sync_count) == -1) {
> -                return -1;
> -            } else {
> -                /* update *current_data when the page has been
> -                   inserted into cache */
> -                *current_data = get_cached_data(XBZRLE.cache, current_addr);
> -            }
> +        if (!last_stage &&
> +            !cache_insert(XBZRLE.cache, current_addr, *current_data,
> +                          ram_counters.dirty_sync_count)) {
> +            /*
> +             * update *current_data when the page has been inserted into
> +             * cache
> +             */
> +            *current_data = get_cached_data(XBZRLE.cache, current_addr);

No this change doesn't gain anything and I find the original easier to
read.

This function is really subtle, every time I do anything with it I have
to think really hard about it, so ease of reading is more important.

Dave

>          }
>          return -1;
>      }
> -- 
> 2.19.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Wei Yang June 9, 2019, 7:49 p.m. UTC | #2
On Fri, Jun 07, 2019 at 08:01:14PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> For cache miss condition not in last_stage, we need to insert data into
>> cache. When this step succeed, current_data should be updated. While no
>> matter these checks pass or not, -1 is returned.
>> 
>> Based on this, the logic in cache miss handling could be simplified a
>> little.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/ram.c | 17 ++++++++---------
>>  1 file changed, 8 insertions(+), 9 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 878cd8de7a..67ba075cc4 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1572,15 +1572,14 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>>      if (!cache_is_cached(XBZRLE.cache, current_addr,
>>                           ram_counters.dirty_sync_count)) {
>>          xbzrle_counters.cache_miss++;
>> -        if (!last_stage) {
>> -            if (cache_insert(XBZRLE.cache, current_addr, *current_data,
>> -                             ram_counters.dirty_sync_count) == -1) {
>> -                return -1;
>> -            } else {
>> -                /* update *current_data when the page has been
>> -                   inserted into cache */
>> -                *current_data = get_cached_data(XBZRLE.cache, current_addr);
>> -            }
>> +        if (!last_stage &&
>> +            !cache_insert(XBZRLE.cache, current_addr, *current_data,
>> +                          ram_counters.dirty_sync_count)) {
>> +            /*
>> +             * update *current_data when the page has been inserted into
>> +             * cache
>> +             */
>> +            *current_data = get_cached_data(XBZRLE.cache, current_addr);
>
>No this change doesn't gain anything and I find the original easier to
>read.
>
>This function is really subtle, every time I do anything with it I have
>to think really hard about it, so ease of reading is more important.
>

Yep, I agree ease of reading is more important.

Since the original version looks better, I will keep current code. :-)

>Dave
>
>>          }
>>          return -1;
>>      }
>> -- 
>> 2.19.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

diff --git a/migration/ram.c b/migration/ram.c
index 878cd8de7a..67ba075cc4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1572,15 +1572,14 @@  static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
     if (!cache_is_cached(XBZRLE.cache, current_addr,
                          ram_counters.dirty_sync_count)) {
         xbzrle_counters.cache_miss++;
-        if (!last_stage) {
-            if (cache_insert(XBZRLE.cache, current_addr, *current_data,
-                             ram_counters.dirty_sync_count) == -1) {
-                return -1;
-            } else {
-                /* update *current_data when the page has been
-                   inserted into cache */
-                *current_data = get_cached_data(XBZRLE.cache, current_addr);
-            }
+        if (!last_stage &&
+            !cache_insert(XBZRLE.cache, current_addr, *current_data,
+                          ram_counters.dirty_sync_count)) {
+            /*
+             * update *current_data when the page has been inserted into
+             * cache
+             */
+            *current_data = get_cached_data(XBZRLE.cache, current_addr);
         }
         return -1;
     }