[PATCH] Configure: Fix cacheline size for aarch64 platforms

debayang.qdt debayang.qdt at qualcommdatacenter.com
Wed Dec 13 16:17:13 UTC 2017


Thanks. Looks good.

Only the second patch commit log description may be corrected.

"... In case not supported, fallback to ngx_cpuinfo() or compile time defaults."
To
" .. In case not supported, fallback to compile time defaults."

Thanks
Debayan

-----Original Message-----
From: Maxim Dounin [mailto:mdounin at mdounin.ru] 
Sent: Wednesday, December 13, 2017 9:23 PM
To: nginx-devel at nginx.org
Cc: debayang.qdt <debayang.qdt at qualcommdatacenter.com>
Subject: Re: [PATCH] Configure: Fix cacheline size for aarch64 platforms

Hello!

On Mon, Dec 11, 2017 at 04:52:32PM +0000, debayang.qdt wrote:

[...]

> >> - This needs some thinking about about priorities between
>   sysconf(_SC_LEVEL1_DCACHE_LINESIZE) and ngx_cpuinfo().  We've
>   seen cases when ngx_cpuinfo() detects wrong cache line size in
>   virtualized environments (https://trac.nginx.org/nginx/ticket/352),
>   so we may want to consider using sysconf(_SC_LEVEL1_DCACHE_LINESIZE)
>   with higher priority (not sure though).
> 
> In general it may be better to rely on the arch independent code 
> unless the specific tunings in ngx_cpuinfo() is known to give a noticeable performance impact.

Yes.  The problem though is that ngx_cpuinfo() is actually the place where we apply such specific tunings right now.  For example, currently we do the following in ngx_cpuinfo():

        /*
         * Pentium 4, although its cache line size is 64 bytes,
         * it prefetches up to two cache lines during memory read
         */
        case 15:
            ngx_cacheline_size = 128;
            break;

While this particular part of the code is probably not important now, I don't think we want to loose the possibility to do such optimizations.

So probably we should preserve ngx_cpuinfo() as the last step for now.  In the future we can consider improving it to preserve ngx_cacheline_size if it was set by sysconf() and likely correct. 

[...]

> # HG changeset patch
> # User Debayan Ghosh <debayang.qdt at qualcommdatacenter.com>
> # Date 1513004735 0
> #      Mon Dec 11 15:05:35 2017 +0000
> # Node ID b765307e9f516c396da24019724f82c2c8c38677
> # Parent  d3235149d17f7745d3ac246a6cdcc81a56698f7b
> Configure: Set default cacheline size to 64 for aarch64 platforms.
> 
> diff -r d3235149d17f -r b765307e9f51 auto/os/conf
> --- a/auto/os/conf	Thu Dec 07 17:09:36 2017 +0300
> +++ b/auto/os/conf	Mon Dec 11 15:05:35 2017 +0000
> @@ -110,6 +110,11 @@
>          NGX_MACH_CACHE_LINE=64
>      ;;
>  
> +    aarch64 )
> +        have=NGX_ALIGNMENT value=16 . auto/define
> +        NGX_MACH_CACHE_LINE=64
> +    ;;
> +
>      *)
>          have=NGX_ALIGNMENT value=16 . auto/define
>          NGX_MACH_CACHE_LINE=32
> 
> 

Queued (with minor change in commit log), thnx.

