Skip to contentSkip to conversation

updated ipc lock patch

PATCH Rusty Russell 2 participants 6 messages Oct 28, 2002

lore
Share & tools
lore
#0 lore raw

include/linux/msg.h

Quoted 3 lines

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal working-2.5.44-mm4-ipc-rcu/include/linux/msg.h working-2.5.44-mm4-ipc-rcu-fix/include/linux/msg.h

include/linux/msg.h +0 -0

Oops. That patch had some fluff in msg.h and sem.h. Delete those, or just use this instead (still against Mingming's mm4 "ignore kmalloc failure" patch):

ipc/util.c +51 -35
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal working-2.5.44-mm4-ipc-rcu/ipc/util.c working-2.5.44-mm4-ipc-rcu-fix/ipc/util.c
@@ -213,21 +213,49 @@ struct kern_ipc_perm* ipc_rmid(struct ip
 	return p;
 }
 
+struct ipc_rcu_kmalloc
+{
+	struct rcu_head rcu;
+	/* "void *" makes sure alignment of following data is sane. */
+	void *data[0];
+};
+
+struct ipc_rcu_vmalloc
+{
+	struct rcu_head rcu;
+	struct work_struct work;
+	/* "void *" makes sure alignment of following data is sane. */
+	void *data[0];
+};
+
+static inline int use_vmalloc(int size)
+{
+	/* Too big for a single page? */
+	if (sizeof(struct ipc_rcu_kmalloc) + size > PAGE_SIZE)
+		return 1;
+	return 0;
+}
+
 /**
  *	ipc_alloc	-	allocate ipc space
  *	@size: size desired
  *
  *	Allocate memory from the appropriate pools and return a pointer to it.
- *	NULL is returned if the allocation fails
+ *	NULL is returned if the allocation fails.  This can be freed with
+ *	ipc_free (to free immediately) or ipc_rcu_free (to free once safe).
  */
- 
 void* ipc_alloc(int size)
 {
 	void* out;
-	if(size > PAGE_SIZE)
-		out = vmalloc(size);
-	else
-		out = kmalloc(size, GFP_KERNEL);
+	/* We prepend the allocation with the rcu struct, and
+           workqueue if necessary (for vmalloc). */
+	if (use_vmalloc(size)) {
+		out = vmalloc(sizeof(struct ipc_rcu_vmalloc) + size);
+		if (out) out += sizeof(struct ipc_rcu_vmalloc);
+	} else {
+		out = kmalloc(sizeof(struct ipc_rcu_kmalloc)+size, GFP_KERNEL);
+		if (out) out += sizeof(struct ipc_rcu_kmalloc);
+	}
 	return out;
 }
 
@@ -242,48 +270,36 @@ void* ipc_alloc(int size)
  
 void ipc_free(void* ptr, int size)
 {
-	if(size > PAGE_SIZE)
-		vfree(ptr);
+	if (use_vmalloc(size))
+		vfree(ptr - sizeof(struct ipc_rcu_vmalloc));
 	else
-		kfree(ptr);
+		kfree(ptr - sizeof(struct ipc_rcu_kmalloc));
 }
 
 /* 
  * Since RCU callback function is called in bh,
  * we need to defer the vfree to schedule_work
  */
-static void ipc_free_scheduled(void* arg)
+static void ipc_schedule_free(void *arg)
 {
-	struct rcu_ipc_free *a = (struct rcu_ipc_free *)arg;
-	vfree(a->ptr);
-	kfree(a);
-}
+	struct ipc_rcu_vmalloc *free = arg;
 
-static void ipc_free_callback(void* arg)
-{
-	struct rcu_ipc_free *a = (struct rcu_ipc_free *)arg;
-	/* 
-	 * if data is vmalloced, then we need to delay the free
-	 */
-	if (a->size > PAGE_SIZE) {
-		INIT_WORK(&a->work, ipc_free_scheduled, arg);
-		schedule_work(&a->work);
-	} else {
-		kfree(a->ptr);
-		kfree(a);
-	}
+	INIT_WORK(&free->work, vfree, free);
+	schedule_work(&free->work);
 }
 
 void ipc_rcu_free(void* ptr, int size)
 {
-	struct rcu_ipc_free* arg;
-
-	arg = (struct rcu_ipc_free *) kmalloc(sizeof(*arg), GFP_KERNEL);
-	if (arg == NULL)
-		return;
-	arg->ptr = ptr;
-	arg->size = size;
-	call_rcu(&arg->rcu_head, ipc_free_callback, arg);
+	if (use_vmalloc(size)) {
+		struct ipc_rcu_vmalloc *free;
+		free = ptr - sizeof(*free);
+		call_rcu(&free->rcu, ipc_schedule_free, free);
+	} else {
+		struct ipc_rcu_kmalloc *free;
+		free = ptr - sizeof(*free);
+		/* kfree takes a "const void *" so gcc warns.  So we cast. */
+		call_rcu(&free->rcu, (void (*)(void *))kfree, free);
+	}
 }
 
 /**

--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#1 lore raw

ipc/util.c struct kern_ipc_perm* ipc_rmid(struct ip · line 221

Quoted 6 lines · Rusty Russell
ipc/util.c +6 -0
+struct ipc_rcu_kmalloc
+{
+	struct rcu_head rcu;
+	/* "void *" makes sure alignment of following data is sane. */
+	void *data[0];
+};

Rusty, why not using gcc "aligned" keywords instead of black magic :

void *data[0];

- Davide

#2 lore raw

In message <[email protected]> y ou write:

Quoted 11 lines · Rusty Russell

ipc/util.c struct kern_ipc_perm* ipc_rmid(struct ip · line 221

Quoted 7 lines
ipc/util.c +6 -0
+struct ipc_rcu_kmalloc
+{
+	struct rcu_head rcu;
+	/* "void *" makes sure alignment of following data is sane. */
+	void *data[0];
+};

Rusty, why not using gcc "aligned" keywords instead of black magic :

Quoted 1 line

void *data[0];

I think it's clearer *why* it's being done than:

	struct ipc_rcu_kmalloc
	{
		struct rcu_head rcu;
	} __attribute__((aligned(__alignof__(void *))));

And simpler than:

	struct ipc_rcu_kmalloc
	{
		struct rcu_head rcu;
		/* "void *" makes sure alignment of following data is sane. */
		char data[0] __attribute__((aligned(__alignof__(void *))));
	};

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#3 lore raw
Quoted 13 lines · Rusty Russell

In message <[email protected]> y ou write:

Quoted 11 lines · Rusty Russell

ipc/util.c struct kern_ipc_perm* ipc_rmid(struct ip · line 221

Quoted 7 lines
ipc/util.c +6 -0
+struct ipc_rcu_kmalloc
+{
+	struct rcu_head rcu;
+	/* "void *" makes sure alignment of following data is sane. */
+	void *data[0];
+};

Rusty, why not using gcc "aligned" keywords instead of black magic :

Quoted 8 lines
Quoted 1 line

void *data[0];

I think it's clearer *why* it's being done than:

	struct ipc_rcu_kmalloc
	{
		struct rcu_head rcu;
	} __attribute__((aligned(__alignof__(void *))));

Well, not really Rusty. The above syntax uses documented gcc features already used inside the kernel, while the fact that void *data[0]; enforces alignment it is not ( to my knowledge ) documented anywhere. You can also avoid the comment using the aligned syntax ...

- Davide

#4 lore raw

In message <[email protected]> yo u write:

Quoted 9 lines · Rusty Russell
Quoted 7 lines

I think it's clearer *why* it's being done than:

	struct ipc_rcu_kmalloc
	{
		struct rcu_head rcu;
	} __attribute__((aligned(__alignof__(void *))));
Quoted 4 lines

Well, not really Rusty. The above syntax uses documented gcc features already used inside the kernel, while the fact that void *data[0]; enforces alignment it is not ( to my knowledge ) documented anywhere. You can also avoid the comment using the aligned syntax ...

A comment is definitely required: you must say *why* you are aligning the structure (a clearer comment would be better, of course):

	/* We return a pointer after the structure to the unwitting
	   caller: ensure safe alignment. */

The alignment of a structure member of type X is the alignment of type
X: this seems obvious to me. And "type X data[0];" is the standard way of representing a variable struct.

Have we picked all the nits yet? 8)
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#5 lore raw
Quoted 19 lines · Rusty Russell

In message <[email protected]> yo u write:

Quoted 9 lines · Rusty Russell
Quoted 7 lines

I think it's clearer *why* it's being done than:

	struct ipc_rcu_kmalloc
	{
		struct rcu_head rcu;
	} __attribute__((aligned(__alignof__(void *))));
Quoted 4 lines

Well, not really Rusty. The above syntax uses documented gcc features already used inside the kernel, while the fact that void *data[0]; enforces alignment it is not ( to my knowledge ) documented anywhere. You can also avoid the comment using the aligned syntax ...

A comment is definitely required: you must say *why* you are aligning the structure (a clearer comment would be better, of course):

Quoted 8 lines
	/* We return a pointer after the structure to the unwitting
	   caller: ensure safe alignment. */

The alignment of a structure member of type X is the alignment of type
X: this seems obvious to me. And "type X data[0];" is the standard way of representing a variable struct.

Have we picked all the nits yet? 8)

Rusty, I give you two reason why you should be using the gcc documented "aligned" features :)

1) using "void *data[0];" and expecting an alignement, being undocumented, will force not obvious constraints for the gcc folks

2) I was looking at my latest copy of the ISO standard, section :
6.7.5.2 "Array declarators" - Point 1)
where it is explicitly declared that the constant expression that declared the size of the array must be greater then zero

Looking at how gcc folks is driving towards standards I will expect them screaming at you in a near future :)

- Davide