[haiku-gsoc] Re: XSI semaphore implamentation patch #2

2008/8/1 Ingo Weinhold <ingo_weinhold@xxxxxx>:
>> Hmm...  Let's say I use the following structure, and you agree :-)
>>
>> struct undoRequest : DoublyLinkedList<undoRequest> {
>>      union {
>>           team *team;  // Used in the sem list
>>           XsiSemaphoreSet *semaphore_set; // Used in the team
>>      } p;
>>      short  undo_value;
>>      int      semaphore_number;
>> }
>
> Nope, I don't agree. As I wrote, I would go with an entry per semaphore set
> and team:
>
> struct UndoEntry : DoublyLinkedListLinkImpl<UndoEntry> {
>        DoublyLinkedListLink<UndoEntry> team_link;
>        XsiSemaphoreSet*                                semaphore_set;
>        struct team*                                    team;
>        int16                                                   undo_values[0];
> };
>
> The entry would be in both the semaphore set and the team list. For each
> semaphore in the set there is an "undo_values" array element.
...
>
> One has to create only one entry which is added to both lists (currently you
> create on entry and add it to the global list and have to increment the
> counter in the team). If the semaphore set does already have an entry for the
> team, merely the array element for the concerned semaphore needs to be
> adjusted.

I though of adding the same object to both list, but that requires 4 links
in the same object instead of two, and I didn't think that would be
possible, but I see
you have added a double link element. I'll see how what works. Thanks!

> In the SETALL case, removing would be possible, but memset()ing the arrays to
> 0 is probably the better choice.

Ok.

>> > It doesn't make a difference when SETVAL/SETALL are called or whether
>> > there
>> > are undo entries for these semaphores, since you unconditionally iterate
>> > through the whole global undo entry list in any case.
>>
>> Exactly. And that can be easily fixed with a counter in the set.
>
> Yep, that can and probably should be done, if you don't implement my proposal.

I'll implement your proposal, after I'm done with testing.

>
> That reminds me: You have a locking order inversion in xsi_sem_undo(): It's
> sUndoListLock -> sXsiSemaphoreSetLock, while it is the other way around in
> _user_xsi_sem{op,ctl}().

I fixed that in the previous patch, which I'm going to commit. It
reverses the order in
xsi_sem_undo.

>
>> Seriously, I understand your concern about perfomances and I don't mean
>> in any way to waist your time, but I thought optimization in this situation
>> could be postponed.
>
> Yep, I'd much prefer unit tests first.

So I suppose those I've ported are not to be considered enough. Never mind,
I'll write an unit test more or like the one you wrote for the realtime sem.

>
> CU, Ingo
>

Thanks for your suggestions.

Regards,
-- 
Salvatore Benedetto (a.k.a. emitrax)
Student of Computer Engineer
University of Pisa
www.haiku-os.it

Other related posts: