generic rwsem [Re: Alpha "process table hang"]

Andrea,

Having thought about the matter a bit, I know what the problem is:

As stated in the email with the latest patch, I haven't yet extended this to cover any architecture but i386. That patch was actually put up for comments, though it got included anyway.

Therefore, all other archs use the old (and probably) broken implementations!

I'll quickly knock up a patch to fix the other archs. This should also fix the alpha problem.

As for making the stuff I had done less generic, and more specific, I only made it more generic because I got asked to by a number of people. It was suggested that I move the contention functions into lib/rwsem.c and make them common.

As far as using atomic_add_return() goes, the C compiler cannot make the fastpath anywhere near as efficient, because amongst other things, I can make use of the condition flags set in EFLAGS and the compiler can't.

You do my code an injustice there... I've put comments in mine.

David

I am sure ppc couldn't race (at least unless up_read/up_write were excuted from irq/softnet context and that never happens in 2.4.4pre3, see below ;).

This is not the point. The point is that we want a generic implementation in C always available as a fallback and my one is IMHO better to what is in 2.4.4pre3, and secondly it has a superior API to replace the generic implementation with a per-arch implementation. So the only thing left to do is to plugin your x86 specific implementation in my patch using the simple API I provide to override the generic implementation completly and I preferred if you could do that at least for the 386/486 case (you know your code better). If you're not interested I probably end ingoring the <586 compiles by implementing only the atomic_*_return with xadd for >=586 and a CONFIG_RWSEM_ATOMIC_RETURN config option in the common code so I will optimize almost all archs in one go as I think that's the way to go and so I prefer to invest time in such direction only.

And the generic part was implemented bad and that's why I rewrote it to boot my alpha.

All we need to do is to avoid the spinlock until we enter the fast path. We only need the fast path to be a few asm inlined opcodes that jumps out of line if there's contention. I don't think other optimizations are interesting.

That can obviously be done for example with C code like this:

	count = atomic_inc_return(&sem->count);
	if (__builtin_expect(count == 0, 0))
		slow_path()

The above is the perfect C implementation IMHO, but it cannot be the most generic one because [34]86 doesn't have xadd and they cannot implement atomic_*_return and friends.

And incidentally the above is what (I guess Richard) did on the alpha and that should really go into common code instead of having asm-i386/rwsem-xadd.h asm-alpha/rwsem.h etcc.etc... just implement atomic_inc_return using xadd in asm-i386/atomic.h, that's much better design IMHO.

Put it this way: my one is readable enough that I don't need to add comments ;). More serously I may be biased in the readability point, but if I read lib/rwsem.c and include/linux/rwsem*.h before applying the patch and after applying the patch I have no dobut on what I want to run on my computer (I'm not talking about asm-i386/rwsem*.h of course). You are of course free to keep your one if you prefer it but I don't see technical arguments for that decision.

BTW, Andrew Morton is been so kind to audit my code and he noticed my patch was not allowing up_read/up_write to be called from irqs because I forgotten an _irq in the down_write (thanks Andrew!). I didn't catched during regression testing because nobody in the whole 2.4.4pre3 kernel is running either up_read or up_write from irq/softirq context, so it cannot destabilize the runtime but nevertheless that was a leftover also shared by the ppc port code in 2.4.3. So if you just started the alpha regression testing on the -1 revision go head and don't stop because you are reading this. just for Linus I released a -2 new version of the patch but again upgrade is not necessary for production (at least unless you use drivers outside the kernel tree that could release rwsem from irq/softirq context).

	ftp://ftp.us.kernel.org/pub/linux/kernel/people/andrea/patches/v2.4/2.4.4pre3/rwsem-generic-2

I didn't exported rwsem.c if CONFIG_RWSEM_GENERIC is set to n as suggested by Christoph yet because the old code couldn't be buggy and it's not obvious to me that the other way around is correct (Christoph are you sure we can export an object file that is not even compiled/generated? If answer is yes the export mechanism must be smart enough to discard that file if not present but I'm not sure if that's the case ;)

thanks for your comments.

Andrea

Hi Andrea,

Yes! All the objects in export-objs only get additional depencies in Rules.make - but if they do not get compiled at all that depencies won't matter either. All other makefile work this way, btw.

In my first mail I forgot that the makefile can be optimized even further, the hunk should look like this:
(NOTE: the patch is handwritten, no apply gurantee)

diff -urN 2.4.4pre3/lib/Makefile rwsem/lib/Makefile
@@ -10,10 +10,12 @@
 
 export-objs := cmdline.o rwsem.o
 
-obj-y := errno.o ctype.o string.o vsprintf.o brlock.o cmdline.o rwsem.o
+obj-y := errno.o ctype.o string.o vsprintf.o brlock.o cmdline.o
 
 ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
   obj-y += dec_and_lock.o
 endif
 
+obj-$(CONFIG_GENERIC_RWSEM)	+= rwsem.o
+
 include $(TOPDIR)/Rules.make

Christoph

Of course it doesn't work. We've performed a software upgrade.

ok thanks for the confirm.

Yes, I didn't used the -$() form only because I thought I had to make conditional the export-objs too.

I applied it.

Andrea