Welcome! Log In Create A New Profile

Advanced

Re: cache: move open to thread pool

Maxim Dounin
August 10, 2018 07:42AM
Hello!

On Thu, Aug 09, 2018 at 02:43:19PM -0700, Ka-Hing Cheung via nginx-devel wrote:

> On Wed, Aug 8, 2018 at 11:16 AM, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > We've thought about offloading more operations to thread pools,
> > and that's basically why "aio_write" is a separate directive, so
> > more directives like "aio_<some_operation>" can be added in the
> > future. It's good to see this is finally happening.
> >
> > Unfortunately, there are number problems with the patch, and it
> > needs more work before it can be committed. In particular:
> >
> > - The open() calls are offloaded to thread pools unconditionally.
> > This may actually reduce performance in various typical
> > workloads where open() does not block. Instead, there should be
> > an "aio_open on|off" directive, similar to "aio_write".
> >
> > - The code bypasses open file cache, and uses a direct call
> > in the http cache code instead. While it might be ok in your
> > setup, it looks like an incomplete solution from the generic point
> > of view. A better solution would be to introduce a generic
> > interface in ngx_open_cached_file() to allow use of thread
> > pools.
>
> Agreed that not everyone wants threaded open() with aio "threads" (if
> low CPU overhead is more important than low latency for example). The
> semantic of "aio_open on" is uncleared though when aio is on but not
> "threads". Having open file cache working with threaded open is nice
> to have (although people who need "aio_open" probably also have so
> much cached assets that they won't need open file cache).

As long as the "aio" is set to non-threaded variant so "open" is
not available, it is perfectly fine that "aio_open on"
won't do anything. This is what currently happens with
"aio_write", see http://nginx.org/r/aio_write.

[...]

> > - In the ngx_http_file_cache_thread_open() function you allocate
> > thread task of size sizeof(ngx_thread_file_open_ctx_t) and store
> > it in the file->thread_task. There is no guarantee that this
> > task will be compatible with other tasks stored in
> > file->thread_task. A better solution would be to extend
> > ngx_thread_file_ctx_t and use the same context for all
> > file-related threaded operations.
>
> does this matter? the different thread_task won't overlap in their
> usage (you can't read a file until after its opened) so there's no
> reason they need to be compatible. Isn't that why the task ctx is void
> * and ngx_thread_task_alloc takes a generic size?

While this probably never happens with the current code, original
idea was that file->thread_task can be re-used for other
file-related operations.

[...]

> > - The code uses memory pool operations wihin a thread,
> > specifically ngx_pool_cleanup_add(). Memory pools are not
> > thread safe, and the behaviour of this code is therefore
> > undefined.
>
> Agreed that it may not be very clean but is this a problem in
> practice? The pool is tied to the request and shouldn't shared with
> other threads. Asking mostly to clarify my understandings with nginx
> memory pools.

In practice the pool can be used in the main thread if some
request-related events happen, or it can be used by another
subrequest.

[...]

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

cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 296 August 07, 2018 05:28PM

Re: cache: move open to thread pool

Maxim Dounin 50 August 08, 2018 02:18PM

RE: cache: move open to thread pool

erankor 44 August 08, 2018 03:46PM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 24 August 27, 2018 06:00PM

RE: cache: move open to thread pool

erankor 27 August 28, 2018 05:18AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 39 August 09, 2018 05:44PM

Re: cache: move open to thread pool

Maxim Dounin 40 August 10, 2018 07:42AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 26 August 27, 2018 05:40PM

Re: cache: move open to thread pool

Maxim Dounin 20 September 03, 2018 12:10PM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 20 September 04, 2018 08:00PM

Re: cache: move open to thread pool

Maxim Dounin 22 September 13, 2018 02:12PM

Re: cache: move open to thread pool

Roman Arutyunyan 23 October 04, 2018 05:34AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 96
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 254 on July 05, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready