<div dir="ltr">Hi Maxim,<div><br></div><div>sorry for the long delay. I hope you remember my issue.</div><div>In attach is the new patch with the changes you suggest.</div><div>Can you check it again? I hope it can be applied to nginx code now.</div>
<div><br></div><div>About this point "<span style="font-family:arial,sans-serif;font-size:13px">2. There is probably no need to check both prev and next.",</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">I check both pointers to avoid a segmentation fault, since in some situations the next can be NULL and the prev may point to </span><font face="arial, sans-serif">pool->free.</font></div>
<div><font face="arial, sans-serif">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. </font></div><div><font face="arial, sans-serif">I just made a double check to protect.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">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.</font></div>
<div><br></div><div>Regards,</div><div>Wandenberg</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Oct 6, 2013 at 6:37 AM, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello!<br>
<div class="im"><br>
On Wed, Jul 31, 2013 at 12:28:02AM -0300, Wandenberg Peixoto wrote:<br>
<br>
> Hello!<br>
><br>
> Thanks for your help. I hope that the patch be OK now.<br>
> I don't know if the function and variable names are on nginx pattern.<br>
> Feel free to change the patch.<br>
> If you have any other point before accept it, will be a pleasure to fix it.<br>
<br>
</div>Sorry for long delay. As promised, I've looked into this. Apart<br>
from multiple style problems, I see at least one case of possible use of<br>
uninitialized memory. See below for comments.<br>
<div class="im"><br>
><br>
> --- src/core/ngx_slab.c 2013-05-06 07:27:10.000000000 -0300<br>
> +++ src/core/ngx_slab.c 2013-07-31 00:21:08.043034442 -0300<br>
> @@ -615,6 +615,26 @@ fail:<br>
><br>
><br>
> static ngx_slab_page_t *<br>
> +ngx_slab_merge_with_neighbour(ngx_slab_pool_t *pool, ngx_slab_page_t *page)<br>
<br>
</div>A side note: there are multiple issues with style in this patch.<br>
In no particular order:<br>
<br>
1. There is no reason to return ngx_slab_page_t * here. Just<br>
ngx_int_t should be enough (or, even void would be good enough,<br>
see below).<br>
<br>
2. It is recommended do place function implementation below it's<br>
use (and add a prototype). This allows to read the code linearly,<br>
top-down.<br>
<div class="im"><br>
> +{<br>
> + ngx_slab_page_t *neighbour = &page[page->slab];<br>
<br>
</div>Here "neighbour" may point outside of the allocated page<br>
structures if we are freeing the last page in the pool.<br>
<div class="im"><br>
> + if (((ngx_slab_page_t *) neighbour->prev != NULL) && (neighbour->next<br>
> != NULL) && ((neighbour->prev & NGX_SLAB_PAGE_MASK) == NGX_SLAB_PAGE)) {<br>
<br>
</div>In no particular order:<br>
<br>
1. Style problems - more than 80 chars, extra parens.<br>
<br>
2. There is probably no need to check both prev and next.<br>
<br>
3. See below about casting neighbour->prev to (ngx_slab_page_t *).<br>
<div class="im"><br>
> + page->slab += neighbour->slab;<br>
> +<br>
> + ((ngx_slab_page_t *) neighbour->prev)->next = neighbour->next;<br>
> + neighbour->next->prev = neighbour->prev;<br>
<br>
</div>Casting neighbour->prev to (ngx_slab_page_t *) without removing<br>
last NGX_SLAB_PAGE_MASK bits isn't really a good idea. It works<br>
here as NGX_SLAB_PAGE == 0, but may fail in other cases.<br>
<br>
It would be much better to use a snippet already used in other<br>
places:<br>
<br>
prev = (ngx_slab_page_t *) (p->prev & ~NGX_SLAB_PAGE_MASK);<br>
prev->next = p->next;<br>
p->next->prev = p->prev;<br>
<div class="im"><br>
> + neighbour->slab = NGX_SLAB_PAGE_FREE;<br>
> + neighbour->prev = (uintptr_t) &pool->free;<br>
> + neighbour->next = &pool->free;<br>
<br>
</div>Unused page structures used to be zeroed. Any reason for<br>
prev/next pointing to pool->free?<br>
<br>
It looks like something like<br>
<br>
p->slab = NGX_SLAB_PAGE_FREE;<br>
p->next = NULL;<br>
p->prev = NGX_SLAB_PAGE;<br>
<br>
would be reasonable here.<br>
<div class="im"><br>
> +<br>
> + return page;<br>
> + }<br>
> + return NULL;<br>
> +}<br>
<br>
</div>See above, there is no need to return page as it's unused.<br>
Something like return NGX_OK / return NGX_DECLINED would be better<br>
here - if we are going to check if any pages were actually merged.<br>
<br>
Additionally, an empty line should be before last return.<br>
<div class="im"><br>
> +<br>
> +<br>
> +static ngx_slab_page_t *<br>
> ngx_slab_alloc_pages(ngx_slab_pool_t *pool, ngx_uint_t pages)<br>
> {<br>
> ngx_slab_page_t *page, *p;<br>
> @@ -657,6 +677,19 @@ ngx_slab_alloc_pages(ngx_slab_pool_t *po<br>
> }<br>
> }<br>
><br>
> + ngx_flag_t retry = 0;<br>
> + for (page = pool->free.next; page != &pool->free;) {<br>
> + if (ngx_slab_merge_with_neighbour(pool, page)) {<br>
> + retry = 1;<br>
> + } else {<br>
> + page = page->next;<br>
> + }<br>
> + }<br>
> +<br>
> + if (retry) {<br>
> + return ngx_slab_alloc_pages(pool, pages);<br>
> + }<br>
> +<br>
<br>
</div>Apart from multiple style issues here, I think we should choose<br>
one aproach: either loop over free pages and merge them here, or<br>
maintain all free pages merged in ngx_slab_free_pages() (that is,<br>
merge in both directions there).<br>
<div class="im"><br>
> ngx_slab_error(pool, NGX_LOG_CRIT, "ngx_slab_alloc() failed: no<br>
> memory");<br>
><br>
> return NULL;<br>
> @@ -687,6 +720,8 @@ ngx_slab_free_pages(ngx_slab_pool_t *poo<br>
> page->next->prev = (uintptr_t) page;<br>
><br>
> pool->free.next = page;<br>
> +<br>
> + ngx_slab_merge_with_neighbour(pool, page);<br>
> }<br>
<br>
</div>See above.<br>
<br>
Overral it looks much better than previous patch, but still needs<br>
more work.<br>
<div class="HOEnZb"><div class="h5"><br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.org/en/donation.html" target="_blank">http://nginx.org/en/donation.html</a><br>
<br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</div></div></blockquote></div><br></div>