Help with shared memory usage

[Maxim Dounin]

Hello!

On Wed, Jul 31, 2013 at 12:28:02AM -0300, Wandenberg Peixoto wrote:

> Hello!
> 
> Thanks for your help. I hope that the patch be OK now.
> I don't know if the function and variable names are on nginx pattern.
> Feel free to change the patch.
> If you have any other point before accept it, will be a pleasure to fix it.

Sorry for long delay.  As promised, I've looked into this.  Apart 
from multiple style problems, I see at least one case of possible use of 
uninitialized memory.  See below for comments.

> 
> --- src/core/ngx_slab.c    2013-05-06 07:27:10.000000000 -0300
> +++ src/core/ngx_slab.c    2013-07-31 00:21:08.043034442 -0300
> @@ -615,6 +615,26 @@ fail:
> 
> 
>  static ngx_slab_page_t *
> +ngx_slab_merge_with_neighbour(ngx_slab_pool_t *pool, ngx_slab_page_t *page)

A side note: there are multiple issues with style in this patch.  
In no particular order:

1. There is no reason to return ngx_slab_page_t * here.  Just 
ngx_int_t should be enough (or, even void would be good enough, 
see below).

2. It is recommended do place function implementation below it's 
use (and add a prototype).  This allows to read the code linearly, 
top-down.

> +{
> +    ngx_slab_page_t *neighbour = &page[page->slab];

Here "neighbour" may point outside of the allocated page 
structures if we are freeing the last page in the pool.

> +    if (((ngx_slab_page_t *) neighbour->prev != NULL) && (neighbour->next
> != NULL) && ((neighbour->prev & NGX_SLAB_PAGE_MASK) == NGX_SLAB_PAGE)) {

In no particular order:

1. Style problems - more than 80 chars, extra parens.

2. There is probably no need to check both prev and next.

3. See below about casting neighbour->prev to (ngx_slab_page_t *).

> +        page->slab += neighbour->slab;
> +
> +        ((ngx_slab_page_t *) neighbour->prev)->next = neighbour->next;
> +        neighbour->next->prev = neighbour->prev;

Casting neighbour->prev to (ngx_slab_page_t *) without removing 
last NGX_SLAB_PAGE_MASK bits isn't really a good idea.  It works 
here as NGX_SLAB_PAGE == 0, but may fail in other cases.

It would be much better to use a snippet already used in other 
places:

        prev = (ngx_slab_page_t *) (p->prev & ~NGX_SLAB_PAGE_MASK);
        prev->next = p->next;
        p->next->prev = p->prev;

> +        neighbour->slab = NGX_SLAB_PAGE_FREE;
> +        neighbour->prev = (uintptr_t) &pool->free;
> +        neighbour->next = &pool->free;

Unused page structures used to be zeroed.  Any reason for 
prev/next pointing to pool->free?

It looks like something like

        p->slab = NGX_SLAB_PAGE_FREE;
        p->next = NULL;
        p->prev = NGX_SLAB_PAGE;

would be reasonable here.

> +
> +        return page;
> +    }
> +    return NULL;
> +}

See above, there is no need to return page as it's unused.  
Something like return NGX_OK / return NGX_DECLINED would be better 
here - if we are going to check if any pages were actually merged.

Additionally, an empty line should be before last return.

> +
> +
> +static ngx_slab_page_t *
>  ngx_slab_alloc_pages(ngx_slab_pool_t *pool, ngx_uint_t pages)
>  {
>      ngx_slab_page_t  *page, *p;
> @@ -657,6 +677,19 @@ ngx_slab_alloc_pages(ngx_slab_pool_t *po
>          }
>      }
> 
> +    ngx_flag_t retry = 0;
> +    for (page = pool->free.next; page != &pool->free;) {
> +        if (ngx_slab_merge_with_neighbour(pool, page)) {
> +            retry = 1;
> +        } else {
> +            page = page->next;
> +        }
> +    }
> +
> +    if (retry) {
> +        return ngx_slab_alloc_pages(pool, pages);
> +    }
> +

Apart from multiple style issues here, I think we should choose 
one aproach: either loop over free pages and merge them here, or 
maintain all free pages merged in ngx_slab_free_pages() (that is, 
merge in both directions there).

>      ngx_slab_error(pool, NGX_LOG_CRIT, "ngx_slab_alloc() failed: no
> memory");
> 
>      return NULL;
> @@ -687,6 +720,8 @@ ngx_slab_free_pages(ngx_slab_pool_t *poo
>      page->next->prev = (uintptr_t) page;
> 
>      pool->free.next = page;
> +
> +    ngx_slab_merge_with_neighbour(pool, page);
>  }

See above.

Overral it looks much better than previous patch, but still needs 
more work.

-- 
Maxim Dounin
http://nginx.org/en/donation.html