Patchwork tzset did not catch changes to localtime  [BZ #21060 ]

login
register
mail settings
Submitter ma.jiang@zte.com.cn
Date Dec. 17, 2018, 9:17 a.m.
Message ID <201812171717582865803@zte.com.cn>
Download mbox | patch
Permalink /patch/30690/
State New
Headers show

Comments

ma.jiang@zte.com.cn - Dec. 17, 2018, 9:17 a.m.
Hi Calos,
  Sorry for the delay. We are busy doing other stuffs this year.
  I have write a small benchmark as you suggested. The result shows that the patch 
  will not hurt performance in normal cases(When TZ is not set).
#include <time.h>
volatile time_t re;
int main()
{
        const unsigned long int tt = 1024*1024;
        const unsigned li = 1000*1000 * 5;
        unsigned int i = 0;

        struct tm * x = gmtime((time_t *)&tt);
        while( i++ < li)
        {
                re = mktime (x);
        }
        return 0;
}
This test takes about 10 seconds to finish, and is not affected by the patch.
real    0m11.113s
user    0m3.592s
sys     0m7.508s
The behaviour seems strange, so I have done some investigations. 
I found most time were used by stat (in __tzfile_read). 
This is caused by following codes in tzset_internal (in tzset.c):

  /* Check whether the value changed since the last run.  */
  if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
    /* No change, simply return.  */
    return;

  if (tz == NULL)
    /* No user specification; use the site-wide default.  */
    tz = TZDEFAULT;
	
When TZ is not set, old_tz is TZDEFAULT (which is not NULL), but tz is NULL.
So, tzset will read the TZDEFAULT file when TZ is not set. Fortunately, there 
have been codes that use stat to avoid unnecessary reloads since 2004(in __tzfile_read).

When TZ is set to "/etc/localtime", the test will end in about 1 second, this time 
tzset will not try stat localtime). Although much faster, this is not right as 
the /etc/localtime could be modified silently by others.

I have build a new patch that make use of __use_tzfile to avoid the wrong behaviour.
After patched, tzset should run as fast as the current one when TZ is set to something 
that does not rely on tzfile.
Is this patch OK now?







------------------原始邮件------------------
发件人:CarlosO'Donell <carlos@redhat.com>
收件人:马江10100629;libc-alpha@sourceware.org <libc-alpha@sourceware.org>;
日 期 :2017年11月02日 23:33
主 题 :Re: [PATCH]   tzset did not catch changes to localtime  [BZ #21060 ]
On 11/02/2017 02:03 AM, ma.jiang@zte.com.cn wrote:
> Hi, If users modified the local timezone file (usually
> /etc/localtime) while keep the TZ environment variable untouched,
> tzset will not recalculate tzname. This is not right. Patch attached
> should fix this problem, is it OK for trunk?

Thank you for the patch! The idea you present is interesting, but it
makes tzset() expensive when run in a loop, and several interfaces
call tzset() unconditionally to update the timezone in the event the
API call is the first in the sequence. For example mktime must call
tzset, and previously this might have been fast, but now it re-reads
the file every time. I don't think this is an acceptable solution
to the problem, but without a microbenchmark we don't know. So one
way to make your point would be to contribute a microbenchmark for
tzset or mktime, and show that the performance difference is
negligible. The other alternative is to go a layer down and stat
/etc/localtime (if that file is going to be used), and therefore
provide a quick way to avoid reloading the file if it hasn't changed.
All of this also will need a test case, which may or may not be
possible e.g. test starts in one timezone, updates the file used
(via TZ to full path of timezone), and then calls tzset, and verifies
that timezone has changed.

Lastly, this patch is a non-trivial number of lines and the copyright
status of ZTE is not yet clear. I will continue the copyright
discussion off-list with you directly.

Thank you again for your contribution.

--
Cheers,
Carlos.
Andreas Schwab - Dec. 17, 2018, 9:39 a.m.
On Dez 17 2018, <ma.jiang@zte.com.cn> wrote:

> +  /* Check whether the tz value changed since the last run.  */
> +  int tz_changed = old_tz ? strcmp (tz, old_tz) : 1;

This should be written as old_tz == NULL || strcmp (tz, old_tz) != 0,
and tz_changed should be bool.

> +  if ((old_tz == NULL)
> +      || (old_tz != NULL && tz_changed))

This is the same as tz_changed.

> +  {
> +    /* Save the value of `tz'. */
> +    free (old_tz);
> +    old_tz = __strdup (tz);
> +  }

Wrong indentation.

Andreas.

Patch

--- a/time/tzset.c
+++ b/time/tzset.c
@@ -386,22 +386,28 @@  tzset_internal (int always)
   if (tz && *tz == ':')
     ++tz;

-  /* Check whether the value changed since the last run.  */
-  if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
-    /* No change, simply return.  */
-    return;
-
   if (tz == NULL)
     /* No user specification; use the site-wide default.  */
     tz = TZDEFAULT;

+  /* Check whether the tz value changed since the last run.  */
+  int tz_changed = old_tz ? strcmp (tz, old_tz) : 1;
+  if ((old_tz == NULL)
+      || (old_tz != NULL && tz_changed))
+  {
+    /* Save the value of `tz'. */
+    free (old_tz);
+    old_tz = __strdup (tz);
+  }
+  
+  /* When using tzfile, must check whether it changed. */
+  if (!__use_tzfile && !tz_changed)
+    return;
+
+  /* Going to recaculate values. */
   tz_rules[0].name = NULL;
   tz_rules[1].name = NULL;

-  /* Save the value of `tz'.  */
-  free (old_tz);
-  old_tz = tz ? __strdup (tz) : NULL;
-
   /* Try to read a data file.  */
   __tzfile_read (tz, 0, NULL);
   if (__use_tzfile)