Processes and semaphores in C

Question

I have this question in my exam and I want to know if my answer is correct.

Consider the following program written in C language to implement a reader:

// to simplify the exercise, are not checked the return values ​​of calls to the operating system
 int main(int argc, char* argv[]) {
 sem_t *sem_wrt;
 int fd, readcount;
 fd=shm_open("/readcount",O_RDWR|O_CREAT,S_IRUSR|S_IWUSR);
 ftruncate(fd, sizeof(int));
 readcount=mmap(0,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0);
 sem_wrt=sem_open("/write",O_CREAT ,0xFFFFFFFF,1);
 while(1) {
 readcount++;
 if (readcount==1) sem_wait (wrt);
 my_read();
 (readcount)--;
 if (readcount==0) sem_post (wrt);
 }
 }

After some time of using this program, it was found that the program only works properly when it is executed only by a process simultaneously.

Change the code of the program in order to provide a solution to this problem.

int main(int argc, char* argv[]) {
 sem_t *sem_wrt;
 int fd, readcount;
 fd=shm_open("/readcount",O_RDWR|O_CREAT,S_IRUSR|S_IWUSR);
 ftruncate(fd, sizeof(int));
 readcount=mmap(0,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0);
 sem_wrt=sem_open("/write",O_CREAT ,0xFFFFFFFF,1);
 //sem_mutex=sem_open("/mutex",O_CREAT ,0xFFFFFFFF,1);
 while(1) {
 //sem_wait(mutex);
 readcount++;
 if (readcount==1) sem_wait (wrt);
 //sem_post(mutex);
 my_read();
 //sem_wait(mutex);
 (readcount)--;
 if (readcount==0) sem_post (wrt);
 sem_post(mutex);
 }
 }

Answer 1

int fd, readcount;
// ...
readcount=mmap(0,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0);

If this is the code you're using, it probably gave you a warning which you ignored. mmap returns void *. You should change the declaration of readcount to int *. Also, the size of the mapping should be a multiple of the page size. I'd be sure to re-read the manual for mmap, carefully read any compiler warnings, and make sure your code checks for errors.

At any rate, if int *readcount; refers to shared memory, the following will not work:

++*readcount;

The problem is this line is conceptually broken into a few steps:

1. Fetch *readcount into register
2. Increment register
3. Store back into *readcount.

If multiple processes perform steps 1/2/3 in lockstep, and the initial value is 0, they will both store 1 back.

You could use a GCC extension for compare and swap to make an increment safe:

int fetch;
do
{
 fetch = *readcount;
} while (!__sync_bool_compare_and_swap(readcount, fetch, fetch+1));

However I'm not sure if this will work right if you do it on memory mapped from a file by mmap. Keeping this kind of atomicity in a page fault handler seems complex and I'm not sure if the standard guarantees that it should work.