Re: [PATCH] 2.4.18-pre9, trylock for read/write semaphores

From: David Howells (dhowells@redhat.com)
Date: Wed Feb 13 2002 - 03:19:43 EST


Hi Brian,

Having examined your code again, I've got some comments on it:

I think the following would be more elegant:

        static inline int __down_read_trylock(struct rw_semaphore *sem)
        {
                signed long old, new;

                do {
                        old = (volatile signed long)sem->count;
                        if (old < RWSEM_UNLOCKED_VALUE)
                                return 0;
                        new = old + RWSEM_ACTIVE_READ_BIAS;
                } while (cmpxchg(&sem->count, old, new) != old);

                return 1;
        }

I'm also not sure that the cast has any effect in the following excerpt from
the above:

        old = (volatile signed long)sem->count;

What you may actually want is:

        static inline int __down_read_trylock(struct rw_semaphore *sem)
        {
                volatile signed long *pcount;
                signed long old, new;

                pcount = &sem->count;
                do {
                        old = *pcount;
                        if (old < RWSEM_UNLOCKED_VALUE)
                                return 0;
                        new = old + RWSEM_ACTIVE_READ_BIAS;
                } while (cmpxchg(pcount, old, new) != old);

                return 1;
        }

Or maybe even:

        static inline int __down_read_trylock(struct rw_semaphore *sem)
        {
                __s32 result, tmp;
                __asm__ __volatile__(
                        "# beginning __down_read_trylock\n\t"
                        " movl %0,%1\n\t"
                        "1:\n\t"
                        " movl %1,%2\n\t"
                        " addl %3,%2\n\t"
                        " jle 2f\n\t"
        LOCK_PREFIX " cmpxchgl %2,%0\n\t"
                        " jnz 1b\n\t"
                        "# ending __down_read_trylock\n\t"
                        : "+m"(sem->count), "=&a"(result), "+&r"(tmp)
                        : "i"(RWSEM_ACTIVE_READ_BIAS)
                        : "memory", "cc");
                return result>=0 ? 1 : 0;
        }

Using this inline assembly has three advantages over mixing lots of C into it:

 (1) If the sign/zero flags are set as the result of calculating the new
     value, then the old value was negative, and so the trylock has failed.

 (2) CMPXCHG sets the zero flag on success and clears it on failure (something
     that C can't test with the current inline asm setup).

 (3) CMPXCHG fetches the current value of sem->count into EAX if it fails to
     swap (so we don't need to get it again - hence why "1:" is where it is).

Point (3) could be incorporated into the C version. It may also be desirable
to include a rep;nop in whichever version is used, but I don't think it's
going to spin enough for that.

David
-
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/



This archive was generated by hypermail 2b29 : Fri Feb 15 2002 - 21:00:52 EST