[prev in list] [next in list] [prev in thread] [next in thread] 

List:       haiku-commits
Subject:    [haiku-commits] Re: r34375 - in haiku/trunk/src/libs/compat/freebsd_network:
From:       Colin Günther <coling () gmx ! de>
Date:       2009-11-30 15:33:58
Message-ID: 4B13E5E6.2000108 () gmx ! de
[Download RAW message or body]

Hi Ingo,
thanks for the remarks.
Basically I'm trying to figure out how to implement the sleepqueue 
functionality of FreeBSD by using Haiku's condition variable system.

Ingo Weinhold schrieb:
[...] The other issues you pointed out were thankfully applied.
>>  
>>      if (status == B_OK)
>> @@ -78,7 +110,12 @@
>>  
>>  
>>  void
>> -_cv_signal(struct cv* conditionVariable)
>> +_cv_signal(struct cv* conditionVariablePointer)
>>  {
>> -    conditionVariable->condVar->NotifyOne();
>> +    InterruptsSpinLocker _(sConditionVariablesLock);
>> +    ConditionVariable* conditionVariable
>> +        = sConditionVariableHash.Lookup(conditionVariablePointer);
>> +    if (conditionVariablePointer->cv_waiters > 0)
>> +        conditionVariablePointer->cv_waiters--;
>> +    conditionVariable->NotifyOne();
>>  }
>>     
>
> What's the point of cv_waiters? It's never read.
>   
cv_waiters is originally used in the FreeBSD struct cv definition. And 
due to removing the condVar structure member I needed something else to 
put in this structure. And while I'm at it I wanne fully implement it, 
at least.
>   
>> Modified: haiku/trunk/src/libs/compat/freebsd_network/compat/sys/condvar.h
>> ===================================================================
>> --- haiku/trunk/src/libs/compat/freebsd_network/compat/sys/condvar.h    
>> 2009-11-30 12:48:35 UTC (rev 34374)
>> +++ haiku/trunk/src/libs/compat/freebsd_network/compat/sys/condvar.h    
>> 2009-11-30 13:17:04 UTC (rev 34375)
>> @@ -8,13 +8,15 @@
>>  
>>  #include <sys/queue.h>
>>  
>> +
>>  struct cv {
>> -    struct ConditionVariable* condVar;
>> +    int cv_waiters;
>>  };
>>     
>
> The previous implementation of the struct seemed to make more sense to me. 
> The lookup in _cv_signal() and the wait functions would be superfluous (and 
> thus the locking) and you wouldn't need the hash table anymore. Instead I 
> would add a link for a singly linked list, and use a list for the cleanup.
>   
I can understand that this looks misterious at the moment. Things will 
make more sense, once I get to the point where I'm implementing the 
msleep and wakeup functions. The idea of sleepqueues is what I'm trying 
to achieve, so using a ConditionVariable member would be kind of a 
double effort.

> CU, Ingo
>
>   
-Colin

[prev in list] [next in list] [prev in thread] [next in thread]