[PATCHv3,14/30] page-xfer: dump compatible iovec

Submitted by Pavel Emelianov on July 5, 2016, 11:35 a.m.

Details

Message ID 577B9B99.90101@virtuozzo.com
State Rejected
Series "x86 Compatible C/R, part 2"
Headers show

Commit Message

Pavel Emelianov July 5, 2016, 11:35 a.m.
>>> ========================== Run zdtm/static/env00 in h ==========================
>>> Start test
>>> ./env00 --pidfile=env00.pid --outfile=env00.out --envname=ENV_00_TEST
>>> Run criu pre-dump
>>> Run criu pre-dump
>>> Run criu dump
>>> Run criu restore
>>> =[log]=> dump/zdtm/static/env00/24/3/restore.log
>>> ------------------------ grep Error ------------------------
>>> (00.035223)     24: Error (mem.c:826): Page entry address 400000 outside of VMA 7fffddbcf000-7fffddbd1000
>>> (00.035323) Error (cr-restore.c:1885): Restoring FAILED.
>>> ------------------------ ERROR OVER ------------------------
>>> ################# Test zdtm/static/env00 FAIL at CRIU restore ##################
>>> ##################################### FAIL #####################################
>>>
>>> https://github.com/xemul/criu/issues/185
>>
>> The pagemap image for test becomes unsorted.
> 
> Yes, thanks, I have reproduced and also found image unsorted.
> Working on fix.



Is it just like this?

-- Pavel

Patch hide | download patch | download mbox

