<div dir="ltr">Hello Maxim,<div><br></div><div><div>I executed my tests again and seems that your improved patch version is working fine too.</div><div><br></div><div>Did you plan to merge it on nginx core soon?</div><div>

<br></div><div><span style="font-family:arial,sans-serif;font-size:13px">-agentzh</span><br></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Did you have opportunity to check if it works for you?</span></div>

<div><br></div><div>Regards</div></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 28, 2014 at 3:38 PM, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span> wrote:<br>

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