<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>This patch looks good to me. Just a couple questions about object accounting...</div><div> </div><div> brassow</div> <div><div>On Jun 15, 2009, at 12:21 PM, <a href="mailto:heinzm@redhat.com">heinzm@redhat.com</a> wrote:</div> <blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">+int dm_mem_cache_grow(struct dm_mem_cache_client *cl, unsigned objects) +{ + unsigned pages = objects * cl->chunks * cl->pages_per_chunk; + struct page_list *pl, *last; + + BUG_ON(!pages); + pl = alloc_cache_pages(pages); + if (!pl) + return -ENOMEM; + + last = pl; + while (last->next) + last = last->next; + + spin_lock_irq(&cl->lock); + last->next = cl->free_list; + cl->free_list = pl; + cl->free_pages += pages; + cl->total_pages += pages; + cl->objects++; </blockquote><div> </div><div>Should this be 'cl->objects += objects'?</div> <blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">+ spin_unlock_irq(&cl->lock); + + mempool_resize(cl->objs_pool, cl->objects, GFP_NOIO); </blockquote><div> </div><div>Do we need to check return value here?</div> <blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">+ return 0; +} +EXPORT_SYMBOL(dm_mem_cache_grow); + +/* Shrink a clients cache by an amount of pages */ +int dm_mem_cache_shrink(struct dm_mem_cache_client *cl, unsigned objects) +{ + int r; + unsigned pages = objects * cl->chunks * cl->pages_per_chunk, p = pages; + unsigned long flags; + struct page_list *last = NULL, *pl, *pos; + + BUG_ON(!pages); + + spin_lock_irqsave(&cl->lock, flags); + pl = pos = cl->free_list; + while (p-- && pos->next) { + last = pos; + pos = pos->next; + } + + if (++p) + r = -ENOMEM; + else { + r = 0; + cl->free_list = pos; + cl->free_pages -= pages; + cl->total_pages -= pages; + cl->objects--; </blockquote><div> </div><div>'cl->objects -= objects'?</div> <blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">+ last->next = NULL; + } + spin_unlock_irqrestore(&cl->lock, flags); + + if (!r) { + free_cache_pages(pl); + mempool_resize(cl->objs_pool, cl->objects, GFP_NOIO); </blockquote><div> </div><div>When shrinking, 'mempool_resize' will not fail... no need to check return value?</div> <blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">+ } + + return r; +} +EXPORT_SYMBOL(dm_mem_cache_shrink); + +/* + * Allocate/free a memory object + * + * Can be called from interrupt context + */ +struct dm_mem_cache_object *dm_mem_cache_alloc(struct dm_mem_cache_client *cl) +{ + int r = 0; + unsigned pages = cl->chunks * cl->pages_per_chunk; + unsigned long flags; + struct dm_mem_cache_object *obj; + + obj = mempool_alloc(cl->objs_pool, GFP_NOIO); + if (!obj) + return ERR_PTR(-ENOMEM); + + spin_lock_irqsave(&cl->lock, flags); + if (pages > cl->free_pages) + r = -ENOMEM; + else + cl->free_pages -= pages; + spin_unlock_irqrestore(&cl->lock, flags); </blockquote><div> </div><div>It's not important, but 'free_chunks' adjusts the 'cl->free_pages' count for us... That made me look for where 'cl->free_pages' was adjusted in 'alloc_chunks', but that is done here. Could we push this critical section into alloc_chunks and then just check the return code of that?</div> <blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">+ + if (r) { + mempool_free(obj, cl->objs_pool); + return ERR_PTR(r); + } + + alloc_chunks(cl, obj); + return obj; +} +EXPORT_SYMBOL(dm_mem_cache_alloc); + +void dm_mem_cache_free(struct dm_mem_cache_client *cl, + struct dm_mem_cache_object *obj) +{ + free_chunks(cl, obj); + mempool_free(obj, cl->objs_pool); +} +EXPORT_SYMBOL(dm_mem_cache_free); + +MODULE_DESCRIPTION(DM_NAME " dm memory cache"); +MODULE_AUTHOR("Heinz Mauelshagen <<a href="mailto:hjm@redhat.com">hjm@redhat.com</a>>"); +MODULE_LICENSE("GPL"); </blockquote></div> <div> </div></body></html>