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

2008/8/12 Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>:
>> +             if (fSemaphores)
>> +                     delete []fSemaphores;
>
> Maybe you should make sure that there aren't any entries left in
> fUndoList

I do that right before calling delete on the set.

>> +                     // Check if it's the very first sem_undo request for 
>> this
>> team
>> +                     if (team->xsi_sem_context == NULL) {
>> +                             team->xsi_sem_context = new(std::nothrow)
>> xsi_sem_context;
>> +                             if (team->xsi_sem_context == NULL) {
>> +                                     free(request);
>> +                                     free(request->undo_values);
>> +                                     return B_NO_MEMORY;
>> +                             }
>> +                     }
>
> Since you doesn't lock anything here, what makes you think that no
> other semaphore could create such a context in this very moment? You
> could use atomic_pointer_test_and_set() here to work around this
> without holding a dedicated lock (like a global creation lock).

Interesting. I didn't think of that. I'll look into
atomic_pointer_test_and_set().

>
>> +     // Lock the semaphore set itself and release both the semaphore
>> +     // set hash table lock and the ipc hash table lock _only_ if
>> +     // the command it's not IPC_RMID, this prevents undesidered
>> +     // situation from happening while (hopefully) improving the
>> +     // concurrency.
>> +     if (command != IPC_RMID) {
>> +             mutex_lock(&semaphoreSet->Lock());
>> +             setLocker.Unlock();
>> +             ipcLocker.Unlock();
>> +     }
>
> I haven't looked closely into this, but this doesn't look like it could
> work - the code afterwards accesses the semaphore set without holding
> its lock in case of IPC_RMID (even though it says it would be useless,
> I don't really see that). Why don't you just always hold the semaphore
> set lock in this situation?

The reason why I don't hold the set lock itself it's because it gets deleted
in case of IPC_RMID. Since I usually release both the hash tables lock
in favor of the set lock itself to improve concurrency, if I'd do that
even for IPC_RMID
it could happen that another team would wait on the same set that eventually
would get deleted, generating ugly situation.

This way works because for every operation (semctl, semop, semget) a team has
to get the semaphore set lock first, in which case once acquired (that
is after the deletion
of the set) it would fail in the lookup operation.


> BTW you could easily use a MutexLocker for this instead like this:
>        MutexLocker setLocker;
>        if (...)
>                setLocker.SetTo(&semaphoreSet->Lock());

Oh.. nice! That would improve the code quite a lot. I could remove
all the release_mutex calls. Thanks!

>
>> -     MutexLocker lock(sXsiSemaphoreSetLock);
>> +     MutexLocker hashLock(sXsiSemaphoreSetLock);
>
> You mix lock/locker, you should use "locker" consistently.

because I haven't understand what's the preferred name by our guidelines
if any ;-)


> All in all, it looks good, at least I can't spot anything else by
> reviewing it :-)

I've tested quite heavily (running differents tests concurrently) and seems to
work fine, but I can't assure bug-free code ;-)

> And again, sorry for the delay.

No problem.

>
> Bye,
>   Axel.
>

I'll apply all your suggestions and commit the code if you don't have
anything against.

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

Other related posts: