OK, thanks.
So I took a look. Wish I hadn't :( The locking rules in there
are outrageously uncommented. You must be brave people.
What about this code?
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);
}
Are we sure that it's never called under locks?
And it seems that if the kmalloc fails, we decide to leak some
memory, yes?
If so it would be better to use GFP_ATOMIC there. Avoids any
locking problems and also increases the chance of the allocation
succeeding. (With an explanatory comment, naturally :)).
Even better: is it possible to embed the rcu_ipc_free inside the
object-to-be-freed? Perhaps not?
Stylistically, it is best to not typecast the return value
from kmalloc, btw. You should never typecast the return
value of anything which returns a void *, because it weakens
your compile-time checking. Example:
foo *bar = (foo *)zot();
The compiler will swallow that, regardless of what zot() returns.
Someone could go and change zot() to return a reiserfs_inode *
and you would never know about it. Whereas:
foo *bar = zot();
Says to the compiler "zot() must return a bar * or a void *",
which is much tighter checking, yes?
There is an insane amount of inlining in the ipc code. I
couldn't keep my paws off it.
Before:
mnm:/usr/src/25> size ipc/*.o
text data bss dec hex filename
28346 224 192 28762 705a ipc/built-in.o
7390 20 64 7474 1d32 ipc/msg.o
11236 16 64 11316 2c34 ipc/sem.o
8136 160 64 8360 20a8 ipc/shm.o
1584 0 0 1584 630 ipc/util.o
After:
mnm:/usr/src/25> size ipc/*.o
text data bss dec hex filename
19274 224 192 19690 4cea ipc/built-in.o
4846 20 64 4930 1342 ipc/msg.o
7636 16 64 7716 1e24 ipc/sem.o
4808 160 64 5032 13a8 ipc/shm.o
1984 0 0 1984 7c0 ipc/util.o
--- 25/ipc/util.h~ipc-akpm Thu Oct 24 16:03:32 2002
+++ 25-akpm/ipc/util.h Thu Oct 24 16:08:25 2002
@@ -54,63 +54,11 @@ void* ipc_alloc(int size);
void ipc_free(void* ptr, int size);
void ipc_rcu_free(void* arg, int size);
-extern inline struct kern_ipc_perm* ipc_get(struct ipc_ids* ids, int id)
-{
- struct kern_ipc_perm* out;
- int lid = id % SEQ_MULTIPLIER;
- if(lid >= ids->size)
- return NULL;
- rmb();
- out = ids->entries[lid].p;
- return out;
-}
-
-extern inline struct kern_ipc_perm* ipc_lock(struct ipc_ids* ids, int id)
-{
- struct kern_ipc_perm* out;
- int lid = id % SEQ_MULTIPLIER;
-
- rcu_read_lock();
- if(lid >= ids->size) {
- rcu_read_unlock();
- return NULL;
- }
- rmb();
- out = ids->entries[lid].p;
- if(out == NULL) {
- rcu_read_unlock();
- return NULL;
- }
- spin_lock(&out->lock);
-
- /* ipc_rmid() may have already freed the ID while ipc_lock
- * was spinning: here verify that the structure is still valid
- */
- if (out->deleted) {
- spin_unlock(&out->lock);
- rcu_read_unlock();
- return NULL;
- }
- return out;
-}
-
-extern inline void ipc_unlock(struct kern_ipc_perm* perm)
-{
- spin_unlock(&perm->lock);
- rcu_read_unlock();
-}
-
-extern inline int ipc_buildid(struct ipc_ids* ids, int id, int seq)
-{
- return SEQ_MULTIPLIER*seq + id;
-}
-
-extern inline int ipc_checkid(struct ipc_ids* ids, struct kern_ipc_perm* ipcp, int uid)
-{
- if(uid/SEQ_MULTIPLIER != ipcp->seq)
- return 1;
- return 0;
-}
+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_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);
void kernel_to_ipc64_perm(struct kern_ipc_perm *in, struct ipc64_perm *out);
void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out);
--- 25/ipc/util.c~ipc-akpm Thu Oct 24 16:07:07 2002
+++ 25-akpm/ipc/util.c Thu Oct 24 16:07:51 2002
@@ -359,6 +359,61 @@ void ipc64_perm_to_ipc_perm (struct ipc6
out->seq = in->seq;
}
+struct kern_ipc_perm* ipc_get(struct ipc_ids* ids, int id)
+{
+ struct kern_ipc_perm* out;
+ int lid = id % SEQ_MULTIPLIER;
+ if(lid >= ids->size)
+ return NULL;
+ rmb();
+ out = ids->entries[lid].p;
+ return out;
+}
+
+struct kern_ipc_perm* ipc_lock(struct ipc_ids* ids, int id)
+{
+ struct kern_ipc_perm* out;
+ int lid = id % SEQ_MULTIPLIER;
+
+ rcu_read_lock();
+ if(lid >= ids->size)
+ goto fail;
+ rmb();
+ out = ids->entries[lid].p;
+ if (out == NULL)
+ goto fail;
+ spin_lock(&out->lock);
+
+ /* ipc_rmid() may have already freed the ID while ipc_lock
+ * was spinning: here verify that the structure is still valid
+ */
+ if (!out->deleted)
+ return out;
+
+ spin_unlock(&out->lock);
+fail:
+ rcu_read_unlock();
+ return NULL;
+}
+
+void ipc_unlock(struct kern_ipc_perm* perm)
+{
+ spin_unlock(&perm->lock);
+ rcu_read_unlock();
+}
+
+int ipc_buildid(struct ipc_ids* ids, int id, int seq)
+{
+ return SEQ_MULTIPLIER*seq + id;
+}
+
+int ipc_checkid(struct ipc_ids* ids, struct kern_ipc_perm* ipcp, int uid)
+{
+ if(uid/SEQ_MULTIPLIER != ipcp->seq)
+ return 1;
+ return 0;
+}
+
#ifndef __ia64__
/**
.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/