Re: [Xen-devel] Upgrade from xen-4.1-testing to xen-unstable report

[Teck Choon Giam <giamteckchoon@xxxxxxxxx>]

On Thu, Apr 12, 2012 at 10:50 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Thu, 2012-04-12 at 15:42 +0100, Lin Ming wrote:
>> On Thu, 2012-04-12 at 15:03 +0100, Ian Campbell wrote:
>> > On Thu, 2012-04-12 at 13:50 +0100, Lin Ming wrote:
>> > > On Thu, 2012-04-12 at 07:08 +0000, Ian Campbell wrote:
>> > > > On Wed, 2012-04-11 at 22:23 +0100, Teck Choon Giam wrote:
>> > > > > Hi,
>> > > > >
>> > > > > This is just my experience about issues I encountered when upgrade
>> > > > > from xen-4.1-testing changeset 23277:80130491806f to xen-unstable
>> > > > > changeset 25191:a95fc7decc83.
>> > > > >
>> > > > > 1. Immediately after upgrade, xl list show such error:
>> > > > >
>> > > > > # xl list
>> > > > > libxl: error: libxl.c:506:libxl_list_domain: geting domain info list:
>> > > > > Permission denied
>> > > > > libxl_domain_infolist failed.
>> > > > >
>> > > > > After a reboot, it is fine.  Any idea why such behaviour?  Imagine if
>> > > > > there are running domUs... this might cause issues to shutdown?  I
>> > > > > will downgrade and repeat such test to confirm.  Might be worth a 
>> > > > > note
>> > > > > in upgrading note about this if this is intended?
>> > > >
>> > > > The tools and the hypervisor are a matched pair so you would need to
>> > > > reboot the system in order to use the new tools. This has always been
>> > > > the case with Xen upgrades.
>> > > >
>> > > > > 2. localtime setting not working.  Set to localtime=1 doesn't seems 
>> > > > > to
>> > > > > work whereby setting rtc_timeoffset works.  Any idea?
>> > > >
>> > > > I've CC'd Lin Ming who implemented both of those.
>> > >
>> > > Just did a quick gdb debug, seems libxl__domain_build_info_setdefault
>> > > was called 3 times. So below statement was executed 3 times too.
>> > >
>> > > b_info->rtc_timeoffset += tm->tm_gmtoff;
>> >
>> > Oh, that sounds like the problem. The setdefault functions are supposed
>> > to be idempotent, IOW calling it two or more times should give the same
>> > result as calling it just once.
>> >
>> > I think you need to move this logic from setdefault
>> >     if (libxl_defbool_val(b_info->localtime)) {
>> >         time_t t;
>> >         struct tm *tm;
>> >
>> >         t = time(NULL);
>> >         tm = localtime(&t);
>> >
>> >         b_info->rtc_timeoffset += tm->tm_gmtoff;
>> >     }
>> >
>> > into libxl__build_pre and do that calculation on a temporary variable
>> > whjich you pass to xc_domain_set_time_offset rather than modifying
>> > b_info.
>>
>> How about below patch?

I am currently compiling xen-unstable changeset 25193:c6bde42c8845
with this patch.  Will test then report back.

Thanks.

Kindest regards,
Giam Teck Choon

>>
>> From e06589cb5f1efd885f9589405d0f4ecc97427ad6 Mon Sep 17 00:00:00 2001
>> From: Lin Ming <mlin@xxxxxxxxxxxxx>
>> Date: Thu, 12 Apr 2012 22:36:24 +0800
>> Subject: [PATCH] libxl: fix rtc_timeoffset setting
>>
>> libxl__domain_build_info_setdefault may be called several times, so
>> rtc_timeoffset can't be set in it.
>>
>> Move rtc_timeoffset setting logic to libxl__build_pre.
>>
>> Reported-by: Teck Choon Giam <giamteckchoon@xxxxxxxxx>
>> Signed-off-by: Lin Ming <mlin@xxxxxxxxxxxxx>
>
> Looks good to me.
>
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
>
>> ---
>>  tools/libxl/libxl_create.c |    9 ---------
>>  tools/libxl/libxl_dom.c    |   17 +++++++++++++++--
>>  2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index e63c7bd..e706124 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -127,15 +127,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>          b_info->target_memkb = b_info->max_memkb;
>>
>>      libxl_defbool_setdefault(&b_info->localtime, false);
>> -    if (libxl_defbool_val(b_info->localtime)) {
>> -        time_t t;
>> -        struct tm *tm;
>> -
>> -        t = time(NULL);
>> -        tm = localtime(&t);
>> -
>> -        b_info->rtc_timeoffset += tm->tm_gmtoff;
>> -    }
>>
>>      libxl_defbool_setdefault(&b_info->disable_migrate, false);
>>
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index 0bdd654..91c4bd8 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -65,6 +65,8 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>>      libxl_ctx *ctx = libxl__gc_owner(gc);
>>      int tsc_mode;
>>      char *xs_domid, *con_domid;
>> +    uint32_t rtc_timeoffset;
>> +
>>      xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
>>      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus, &info->cpumap);
>>      xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + 
>> LIBXL_MAXMEM_CONSTANT);
>> @@ -91,8 +93,19 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>>      if (libxl_defbool_val(info->disable_migrate))
>>          xc_domain_disable_migrate(ctx->xch, domid);
>>
>> -    if (info->rtc_timeoffset)
>> -        xc_domain_set_time_offset(ctx->xch, domid, info->rtc_timeoffset);
>> +    rtc_timeoffset = info->rtc_timeoffset;
>> +    if (libxl_defbool_val(info->localtime)) {
>> +        time_t t;
>> +        struct tm *tm;
>> +
>> +        t = time(NULL);
>> +        tm = localtime(&t);
>> +
>> +        rtc_timeoffset += tm->tm_gmtoff;
>> +    }
>> +
>> +    if (rtc_timeoffset)
>> +        xc_domain_set_time_offset(ctx->xch, domid, rtc_timeoffset);
>>
>>      if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>>          unsigned long shadow;
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel