Hello Maxim,
I changed the patch to check only the p->next pointer.
And checking if the page is in an address less than the (pool->pages +
pages).
+ ngx_slab_page_t *prev, *p;
+ ngx_uint_t pages;
+ size_t size;
+
+ size = pool->end - (u_char *) pool - sizeof(ngx_slab_pool_t);
+ pages = (ngx_uint_t) (size / (ngx_pagesize + sizeof(ngx_slab_page_t)));
+
+ p = &page[page->slab];
+
+ if ((p < pool->pages + pages) &&
+ (p->next != NULL) &&
+ ((p->prev & NGX_SLAB_PAGE_MASK) == NGX_SLAB_PAGE)) {
I hope that now I did the right checks.
Regards,
Wandenberg
On Fri, Dec 20, 2013 at 2:49 PM, Maxim Dounin <mdou...@mdounin.ru> wrote:
> Hello!
>
> On Tue, Dec 17, 2013 at 09:05:16PM -0200, Wandenberg Peixoto wrote:
>
> > 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.
>
> As far as I see, all pages in the pool->free list are expected to
> have both p->prev and p->next set. And all pages with type
> NGX_SLAB_PAGE will be either on the pool->free list, or will have
> p->next set to NULL.
>
> [...]
>
> > > > +{
> > > > + 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.
>
> It looks like you've tried to address this problem with the
> following check:
>
> > +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;
> > + }
>
> This looks wrong, as pool->end points to the end of the pool, not
> the end of allocated page structures.
>
> --
> Maxim Dounin
> http://nginx.org/
>
> _______________________________________________
> 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-26 22:02:37.103802929 -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,58 @@
}
}
+ 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;
+ ngx_uint_t pages;
+ size_t size;
+
+ size = pool->end - (u_char *) pool - sizeof(ngx_slab_pool_t);
+ pages = (ngx_uint_t) (size / (ngx_pagesize + sizeof(ngx_slab_page_t)));
+
+ p = &page[page->slab];
+
+ if ((p < pool->pages + pages) &&
+ (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