diff --git a/criu/page-xfer.c b/criu/page-xfer.c
index 8c2c799..967a587 100644
--- a/criu/page-xfer.c
+++ b/criu/page-xfer.c
@@ -370,7 +370,7 @@  int page_xfer_dump_pages(struct page_xfer *xfer, struct page_pipe *pp,
                                struct iovec hole = get_iov(pp->holes, cur_hole,
                                                pp->compat_iov);
 
-                               if (hole.iov_base < iov.iov_base)
+                               if (hole.iov_base >= iov.iov_base)
                                        break;
                                ret = page_xfer_dump_hole(xfer, &hole, off);
                                if (ret)

Comments

Dmitry Safonov July 5, 2016, 11:35 a.m.
On 07/05/2016 02:35 PM, Pavel Emelyanov wrote:
>
>>>> ========================== Run zdtm/static/env00 in h ==========================
>>>> Start test
>>>> ./env00 --pidfile=env00.pid --outfile=env00.out --envname=ENV_00_TEST
>>>> Run criu pre-dump
>>>> Run criu pre-dump
>>>> Run criu dump
>>>> Run criu restore
>>>> =[log]=> dump/zdtm/static/env00/24/3/restore.log
>>>> ------------------------ grep Error ------------------------
>>>> (00.035223)     24: Error (mem.c:826): Page entry address 400000 outside of VMA 7fffddbcf000-7fffddbd1000
>>>> (00.035323) Error (cr-restore.c:1885): Restoring FAILED.
>>>> ------------------------ ERROR OVER ------------------------
>>>> ################# Test zdtm/static/env00 FAIL at CRIU restore ##################
>>>> ##################################### FAIL #####################################
>>>>
>>>> https://github.com/xemul/criu/issues/185
>>>
>>> The pagemap image for test becomes unsorted.
>>
>> Yes, thanks, I have reproduced and also found image unsorted.
>> Working on fix.
>
>
> diff --git a/criu/page-xfer.c b/criu/page-xfer.c
> index 8c2c799..967a587 100644
> --- a/criu/page-xfer.c
> +++ b/criu/page-xfer.c
> @@ -370,7 +370,7 @@ int page_xfer_dump_pages(struct page_xfer *xfer, struct page_pipe *pp,
>                                 struct iovec hole = get_iov(pp->holes, cur_hole,
>                                                 pp->compat_iov);
>
> -                               if (hole.iov_base < iov.iov_base)
> +                               if (hole.iov_base >= iov.iov_base)
>                                         break;
>                                 ret = page_xfer_dump_hole(xfer, &hole, off);
>                                 if (ret)
>
> Is it just like this?
>

Oh, right, I was starring at this and wondering, why it's skipped.
Big thanks, sorry for this typo.
Dmitry Safonov July 5, 2016, 11:47 a.m.
On 07/05/2016 02:35 PM, Pavel Emelyanov wrote:
>
>>>> ========================== Run zdtm/static/env00 in h ==========================
>>>> Start test
>>>> ./env00 --pidfile=env00.pid --outfile=env00.out --envname=ENV_00_TEST
>>>> Run criu pre-dump
>>>> Run criu pre-dump
>>>> Run criu dump
>>>> Run criu restore
>>>> =[log]=> dump/zdtm/static/env00/24/3/restore.log
>>>> ------------------------ grep Error ------------------------
>>>> (00.035223)     24: Error (mem.c:826): Page entry address 400000 outside of VMA 7fffddbcf000-7fffddbd1000
>>>> (00.035323) Error (cr-restore.c:1885): Restoring FAILED.
>>>> ------------------------ ERROR OVER ------------------------
>>>> ################# Test zdtm/static/env00 FAIL at CRIU restore ##################
>>>> ##################################### FAIL #####################################
>>>>
>>>> https://github.com/xemul/criu/issues/185
>>>
>>> The pagemap image for test becomes unsorted.
>>
>> Yes, thanks, I have reproduced and also found image unsorted.
>> Working on fix.
>
>
> diff --git a/criu/page-xfer.c b/criu/page-xfer.c
> index 8c2c799..967a587 100644
> --- a/criu/page-xfer.c
> +++ b/criu/page-xfer.c
> @@ -370,7 +370,7 @@ int page_xfer_dump_pages(struct page_xfer *xfer, struct page_pipe *pp,
>                                 struct iovec hole = get_iov(pp->holes, cur_hole,
>                                                 pp->compat_iov);
>
> -                               if (hole.iov_base < iov.iov_base)
> +                               if (hole.iov_base >= iov.iov_base)
>                                         break;
>                                 ret = page_xfer_dump_hole(xfer, &hole, off);
>                                 if (ret)
>
> Is it just like this?


And while we on it, don't you mind, if I change:
static struct iovec get_iov(...)
to this:
static struct iovec *get_iov(...)

And introduce put_iov(struct iovec *, bool compat)
to eleminate needless copy here.
On the moment of patch I wanted to return pointer to static
iovec_compat, but found the need in reentrancy, so left
coping here, but it's pretty annoing.
Dmitry Safonov July 5, 2016, 11:48 a.m.
On 07/05/2016 02:47 PM, Dmitry Safonov wrote:
> On 07/05/2016 02:35 PM, Pavel Emelyanov wrote:
>>
>>>>> ========================== Run zdtm/static/env00 in h
>>>>> ==========================
>>>>> Start test
>>>>> ./env00 --pidfile=env00.pid --outfile=env00.out --envname=ENV_00_TEST
>>>>> Run criu pre-dump
>>>>> Run criu pre-dump
>>>>> Run criu dump
>>>>> Run criu restore
>>>>> =[log]=> dump/zdtm/static/env00/24/3/restore.log
>>>>> ------------------------ grep Error ------------------------
>>>>> (00.035223)     24: Error (mem.c:826): Page entry address 400000
>>>>> outside of VMA 7fffddbcf000-7fffddbd1000
>>>>> (00.035323) Error (cr-restore.c:1885): Restoring FAILED.
>>>>> ------------------------ ERROR OVER ------------------------
>>>>> ################# Test zdtm/static/env00 FAIL at CRIU restore
>>>>> ##################
>>>>> ##################################### FAIL
>>>>> #####################################
>>>>>
>>>>> https://github.com/xemul/criu/issues/185
>>>>
>>>> The pagemap image for test becomes unsorted.
>>>
>>> Yes, thanks, I have reproduced and also found image unsorted.
>>> Working on fix.
>>
>>
>> diff --git a/criu/page-xfer.c b/criu/page-xfer.c
>> index 8c2c799..967a587 100644
>> --- a/criu/page-xfer.c
>> +++ b/criu/page-xfer.c
>> @@ -370,7 +370,7 @@ int page_xfer_dump_pages(struct page_xfer *xfer,
>> struct page_pipe *pp,
>>                                 struct iovec hole = get_iov(pp->holes,
>> cur_hole,
>>                                                 pp->compat_iov);
>>
>> -                               if (hole.iov_base < iov.iov_base)
>> +                               if (hole.iov_base >= iov.iov_base)
>>                                         break;
>>                                 ret = page_xfer_dump_hole(xfer, &hole,
>> off);
>>                                 if (ret)
>>
>> Is it just like this?
>
>
> And while we on it, don't you mind, if I change:
> static struct iovec get_iov(...)
> to this:
> static struct iovec *get_iov(...)
>
> And introduce put_iov(struct iovec *, bool compat)
> to eleminate needless copy here.
> On the moment of patch I wanted to return pointer to static
> iovec_compat, but found the need in reentrancy, so left
> coping here, but it's pretty annoing.

Or maybe it's not worth?
Just coping two pointers/numbers and additional function call
is quite the same.
Pavel Emelianov July 5, 2016, 12:11 p.m.
On 07/05/2016 02:47 PM, Dmitry Safonov wrote:
> On 07/05/2016 02:35 PM, Pavel Emelyanov wrote:
>>
>>>>> ========================== Run zdtm/static/env00 in h ==========================
>>>>> Start test
>>>>> ./env00 --pidfile=env00.pid --outfile=env00.out --envname=ENV_00_TEST
>>>>> Run criu pre-dump
>>>>> Run criu pre-dump
>>>>> Run criu dump
>>>>> Run criu restore
>>>>> =[log]=> dump/zdtm/static/env00/24/3/restore.log
>>>>> ------------------------ grep Error ------------------------
>>>>> (00.035223)     24: Error (mem.c:826): Page entry address 400000 outside of VMA 7fffddbcf000-7fffddbd1000
>>>>> (00.035323) Error (cr-restore.c:1885): Restoring FAILED.
>>>>> ------------------------ ERROR OVER ------------------------
>>>>> ################# Test zdtm/static/env00 FAIL at CRIU restore ##################
>>>>> ##################################### FAIL #####################################
>>>>>
>>>>> https://github.com/xemul/criu/issues/185
>>>>
>>>> The pagemap image for test becomes unsorted.
>>>
>>> Yes, thanks, I have reproduced and also found image unsorted.
>>> Working on fix.
>>
>>
>> diff --git a/criu/page-xfer.c b/criu/page-xfer.c
>> index 8c2c799..967a587 100644
>> --- a/criu/page-xfer.c
>> +++ b/criu/page-xfer.c
>> @@ -370,7 +370,7 @@ int page_xfer_dump_pages(struct page_xfer *xfer, struct page_pipe *pp,
>>                                 struct iovec hole = get_iov(pp->holes, cur_hole,
>>                                                 pp->compat_iov);
>>
>> -                               if (hole.iov_base < iov.iov_base)
>> +                               if (hole.iov_base >= iov.iov_base)
>>                                         break;
>>                                 ret = page_xfer_dump_hole(xfer, &hole, off);
>>                                 if (ret)
>>
>> Is it just like this?
> 
> 
> And while we on it, don't you mind, if I change:
> static struct iovec get_iov(...)
> to this:
> static struct iovec *get_iov(...)

From my perspective if we want optimization like this we'd better do 

static void get_iov(struct iov *into, ...);

;)

-- Pavel

> And introduce put_iov(struct iovec *, bool compat)
> to eleminate needless copy here.
> On the moment of patch I wanted to return pointer to static
> iovec_compat, but found the need in reentrancy, so left
> coping here, but it's pretty annoing.
> .
>
Dmitry Safonov July 5, 2016, 12:18 p.m.
On 07/05/2016 03:11 PM, Pavel Emelyanov wrote:
> On 07/05/2016 02:47 PM, Dmitry Safonov wrote:
>> And while we on it, don't you mind, if I change:
>> static struct iovec get_iov(...)
>> to this:
>> static struct iovec *get_iov(...)
>
> From my perspective if we want optimization like this we'd better do
>
> static void get_iov(struct iov *into, ...);
>
> ;)
>

Yep, that's better. I'll make a patch, thanks again :)