On Tue, 10 Mar 2009, sanjay nadkarni (Laptop) wrote: > Hopefully third time is a charm..the review is located at: > > http://cr.opensolaris.org/~nadkarni/bug6679115+6805505/
lofi.c: lines 190-194: I would prefer LiveCD/installer specific comments to not be added here. Just mentioning that the caching helps with duplicate requests as well as with memory footprint would be adequate. lofi.c: In general, these changes introduce a link list of compressed segments. The kernel has generic list_create/ list_add/list_delete, etc interfaces that are in use by a number of the sub-systems. These generic functions make list manipulation very less error prone and there's even mdb support for them. Is there a reason why these weren't used here as well? It should be fairly easy to port these changes to use the list_* functions. lofi.c: line 237: Nit - Since this frees the compressed cache, should it be named lofi_free_comp_cache instead? lofi.c: line 907: It seems like the lc_data field is always going to be of the lsp->ls_uncomp_seg_sz size. If the lofi_max_comp_cache is "1" (like it will be by default), we're going to be continually doing kmem_alloc/kmem_free of the same size. It seems to me that this would be perfect case for creating a kmem_cache instead so you hit that instead of going to the memory allocator every time. lofi.c: line 1902: This mutex needs to be destroyed perhaps in lofi_free_handle? Alok
