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

"Salvatore Benedetto" <emitrax@xxxxxxxxx> wrote:
> I reworked the way sem_undo request are handled following Ingo 
> suggestions.
> I've also fixed some other minor bugs.
> 
> I've added a lock per set to improve concurrency, plus I needed one 
> to lock
> the private sem_undo list of the set.
> 
> I've tested and seems to work just fine, although I think I'll rework
> the flow of semget and how IDs are assigned.

Sorry for the delay, and here we go:
> -typedef DoublyLinkedList<sem_undo> SemaphoreUndoList;
> +typedef DoublyLinkedList<sem_undo> undoList;
> +typedef DoublyLinkedList<sem_undo,
> +     DoublyLinkedListMemberGetLink<sem_undo, &sem_undo::team_link> > 
> teamList;

That would be UndoList, and TeamList; it's a C++ type.

> +     xsi_sem_context()
> +             :
> +             undo_list()

Empty constructors can be left out.

> +             if (fSemaphores)
> +                     delete []fSemaphores;

No need to check against NULL, plus the [] goes to delete, not 
fSemaphores.
Maybe you should make sure that there aren't any entries left in 
fUndoList 

> +             DoublyLinkedList<sem_undo>::Iterator iterator = 
> fUndoList.GetIterator();

You could use your UndoList typedef here :-)

> +                     struct sem_undo *request
> +                             = (struct sem_undo *)malloc(sizeof(struct 
> sem_undo));

sem_undo has C++ type members, so new(std::nothrow) sounds like the 
better choice here (even though it shouldn't do any harm in this 
particular case - but one day there might be some ASSERTs in the 
DoublyLinkedList implementation where this could become a problem).

> +                     // 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).

> +             // Remove and free the sem_undo structure from both list

spelling: lists

> +     // 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?
BTW you could easily use a MutexLocker for this instead like this:
        MutexLocker setLocker;
        if (...)
                setLocker.SetTo(&semaphoreSet->Lock());

> -     MutexLocker lock(sXsiSemaphoreSetLock);
> +     MutexLocker hashLock(sXsiSemaphoreSetLock);

You mix lock/locker, you should use "locker" consistently.

> +                     while (struct sem_undo *entry
> +                             = semaphoreSet->UndoList().RemoveHead()) {
[...]
> +                                     if 
> (semaphoreSet->RecordUndo(semaphoreNumber, 
> operation)
> +                                             != B_OK) {

I would indent the second like one more tab, since it's a single term 
(unlike when you use &&/|| operators where you then have multiple terms 
on the same level).

All in all, it looks good, at least I can't spot anything else by 
reviewing it :-)
And again, sorry for the delay.

Bye,
   Axel.

Other related posts: