On Friday 28 March 2008 01:15, Matthew Wilcox wrote: > On Wed, Mar 26, 2008 at 01:29:58PM -0700, Luck, Tony wrote: > > This looks a lot cleaner than my ia64 specific code that > > used cmpxchg() for the down() operation and fetchadd for > > the up() ... using a brand new semaphore_spin data type. > > I did brifly consider creating a spinaphore data type, but it's > significantly less code to create down_spin(). > > > It appears to work ... I tried to do some timing comparisons > > of this generic version against my arch specific one, but the > > hackbench test case has a run to run variation of a factor of > > three (from 1min9sec to 3min44sec) so it is hopeless to try > > and see some small percentage difference. > > Thanks for testing and putting this together in patch form. I've fixed it > up to address Jens' astute comment and added it to my semaphore patchset. > > http://git.kernel.org/?p=linux/kernel/git/willy/misc.git;a=shortlog;h=semap >hore-20080327 > > Stephen, I've updated the 'semaphore' tag to point ot the same place as > semaphore-20080327, so please change your linux-next tree from pulling > semaphore-20080314 to just pulling plain 'semaphore'. I'll use this > method of tagging from now on. > > Here's the edited patch. > > commit 517df6fedc88af3f871cf827a62ef1a1a2073645 > Author: Matthew Wilcox <matthew@xxxxxx> > Date: Thu Mar 27 09:49:26 2008 -0400 > > Add down_spin() > > ia64 would like to use a semaphore in flush_tlb_all() as it can have > multiple tokens. Unfortunately, it's currently nested inside a > spinlock, so they can't sleep. down_spin() is the cheapest solution to > implement. Uhm, how do you use this exactly? All other holders of this semaphore must have preempt disabled and not sleep, right? (and so you need a new down() that disables preempt too) So the only difference between this and a spinlock I guess is that waiters can sleep rather than spin on contention (except this down_spin guy, which sleeps). Oh, I see from the context of Tony's message... so this can *only* be used when preempt is off, and *only* against other down_spin lockers. Bad idea to be hack this into the semaphore code, IMO. It would take how many lines to implement it properly? struct { atomic_t cur; int max; } ss_t; void spin_init(ss_t *ss, int max) { &ss->cur = ATOMIC_INIT(0); &ss->max = max; } void spin_take(ss_t *ss) { preempt_disable(); while (unlikely(!atomic_add_unless(&ss->cur, 1, &ss->max))) { while (atomic_read(&ss->cur) == ss->max) cpu_relax(); } } void spin_put(ss_t *ss) { smp_mb(); atomic_dec(&ss->cur); preempt_enable(); } About the same number as down_spin(). And it is much harder to misuse. So LOC isn't such a great argument for this kind of thing. My 2c -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html