Hello Maxim,

I executed my tests again and seems that your improved patch version is
working fine too.

Did you plan to merge it on nginx core soon?

-agentzh

Did you have opportunity to check if it works for you?

Regards



On Wed, May 28, 2014 at 3:38 PM, Maxim Dounin <mdounin@mdounin.ru> wrote:

> Hello!
>
> On Sun, May 11, 2014 at 10:13:52PM -0700, Yichun Zhang (agentzh) wrote:
>
> > Hello!
> >
> > On Mon, Jul 29, 2013 at 10:11 AM, Maxim Dounin wrote:
> > > Additionally, doing a full merge of all free blocks on a free
> > > operation looks too much. It might be something we want to do on
> > > allocation failure, but not on a normal path in
> > > ngx_slab_free_pages(). And/or something lightweight may be done
> > > in ngx_slab_free_pages(), e.g., checking if pages following pages
> > > we are freeing are free too, and merging them in this case.
> > >
> >
> > I'd propose an alternative patch taking the second approach, that is,
> > merging adjacent free pages (for both the previous and next blocks) in
> > ngx_slab_free_pages(). This approach has the following advantages:
> >
> > 1. It can effectively distribute the merging computations across all
> > the page free operations, which can prevent potential frequent and
> > long stalls when actually running out of large enough free blocks
> > along the "free" list that is already very long for large zones (which
> > usually consists of tons of one-page blocks upon allocation
> > failures).
> >
> > 2. it can also make multi-page allocations generally faster because
> > we're merging pages immediately when we can and thus it's more likely
> > to find large enough free blocks along the (relatively short) free
> > list for ngx_slab_alloc_pages().
> >
> > The only downside is that I have to introduce an extra field
> > "prev_slab" (8-byte for x86_64) in ngx_slab_page_t in my patch, which
> > makes the slab page metadata a bit larger.
>
> Below is a patch which does mostly the same without introducing
> any additional per-page fields. Please take a look if it works
> for you.
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1401302011 -14400
> # Wed May 28 22:33:31 2014 +0400
> # Node ID 7fb45c6042324e6cd92b0fb230c67a9c8c75681c
> # Parent 80bd391c90d11de707a05fcd0c9aa2a09c62877f
> Core: slab allocator defragmentation.
>
> Large allocations from a slab pool result in free page blocks being
> fragmented,
> eventually leading to a situation when no further allocation larger than a
> page
> size are possible from the pool. While this isn't a problem for nginx
> itself,
> it is known to be bad for various 3rd party modules. Fix is to merge
> adjacent
> blocks of free pages in the ngx_slab_free_pages() function.
>
> Prodded by Wandenberg Peixoto and Yichun Zhang.
>
> diff --git a/src/core/ngx_slab.c b/src/core/ngx_slab.c
> --- a/src/core/ngx_slab.c
> +++ b/src/core/ngx_slab.c
> @@ -129,6 +129,8 @@ ngx_slab_init(ngx_slab_pool_t *pool)
> pool->pages->slab = pages;
> }
>
> + pool->last = pool->pages + pages;
> +
> pool->log_nomem = 1;
> pool->log_ctx = &pool->zero;
> pool->zero = '\0';
> @@ -626,6 +628,8 @@ ngx_slab_alloc_pages(ngx_slab_pool_t *po
> if (page->slab >= pages) {
>
> if (page->slab > pages) {
> + page[page->slab - 1].prev = (uintptr_t) &page[pages];
> +
> page[pages].slab = page->slab - pages;
> page[pages].next = page->next;
> page[pages].prev = page->prev;
> @@ -672,7 +676,8 @@ static void
> ngx_slab_free_pages(ngx_slab_pool_t *pool, ngx_slab_page_t *page,
> ngx_uint_t pages)
> {
> - ngx_slab_page_t *prev;
> + ngx_uint_t type;
> + ngx_slab_page_t *prev, *join;
>
> page->slab = pages--;
>
> @@ -686,6 +691,53 @@ ngx_slab_free_pages(ngx_slab_pool_t *poo
> page->next->prev = page->prev;
> }
>
> + join = page + page->slab;
> +
> + if (join < pool->last) {
> + type = join->prev & NGX_SLAB_PAGE_MASK;
> +
> + if (type == NGX_SLAB_PAGE && join->next != NULL) {
> + pages += join->slab;
> + page->slab += join->slab;
> +
> + prev = (ngx_slab_page_t *) (join->prev & ~NGX_SLAB_PAGE_MASK);
> + prev->next = join->next;
> + join->next->prev = join->prev;
> +
> + join->slab = NGX_SLAB_PAGE_FREE;
> + join->next = NULL;
> + join->prev = NGX_SLAB_PAGE;
> + }
> + }
> +
> + if (page > pool->pages) {
> + join = page - 1;
> + type = join->prev & NGX_SLAB_PAGE_MASK;
> +
> + if (type == NGX_SLAB_PAGE && join->slab == NGX_SLAB_PAGE_FREE) {
> + join = (ngx_slab_page_t *) (join->prev & ~NGX_SLAB_PAGE_MASK);
> + }
> +
> + if (type == NGX_SLAB_PAGE && join->next != NULL) {
> + pages += join->slab;
> + join->slab += page->slab;
> +
> + prev = (ngx_slab_page_t *) (join->prev & ~NGX_SLAB_PAGE_MASK);
> + prev->next = join->next;
> + join->next->prev = join->prev;
> +
> + page->slab = NGX_SLAB_PAGE_FREE;
> + page->next = NULL;
> + page->prev = NGX_SLAB_PAGE;
> +
> + page = join;
> + }
> + }
> +
> + if (pages) {
> + page[pages].prev = (uintptr_t) page;
> + }
> +
> page->prev = (uintptr_t) &pool->free;
> page->next = pool->free.next;
>
> diff --git a/src/core/ngx_slab.h b/src/core/ngx_slab.h
> --- a/src/core/ngx_slab.h
> +++ b/src/core/ngx_slab.h
> @@ -29,6 +29,7 @@ typedef struct {
> size_t min_shift;
>
> ngx_slab_page_t *pages;
> + ngx_slab_page_t *last;
> ngx_slab_page_t free;
>
> u_char *start;
>
> --
> Maxim Dounin
> http://nginx.org/
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel