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@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@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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel