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);
