| Message ID | 20190606013138.13312-3-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xbzrle: improve readability a little | expand |
* 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
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 --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; }
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(-)