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 _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel