Skip to contentSkip to conversation

ipc 1/3: Add refcount to ipc_rcu_alloc

PATCH In review Manfred Spraul 2 participants 5 messages Jul 3, 2004

lore
Share & tools
lore

Manfred Spraul

author
#0 lore raw

PATCH 1/1 ipc 1/3: Add refcount to ipc_rcu_alloc

Hi,

the lifetime of the ipc objects (sem array, msg queue, shm mapping) is controlled by kern_ipc_perms->lock - a spinlock. There is no simple way to reacquire this spinlock after it was dropped to schedule()/kmalloc/copy_{to,from}_user/whatever.

The attached patch adds a reference count as a preparation to get rid of sem_revalidate().

Andrew, could you add it to -mm?

Signed-off-by Manfred Spraul <[email protected]>
ipc/msg.c +3 -3
diff -u 2.6/ipc/msg.c build-2.6/ipc/msg.c
@@ -100,14 +100,14 @@
 	msq->q_perm.security = NULL;
 	retval = security_msg_queue_alloc(msq);
 	if (retval) {
-		ipc_rcu_free(msq, sizeof(*msq));
+		ipc_rcu_putref(msq);
 		return retval;
 	}
 
 	id = ipc_addid(&msg_ids, &msq->q_perm, msg_ctlmni);
 	if(id == -1) {
 		security_msg_queue_free(msq);
-		ipc_rcu_free(msq, sizeof(*msq));
+		ipc_rcu_putref(msq);
 		return -ENOSPC;
 	}
 

@@ -193,7 +193,7 @@
 	}
 	atomic_sub(msq->q_cbytes, &msg_bytes);
 	security_msg_queue_free(msq);
-	ipc_rcu_free(msq, sizeof(struct msg_queue));
+	ipc_rcu_putref(msq);
 }
 
 asmlinkage long sys_msgget (key_t key, int msgflg)
ipc/sem.c +3 -3
diff -u 2.6/ipc/sem.c build-2.6/ipc/sem.c
@@ -179,14 +179,14 @@
 	sma->sem_perm.security = NULL;
 	retval = security_sem_alloc(sma);
 	if (retval) {
-		ipc_rcu_free(sma, size);
+		ipc_rcu_putref(sma);
 		return retval;
 	}
 
 	id = ipc_addid(&sem_ids, &sma->sem_perm, sc_semmni);
 	if(id == -1) {
 		security_sem_free(sma);
-		ipc_rcu_free(sma, size);
+		ipc_rcu_putref(sma);
 		return -ENOSPC;
 	}
 	used_sems += nsems;

@@ -473,7 +473,7 @@
 	used_sems -= sma->sem_nsems;
 	size = sizeof (*sma) + sma->sem_nsems * sizeof (struct sem);
 	security_sem_free(sma);
-	ipc_rcu_free(sma, size);
+	ipc_rcu_putref(sma);
 }
 
 static unsigned long copy_semid_to_user(void __user *buf, struct semid64_ds *in, int version)
ipc/shm.c +3 -3
diff -u 2.6/ipc/shm.c build-2.6/ipc/shm.c
@@ -117,7 +117,7 @@
 		shmem_lock(shp->shm_file, 0);
 	fput (shp->shm_file);
 	security_shm_free(shp);
-	ipc_rcu_free(shp, sizeof(struct shmid_kernel));
+	ipc_rcu_putref(shp);
 }
 
 /*

@@ -194,7 +194,7 @@
 	shp->shm_perm.security = NULL;
 	error = security_shm_alloc(shp);
 	if (error) {
-		ipc_rcu_free(shp, sizeof(*shp));
+		ipc_rcu_putref(shp);
 		return error;
 	}
 

@@ -234,7 +234,7 @@
 	fput(file);
 no_file:
 	security_shm_free(shp);
-	ipc_rcu_free(shp, sizeof(*shp));
+	ipc_rcu_putref(shp);
 	return error;
 }
ipc/util.c +41 -12

105 lines not on this page · raw message

ipc/util.h +9 -3
diff -u 2.6/ipc/util.h build-2.6/ipc/util.h
@@ -45,14 +45,20 @@
  */
 void* ipc_alloc(int size);
 void ipc_free(void* ptr, int size);
-/* for allocation that need to be freed by RCU
- * both function can sleep
+
+/* 
+ * For allocation that need to be freed by RCU.
+ * Objects are reference counted, they start with reference count 1.
+ * getref increases the refcount, the putref call that reduces the recount
+ * to 0 schedules the rcu destruction. Caller must guarantee locking.
  */
 void* ipc_rcu_alloc(int size);
-void ipc_rcu_free(void* arg, int size);
+void ipc_rcu_getref(void *ptr);
+void ipc_rcu_putref(void *ptr);
 
 struct kern_ipc_perm* ipc_get(struct ipc_ids* ids, int id);
 struct kern_ipc_perm* ipc_lock(struct ipc_ids* ids, int id);
+void ipc_lock_by_ptr(struct kern_ipc_perm *ipcp);
 void ipc_unlock(struct kern_ipc_perm* perm);
 int ipc_buildid(struct ipc_ids* ids, int id, int seq);
 int ipc_checkid(struct ipc_ids* ids, struct kern_ipc_perm* ipcp, int uid);
#1 lore raw
Quoted 8 lines · Manfred Spraul

the lifetime of the ipc objects (sem array, msg queue, shm mapping) is
controlled by kern_ipc_perms->lock - a spinlock. There is no simple way to reacquire this spinlock after it was dropped to schedule()/kmalloc/copy_{to,from}_user/whatever.

The attached patch adds a reference count as a preparation to get rid of sem_revalidate().

The pointer offsetting tricks are rather unattractive. Is it not possible to simple aggregate the refcount into the refcounted structure, use container_of()?

author
#2 lore raw
Quoted 5 lines · Andrew Morton

The pointer offsetting tricks are rather unattractive. Is it not possible to simple aggregate the refcount into the refcounted structure, use container_of()?

Patch attached: it worked quite well and I've used the chance to reduce the size of the vmalloc header a bit: the work_struct and the rcu_head structures can be a union.

Signed-off-by Manfred Spraul <[email protected]>
ipc/msg.c +3 -3
diff -u 2.6/ipc/msg.c build-2.6/ipc/msg.c
@@ -100,14 +100,14 @@
 	msq->q_perm.security = NULL;
 	retval = security_msg_queue_alloc(msq);
 	if (retval) {
-		ipc_rcu_free(msq, sizeof(*msq));
+		ipc_rcu_putref(msq);
 		return retval;
 	}
 
 	id = ipc_addid(&msg_ids, &msq->q_perm, msg_ctlmni);
 	if(id == -1) {
 		security_msg_queue_free(msq);
-		ipc_rcu_free(msq, sizeof(*msq));
+		ipc_rcu_putref(msq);
 		return -ENOSPC;
 	}
 

@@ -193,7 +193,7 @@
 	}
 	atomic_sub(msq->q_cbytes, &msg_bytes);
 	security_msg_queue_free(msq);
-	ipc_rcu_free(msq, sizeof(struct msg_queue));
+	ipc_rcu_putref(msq);
 }
 
 asmlinkage long sys_msgget (key_t key, int msgflg)
ipc/sem.c +3 -3
diff -u 2.6/ipc/sem.c build-2.6/ipc/sem.c
@@ -179,14 +179,14 @@
 	sma->sem_perm.security = NULL;
 	retval = security_sem_alloc(sma);
 	if (retval) {
-		ipc_rcu_free(sma, size);
+		ipc_rcu_putref(sma);
 		return retval;
 	}
 
 	id = ipc_addid(&sem_ids, &sma->sem_perm, sc_semmni);
 	if(id == -1) {
 		security_sem_free(sma);
-		ipc_rcu_free(sma, size);
+		ipc_rcu_putref(sma);
 		return -ENOSPC;
 	}
 	used_sems += nsems;

@@ -473,7 +473,7 @@
 	used_sems -= sma->sem_nsems;
 	size = sizeof (*sma) + sma->sem_nsems * sizeof (struct sem);
 	security_sem_free(sma);
-	ipc_rcu_free(sma, size);
+	ipc_rcu_putref(sma);
 }
 
 static unsigned long copy_semid_to_user(void __user *buf, struct semid64_ds *in, int version)
ipc/shm.c +3 -3
diff -u 2.6/ipc/shm.c build-2.6/ipc/shm.c
@@ -117,7 +117,7 @@
 		shmem_lock(shp->shm_file, 0);
 	fput (shp->shm_file);
 	security_shm_free(shp);
-	ipc_rcu_free(shp, sizeof(struct shmid_kernel));
+	ipc_rcu_putref(shp);
 }
 
 /*

@@ -194,7 +194,7 @@
 	shp->shm_perm.security = NULL;
 	error = security_shm_alloc(shp);
 	if (error) {
-		ipc_rcu_free(shp, sizeof(*shp));
+		ipc_rcu_putref(shp);
 		return error;
 	}
 

@@ -234,7 +234,7 @@
 	fput(file);
 no_file:
 	security_shm_free(shp);
-	ipc_rcu_free(shp, sizeof(*shp));
+	ipc_rcu_putref(shp);
 	return error;
 }
ipc/util.c +67 -27

164 lines not on this page · raw message

ipc/util.h +9 -3
diff -u 2.6/ipc/util.h build-2.6/ipc/util.h
@@ -45,14 +45,20 @@
  */
 void* ipc_alloc(int size);
 void ipc_free(void* ptr, int size);
-/* for allocation that need to be freed by RCU
- * both function can sleep
+
+/* 
+ * For allocation that need to be freed by RCU.
+ * Objects are reference counted, they start with reference count 1.
+ * getref increases the refcount, the putref call that reduces the recount
+ * to 0 schedules the rcu destruction. Caller must guarantee locking.
  */
 void* ipc_rcu_alloc(int size);
-void ipc_rcu_free(void* arg, int size);
+void ipc_rcu_getref(void *ptr);
+void ipc_rcu_putref(void *ptr);
 
 struct kern_ipc_perm* ipc_get(struct ipc_ids* ids, int id);
 struct kern_ipc_perm* ipc_lock(struct ipc_ids* ids, int id);
+void ipc_lock_by_ptr(struct kern_ipc_perm *ipcp);
 void ipc_unlock(struct kern_ipc_perm* perm);
 int ipc_buildid(struct ipc_ids* ids, int id, int seq);
 int ipc_checkid(struct ipc_ids* ids, struct kern_ipc_perm* ipcp, int uid);
#3 lore raw
Quoted 7 lines · Manfred Spraul

+struct ipc_rcu_hdr
+{
+ int refcount;
+ int is_vmalloc;
+ void *data[0];
+};

That's not what I meant ;)

struct ipc_rcu_hdr

{
	int refcount;
	int is_vmalloc;
};

Then place one of these inside struct msg_queue, one inside struct sem_array, etc.

#define ipc_rcu_putref(p)
	if (--p->rcu_hdr.refcount == 0)
		ipc_rcu_putref_final(p, &p->rcu_hdr)

or whatever.

That's assuming struct kref isn't suitable. I guess you don't want the atomic_t.

author
#4 lore raw
Quoted 14 lines · Andrew Morton

ipc/util.c line 294

Quoted 8 lines · Manfred Spraul
ipc/util.c +6 -0
+struct ipc_rcu_hdr
+{
+	int refcount;
+	int is_vmalloc;
+	void *data[0];
+};

That's not what I meant ;)

Quoted 11 lines

struct ipc_rcu_hdr

{
	int refcount;
	int is_vmalloc;
};

Then place one of these inside struct msg_queue, one inside struct sem_array, etc.

No. The size of the headers depends on the allocation size: 8 bytes for allocs < 4088 bytes, 60 bytes for larger allocs. I don't want to expose that implementation detail outside of util.c. ipc_rcu_alloc allocates an arbitrary sized refcounted memory block that is released through the rcu framework. Everything else is hidden in util.c.

--
Manfred