2009/3/10 Alok Aggarwal <Alok.Aggarwal at sun.com>: > > 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.
LiveCD/installer comments removed. > 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. No reason. I'll change it 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? Ok, I've renamed it. > 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. There is no invariant constructed object state that could be preserved by using kmem_cache. And the allocations / frees shouldn't happen that frequently (once per lofi compressed read when the cache can't be used). I doubt that using kmem_cache will result in any significant performance increase. > lofi.c: line 1902: This mutex needs to be destroyed perhaps > in lofi_free_handle? Yep. Btw. the same problem exists with the lsp->ls_vp_lock mutex, I'm destroying that in lofi_free_handle, too. > Alok >
