On Wed, 2011-01-05 at 11:43 -0800, Linus Torvalds wrote: > On Wed, Jan 5, 2011 at 11:14 AM, Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> wrote: > > > > There appear to be only two callsites of said horror, one in the exit > > path and one in ata-eh, neither appear to be performance critical so I > > replaced them with a simple lock-unlock sequence. > > Again, WHY? > > What's the problem with the current code? Instead of generating ugly > patches to change it, and instead of removing it, just say what the > PROBLEM is. Well, I don't care about the primitive anymore, and Nick had some reasonable arguments on why its not a good primitive to have. So in a brief moment I decided to see what it would take to make it go away. Apparently you don't like it, I'm fine with that, consider the patch discarded. > Some simple helper functions to extract the tail/head part of the > ticket lock to make the comparisons understandable, Jeremy has a number of pending patches making things more pretty. If you wish I can revisit this once that work hits your tree. http://lkml.org/lkml/2010/11/16/479 He makes the thing looks like: +#if (CONFIG_NR_CPUS < 256) +typedef u8 __ticket_t; +#else +typedef u16 __ticket_t; +#endif + +#define TICKET_SHIFT (sizeof(__ticket_t) * 8) +#define TICKET_MASK ((__ticket_t)((1 << TICKET_SHIFT) - 1)) + typedef struct arch_spinlock { + union { + unsigned int slock; + struct __raw_tickets { + __ticket_t head, tail; + } tickets; + }; } arch_spinlock_t; > together with > always accessing the lock with the proper ACCESS_ONCE() would have > made your previous patch acceptable. I'm still not quite seeing where I was missing an ACCESS_ONCE(), the second loop had a cpu_relax() in, which is a compiler barrier so it forces a reload that way. > But you ignored that feedback, > and instead you now want to do a "let's just remove it entirely patch" > that is even worse. My locking improved and became a lot more obvious by not using the primitive, so for the work I was doing not using it seemed the better solution. And as said, this was inspired by Nick's comments and it was a quick edit to see what it would take. -- 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