> # HG changeset patch
> # User Debayan Ghosh <debayang.qdt at qualcommdatacenter.com>
> # Date 1513009691 0
> #      Mon Dec 11 16:28:11 2017 +0000
> # Node ID 090538eb7d973d8cc690f8f413ed5c7b5d3338b8
> # Parent  b765307e9f516c396da24019724f82c2c8c38677
> Use sysconf to determine cacheline size at runtime.
> 
> Determine cacheline size at runtime if supported using 
> sysconf(_SC_LEVEL1_DCACHE_LINESIZE). In case not supported, fallback 
> to ngx_cpuinfo() or compile time defaults.
> 
> diff -r b765307e9f51 -r 090538eb7d97 auto/unix
> --- a/auto/unix	Mon Dec 11 15:05:35 2017 +0000
> +++ b/auto/unix	Mon Dec 11 16:28:11 2017 +0000
> @@ -964,6 +964,16 @@
>  . auto/feature
>  
>  
> +ngx_feature="sysconf(_SC_LEVEL1_DCACHE_LINESIZE)"
> +ngx_feature_name="NGX_HAVE_LEVEL1_DCACHE_LINESIZE"
> +ngx_feature_run=no
> +ngx_feature_incs=
> +ngx_feature_path=
> +ngx_feature_libs=
> +ngx_feature_test="sysconf(_SC_LEVEL1_DCACHE_LINESIZE)"
> +. auto/feature
> +
> +
>  ngx_feature="openat(), fstatat()"
>  ngx_feature_name="NGX_HAVE_OPENAT"
>  ngx_feature_run=no
> diff -r b765307e9f51 -r 090538eb7d97 src/os/unix/ngx_posix_init.c
> --- a/src/os/unix/ngx_posix_init.c	Mon Dec 11 15:05:35 2017 +0000
> +++ b/src/os/unix/ngx_posix_init.c	Mon Dec 11 16:28:11 2017 +0000
> @@ -36,6 +36,9 @@
>  {
>      ngx_time_t  *tp;
>      ngx_uint_t   n;
> +#if (NGX_HAVE_LEVEL1_DCACHE_LINESIZE)
> +    ngx_int_t cpu_cache_line;
> +#endif

This at least needs to be fixed to match style.  Also, probably using "long" as sysconf() returns is a good idea.  I would also use a shorter name for the variable.

>  
>  #if (NGX_HAVE_OS_SPECIFIC_INIT)
>      if (ngx_os_specific_init(log) != NGX_OK) { @@ -64,6 +67,17 @@

Just a side note: it looks like your mail client corrupted the patch.

>  
>      ngx_cpuinfo();
>  
> +#if (NGX_HAVE_LEVEL1_DCACHE_LINESIZE)
> +    /*
> +     * Fetch cacheline from system configuration if available.
> +     * This is given priority over ngx_cpuinfo().
> +     */

Comment looks unneeded, as it adds more or less nothing to the code.

> +    cpu_cache_line = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
> +    if(cpu_cache_line > 0) {

Missing space between "if" and "(".

> +        ngx_cacheline_size = cpu_cache_line;
> +    }
> +#endif
> +    
>      if (getrlimit(RLIMIT_NOFILE, &rlmt) == -1) {
>          ngx_log_error(NGX_LOG_ALERT, log, errno,
>                        "getrlimit(RLIMIT_NOFILE) failed");

Here is a version of the patch updated according to the comments above.  Please let me know if it looks good for you, I'll commit it then:

# HG changeset patch
# User Debayan Ghosh <debayang.qdt at qualcommdatacenter.com>
# Date 1513009691 0
#      Mon Dec 11 16:28:11 2017 +0000
# Node ID 70da21da25e2117bcfb1639ad1e873f28aff44b5
# Parent  e4c21e4172773fa0df7b356da9bf2ea852634a66
Use sysconf to determine cacheline size at runtime.

Determine cacheline size at runtime if supported using sysconf(_SC_LEVEL1_DCACHE_LINESIZE). In case not supported, fallback to ngx_cpuinfo() or compile time defaults.

diff --git a/auto/unix b/auto/unix
--- a/auto/unix
+++ b/auto/unix
@@ -964,6 +964,16 @@ ngx_feature_test="sysconf(_SC_NPROCESSOR
 . auto/feature
 
 
+ngx_feature="sysconf(_SC_LEVEL1_DCACHE_LINESIZE)"
+ngx_feature_name="NGX_HAVE_LEVEL1_DCACHE_LINESIZE"
+ngx_feature_run=no
+ngx_feature_incs=
+ngx_feature_path=
+ngx_feature_libs=
+ngx_feature_test="sysconf(_SC_LEVEL1_DCACHE_LINESIZE)"
+. auto/feature
+
+
 ngx_feature="openat(), fstatat()"
 ngx_feature_name="NGX_HAVE_OPENAT"
 ngx_feature_run=no
diff --git a/src/os/unix/ngx_posix_init.c b/src/os/unix/ngx_posix_init.c
--- a/src/os/unix/ngx_posix_init.c
+++ b/src/os/unix/ngx_posix_init.c
@@ -36,6 +36,9 @@ ngx_os_init(ngx_log_t *log)  {
     ngx_time_t  *tp;
     ngx_uint_t   n;
+#if (NGX_HAVE_LEVEL1_DCACHE_LINESIZE)
+    long         size;
+#endif
 
 #if (NGX_HAVE_OS_SPECIFIC_INIT)
     if (ngx_os_specific_init(log) != NGX_OK) { @@ -62,6 +65,13 @@ ngx_os_init(ngx_log_t *log)
         ngx_ncpu = 1;
     }
 
+#if (NGX_HAVE_LEVEL1_DCACHE_LINESIZE)
+    size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
+    if (size > 0) {
+        ngx_cacheline_size = size;
+    }
+#endif
+
     ngx_cpuinfo();
 
     if (getrlimit(RLIMIT_NOFILE, &rlmt) == -1) {


--
Maxim Dounin
http://mdounin.ru/


More information about the nginx-devel mailing list