Re: [RFC][PATCH 2/4] interruptible rwsem operations (i386, core)

[Nick Piggin <nickpiggin@xxxxxxxxxxxx>]

David Howells wrote:
> Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxx> wrote:
>
>
> > Add functions down_read_interruptible, and down_write_interruptible to rw
> > semaphores. Implement these for i386.
> > ...
>
>
> > +static inline int
> > +rwsem_down_interruptible_failed_common(struct rw_semaphore *sem,
> > +			struct rwsem_waiter *waiter, signed long adjustment)
> > +{
> > ...
>
>
> I wonder if you should check to see if there are any readers that can be woken
> up if a sleeping writer is interrupted, but I can't think of a simple way to
> do it.

I think it will, won't it?

> > -struct rw_semaphore fastcall __sched *
> > -rwsem_down_read_failed(struct rw_semaphore *sem)
> > +void fastcall __sched rwsem_down_read_failed(struct rw_semaphore *sem)
>
>
> Please don't.
>
>
> > @@ -199,14 +253,33 @@ rwsem_down_read_failed(struct rw_semapho
> > 				RWSEM_WAITING_BIAS - RWSEM_ACTIVE_BIAS);
> >
> > 	rwsemtrace(sem, "Leaving rwsem_down_read_failed");
> > -	return sem;
>
>
> Ditto.
>
>
> > -struct rw_semaphore fastcall __sched *
> > -rwsem_down_write_failed(struct rw_semaphore *sem)
> > +void fastcall __sched rwsem_down_write_failed(struct rw_semaphore *sem)
>
>
> Ditto.
>
>
> > @@ -216,10 +289,31 @@ rwsem_down_write_failed(struct rw_semaph
> > 	rwsem_down_failed_common(sem, &waiter, -RWSEM_ACTIVE_BIAS);
> >
> > 	rwsemtrace(sem, "Leaving rwsem_down_write_failed");
> > -	return sem;
>
>
> Ditto.
>
>
> > @@ -99,11 +103,12 @@ static inline void __down_read(struct rw
> > {
> > 	__asm__ __volatile__(
> > 		"# beginning down_read\n\t"
> > -LOCK_PREFIX	"  incl      (%%eax)\n\t" /* adds 0x00000001, returns the old value */
> > +LOCK_PREFIX	"  incl      %0\n\t" /* adds 0x00000001, returns the old value */
>
>
> Ditto.
>
>
> > 		"  js        2f\n\t" /* jump if we weren't granted the lock */
> > 		"1:\n\t"
> > 		LOCK_SECTION_START("")
> > 		"2:\n\t"
> > +		"  movl      %2,%%eax\n\t"
>
>
> Splat.
>
>
> > 		"  pushl     %%ecx\n\t"
> > 		"  pushl     %%edx\n\t"
> > 		"  call      rwsem_down_read_failed\n\t"
>
>
> Splat.
>
>
> > @@ -113,11 +118,41 @@ LOCK_PREFIX	"  incl      (%%eax)\n\t" /*
> > 		LOCK_SECTION_END
> > 		"# ending down_read\n\t"
> > 		: "=m"(sem->count)
> > -		: "a"(sem), "m"(sem->count)
> > +		: "m"(sem->count), "m"(sem)
> > 		: "memory", "cc");
> > }
>
>
> You appear to be corrupting EAX.
>
>
> > +static inline int __down_read_interruptible(struct rw_semaphore *sem)
>
>
> Will corrupt EAX.

I'll fix these up. You're right by the looks.

> > 		"# beginning down_write\n\t"
> > -LOCK_PREFIX	"  xadd      %%edx,(%%eax)\n\t" /* subtract 0x0000ffff, returns the old value */
> > +LOCK_PREFIX	"  xadd      %%edx,%0\n\t" /* subtract 0x0000ffff, returns the old value */
>
>
> Again, please don't. It's a lot more readable when it mentions EAX directly,
> plus it's also independent of constraint reordering.

OK I suppose so...

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/