[0/5] Speed up kdat checks (v2)

Submitted by Andrey Vagin on April 27, 2017, 6:26 a.m.

Details

Message ID 20170427062625.GA28504@outlook.office365.com
State New
Headers show

Patch hide | download patch | download mbox

diff --git a/criu/Makefile.crtools b/criu/Makefile.crtools
index 03756ae..0815519 100644
--- a/criu/Makefile.crtools
+++ b/criu/Makefile.crtools
@@ -1,5 +1,6 @@ 
 ccflags-y              += -iquote criu/$(ARCH)
 ccflags-y              += $(COMPEL_UAPI_INCLUDES)
+CFLAGS_kerndat.o       += -DKDAT_MAGIC_2=${shell date +%s}
 CFLAGS_REMOVE_clone-noasan.o += $(CFLAGS-ASAN)
 ldflags-y              += -r
 
diff --git a/criu/include/kerndat.h b/criu/include/kerndat.h
index 84d0707..1d3cf70 100644
--- a/criu/include/kerndat.h
+++ b/criu/include/kerndat.h
@@ -35,6 +35,7 @@  struct kerndat_s {
         * value to avoid loading of bad kdat bits.
         */
        u32 magic;
+       u32 magic2;
        dev_t shmem_dev;
        int last_cap;
        u64 zero_page_pfn;
diff --git a/criu/kerndat.c b/criu/kerndat.c
index d0f0d2b..a07f916 100644
--- a/criu/kerndat.c
+++ b/criu/kerndat.c
@@ -678,7 +678,8 @@  static int kerndat_try_load_cache(void)
 
        close(fd);
 
-       if (ret != sizeof(kdat) || kdat.magic != KDAT_MAGIC_1) {
+       if (ret != sizeof(kdat) ||
+           kdat.magic != KDAT_MAGIC_1 || kdat.magic2 != KDAT_MAGIC_2) {
                pr_warn("Stale %s file\n", KERNDAT_CACHE_FILE);
                unlink(KERNDAT_CACHE_FILE);
                return 1;
@@ -719,6 +720,7 @@  static int kerndat_save_cache(void)
        }
 
        kdat.magic = KDAT_MAGIC_1;
+       kdat.magic2 = KDAT_MAGIC_2;
        ret = write(fd, &kdat, sizeof(kdat));
        if (ret != sizeof(kdat))
                pr_warn("Kerndat cache writing failed.\n");

Comments

Dmitry Safonov April 27, 2017, 9:02 a.m.
2017-04-27 9:26 GMT+03:00 Andrei Vagin <avagin@virtuozzo.com>:
> On Wed, Apr 26, 2017 at 02:05:52PM +0300, Dmitry Safonov wrote:
>> 2017-04-26 13:26 GMT+03:00 Dmitry Safonov <0x7f454c46@gmail.com>:
>> > 2017-04-26 12:23 GMT+03:00 Pavel Emelyanov <xemul@virtuozzo.com>:
>> >> In the previous discussion it was suggested not to compile-in
>> >> the kdat bits, but rather cache them in some place, that gets
>> >> evicted on node reboot.
>> >>
>> >> Here's how this can look like.
>> >>
>> >> Briefly -- when kerndat.c has finished auto-detection is writes
>> >> the kdat variable into the /run/criu.kdat file. On the next
>> >> start the file in question is loaded into memory (if present)
>> >> as is.
>> >>
>> >> As the next step we can move the cpu_init() stuff into kdat and
>> >> (as Dima suggested) vdso_init().
>> >
>> > Looks good to me,
>> > Reviewed-by: Dmitry Safonov <dsafonov@virtuozzo.com>
>> >
>> > But I also suggest adding CRIU_GITID to that file (somewhere
>> > at the beginning). So, if CRIU package has been updated and
>> > kerndat_s was changed - to check the version and regenerate a file.
>>
>> Or maybe to simplify it for now and avoid synchronization,
>> rely on package maintainer again and document somewhere that on
>> CRIU's package update criu.kdat should be removed.
>>
>
> maybe something like this?

Yep, that should work, LGTM.

> diff --git a/criu/Makefile.crtools b/criu/Makefile.crtools
> index 03756ae..0815519 100644
> --- a/criu/Makefile.crtools
> +++ b/criu/Makefile.crtools
> @@ -1,5 +1,6 @@
>  ccflags-y              += -iquote criu/$(ARCH)
>  ccflags-y              += $(COMPEL_UAPI_INCLUDES)
> +CFLAGS_kerndat.o       += -DKDAT_MAGIC_2=${shell date +%s}
>  CFLAGS_REMOVE_clone-noasan.o += $(CFLAGS-ASAN)
>  ldflags-y              += -r
>
> diff --git a/criu/include/kerndat.h b/criu/include/kerndat.h
> index 84d0707..1d3cf70 100644
> --- a/criu/include/kerndat.h
> +++ b/criu/include/kerndat.h
> @@ -35,6 +35,7 @@ struct kerndat_s {
>          * value to avoid loading of bad kdat bits.
>          */
>         u32 magic;
> +       u32 magic2;
>         dev_t shmem_dev;
>         int last_cap;
>         u64 zero_page_pfn;
> diff --git a/criu/kerndat.c b/criu/kerndat.c
> index d0f0d2b..a07f916 100644
> --- a/criu/kerndat.c
> +++ b/criu/kerndat.c
> @@ -678,7 +678,8 @@ static int kerndat_try_load_cache(void)
>
>         close(fd);
>
> -       if (ret != sizeof(kdat) || kdat.magic != KDAT_MAGIC_1) {
> +       if (ret != sizeof(kdat) ||
> +           kdat.magic != KDAT_MAGIC_1 || kdat.magic2 != KDAT_MAGIC_2) {
>                 pr_warn("Stale %s file\n", KERNDAT_CACHE_FILE);
>                 unlink(KERNDAT_CACHE_FILE);
>                 return 1;
> @@ -719,6 +720,7 @@ static int kerndat_save_cache(void)
>         }
>
>         kdat.magic = KDAT_MAGIC_1;
> +       kdat.magic2 = KDAT_MAGIC_2;
>         ret = write(fd, &kdat, sizeof(kdat));
>         if (ret != sizeof(kdat))
>                 pr_warn("Kerndat cache writing failed.\n");
>
>
Pavel Emelianov April 27, 2017, 1:51 p.m.
On 04/27/2017 12:02 PM, Dmitry Safonov wrote:
> 2017-04-27 9:26 GMT+03:00 Andrei Vagin <avagin@virtuozzo.com>:
>> On Wed, Apr 26, 2017 at 02:05:52PM +0300, Dmitry Safonov wrote:
>>> 2017-04-26 13:26 GMT+03:00 Dmitry Safonov <0x7f454c46@gmail.com>:
>>>> 2017-04-26 12:23 GMT+03:00 Pavel Emelyanov <xemul@virtuozzo.com>:
>>>>> In the previous discussion it was suggested not to compile-in
>>>>> the kdat bits, but rather cache them in some place, that gets
>>>>> evicted on node reboot.
>>>>>
>>>>> Here's how this can look like.
>>>>>
>>>>> Briefly -- when kerndat.c has finished auto-detection is writes
>>>>> the kdat variable into the /run/criu.kdat file. On the next
>>>>> start the file in question is loaded into memory (if present)
>>>>> as is.
>>>>>
>>>>> As the next step we can move the cpu_init() stuff into kdat and
>>>>> (as Dima suggested) vdso_init().
>>>>
>>>> Looks good to me,
>>>> Reviewed-by: Dmitry Safonov <dsafonov@virtuozzo.com>
>>>>
>>>> But I also suggest adding CRIU_GITID to that file (somewhere
>>>> at the beginning). So, if CRIU package has been updated and
>>>> kerndat_s was changed - to check the version and regenerate a file.
>>>
>>> Or maybe to simplify it for now and avoid synchronization,
>>> rely on package maintainer again and document somewhere that on
>>> CRIU's package update criu.kdat should be removed.
>>>
>>
>> maybe something like this?
> 
> Yep, that should work, LGTM.

OK, will incorporate this into v4, thanks :)

>> diff --git a/criu/Makefile.crtools b/criu/Makefile.crtools
>> index 03756ae..0815519 100644
>> --- a/criu/Makefile.crtools
>> +++ b/criu/Makefile.crtools
>> @@ -1,5 +1,6 @@
>>  ccflags-y              += -iquote criu/$(ARCH)
>>  ccflags-y              += $(COMPEL_UAPI_INCLUDES)
>> +CFLAGS_kerndat.o       += -DKDAT_MAGIC_2=${shell date +%s}
>>  CFLAGS_REMOVE_clone-noasan.o += $(CFLAGS-ASAN)
>>  ldflags-y              += -r
>>
>> diff --git a/criu/include/kerndat.h b/criu/include/kerndat.h
>> index 84d0707..1d3cf70 100644
>> --- a/criu/include/kerndat.h
>> +++ b/criu/include/kerndat.h
>> @@ -35,6 +35,7 @@ struct kerndat_s {
>>          * value to avoid loading of bad kdat bits.
>>          */
>>         u32 magic;
>> +       u32 magic2;
>>         dev_t shmem_dev;
>>         int last_cap;
>>         u64 zero_page_pfn;
>> diff --git a/criu/kerndat.c b/criu/kerndat.c
>> index d0f0d2b..a07f916 100644
>> --- a/criu/kerndat.c
>> +++ b/criu/kerndat.c
>> @@ -678,7 +678,8 @@ static int kerndat_try_load_cache(void)
>>
>>         close(fd);
>>
>> -       if (ret != sizeof(kdat) || kdat.magic != KDAT_MAGIC_1) {
>> +       if (ret != sizeof(kdat) ||
>> +           kdat.magic != KDAT_MAGIC_1 || kdat.magic2 != KDAT_MAGIC_2) {
>>                 pr_warn("Stale %s file\n", KERNDAT_CACHE_FILE);
>>                 unlink(KERNDAT_CACHE_FILE);
>>                 return 1;
>> @@ -719,6 +720,7 @@ static int kerndat_save_cache(void)
>>         }
>>
>>         kdat.magic = KDAT_MAGIC_1;
>> +       kdat.magic2 = KDAT_MAGIC_2;
>>         ret = write(fd, &kdat, sizeof(kdat));
>>         if (ret != sizeof(kdat))
>>                 pr_warn("Kerndat cache writing failed.\n");
>>
>>
>