Hello!

On Sat, Dec 09, 2017 at 02:05:04PM +0000, debayang.qdt wrote:

> Hello!
>
> On Fri, Dec 08, 2017 at 06:43:15PM +0000, debayang.qdt wrote:
>
> > Hello,
> >
> > 64 bytes should be the minimum size.
> >> Any links to support this claim?
>
> AFAIK several aarch64 server vendors - centriq/thunderx/xgene/A53/A57.. implementations - uses either 64B/128B cache lines .
> So keeping it at 64 bytes may be ok.
> Also in linux kernel : https://elixir.free-electrons.com/linux/v4.14.4/source/arch/arm64/include/asm/cache.h#L73

This seems to use either CTR_EL0 or 128, and not even mentions 64.
Anyway, I have no objections to 64, as it is the cache line size
on the aarch64 boxes I have access to.

> > However, A53 is a small segment of arm64 based servers .
> > Multiple aarch64 based servers uses 128 byte cache lines.
> > For e.g Centriq 2400 aarch64 server from Qualcomm Datacenter Technologies uses 128 bytes cache line.
> AFAIK, using a smaller cache line size is mostly fine and imply almost no performance difference in nginx. On ther other hand, using larger cache line size can easily result in multiple memory access where just one should be enough, resulting in suboptimal performance. That's why we generally assume 32 unless we know better.
>
> > To be very accurate , we may read the dcache line size from CTR_EL0 register.
> > Let me know what you think.
>
> >> Given the level of popularity of aarch64 servers, I would rather not. I would prefer to rely on something architecture-independent like sysconf(_SC_LEVEL1_DCACHE_LINESIZE), though it looks like it doesn't work on aarch64 yet, see https://bugzilla.redhat.com/show_bug.cgi?id=1190638.
>
> The aarch64 support is recently added in the glibc trunk .
> https://sourceware.org/git/?p=glibc.git;a=commit;h=6d58ce5e5072945d44f2dba83ad16cd6febd056c
> The implementation above reads the CTR_EL0 to get the cache information .
>
> So , as you mentioned - it may make sense to use the sysconf to determine the cache line at runtime . In case it's not available on the system (value would be 0), we can fall back to the compile time defaults .
> Below patch in nginx for the same.
>
>
> # HG changeset patch
> # User Debayan Ghosh <debayang.qdt@qualcommdatacenter.com>
> # Date 1512811864 0
> # Sat Dec 09 09:31:04 2017 +0000
> # Node ID 217d0ed8730190a986c079c300169e10b90b4a82
> # Parent c71aed1f6f88b8c5d56a4360ca4521a62a4c17ce
> Nginx: Use sysconf to determine cacheline size

There is no need for the "Nginx" prefix.
Trailing dot is missing.

>
> Using sysconf(_SC_LEVEL1_DCACHE_LINESIZE) to determine cache line size on the system.
> This will make it architecture independent.
> Falls back to using arch specific compile time defaults if above not supported.

Please keep lines under 80 chars.

>
> diff -r c71aed1f6f88 -r 217d0ed87301 auto/unix
> --- a/auto/unix Tue Nov 28 15:19:59 2017 +0000
> +++ b/auto/unix Sat Dec 09 09:31:04 2017 +0000
> @@ -963,6 +963,15 @@
> ngx_feature_test="sysconf(_SC_NPROCESSORS_ONLN)"
> . 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"

Style: there should be two empty lines before the feature test.

> diff -r c71aed1f6f88 -r 217d0ed87301 src/os/unix/ngx_posix_init.c
> --- a/src/os/unix/ngx_posix_init.c Tue Nov 28 15:19:59 2017 +0000
> +++ b/src/os/unix/ngx_posix_init.c Sat Dec 09 09:31:04 2017 +0000
> @@ -48,8 +48,17 @@
> }
>
> ngx_pagesize = getpagesize();
> +#if (NGX_HAVE_LEVEL1_DCACHE_LINESIZE)
> + ngx_int_t cpu_cache_line = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
> + /* Below workaround to use default values on some environments
> + where the cache line size yet not available */
> + ngx_cacheline_size = (cpu_cache_line == 0) ?
> + NGX_CPU_CACHE_LINE : cpu_cache_line;
> +#else
> + /* Use default cache line for the architecture */
> ngx_cacheline_size = NGX_CPU_CACHE_LINE;
> -
> +#endif
> +
> for (n = ngx_pagesize; n >>= 1; ngx_pagesize_shift++) { /* void */ }
>
> #if (NGX_HAVE_SC_NPROCESSORS_ONLN)

In no particular order:

- There are multiple style issues, including:

- Variables should be declared at the function start, or, in case of
a conditional compilation, at the start of a block in #if.

- Multi-line comments should look like, like in style(9):

/*
* Multi-line comments look like this. Make them real
* sentences. Fill them so they look like real paragraphs.
*/

- I would rather suggest to leave the default as is, and introduce
a separate block with sysconf() somewhere after it. This will
result in much more readable code.

- There should be error handling, sysconf() can return -1.

- 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).

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel