2008-02-12 16:27:26

by Pierre Peiffer

Subject: [PATCH 2.6.24-mm1 1/8] (resend) IPC/semaphores: code factorisation

From: Pierre Peiffer <[email protected]>

Trivial patch which adds some small locking functions and makes use of them
to factorize some part of the code and to make it cleaner.

Signed-off-by: Pierre Peiffer <[email protected]>
Acked-by: Serge Hallyn <[email protected]>
---

ipc/sem.c | 61 +++++++++++++++++++++++++++++++------------------------------
1 file changed, 31 insertions(+), 30 deletions(-)

Index: b/ipc/sem.c
===================================================================
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -180,6 +180,25 @@ static inline struct sem_array *sem_lock
return container_of(ipcp, struct sem_array, sem_perm);
}

+static inline void sem_lock_and_putref(struct sem_array *sma)
+{
+ ipc_lock_by_ptr(&sma->sem_perm);
+ ipc_rcu_putref(sma);
+}
+
+static inline void sem_getref_and_unlock(struct sem_array *sma)
+{
+ ipc_rcu_getref(sma);
+ ipc_unlock(&(sma)->sem_perm);
+}
+
+static inline void sem_putref(struct sem_array *sma)
+{
+ ipc_lock_by_ptr(&sma->sem_perm);
+ ipc_rcu_putref(sma);
+ ipc_unlock(&(sma)->sem_perm);
+}
+
static inline void sem_rmid(struct ipc_namespace *ns, struct sem_array *s)
{
ipc_rmid(&sem_ids(ns), &s->sem_perm);
@@ -698,19 +717,15 @@ static int semctl_main(struct ipc_namesp
int i;

if(nsems > SEMMSL_FAST) {
- ipc_rcu_getref(sma);
- sem_unlock(sma);
+ sem_getref_and_unlock(sma);

sem_io = ipc_alloc(sizeof(ushort)*nsems);
if(sem_io == NULL) {
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
- sem_unlock(sma);
+ sem_putref(sma);
return -ENOMEM;
}

- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
+ sem_lock_and_putref(sma);
if (sma->sem_perm.deleted) {
sem_unlock(sma);
err = -EIDRM;
@@ -731,38 +746,30 @@ static int semctl_main(struct ipc_namesp
int i;
struct sem_undo *un;

- ipc_rcu_getref(sma);
- sem_unlock(sma);
+ sem_getref_and_unlock(sma);

if(nsems > SEMMSL_FAST) {
sem_io = ipc_alloc(sizeof(ushort)*nsems);
if(sem_io == NULL) {
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
- sem_unlock(sma);
+ sem_putref(sma);
return -ENOMEM;
}
}

if (copy_from_user (sem_io, arg.array, nsems*sizeof(ushort))) {
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
- sem_unlock(sma);
+ sem_putref(sma);
err = -EFAULT;
goto out_free;
}

for (i = 0; i < nsems; i++) {
if (sem_io[i] > SEMVMX) {
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
- sem_unlock(sma);
+ sem_putref(sma);
err = -ERANGE;
goto out_free;
}
}
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
+ sem_lock_and_putref(sma);
if (sma->sem_perm.deleted) {
sem_unlock(sma);
err = -EIDRM;
@@ -1042,14 +1049,11 @@ static struct sem_undo *find_undo(struct
return ERR_PTR(PTR_ERR(sma));

nsems = sma->sem_nsems;
- ipc_rcu_getref(sma);
- sem_unlock(sma);
+ sem_getref_and_unlock(sma);

new = kzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, GFP_KERNEL);
if (!new) {
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
- sem_unlock(sma);
+ sem_putref(sma);
return ERR_PTR(-ENOMEM);
}
new->semadj = (short *) &new[1];
@@ -1060,13 +1064,10 @@ static struct sem_undo *find_undo(struct
if (un) {
spin_unlock(&ulp->lock);
kfree(new);
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
- sem_unlock(sma);
+ sem_putref(sma);
goto out;
}
- ipc_lock_by_ptr(&sma->sem_perm);
- ipc_rcu_putref(sma);
+ sem_lock_and_putref(sma);
if (sma->sem_perm.deleted) {
sem_unlock(sma);
spin_unlock(&ulp->lock);

--
Pierre Peiffer

2008-02-13 20:07:27

by Alexey Dobriyan

Subject: Re: [PATCH 2.6.24-mm1 1/8] (resend) IPC/semaphores: code factorisation

On Tue, Feb 12, 2008 at 05:13:41PM +0100, [email protected] wrote:
> Trivial patch which adds some small locking functions and makes use of them
> to factorize some part of the code and to make it cleaner.

What's wrong with consolidation activity in general is that one need to
follow tags many times to realise what on earth function really does.

> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -180,6 +180,25 @@ static inline struct sem_array *sem_lock
> return container_of(ipcp, struct sem_array, sem_perm);
> }
>
> +static inline void sem_lock_and_putref(struct sem_array *sma)
> +{
> + ipc_lock_by_ptr(&sma->sem_perm);
> + ipc_rcu_putref(sma);
> +}
> +
> +static inline void sem_getref_and_unlock(struct sem_array *sma)
> +{
> + ipc_rcu_getref(sma);
> + ipc_unlock(&(sma)->sem_perm);
> +}
> +
> +static inline void sem_putref(struct sem_array *sma)
> +{

It should be called sem_lock_putref_and_unlock() at which point it
becomes obvious that this patch is from useless-wrappers land.

> + ipc_lock_by_ptr(&sma->sem_perm);
> + ipc_rcu_putref(sma);
> + ipc_unlock(&(sma)->sem_perm);

On the other hand PUT is associated with atomic_dec_and_test() which
does no locking itself.

2008-02-13 21:54:25

by Pierre Peiffer

Subject: Re: [PATCH 2.6.24-mm1 1/8] (resend) IPC/semaphores: code factorisation

On Feb 13, 2008 9:07 PM, Alexey Dobriyan <[email protected]> wrote:
> On Tue, Feb 12, 2008 at 05:13:41PM +0100, [email protected] wrote:
> > Trivial patch which adds some small locking functions and makes use of them
> > to factorize some part of the code and to make it cleaner.
>
> What's wrong with consolidation activity in general is that one need to
> follow tags many times to realise what on earth function really does.

Funny...
What's right with consolidation in general is that it avoids the
readers to read again and again the same piece of code and helps them
to focus on what the code really does.

--
Pierre