Put size in array to get rid of barriers in grow_ary()
RFC PATCH 1/1 Put size in array to get rid of barriers in grow_ary()
Hello!
The grow_ary() code has a number of explicit memory barriers, as does ipc_lock(). This patch gets rid of the need for some of these by placing the array size in the same block of memory containing the array itself, so that the array and the size cannot possibly get out of sync. Also uses rcu_assign_pointer() to get rid of the remaining smp_wmb().
Untested, therefore probably broken.
Thoughts?
Thanx, Paul
msg.c | 3 +- sem.c | 2 - util.c | 82 +++++++++++++++++++++++++++++------------------------------------ util.h | 8 +++--- 4 files changed, 44 insertions(+), 51 deletions(-)
ipc/msg.c +2 -1
@@ -380,7 +380,8 @@ asmlinkage long sys_msgctl (int msqid, iint success_return;if (!buf)return -EFAULT ; -if (cmd ==MSG_STAT && msqid >= msg_ids.size) +if (cmd ==MSG_STAT && + (msg_ids.entries ==NULL || msqid >= msg_ids.entries->size))return -EINVAL ;memset (&tbuf,0 ,sizeof (tbuf));
ipc/sem.c +1 -1
@@ -523,7 +523,7 @@ static int semctl_nolock(int semid, intstruct semid64_ds tbuf;int id; -if (semid >= sem_ids.size) +if (sem_ids.entries ==NULL || semid >= sem_ids.entries->size)return -EINVAL ;memset (&tbuf,0 ,sizeof (tbuf));
ipc/util.c +37 -45
171 lines not on this page · raw message
ipc/util.h +4 -4
@@ -16,17 +16,17 @@ void msg_init (void);void shm_init (void );struct ipc_ids { -int size;int in_use;int max_id;unsigned short seq;unsigned short seq_max;struct semaphore sem; -struct ipc_id * entries; +struct ipc_id_ary * entries; }; -struct ipc_id { -struct kern_ipc_perm * p; +struct ipc_id_ary { +int size; +struct kern_ipc_perm *p[0 ]; };void __init ipc_init_ids (struct ipc_ids * ids,int size);
Quoted 7 lines · Paul E. McKenney
The
grow_ary()code has a number of explicit memory barriers, as doesipc_lock(). This patch gets rid of the need for some of these by placing the array size in the same block of memory containing the array itself, so that the array and the size cannot possibly get out of sync. Also usesrcu_assign_pointer()to get rid of the remainingsmp_wmb().
But Paul, if you keep removing all these examples of memory barriers, how can I be expected to learn how to use them properly?
Seriously, good, yes, the fewer "mb"s the better. I could always educate myself from the older source.
Quoted 1 line
Untested, therefore probably broken.
Agreed ;)
Quoted 1 line
Thoughts?
Wouldn't it be a little nicer to start ipc_ids off pointing to a const ipc_id_ary of size 0, to avoid the various entries == NULL tests you had to add?
Hugh
Quoted 11 lines · Hugh Dickins
Quoted 7 lines · Paul E. McKenney
The
grow_ary()code has a number of explicit memory barriers, as doesipc_lock(). This patch gets rid of the need for some of these by placing the array size in the same block of memory containing the array itself, so that the array and the size cannot possibly get out of sync. Also usesrcu_assign_pointer()to get rid of the remainingsmp_wmb().But Paul, if you keep removing all these examples of memory barriers, how can I be expected to learn how to use them properly?
But Hugh, I left quite a few smp_wmb()s in there just for you! ;-)
Quoted 2 lines
Seriously, good, yes, the fewer "mb"s the better. I could always educate myself from the older source.
Agreed!
Quoted 3 lines
Quoted 1 line
Untested, therefore probably broken.
Agreed ;)
Any specifics greatly appreciated, as always...
Quoted 5 lines
Quoted 1 line
Thoughts?
Wouldn't it be a little nicer to start
ipc_idsoff pointing to a constipc_id_aryof size 0, to avoid the various entries == NULL tests you had to add?
I like this one!!! Will put a patch together. Manfred's recent patch applied a refcount, which negates the const part, but should be no problem to put a size-zero structure in the struct ipc_ids. (Having a separately allocated structure puts me back to checking NULL pointers due to possibility of allocation failure.)
Thanx, Paul
Quoted 10 lines · Paul E. McKenney
Quoted 3 lines · Hugh Dickins
Wouldn't it be a little nicer to start
ipc_idsoff pointing to a constipc_id_aryof size 0, to avoid the various entries == NULL tests you had to add?I like this one!!! Will put a patch together. Manfred's recent patch applied a refcount, which negates the const part, but should be no problem to put a size-zero structure in the struct
ipc_ids. (Having a separately allocated structure puts me back to checking NULL pointers due to possibility of allocation failure.)
And here, finally, is the updated patch. Still untested.
Thoughts?
Thanx, Paul
ipc/msg.c +1 -1
@@ -380,7 +380,7 @@ asmlinkage long sys_msgctl (int msqid, iint success_return;if (!buf)return -EFAULT ; -if (cmd ==MSG_STAT && msqid >= msg_ids.size) +if (cmd ==MSG_STAT && msqid >= msg_ids.entries->size)return -EINVAL ;memset (&tbuf,0 ,sizeof (tbuf));
ipc/sem.c +1 -1
@@ -523,7 +523,7 @@ static int semctl_nolock(int semid, intstruct semid64_ds tbuf;int id; -if (semid >= sem_ids.size) +if (semid >= sem_ids.entries->size)return -EINVAL ;memset (&tbuf,0 ,sizeof (tbuf));
ipc/util.c +36 -44
170 lines not on this page · raw message
ipc/util.h +7 -6
@@ -15,18 +15,19 @@ void sem_init (void);void msg_init (void );void shm_init (void ); -struct ipc_ids { +struct ipc_id_ary {int size; +struct kern_ipc_perm *p[0 ]; +}; + +struct ipc_ids {int in_use;int max_id;unsigned short seq;unsigned short seq_max;struct semaphore sem; -struct ipc_id * entries; -}; - -struct ipc_id { -struct kern_ipc_perm * p; +struct ipc_id_ary nullentry; +struct ipc_id_ary * entries; };void __init ipc_init_ids (struct ipc_ids * ids,int size);
Quoted 6 lines · Paul E. McKenney
Quoted 5 lines · Paul E. McKenney
Quoted 3 lines · Hugh Dickins
Wouldn't it be a little nicer to start
ipc_idsoff pointing to a constipc_id_aryof size 0, to avoid the various entries == NULL tests you had to add?
Quoted 7 lines
Quoted 5 lines
I like this one!!! Will put a patch together. Manfred's recent patch applied a refcount, which negates the const part, but should be no problem to put a size-zero structure in the struct
ipc_ids. (Having a separately allocated structure puts me back to checking NULL pointers due to possibility of allocation failure.)And here, finally, is the updated patch. Still untested.
That looks good, Paul, thank you. (But I too have not tested.)
Hugh
Quoted 6 lines · Paul E. McKenney
And here, finally, is the updated patch. Still untested.
Thoughts?
Looks good.
I've even tried to test it, but doesn't compile with -rc1-bk11 due to missing rcu_assign_pointer.
--
Manfred
Quoted 11 lines · Manfred Spraul
Quoted 6 lines · Paul E. McKenney
And here, finally, is the updated patch. Still untested.
Thoughts?
Looks good.
I've even tried to test it, but doesn't compile with -rc1-bk11 due to missingrcu_assign_pointer.
My bad! It needs the rcu_assign_pointer() patch as pre-req.
http://marc.theaimsgroup.com/?l=linux-kernel&m=109459678719365&w=2
Below is the part of that patch that is actually supplies rcu_assign_pointer(), which should be all that is needed.
Thanx, Paul
include/linux/rcupdate.h +18 -0
@@ -238,6 +238,24 @@ static inline int rcu_pending(int cpu) (_________p1); \ }) +/** +* rcu_assign_pointer - assign (publicize) a pointer to a newly +* initialized structure that will be dereferenced by RCU read-side +* critical sections. Returns the value assigned. +* +* Inserts memory barriers on architectures that require them +* (pretty much all of them other than x86), and also prevents +* the compiler from reordering the code that initializes the +* structure after the pointer assignment. More importantly, this +* call documents which pointers will be dereferenced by RCU read-side +* code. +*/ + +#define rcu_assign_pointer (p, v) ({ \ + smp_wmb(); \ + (p) = (v); \ + }) +extern void rcu_init (void );extern void rcu_check_callbacks (int cpu,int user);extern void rcu_restart_cpu (int cpu);