On Tue, May 5, 2009 at 10:31 PM, Juergen Keil <jrgn.keil at googlemail.com> wrote: > 2009/5/5 Moinak Ghosh <moinakg at belenix.org>: >> On Tue, May 5, 2009 at 6:04 PM, Juergen Keil <jrgn.keil at googlemail.com> >> wrote: >>> 2009/5/4 Alok Aggarwal <Alok.Aggarwal at sun.com>: >>>> I am sending this code review request on behalf >>>> of Moinak Ghosh (CC'd). Please review the changes >>>> for - >>>> >>>> 6802997 Clofi memory allocation behavior can be improved >>>> >>>> Webrev location - >>>> >>>> http://cr.opensolaris.org/~aalok/sponsor-6802997/ >>>> >>>> Moinak's testing has showed that these changes result >>>> in a performance improvement during installation. >>> >>> In lofi.c, lines 1123 .. 1133: >>> >>> 1123 ? ? ? ? ? ? ? ? mutex_enter(&lsp->ls_comp_bufs_lock); >>> 1124 ? ? ? ? ? ? ? ? for (i = 0; i < lofi_taskq_nthreads; i++) { >>> 1125 ? ? ? ? ? ? ? ? ? ? ? ? if (lsp->ls_comp_bufs[i].inuse == 0) { >>> 1126 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? lsp->ls_comp_bufs[i].inuse = 1; >>> 1127 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? j = i; >>> 1128 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break; >>> 1129 ? ? ? ? ? ? ? ? ? ? ? ? } >>> 1130 ? ? ? ? ? ? ? ? } >>> 1131 >>> 1132 ? ? ? ? ? ? ? ? mutex_exit(&lsp->ls_comp_bufs_lock); >>> 1133 ? ? ? ? ? ? ? ? ASSERT(j < lofi_taskq_nthreads); >>> >>> I think this needs to handle the case when all >>> buffers are in use. ?With the current code we >>> exit from the loop with 'j' uninitialized, and could >>> panic a debug kernel with the >>> ASSERT(j < lofi_taskq_nthreads) >>> >> >> ? The number of buffers == number of lofi threads and each thread uses >> ? exactly one buffer at any given point of time. So this scenario will not >> ? occur. > > What do you think about rewriting the loop to use 'j'; > so that in the impossible and internal error case 'j' > is always initialized at the ASSERT? > > And we don't need a 64 bit int for the loop index, > the same type as lofi_taskq_nthreads (int) should > be ok. > > 1027 ? ? ? ? ? ? ? ? int j; > ... > 1124 ? ? ? ? ? ? ? ? for (j = 0; j < lofi_taskq_nthreads; j++) { > 1125 ? ? ? ? ? ? ? ? ? ? ? ? if (lsp->ls_comp_bufs[j].inuse == 0) { > 1126 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? lsp->ls_comp_bufs[j].inuse = 1; > 1128 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break; > 1129 ? ? ? ? ? ? ? ? ? ? ? ? } > 1130 ? ? ? ? ? ? ? ? } > 1131 > 1132 ? ? ? ? ? ? ? ? mutex_exit(&lsp->ls_comp_bufs_lock); > 1133 ? ? ? ? ? ? ? ? ASSERT(j < lofi_taskq_nthreads); >
I think this is a good suggestion. Regards, Moinak. -- ================================ http://www.belenix.org/ http://moinakg.wordpress.com/
