[haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: "Salvatore Benedetto" <emitrax@xxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Wed, 6 Aug 2008 19:48:44 +0000
Hi there,
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.
Regards,
--
Salvatore Benedetto (a.k.a. emitrax)
Student of Computer Engineer
University of Pisa
www.haiku-os.it
Index: src/system/kernel/posix/xsi_semaphore.cpp
===================================================================
--- src/system/kernel/posix/xsi_semaphore.cpp (revision 26843)
+++ src/system/kernel/posix/xsi_semaphore.cpp (working copy)
@@ -24,7 +24,7 @@
#include <util/Vector.h>
-#define TRACE_XSI_SEM
+//#define TRACE_XSI_SEM
#ifdef TRACE_XSI_SEM
# define TRACE(x) dprintf x
# define TRACE_ERROR(x) dprintf x
@@ -50,25 +50,44 @@
typedef DoublyLinkedList<queued_thread> ThreadQueue;
+class XsiSemaphoreSet;
+
struct sem_undo : DoublyLinkedListLinkImpl<sem_undo> {
- sem_undo(struct team *process, int semaphoreSetID, short
semaphoreNumber,
- short undoValue)
+ sem_undo(XsiSemaphoreSet *semaphoreSet, struct team *team, int16
*undoValues)
:
- team(process),
- semaphore_set_id(semaphoreSetID),
- semaphore_number(semaphoreNumber),
- undo_value(undoValue)
+ semaphore_set(semaphoreSet),
+ team(team),
+ undo_values(undoValues)
{
}
- struct team *team;
- int semaphore_set_id;
- short semaphore_number;
- short undo_value;
+ DoublyLinkedListLink<sem_undo> team_link;
+ XsiSemaphoreSet *semaphore_set;
+ struct team *team;
+ int16
*undo_values;
};
-typedef DoublyLinkedList<sem_undo> SemaphoreUndoList;
+typedef DoublyLinkedList<sem_undo> undoList;
+typedef DoublyLinkedList<sem_undo,
+ DoublyLinkedListMemberGetLink<sem_undo, &sem_undo::team_link> >
teamList;
+struct xsi_sem_context {
+ xsi_sem_context()
+ :
+ undo_list()
+ {
+ mutex_init(&lock, "Private team undo_list lock");
+ }
+
+ ~xsi_sem_context()
+ {
+ mutex_destroy(&lock);
+ }
+
+ teamList undo_list;
+ mutex lock;
+};
+
// Xsi semaphore definition
class XsiSemaphore {
public:
@@ -119,22 +138,11 @@
}
}
- // Implemented after sUndoListLock is declared
- void ClearUndos(int semaphoreSetID, short semaphoreNumber);
-
pid_t LastPid() const
{
return fLastPidOperation;
}
- // Record the sem_undo operation in sUndoList. The only limit
- // here is the memory needed for creating a new sem_undo structure.
- // Implemented after sUndoListLock is declared
- int RecordUndo(int semaphoreSetID, short semaphoreNumber, short value);
-
- // Implemented after sUndoListLock is declared
- void RemoveUndo(int semaphoreSetID, short semaphoreNumber, short value);
-
void Revert(short value)
{
fValue -= value;
@@ -202,7 +210,6 @@
fThreadsWaitingToIncrease--;
}
}
-
return result;
}
@@ -248,6 +255,7 @@
fNumberOfSemaphores(numberOfSemaphores),
fSemaphores(0)
{
+ mutex_init(&fLock, "XsiSemaphoreSet private mutex");
SetIpcKey((key_t)-1);
SetPermissions(flags);
fSemaphores = new(std::nothrow)
XsiSemaphore[numberOfSemaphores];
@@ -260,15 +268,50 @@
~XsiSemaphoreSet()
{
- TRACE(("XsiSemaphoreSet::~XsiSemaphoreSet(): removing semaphore
set\n"));
- // Clear all sem_undo left, as our ID will be reused
- // by another set.
- for (uint32 i = 0; i < fNumberOfSemaphores; i++)
- fSemaphores[i].ClearUndos(fID, i);
+ TRACE(("XsiSemaphoreSet::~XsiSemaphoreSet(): removing semaphore
"
+ "set %d\n", fID));
+ mutex_destroy(&fLock);
UnsetID();
- delete []fSemaphores;
+ if (fSemaphores)
+ delete []fSemaphores;
}
+ void ClearUndo(unsigned short semaphoreNumber)
+ {
+ struct team *team = thread_get_current_thread()->team;
+ DoublyLinkedList<sem_undo>::Iterator iterator =
fUndoList.GetIterator();
+ while (iterator.HasNext()) {
+ struct sem_undo *current = iterator.Next();
+ if (current->team == team) {
+ TRACE(("XsiSemaphoreSet::ClearUndo: teamID =
%d, "
+ "semaphoreSetID = %d, semaphoreNumber =
%d\n",
+ fID, semaphoreNumber, (int)team->id));
+ MutexLocker _(team->xsi_sem_context->lock);
+ current->undo_values[semaphoreNumber] = 0;
+ return;
+ }
+ }
+ }
+
+ void ClearUndos()
+ {
+ // Clear all undo_values (POSIX semadj equivalent)
+ // of the calling team. This happens only on semctl SETALL.
+ struct team *team = thread_get_current_thread()->team;
+ DoublyLinkedList<sem_undo>::Iterator iterator =
fUndoList.GetIterator();
+ while (iterator.HasNext()) {
+ struct sem_undo *current = iterator.Next();
+ if (current->team == team) {
+ TRACE(("XsiSemaphoreSet::ClearUndos: teamID =
%d, "
+ "semaphoreSetID = %d\n", (int)team->id,
fID));
+ MutexLocker _(team->xsi_sem_context->lock);
+ memset(current->undo_values, 0,
+ sizeof(int16) * fNumberOfSemaphores);
+ return;
+ }
+ }
+ }
+
void DoIpcSet(struct semid_ds *result)
{
fPermissions.uid = result->sem_perm.uid;
@@ -330,11 +373,100 @@
return fLastSemopTime;
}
+ mutex &Lock()
+ {
+ return fLock;
+ }
+
ushort NumberOfSemaphores() const
{
return fNumberOfSemaphores;
}
+ // Record the sem_undo operation into our private fUndoList and
+ // the team undo_list. The only limit here is the memory needed
+ // for creating a new sem_undo structure.
+ int RecordUndo(short semaphoreNumber, short value)
+ {
+ // Look if there is already a record from the team caller
+ // for the same semaphore set
+ bool notFound = true;
+ struct team *team = thread_get_current_thread()->team;
+ DoublyLinkedList<sem_undo>::Iterator iterator =
fUndoList.GetIterator();
+ while (iterator.HasNext()) {
+ struct sem_undo *current = iterator.Next();
+ if (current->team == team) {
+ // Update its undo value
+ MutexLocker _(team->xsi_sem_context->lock);
+ int newValue =
current->undo_values[semaphoreNumber] + value;
+ if (newValue > USHRT_MAX || newValue <
-USHRT_MAX) {
+
TRACE_ERROR(("XsiSemaphoreSet::RecordUndo: newValue %d "
+ "out of range\n", newValue));
+ return ERANGE;
+ }
+ current->undo_values[semaphoreNumber] =
newValue;
+ notFound = false;
+ TRACE(("XsiSemaphoreSet::RecordUndo: found
record. Team = %d, "
+ "semaphoreSetID = %d, semaphoreNumber =
%d, value = %d\n",
+ (int)team->id, fID, semaphoreNumber,
+ current->undo_values[semaphoreNumber]));
+ break;
+ }
+ }
+
+ if (notFound) {
+ // First sem_undo request from this team for this
+ // semaphore set
+ struct sem_undo *request
+ = (struct sem_undo *)malloc(sizeof(struct
sem_undo));
+ if (request == NULL)
+ return B_NO_MEMORY;
+ request->team = team;
+ request->semaphore_set = this;
+ request->undo_values
+ = (int16 *)malloc(sizeof(int16) *
fNumberOfSemaphores);
+ if (request->undo_values == NULL) {
+ free(request);
+ return B_NO_MEMORY;
+ }
+ // 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;
+ }
+ }
+
+ memset(request->undo_values, 0, sizeof(int16) *
fNumberOfSemaphores);
+ request->undo_values[semaphoreNumber] = value;
+ // Add the request to both XsiSemaphoreSet and team list
+ fUndoList.Add(request);
+ MutexLocker _(team->xsi_sem_context->lock);
+ team->xsi_sem_context->undo_list.Add(request);
+ TRACE(("XsiSemaphoreSet::RecordUndo: new record added.
Team = %d, "
+ "semaphoreSetID = %d, semaphoreNumber = %d,
value = %d\n",
+ (int)team->id, fID, semaphoreNumber, value));
+ }
+ return B_OK;
+ }
+
+ void RevertUndo(short semaphoreNumber, short value)
+ {
+ // This can be called only when RecordUndo fails.
+ struct team *team = thread_get_current_thread()->team;
+ DoublyLinkedList<sem_undo>::Iterator iterator =
fUndoList.GetIterator();
+ while (iterator.HasNext()) {
+ struct sem_undo *current = iterator.Next();
+ if (current->team == team) {
+ MutexLocker _(team->xsi_sem_context->lock);
+ fSemaphores[semaphoreNumber].Revert(value);
+ break;
+ }
+ }
+ }
+
XsiSemaphore* Semaphore(int nth) const
{
return &fSemaphores[nth];
@@ -365,6 +497,11 @@
fPermissions.mode = (flags & 0x01ff);
}
+ undoList &UndoList()
+ {
+ return fUndoList;
+ }
+
// Implemented after sSemaphoreHashTable is declared
void UnsetID();
@@ -380,7 +517,9 @@
time_t fLastSemopTime;
// sem_otime
ushort fNumberOfSemaphores;
// sem_nsems
struct ipc_perm fPermissions;
// sem_perm
- XsiSemaphore *fSemaphores;
+ XsiSemaphore *fSemaphores;
// array of semaphores
+ undoList fUndoList;
// undo list requests
+ mutex fLock;
// private lock
::HashTableLink<XsiSemaphoreSet> fLink;
};
@@ -484,130 +623,14 @@
#define MAX_XSI_SEMAPHORE 512
static OpenHashTable<IpcHashTableDefinition> sIpcHashTable;
static OpenHashTable<SemaphoreHashTableDefinition> sSemaphoreHashTable;
-static SemaphoreUndoList sUndoList;
static mutex sIpcLock;
static mutex sXsiSemaphoreSetLock;
-static mutex sUndoListLock;
static vint32 sNextAvailableID = 1;
static vint32 sXsiSemaphoreCount = 0;
-void
-XsiSemaphore::ClearUndos(int semaphoreSetID, short semaphoreNumber)
-{
- // Clear all undo_value (Posix semadj equivalent),
- // which result in removing the sem_undo record from
- // the global undo list, plus decrementing the related
- // team xsi_sem_undo_requests field.
- // This happens only on semctl SETVAL and SETALL.
- TRACE(("XsiSemaphore::ClearUndos: semaphoreSetID = %d, "
- "semaphoreNumber = %d\n", semaphoreSetID, semaphoreNumber));
- MutexLocker _(sUndoListLock);
- DoublyLinkedList<sem_undo>::Iterator iterator = sUndoList.GetIterator();
- while (iterator.HasNext()) {
- struct sem_undo *current = iterator.Next();
- if (current->semaphore_set_id == semaphoreSetID
- && current->semaphore_number ==
semaphoreNumber) {
- InterruptsSpinLocker lock(gTeamSpinlock);
- if (current->team)
- current->team->xsi_sem_undo_requests--;
- iterator.Remove();
- // Restore interrupts before calling free
- lock.Unlock();
- free(current);
- }
- }
-}
-
-
-int
-XsiSemaphore::RecordUndo(int semaphoreSetID, short semaphoreNumber, short
value)
-{
- // Look if there is already a record from the same
- // team for the same semaphore set && semaphore number
- bool notFound = true;
- struct team *team = thread_get_current_thread()->team;
- MutexLocker _(sUndoListLock);
- DoublyLinkedList<sem_undo>::Iterator iterator = sUndoList.GetIterator();
- while (iterator.HasNext()) {
- struct sem_undo *current = iterator.Next();
- if (current->team == team
- && current->semaphore_set_id == semaphoreSetID
- && current->semaphore_number == semaphoreNumber) {
- // Update its undo value
- TRACE(("XsiSemaphore::RecordUndo: found record. Team =
%d, "
- "semaphoreSetID = %d, semaphoreNumber = %d,
value = %d\n",
- (int)team->id, semaphoreSetID, semaphoreNumber,
- current->undo_value));
- int newValue = current->undo_value + value;
- if (newValue > USHRT_MAX || newValue < -USHRT_MAX) {
- TRACE_ERROR(("XsiSemaphore::RecordUndo:
newValue %d out of range\n",
- newValue));
- return ERANGE;
- }
- current->undo_value = newValue;
- notFound = false;
- break;
- }
- }
-
- if (notFound) {
- // First sem_undo request from this team for this
- // semaphore set && semaphore number
- struct sem_undo *request
- = (struct sem_undo *)malloc(sizeof(struct sem_undo));
- if (request == NULL)
- return B_NO_MEMORY;
- request->team = team;
- request->semaphore_set_id = semaphoreSetID;
- request->semaphore_number = semaphoreNumber;
- request->undo_value = value;
- // Add the request to the global sem_undo list
- InterruptsSpinLocker _(gTeamSpinlock);
- if ((int)(team->xsi_sem_undo_requests + 1) < USHRT_MAX)
- team->xsi_sem_undo_requests++;
- else
- return ENOSPC;
- sUndoList.Add(request);
- TRACE(("XsiSemaphore::RecordUndo: new record added. Team = %d, "
- "semaphoreSetID = %d, semaphoreNumber = %d, value =
%d\n",
- (int)team->id, semaphoreSetID, semaphoreNumber,
- request->undo_value));
- }
- return B_OK;
-}
-
-
-void
-XsiSemaphore::RemoveUndo(int semaphoreSetID, short semaphoreNumber, short
value)
-{
- // This can be called only when RecordUndo fails.
- MutexLocker _(sUndoListLock);
- DoublyLinkedList<sem_undo>::Iterator iterator = sUndoList.GetIterator();
- while (iterator.HasNext()) {
- struct sem_undo *current = iterator.Next();
- if (current->semaphore_set_id == semaphoreSetID
- && current->semaphore_number ==
semaphoreNumber) {
- current->undo_value -= value;
- // Remove the request from sUndoList only if
- // it happens to be the only one made by this
- // process, that is, don't remove any valide
- // sem_undo request made previously by the same
- // process
- if (current->undo_value == 0) {
- InterruptsSpinLocker _(gTeamSpinlock);
- if (current->team)
- current->team->xsi_sem_undo_requests--;
- iterator.Remove();
- free(current);
- }
- }
- }
-}
-
-
// #pragma mark -
@@ -647,50 +670,48 @@
mutex_init(&sIpcLock, "global Posix IPC table");
mutex_init(&sXsiSemaphoreSetLock, "global Posix xsi sem table");
- mutex_init(&sUndoListLock, "global Posix xsi sem undo list");
}
-/*! Function called on team exit when there are sem_undo requests */
+/*! Function called on team exit to process any sem_undo requests */
void
-xsi_sem_undo(team_id teamID, int32 numberOfUndos)
+xsi_sem_undo(struct team *team)
{
- if (numberOfUndos <= 0)
+ if (team->xsi_sem_context == NULL)
return;
- // We must hold the set mutex before the sem_undo
- // one in order to avoid deadlock with RecordUndo
- MutexLocker lock(sXsiSemaphoreSetLock);
- MutexLocker _(sUndoListLock);
- DoublyLinkedList<sem_undo>::Iterator iterator = sUndoList.GetIterator();
- // Look for all sem_undo request from this team
+ // By acquiring first the semaphore hash table lock
+ // we make sure the semaphore set in our sem_undo
+ // list won't get removed by IPC_RMID call
+ MutexLocker _(sXsiSemaphoreSetLock);
+
+ // Process all sem_undo request in the team sem undo list
+ // if any
+ teamList::Iterator iterator
+ = team->xsi_sem_context->undo_list.GetIterator();
while (iterator.HasNext()) {
struct sem_undo *current = iterator.Next();
- if (current->team->id == teamID) {
- // Check whether the semaphore set still exist
- int semaphoreSetID = current->semaphore_set_id;
- XsiSemaphoreSet *semaphoreSet
- = sSemaphoreHashTable.Lookup(semaphoreSetID);
- if (semaphoreSet != NULL) {
- // Revert the changes done by this process
- XsiSemaphore *semaphore
- =
semaphoreSet->Semaphore(current->semaphore_number);
+ XsiSemaphoreSet *semaphoreSet = current->semaphore_set;
+ // Acquire the set lock in order to prevent race
+ // condition with RecordUndo
+ MutexLocker setLocker(semaphoreSet->Lock());
+ MutexLocker _(team->xsi_sem_context->lock);
+ // Revert the changes done by this process
+ for (int i = 0; i < semaphoreSet->NumberOfSemaphores(); i++)
+ if (current->undo_values[i] != 0) {
TRACE(("xsi_sem_undo: TeamID = %d,
SemaphoreSetID = %d, "
- "SemaphoreNumber = %d, undo value =
%d\n", (int)teamID,
- semaphoreSetID,
current->semaphore_number,
- current->undo_value));
- semaphore->Revert(current->undo_value);
- } else
- TRACE(("xsi_sem_undo: semaphore set %d does not
exist "
- "anymore. Ignore record.\n",
semaphoreSetID));
+ "SemaphoreNumber = %d, undo value =
%d\n", (int)team->id,
+ semaphoreSet->ID(), i,
(int)current->undo_values[i]));
+
semaphoreSet->Semaphore(i)->Revert(current->undo_values[i]);
+ }
- // Remove and free the sem_undo structure from sUndoList
- iterator.Remove();
- free(current);
- if (--numberOfUndos == 0)
- break;
- }
+ // Remove and free the sem_undo structure from both list
+ iterator.Remove();
+ semaphoreSet->UndoList().Remove(current);
+ free(current);
}
+ delete team->xsi_sem_context;
+ team->xsi_sem_context = NULL;
}
@@ -776,16 +797,15 @@
"semaphores allowed\n"));
return ENOSPC;
}
- atomic_add(&sXsiSemaphoreCount, 1);
semaphoreSet = new(std::nothrow)
XsiSemaphoreSet(numberOfSemaphores,
flags);
if (semaphoreSet == NULL || !semaphoreSet->InitOK()) {
TRACE_ERROR(("xsi_semget: failed to allocate a new xsi "
"semaphore set\n"));
- atomic_add(&sXsiSemaphoreCount, -1);
return ENOMEM;
}
+ atomic_add(&sXsiSemaphoreCount, numberOfSemaphores);
MutexLocker _(sXsiSemaphoreSetLock);
semaphoreSet->SetID();
@@ -808,7 +828,8 @@
{
TRACE(("xsi_semctl: semaphoreID = %d, semaphoreNumber = %d, command =
%d\n",
semaphoreID, semaphoreNumber, command));
- MutexLocker _(sXsiSemaphoreSetLock);
+ MutexLocker ipcLocker(sIpcLock);
+ MutexLocker setLocker(sXsiSemaphoreSetLock);
XsiSemaphoreSet *semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
if (semaphoreSet == NULL) {
TRACE_ERROR(("xsi_semctl: semaphore set id %d not valid\n",
@@ -821,89 +842,109 @@
"semaphore %d\n", semaphoreNumber, semaphoreID));
return EINVAL;
}
-
if (args != 0 && !IS_USER_ADDRESS(args)) {
TRACE_ERROR(("xsi_semctl: semun address is not valid\n"));
return B_BAD_ADDRESS;
}
+ // 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();
+ }
+
+ int result = 0;
XsiSemaphore *semaphore = semaphoreSet->Semaphore(semaphoreNumber);
switch (command) {
- case GETVAL:
+ case GETVAL: {
if (!semaphoreSet->HasReadPermission()) {
TRACE_ERROR(("xsi_semctl: calling process has
not permission "
"on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
- }
- return semaphore->Value();
+ result = EACCES;
+ } else
+ result = semaphore->Value();
+ break;
+ }
- case SETVAL:
+ case SETVAL: {
if (!semaphoreSet->HasPermission()) {
TRACE_ERROR(("xsi_semctl: calling process has
not permission "
"on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
+ result = EACCES;
+ } else {
+ int value;
+ if (user_memcpy(&value, &args->val,
sizeof(int)) < B_OK) {
+ TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
+ result = B_BAD_ADDRESS;
+ } else if (value > USHRT_MAX) {
+ TRACE_ERROR(("xsi_semctl: value %d out
of range\n", value));
+ result = ERANGE;
+ } else {
+ semaphore->SetValue(value);
+
semaphoreSet->ClearUndo(semaphoreNumber);
+ }
}
- int value;
- if (user_memcpy(&value, &args->val, sizeof(int)) <
B_OK) {
- TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
- return B_BAD_ADDRESS;
- }
- if (value > USHRT_MAX) {
- TRACE_ERROR(("xsi_semctl: value %d out of
range\n", value));
- return ERANGE;
- }
- TRACE(("xsi_semctl: SemaphoreNumber = %d, SETVAL value
= %d\n",
- semaphoreNumber, value));
- semaphore->SetValue(value);
- semaphore->ClearUndos(semaphoreSet->ID(),
semaphoreNumber);
- return 0;
+ break;
+ }
- case GETPID:
+ case GETPID: {
if (!semaphoreSet->HasReadPermission()) {
TRACE_ERROR(("xsi_semctl: calling process has
not permission "
"on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
- }
- return semaphore->LastPid();
+ result = EACCES;
+ } else
+ result = semaphore->LastPid();
+ break;
+ }
- case GETNCNT:
+ case GETNCNT: {
if (!semaphoreSet->HasReadPermission()) {
TRACE_ERROR(("xsi_semctl: calling process has
not permission "
"on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
- }
- return semaphore->ThreadsWaitingToIncrease();
+ result = EACCES;
+ } else
+ result = semaphore->ThreadsWaitingToIncrease();
+ break;
+ }
- case GETZCNT:
+ case GETZCNT: {
if (!semaphoreSet->HasReadPermission()) {
TRACE_ERROR(("xsi_semctl: calling process has
not permission "
"on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
- }
- return semaphore->ThreadsWaitingToBeZero();
+ result = EACCES;
+ } else
+ result = semaphore->ThreadsWaitingToBeZero();
+ break;
+ }
case GETALL: {
if (!semaphoreSet->HasReadPermission()) {
TRACE_ERROR(("xsi_semctl: calling process has
not read "
"permission on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
- }
- for (int i = 0; i < semaphoreSet->NumberOfSemaphores();
i++) {
- semaphore = semaphoreSet->Semaphore(i);
- unsigned short value = semaphore->Value();
- if (user_memcpy(&args->array[i], &value,
sizeof(unsigned short))
- < B_OK) {
- TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
- return B_BAD_ADDRESS;
+ result = EACCES;
+ } else
+ for (int i = 0; i <
semaphoreSet->NumberOfSemaphores(); i++) {
+ semaphore = semaphoreSet->Semaphore(i);
+ unsigned short value =
semaphore->Value();
+ if (user_memcpy(&args->array[i], &value,
+ sizeof(unsigned short)) < B_OK)
{
+ TRACE_ERROR(("xsi_semctl:
user_memcpy failed\n"));
+ result = B_BAD_ADDRESS;
+ break;
+ }
}
- }
- return 0;
+ break;
}
case SETALL: {
@@ -911,20 +952,25 @@
TRACE_ERROR(("xsi_semctl: calling process has
not permission "
"on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
- }
- for (int i = 0; i < semaphoreSet->NumberOfSemaphores();
i++) {
- semaphore = semaphoreSet->Semaphore(i);
- unsigned short value;
- if (user_memcpy(&value, &args->array[i],
sizeof(unsigned short))
- < B_OK) {
- TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
- return B_BAD_ADDRESS;
+ result = EACCES;
+ } else {
+ bool doClear = true;
+ for (int i = 0; i <
semaphoreSet->NumberOfSemaphores(); i++) {
+ semaphore = semaphoreSet->Semaphore(i);
+ unsigned short value;
+ if (user_memcpy(&value,
&args->array[i], sizeof(unsigned short))
+ < B_OK) {
+ TRACE_ERROR(("xsi_semctl:
user_memcpy failed\n"));
+ result = B_BAD_ADDRESS;
+ doClear = false;
+ break;
+ } else
+ semaphore->SetValue(value);
}
- semaphore->SetValue(value);
- semaphore->ClearUndos(semaphoreSet->ID(),
semaphoreNumber);
+ if (doClear)
+ semaphoreSet->ClearUndos();
}
- return 0;
+ break;
}
case IPC_STAT: {
@@ -932,19 +978,20 @@
TRACE_ERROR(("xsi_semctl: calling process has
not read "
"permission on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
+ result = EACCES;
+ } else {
+ struct semid_ds sem;
+ sem.sem_perm = semaphoreSet->IpcPermission();
+ sem.sem_nsems =
semaphoreSet->NumberOfSemaphores();
+ sem.sem_otime = semaphoreSet->LastSemopTime();
+ sem.sem_ctime = semaphoreSet->LastSemctlTime();
+ if (user_memcpy(args->buf, &sem, sizeof(struct
semid_ds))
+ < B_OK) {
+ TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
+ result = B_BAD_ADDRESS;
+ }
}
- struct semid_ds result;
- result.sem_perm = semaphoreSet->IpcPermission();
- result.sem_nsems = semaphoreSet->NumberOfSemaphores();
- result.sem_otime = semaphoreSet->LastSemopTime();
- result.sem_ctime = semaphoreSet->LastSemctlTime();
- if (user_memcpy(args->buf, &result, sizeof(struct
semid_ds))
- < B_OK) {
- TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
- return B_BAD_ADDRESS;
- }
- return 0;
+ break;
}
case IPC_SET: {
@@ -952,19 +999,24 @@
TRACE_ERROR(("xsi_semctl: calling process has
not "
"permission on semaphore %d, key %d\n",
semaphoreSet->ID(),
(int)semaphoreSet->IpcKey()));
- return EACCES;
+ result = EACCES;
+ } else {
+ struct semid_ds sem;
+ if (user_memcpy(&sem, args->buf, sizeof(struct
semid_ds))
+ < B_OK) {
+ TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
+ result = B_BAD_ADDRESS;
+ } else
+ semaphoreSet->DoIpcSet(&sem);
}
- struct semid_ds result;
- if (user_memcpy(&result, args->buf, sizeof(struct
semid_ds))
- < B_OK) {
- TRACE_ERROR(("xsi_semctl: user_memcpy
failed\n"));
- return B_BAD_ADDRESS;
- }
- semaphoreSet->DoIpcSet(&result);
- return 0;
+ break;
}
case IPC_RMID: {
+ // If this was the command, we are still holding
+ // the semaphore set hash table lock along with the
+ // ipc hash table lock, but not the semaphore set
+ // itself lock as it would be useless in this case
if (!semaphoreSet->HasPermission()) {
TRACE_ERROR(("xsi_semctl: calling process has
not "
"permission on semaphore %d, key %d\n",
@@ -974,25 +1026,34 @@
key_t key = semaphoreSet->IpcKey();
Ipc *ipcKey = NULL;
if (key != -1) {
- MutexLocker _(sIpcLock);
ipcKey = sIpcHashTable.Lookup(key);
sIpcHashTable.Remove(ipcKey);
}
sSemaphoreHashTable.Remove(semaphoreSet);
- atomic_add(&sXsiSemaphoreCount,
- semaphoreSet->NumberOfSemaphores());
// Wake up of threads waiting on this set
// happens in the destructor
if (key != -1)
delete ipcKey;
+ atomic_add(&sXsiSemaphoreCount,
-semaphoreSet->NumberOfSemaphores());
+ // Remove any sem_undo request
+ while (struct sem_undo *entry
+ = semaphoreSet->UndoList().RemoveHead()) {
+ MutexLocker
_(entry->team->xsi_sem_context->lock);
+
entry->team->xsi_sem_context->undo_list.Remove(entry);
+ free(entry);
+ }
+
delete semaphoreSet;
return 0;
}
default:
TRACE_ERROR(("xsi_semctl: command %d not valid\n",
command));
- return EINVAL;
+ result = EINVAL;
}
+
+ mutex_unlock(&semaphoreSet->Lock());
+ return result;
}
@@ -1001,13 +1062,15 @@
{
TRACE(("xsi_semop: semaphoreID = %d, ops = %p, numOps = %ld\n",
semaphoreID, ops, numOps));
- MutexLocker lock(sXsiSemaphoreSetLock);
+ MutexLocker hashLock(sXsiSemaphoreSetLock);
XsiSemaphoreSet *semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
if (semaphoreSet == NULL) {
TRACE_ERROR(("xsi_semop: semaphore set id %d not valid\n",
semaphoreID));
return EINVAL;
}
+ MutexLocker locker(semaphoreSet->Lock());
+ hashLock.Unlock();
if (!IS_USER_ADDRESS(ops)) {
TRACE_ERROR(("xsi_semop: sembuf address is not valid\n"));
@@ -1101,19 +1164,23 @@
if (operations[i].sem_op != 0)
waitOnZero = false;
- lock.Unlock();
+ locker.Unlock();
result = semaphore->Wait((int32)operations[i].sem_op,
waitOnZero);
TRACE(("xsi_semop: back to life\n"));
// We are back to life.
// Find out why!
- lock.Lock();
+ hashLock.Lock();
semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
- if (result == EIDRM || result == B_INTERRUPTED) {
+ if (result == EIDRM || result == B_INTERRUPTED
+ || semaphoreSet == NULL) {
TRACE_ERROR(("xsi_semop: semaphore set id %d
got destroyed\n",
semaphoreID));
result = EIDRM;
notDone = false;
+ } else {
+ locker.Lock();
+ hashLock.Unlock();
}
} else {
// everything worked like a charm (so far)
@@ -1128,8 +1195,8 @@
semaphore =
semaphoreSet->Semaphore(semaphoreNumber);
short operation = operations[i].sem_op;
if (operations[i].sem_flg & SEM_UNDO)
- if
(semaphore->RecordUndo(semaphoreSet->ID(),
- semaphoreNumber, operation) !=
B_OK) {
+ if
(semaphoreSet->RecordUndo(semaphoreNumber, operation)
+ != B_OK) {
// Unlikely scenario, but we
might get here.
// Undo everything!
// Start with semaphore
operations
@@ -1143,8 +1210,8 @@
// Remove all previously
registered sem_undo request
for (uint32 j = 0; j < i; j++) {
if
(operations[j].sem_flg & SEM_UNDO)
-
semaphore->RemoveUndo(semaphoreSet->ID(),
-
operations[j].sem_num, operations[j].sem_op);
+
semaphoreSet->RevertUndo(operations[j].sem_num,
+
operations[j].sem_op);
}
result = ENOSPC;
}
Index: src/system/kernel/team.cpp
===================================================================
--- src/system/kernel/team.cpp (revision 26843)
+++ src/system/kernel/team.cpp (working copy)
@@ -703,7 +703,7 @@
team->io_context = NULL;
team->address_space = NULL;
team->realtime_sem_context = NULL;
- team->xsi_sem_undo_requests = 0;
+ team->xsi_sem_context = NULL;
team->thread_list = NULL;
team->main_thread = NULL;
team->loading_info = NULL;
@@ -1327,7 +1327,7 @@
delete_team_user_data(team);
vm_delete_areas(team->address_space);
- xsi_sem_undo(team->id, team->xsi_sem_undo_requests);
+ xsi_sem_undo(team);
delete_owned_ports(team->id);
sem_delete_owned_sems(team->id);
remove_images(team);
@@ -2344,7 +2344,7 @@
vfs_free_io_context(team->io_context);
delete_realtime_sem_context(team->realtime_sem_context);
- xsi_sem_undo(team->id, team->xsi_sem_undo_requests);
+ xsi_sem_undo(team);
delete_owned_ports(teamID);
sem_delete_owned_sems(teamID);
remove_images(team);
Index: headers/private/kernel/thread_types.h
===================================================================
--- headers/private/kernel/thread_types.h (revision 26843)
+++ headers/private/kernel/thread_types.h (working copy)
@@ -67,6 +67,7 @@
struct realtime_sem_context; // defined in realtime_sem.cpp
struct select_info;
struct user_thread; // defined in
libroot/user_thread.h
+struct xsi_sem_context; // defined in xsi_semaphore.cpp
struct death_entry {
struct list_link link;
@@ -121,7 +122,6 @@
#include <condition_variable.h>
#include <util/DoublyLinkedList.h>
-
struct job_control_entry : DoublyLinkedListLinkImpl<job_control_entry> {
job_control_state state; // current team job control
state
thread_id thread; // main thread ID ==
team ID
@@ -183,7 +183,7 @@
int32 flags;
void *io_context;
struct realtime_sem_context *realtime_sem_context;
- int32 xsi_sem_undo_requests;
+ struct xsi_sem_context *xsi_sem_context;
sem_id death_sem; // semaphore to wait on
for dying threads
struct list dead_threads;
int dead_threads_count;
- Follow-Ups:
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Salvatore Benedetto
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Axel Dörfler
- References:
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Salvatore Benedetto
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Ingo Weinhold
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Salvatore Benedetto
Other related posts:
- » [haiku-gsoc] XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- » [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Salvatore Benedetto
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Axel Dörfler
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Salvatore Benedetto
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Ingo Weinhold
- [haiku-gsoc] Re: XSI semaphore implamentation patch #2
- From: Salvatore Benedetto