[Cbe-oss-dev] [PATCH 2/2] libspe: fix unexpected blocking in spe_get_event

[Kazunori Asayama]

This patch fixes following problems of libspe v1.0.1.
Two programs to reproduce these problems are also attached.

1. spe_get_event() causes unexpected blocking when:

   - size of array of `struct spe_event' is greater than 1,
   - there are two or more elements in the array
     which have the same values in `events' fields and in `gid' fields, and,
   - no event is buffered when the function is called,
     and then an event becomes available.

     e.g.)

     spe_gid_t gid;
     struct spe_event events[2];
     .......
     events[0].gid = gid;
     events[0].events = SPE_EVENT_STOP | SPE_EVENT_THREAD_EXIT;
     events[1].gid = gid;
     events[1].events = SPE_EVENT_STOP | SPE_EVENT_THREAD_EXIT;
     spe_get_event(&events, 2, -1);

2. spe_get_event() loses events when:

   - two or more types of events are specified in `events' field of
     `struct spe_event', and,
   - no event is buffered when the function is called,
     and then two or more events become available at the almost same time.

     e.g.)

     - PPU

       spe_gid_t gid;
       struct spe_event event;
       .......
       event.gid = gid;
       event.events = SPE_EVENT_MAILBOX | SPE_EVENT_STOP | SPE_EVENT_THREAD_EXIT;
       spe_get_event(&event, 1, -1);

     - SPU

       int main()
       {
         spu_writech(SPU_WrOutIntrMbox, 0);
         spu_stop(0x1000);
         return 0;
       }

-- 
(ASAYAMA Kazunori
  (asayama at sm.sony.co.jp))
t

--
diff -urp libspe-1.0.1.orig/spe.c libspe-1.0.1-a/spe.c
--- libspe-1.0.1.orig/spe.c	2006-03-27 20:23:32.000000000 +0900
+++ libspe-1.0.1-a/spe.c	2006-03-28 12:31:38.000000000 +0900
@@ -1116,6 +1116,21 @@ spe_kill (speid_t speid, int sig)
 	return rc;
 }

+struct poll_helper {
+	int event;
+	struct thread_store *thread;
+	int type;
+	struct pollfd *pollfd;
+};
+
+static int
+qsort_compare_poll_helper(const void *p1, const void *p2)
+{
+  const struct poll_helper *e1 = p1;
+  const struct poll_helper *e2 = p2;
+  return e1->pollfd->fd - e2->pollfd->fd;
+}
+
 int
 spe_get_event(struct spe_event *pevents, int nevents, int timeout)
 {
@@ -1124,15 +1139,10 @@ spe_get_event(struct spe_event *pevents,
 	int numSPEsToPoll = 0;
 	int setupSPEs =0;
 	int pollRet = 0;
+	int lastSPEfd;

 	struct pollfd *SPEfds;

-	struct poll_helper {
-		int event;
-		struct thread_store *thread;
-		int type;
-	};
-
 	struct poll_helper *phelper;
 	
 	pthread_mutex_lock(&grp_list.mutex);
@@ -1243,6 +1253,7 @@ spe_get_event(struct spe_event *pevents,
 			
 				phelper[setupSPEs].event=i;
 				phelper[setupSPEs].thread=thread;
+				phelper[setupSPEs].pollfd=SPEfds + setupSPEs;
 				phelper[setupSPEs].type=1;
 				//printf("1\n");
 				setupSPEs++;
@@ -1256,6 +1267,7 @@ spe_get_event(struct spe_event *pevents,
 				
 				phelper[setupSPEs].event=i;
 				phelper[setupSPEs].thread=thread;
+				phelper[setupSPEs].pollfd=SPEfds + setupSPEs;
 				phelper[setupSPEs].type=2;
 				//printf("2\n");
 				
@@ -1269,6 +1281,7 @@ spe_get_event(struct spe_event *pevents,

 				phelper[setupSPEs].event=i;
 				phelper[setupSPEs].thread=thread;
+				phelper[setupSPEs].pollfd=SPEfds + setupSPEs;
 				phelper[setupSPEs].type=3;
 				if (SPEfds[setupSPEs].fd == -1)
 					fprintf(stderr, "Warning: spe_get_events: attempting "
@@ -1304,32 +1317,42 @@ spe_get_event(struct spe_event *pevents,
 		return 0;
 	}
 	
+ 	
+	/* Sort results by fd to make it easy to avoid reading twice from the same fd.
+	   There may be more efficient way to do the same thing...
+	*/
+	qsort(phelper, setupSPEs, sizeof(*phelper), qsort_compare_poll_helper);
+	lastSPEfd = -1;
 	
 	for (i=0 ; i < setupSPEs ; i++ )
 	{
-		if (SPEfds[i].revents != 0)
+		if (phelper[i].pollfd->revents != 0 &&
+			lastSPEfd != phelper[i].pollfd->fd &&
+			pevents[phelper[i].event].revents == 0 /* avoid overwriting */)
 		{
 			int rc,data;
 			switch (phelper[i].type) {
 			case 1:
 				// Read ibox data if present

-				rc = read(SPEfds[i].fd, &data, 4);
+				rc = read(phelper[i].pollfd->fd, &data, 4);
 				if (rc == 4)
 				{
 					pevents[phelper[i].event].data = data;
 					pevents[phelper[i].event].speid = phelper[i].thread;
 					pevents[phelper[i].event].revents = SPE_EVENT_MAILBOX;
+					ret_events++;
 				}
 				break;
 			case 2:
                                	// Read pipe data if present

-                               	rc = read(SPEfds[i].fd, &data, 4);
+				rc = read(phelper[i].pollfd->fd, &data, 4);
                                	//pevents[phelper[i].event].revents = pevents[phelper[i].event].revents | data;
                                	pevents[phelper[i].event].revents = data;
 				pevents[phelper[i].event].speid = phelper[i].thread;
 			       	pevents[phelper[i].event].data = phelper[i].thread->ev_data;
+				ret_events++;

 			       	phelper[i].thread->ev_data = 0;
 			       	phelper[i].thread->event = 0;
@@ -1337,22 +1360,24 @@ spe_get_event(struct spe_event *pevents,
 			case 3:
 				// Read tag group data if present

-				rc = read(SPEfds[i].fd, &data, 4);
+				rc = read(phelper[i].pollfd->fd, &data, 4);
 				if (rc == 4)
 				{
 					pevents[phelper[i].event].data = data;
 					pevents[phelper[i].event].speid = phelper[i].thread;
 					pevents[phelper[i].event].revents = SPE_EVENT_TAG_GROUP;
+					ret_events++;
 				}
 				break;
 			}
+			lastSPEfd = phelper[i].pollfd->fd;
 		}
 	}
 	
 	free(SPEfds);
 	free(phelper);
 	//printf("P2\n");
-	return pollRet;
+	return ret_events;
 }

 int
-------------- next part --------------
A non-text attachment was scrubbed...
Name: block.tar
Type: application/octet-stream
Size: 10240 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/cbe-oss-dev/attachments/20060328/cc227554/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: overwrite.tar
Type: application/octet-stream
Size: 10240 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/cbe-oss-dev/attachments/20060328/cc227554/attachment-0001.obj>