Hi Maxim, sorry for the long delay. I hope you remember my issue. In attach is the new patch with the changes you suggest. Can you check it again? I hope it can be applied to nginx code now.
About this point "2. There is probably no need to check both prev and next.", I check both pointers to avoid a segmentation fault, since in some situations the next can be NULL and the prev may point to pool->free. As far as I could follow the code seems to me that could happen one of this pointers, next or prev, point to a NULL. I just made a double check to protect. If I'm wrong, just tell me what can I check to be sure the next and the previous free pages can be accessed, without problems and I will make the changes. Regards, Wandenberg On Sun, Oct 6, 2013 at 6:37 AM, Maxim Dounin <mdou...@mdounin.ru> wrote: > 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 >
--- src/core/ngx_slab.c 2013-12-15 22:10:51.079133826 -0200 +++ src/core/ngx_slab.c 2013-12-15 23:38:46.607147608 -0200 @@ -62,7 +62,8 @@ ngx_uint_t pages); static void ngx_slab_error(ngx_slab_pool_t *pool, ngx_uint_t level, char *text); - +static ngx_int_t ngx_slab_merge_pages(ngx_slab_pool_t *pool, + ngx_slab_page_t *page); static ngx_uint_t ngx_slab_max_size; static ngx_uint_t ngx_slab_exact_size; @@ -658,12 +659,56 @@ } } + ngx_flag_t retry = 0; + for (page = pool->free.next; page != &pool->free;) { + if (ngx_slab_merge_pages(pool, page) == NGX_OK) { + retry = 1; + } else { + page = page->next; + } + } + + if (retry) { + return ngx_slab_alloc_pages(pool, pages); + } + ngx_slab_error(pool, NGX_LOG_CRIT, "ngx_slab_alloc() failed: no memory"); return NULL; } +static ngx_int_t +ngx_slab_merge_pages(ngx_slab_pool_t *pool, ngx_slab_page_t *page) +{ + ngx_slab_page_t *prev, *p; + + p = &page[page->slab]; + if ((u_char *) p >= pool->end) { + return NGX_DECLINED; + } + + if (((ngx_slab_page_t *) p->prev != NULL) && + (p->next != NULL) && + ((p->prev & NGX_SLAB_PAGE_MASK) == NGX_SLAB_PAGE)) { + + page->slab += p->slab; + + prev = (ngx_slab_page_t *) (p->prev & ~NGX_SLAB_PAGE_MASK); + prev->next = p->next; + p->next->prev = p->prev; + + p->slab = NGX_SLAB_PAGE_FREE; + p->next = NULL; + p->prev = NGX_SLAB_PAGE; + + return NGX_OK; + } + + return NGX_DECLINED; +} + + static void ngx_slab_free_pages(ngx_slab_pool_t *pool, ngx_slab_page_t *page, ngx_uint_t pages)
_______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel