| Message ID | 20190606013138.13312-2-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | xbzrle: improve readability a little | expand |
* Wei Yang (richardw.yang@linux.intel.com) wrote: > When we are not in the last_stage, we need to update the cache if page > is not the same. > > Currently this procedure is scattered in two places and mixed with > encoding status check. > > This patch extract this general step out to make the code a little bit > easy to read. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> This function is actually quite subtle, and I think your change will work, but it has changed the behaviour slightly. When we enter the function the *current_data is pointing at actual guest RAM and is changing as we're running. It's critical that the contents of the cache match what was actually sent, so that in the next cycle the correct delta is generated; thus the reason for the two memcpy's is actually different. > --- > migration/ram.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index e9b40d636d..878cd8de7a 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1594,25 +1594,24 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, > encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf, > TARGET_PAGE_SIZE, XBZRLE.encoded_buf, > TARGET_PAGE_SIZE); > + > + /* > + * we need to update the data in the cache, in order to get the same data > + */ > + if (!last_stage && encoded_len != 0) { > + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); > + *current_data = prev_cached_page; > + } > + > if (encoded_len == 0) { > trace_save_xbzrle_page_skipping(); > return 0; > } else if (encoded_len == -1) { > trace_save_xbzrle_page_overflow(); > xbzrle_counters.overflow++; > - /* update data in the cache */ > - if (!last_stage) { > - memcpy(prev_cached_page, *current_data, TARGET_PAGE_SIZE); > - *current_data = prev_cached_page; > - } > return -1; In this case, we've not managed to compress, so we're going to have to transmit the whole page; but remember the guest is still writing. So we update the cache, and then update *current_data to point to the cache so that the caller sends from the cache directly rather than the guest RAM, this ensures that the thing that's sent matches the cache contents. However, note the memcpy is from *current_data, not XBZRLE.current_buf, so it might be slightly newer - this is the first change you have made; you might be sending data that's slightly older; but it's safe because the data sent does match the cache contents. > } > > - /* we need to update the data in the cache, in order to get the same data */ > - if (!last_stage) { > - memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); > - } > - This memcpy is slightly different. Here we've managed to do the compress; so now we update the cache based on what was compressed (current_buf). *current_data isn't used by the caller in this case because it's actually sending the compressed data. So it's safe for you to update it. So I think we need to add comments like that, how about: /* * Update the cache contents, so that it corresponds to the data * sent, in allc ases except where we skip the page. */ > + if (!last_stage && encoded_len != 0) { > + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); /* * In the case where we couldn't compress, ensure that the caller * sends the data from the cache, since the guest might have * changed the RAM since we copied it. */ > + *current_data = prev_cached_page; > + } > /* Send XBZRLE based compressed page */ > bytes_xbzrle = save_page_header(rs, rs->f, block, > offset | RAM_SAVE_FLAG_XBZRLE); Dave > -- > 2.19.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jun 07, 2019 at 07:57:34PM +0100, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.yang@linux.intel.com) wrote: >> When we are not in the last_stage, we need to update the cache if page >> is not the same. >> >> Currently this procedure is scattered in two places and mixed with >> encoding status check. >> >> This patch extract this general step out to make the code a little bit >> easy to read. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > >This function is actually quite subtle, and I think your change will >work, but it has changed the behaviour slightly. > >When we enter the function the *current_data is pointing at actual guest >RAM and is changing as we're running. >It's critical that the contents of the cache match what was actually >sent, so that in the next cycle the correct delta is generated; >thus the reason for the two memcpy's is actually different. > >> --- >> migration/ram.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index e9b40d636d..878cd8de7a 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -1594,25 +1594,24 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, >> encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf, >> TARGET_PAGE_SIZE, XBZRLE.encoded_buf, >> TARGET_PAGE_SIZE); >> + >> + /* >> + * we need to update the data in the cache, in order to get the same data >> + */ >> + if (!last_stage && encoded_len != 0) { >> + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); >> + *current_data = prev_cached_page; >> + } >> + >> if (encoded_len == 0) { >> trace_save_xbzrle_page_skipping(); >> return 0; >> } else if (encoded_len == -1) { >> trace_save_xbzrle_page_overflow(); >> xbzrle_counters.overflow++; >> - /* update data in the cache */ >> - if (!last_stage) { >> - memcpy(prev_cached_page, *current_data, TARGET_PAGE_SIZE); >> - *current_data = prev_cached_page; >> - } >> return -1; > >In this case, we've not managed to compress, so we're going to have to >transmit the whole page; but remember the guest is still writing. So we >update the cache, and then update *current_data to point to the cache >so that the caller sends from the cache directly rather than the guest >RAM, this ensures that the thing that's sent matches the cache contents. > >However, note the memcpy is from *current_data, not XBZRLE.current_buf, >so it might be slightly newer - this is the first change you have made; >you might be sending data that's slightly older; but it's safe because >the data sent does match the cache contents. > >> } >> >> - /* we need to update the data in the cache, in order to get the same data */ >> - if (!last_stage) { >> - memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); >> - } >> - > >This memcpy is slightly different. >Here we've managed to do the compress; so now we update the cache based >on what was compressed (current_buf). *current_data isn't used by the >caller in this case because it's actually sending the compressed data. >So it's safe for you to update it. Yes, you are right. My change will alter the behavior a little. To be specific, when we didn't manage to compress content, the content we sent will be a little *old*. > >So I think we need to add comments like that, how about: > > /* > * Update the cache contents, so that it corresponds to the data > * sent, in allc ases except where we skip the page. > */ >> + if (!last_stage && encoded_len != 0) { >> + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); > /* > * In the case where we couldn't compress, ensure that the caller > * sends the data from the cache, since the guest might have > * changed the RAM since we copied it. > */ > >> + *current_data = prev_cached_page; >> + } >> /* Send XBZRLE based compressed page */ >> bytes_xbzrle = save_page_header(rs, rs->f, block, >> offset | RAM_SAVE_FLAG_XBZRLE); Yep, I agree with this comment. Let me add this in v2. Thanks :-) > >Dave > >> -- >> 2.19.1 >> >-- >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/ram.c b/migration/ram.c index e9b40d636d..878cd8de7a 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1594,25 +1594,24 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE, XBZRLE.encoded_buf, TARGET_PAGE_SIZE); + + /* + * we need to update the data in the cache, in order to get the same data + */ + if (!last_stage && encoded_len != 0) { + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); + *current_data = prev_cached_page; + } + if (encoded_len == 0) { trace_save_xbzrle_page_skipping(); return 0; } else if (encoded_len == -1) { trace_save_xbzrle_page_overflow(); xbzrle_counters.overflow++; - /* update data in the cache */ - if (!last_stage) { - memcpy(prev_cached_page, *current_data, TARGET_PAGE_SIZE); - *current_data = prev_cached_page; - } return -1; } - /* we need to update the data in the cache, in order to get the same data */ - if (!last_stage) { - memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); - } - /* Send XBZRLE based compressed page */ bytes_xbzrle = save_page_header(rs, rs->f, block, offset | RAM_SAVE_FLAG_XBZRLE);
When we are not in the last_stage, we need to update the cache if page is not the same. Currently this procedure is scattered in two places and mixed with encoding status check. This patch extract this general step out to make the code a little bit easy to read. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- migration/ram.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)