Help with shared memory usage

[Wandenberg Peixoto]

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 <mdounin at 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 at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20131217/016979a9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unfrag_slab_memory3.patch
Type: text/x-patch
Size: 1707 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20131217/016979a9/attachment.bin>