Re: [Qemu-devel] [PATCH COLO-Frame v11 14/39] ram: Split host_from_stream_offset() into two helper functions

[Hailiang Zhang]

On 2015/12/2 2:19, Dr. David Alan Gilbert wrote:
> * zhanghailiang (address@hidden) wrote:
> > Split host_from_stream_offset() into two parts:
> > One is to get ram block, which the block idstr may be get from migration
> > stream, the other is to get hva (host) address from block and the offset.
> >
> > Signed-off-by: zhanghailiang <address@hidden>
>
> OK, I see why you're doing this from the next patch.
>
> > ---
> > v11:
> > - New patch
> > ---
> >   migration/ram.c | 29 +++++++++++++++++++++--------
> >   1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index cfe78aa..a161620 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2136,9 +2136,9 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void 
> > *host)
> >    * offset: Offset within the block
> >    * flags: Page flags (mostly to see if it's a continuation of previous block)
> >    */
> > -static inline void *host_from_stream_offset(QEMUFile *f,
> > -                                            ram_addr_t offset,
> > -                                            int flags)
> > +static inline RAMBlock *ram_block_from_stream(QEMUFile *f,
> > +                                              ram_addr_t offset,
> > +                                              int flags)
> >   {
> >       static RAMBlock *block = NULL;
> >       char id[256];
> > @@ -2150,22 +2150,31 @@ static inline void *host_from_stream_offset(QEMUFile *f,
> >               return NULL;
> >           }
> >
> > -        return block->host + offset;
> > +        return block;
> >       }
> > -
> >       len = qemu_get_byte(f);
> >       qemu_get_buffer(f, (uint8_t *)id, len);
> >       id[len] = 0;
> >
> >       block = qemu_ram_block_by_name(id);
> >       if (block && block->max_length > offset) {
> > -        return block->host + offset;
> > +        return block;
> >       }
> >
> >       error_report("Can't find block %s", id);
> >       return NULL;
> >   }
> >
> > +static inline void *host_from_ram_block_offset(RAMBlock *block,
> > +                                               ram_addr_t offset)
> > +{
> > +    if (!block) {
> > +        return NULL;
> > +    }
> > +
> > +    return block->host + offset;
> > +}
>
> That's almost the same as ramblock_ptr in include/exec/ram_addr.h, but
> it assert's rather than doing NULL on errors.

Hmm, i didn't notice that before.

> I'm not sure about this, but can I suggest:
>
>     ram_block_from_stream(QEMUFile *f, int flags)
>
>    doesn't have the offset; just finds the block and handles the CONT.
>
>     bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset);
>
>    actually does the check; put this in exec.c, and declare it in 
> include/exec/ram_addr.h
>
>     void *ramblock_ptr_try(RAMBlock *block, ram_addr_t offset)
>    which returns NULL if offset_in_ramblock fails, and otherwise returns the 
> result
> of ramblock_ptr - again put that in include/exec/ram_addr.h

That sounds good. I will add the ramblock_ptr_try() and keep the 
ramblock_ptr(). They
have different behavior when the check failed. I'm not supposed to break it.

> (I'm not sure about this - I almost suggested changing ramblock_ptr to not do
> the checks, and just add a call to assert(offset_in_ramblock) before each use, 
> but
> that sounded too painful).

Yes, that's not so good.

> Hmm - we check here for block->max_length > offset - where as the check in
> ram_addr.h is used_length - I wonder if we should be using used_length?

Er, i think we should use used_length instead of max_length.

Thanks,
zhanghailiang

> Dave
>
> > +
> >   /*
> >    * If a page (or a whole RDMA chunk) has been
> >    * determined to be zero, then zap it.
> > @@ -2310,7 +2319,9 @@ static int ram_load_postcopy(QEMUFile *f)
> >           trace_ram_load_postcopy_loop((uint64_t)addr, flags);
> >           place_needed = false;
> >           if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE)) {
> > -            host = host_from_stream_offset(f, addr, flags);
> > +            RAMBlock *block = ram_block_from_stream(f, addr, flags);
> > +
> > +            host = host_from_ram_block_offset(block, addr);
> >               if (!host) {
> >                   error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> >                   ret = -EINVAL;
> > @@ -2441,7 +2452,9 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> > version_id)
> >
> >           if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
> >                        RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
> > -            host = host_from_stream_offset(f, addr, flags);
> > +            RAMBlock *block = ram_block_from_stream(f, addr, flags);
> > +
> > +            host = host_from_ram_block_offset(block, addr);
> >               if (!host) {
> >                   error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> >                   ret = -EINVAL;
> > --
> > 1.8.3.1
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>
> .