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