[patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Ingo Molnar]

add two new atomic ops to i386: atomic_dec_call_if_negative() and
atomic_inc_call_if_nonpositive(), which are conditional-call-if
atomic operations. Needed by the new mutex code.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

----

 include/asm-i386/atomic.h |   55 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+)

Index: linux/include/asm-i386/atomic.h
===================================================================
--- linux.orig/include/asm-i386/atomic.h
+++ linux/include/asm-i386/atomic.h
@@ -240,6 +240,61 @@ static __inline__ int atomic_sub_return(
 #define atomic_inc_return(v)  (atomic_add_return(1,v))
 #define atomic_dec_return(v)  (atomic_sub_return(1,v))
 
+/**
+ * atomic_dec_call_if_negative - decrement and call function if negative
+ * @v: pointer of type atomic_t
+ * @fn: function to call if the result is negative
+ *
+ * Atomically decrements @v and calls a function if the result is negative.
+ */
+#define atomic_dec_call_if_negative(v, fn_name)				\
+do {									\
+	fastcall void (*__tmp)(atomic_t *) = fn_name;			\
+									\
+	(void)__tmp;							\
+	typecheck(atomic_t *, v);					\
+									\
+	__asm__ __volatile__(						\
+		LOCK "decl (%%eax)\n"  					\
+		"js 2f\n"						\
+		"1:\n"							\
+		LOCK_SECTION_START("")					\
+		"2: call "#fn_name"\n\t"				\
+		"jmp 1b\n"						\
+		LOCK_SECTION_END					\
+		:							\
+		:"a" (v)						\
+		:"memory","cx","dx");					\
+} while (0)
+
+/**
+ * atomic_inc_call_if_nonpositive - increment and call function if nonpositive
+ * @v: pointer of type atomic_t
+ * @fn: function to call if the result is nonpositive
+ *
+ * Atomically increments @v and calls a function if the result is nonpositive.
+ */
+#define atomic_inc_call_if_nonpositive(v, fn_name)			\
+do {									\
+	fastcall void (*__tmp)(atomic_t *) = fn_name;			\
+									\
+	(void)__tmp;							\
+	typecheck(atomic_t *, v);					\
+									\
+	__asm__ __volatile__(						\
+		LOCK "incl (%%eax)\n"  					\
+		"jle 2f\n"						\
+		"1:\n"							\
+		LOCK_SECTION_START("")					\
+		"2: call "#fn_name"\n\t"				\
+		"jmp 1b\n"						\
+		LOCK_SECTION_END					\
+		:							\
+		:"a" (v)						\
+		:"memory","cx","dx");					\
+} while (0)
+
+
 /* These are x86-specific, used by some header files */
 #define atomic_clear_mask(mask, addr) \
 __asm__ __volatile__(LOCK "andl %0,%1" \

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Linus Torvalds]

On Wed, 21 Dec 2005, Ingo Molnar wrote:
>
> add two new atomic ops to i386: atomic_dec_call_if_negative() and
> atomic_inc_call_if_nonpositive(), which are conditional-call-if
> atomic operations. Needed by the new mutex code.

Umm. This asm is broken. It doesn't mark %eax as changed, so this is only 
reliable if the function you call is

 - a "fastcall" one
 - always returns as its return value the pointer to the atomic count

which is not true (you verify that it's a fastcall, but it's of type 
"void").

Now, it's entirely possible that it's only used in situations where the 
compiler doesn't rely on the value of %eax after the asm anyway, but 
that's just praying. Either mark the value as being changed, or just 
save/restore the registers. Ie either something like

	__asm__ __volatile__(
		LOCK "decl (%0)\n\t"
		"js 2f\n"
		"1:\n"
		LOCK_SECTION_START("")
		"2: call "#fn_name"\n\t"
		"jmp 1b\n"
		LOCK_SECTION_END
		:"=a" (dummy)
		:"0" (v)
		:"memory","cx","dx");

or just do

	__asm__ __volatile__(
		LOCK "decl (%0)\n\t"
		"js 2f\n"
		"1:\n"
		LOCK_SECTION_START("")
		"pushl %%ebx\n\t"
		"pushl %%ecx\n\t"
		"pushl %%eax\n\t"
		"call "#fn_name"\n\t"
		"popl %%eax\n\t"
		"popl %%ecx\n\t"
		"popl %%ebx\n\t"
		"jmp 1b\n"
		LOCK_SECTION_END
		:/* no outputs */
		:"a" (v)
		:"memory");

or some in-between thing (that saves only part of the registers).

The above has not been tested in any way, shape or form. But you get the idea.

		Linus

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Linus Torvalds]

On Wed, 21 Dec 2005, Linus Torvalds wrote:

> Umm. This asm is broken. It doesn't mark %eax as changed, so this is only 
> reliable if the function you call is
> 
>  - a "fastcall" one
>  - always returns as its return value the pointer to the atomic count
> 
> which is not true (you verify that it's a fastcall, but it's of type 
> "void").

Actually (and re-reading the email I sent that wasn't obvious at all), my 
_preferred_ fix is to literally force the use of the above kind of 
function: not save/restore %eax at all, but just say that any function 
that is called by the magic "atomic_*_call_if()" needs to always return 
the argument it gets as its return value too.

That allows the caller to not even have to care. And the callee obviously 
already _has_ that value, so it might as well return it (and in the best 
case it's not going to add any cost at all, either to the caller or the 
callee).

So you might opt to keep the asm the same, just change the calling 
conventions.

		Linus

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Ingo Molnar]

* Linus Torvalds <torvalds@osdl.org> wrote:

> > Umm. This asm is broken. It doesn't mark %eax as changed, so this is only 
> > reliable if the function you call is
> > 
> >  - a "fastcall" one
> >  - always returns as its return value the pointer to the atomic count
> > 
> > which is not true (you verify that it's a fastcall, but it's of type 
> > "void").
> 
> Actually (and re-reading the email I sent that wasn't obvious at all), 
> my _preferred_ fix is to literally force the use of the above kind of 
> function: not save/restore %eax at all, but just say that any function 
> that is called by the magic "atomic_*_call_if()" needs to always 
> return the argument it gets as its return value too.
> 
> That allows the caller to not even have to care. And the callee 
> obviously already _has_ that value, so it might as well return it (and 
> in the best case it's not going to add any cost at all, either to the 
> caller or the callee).
> 
> So you might opt to keep the asm the same, just change the calling 
> conventions.

ok, i've added this fix, thanks. Right now we dont do anything after 
those functions (that's probably how the bug never showed up), but at 
least one interim stage i tried to use the call_if functions at other 
places too, so the potential is there.

	Ingo

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Daniel Jacobowitz]

On Wed, Dec 21, 2005 at 10:57:40AM -0800, Linus Torvalds wrote:
> Actually (and re-reading the email I sent that wasn't obvious at all), my 
> _preferred_ fix is to literally force the use of the above kind of 
> function: not save/restore %eax at all, but just say that any function 
> that is called by the magic "atomic_*_call_if()" needs to always return 
> the argument it gets as its return value too.
> 
> That allows the caller to not even have to care. And the callee obviously 
> already _has_ that value, so it might as well return it (and in the best 
> case it's not going to add any cost at all, either to the caller or the 
> callee).
> 
> So you might opt to keep the asm the same, just change the calling 
> conventions.

This new macro is only going to be used in x86-specific files, right? 
There's no practical way to implement this on lots of other
architectures.

Embedding a call in asm("") can break other things too - for instance,
unwind tables could become inaccurate.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Nicolas Pitre]

On Wed, 21 Dec 2005, Daniel Jacobowitz wrote:

> This new macro is only going to be used in x86-specific files, right? 
> There's no practical way to implement this on lots of other
> architectures.

The default implementation does the call in C.

> Embedding a call in asm("") can break other things too - for instance,
> unwind tables could become inaccurate.

I doubt unwind tables are used at all for the kernel, are they?


Nicolas

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Keith Owens]

On Wed, 21 Dec 2005 15:54:13 -0500 (EST), 
Nicolas Pitre <nico@cam.org> wrote:
>On Wed, 21 Dec 2005, Daniel Jacobowitz wrote:
>
>> This new macro is only going to be used in x86-specific files, right? 
>> There's no practical way to implement this on lots of other
>> architectures.
>
>The default implementation does the call in C.
>
>> Embedding a call in asm("") can break other things too - for instance,
>> unwind tables could become inaccurate.
>
>I doubt unwind tables are used at all for the kernel, are they?

Yes they are.  ia64 absolutely requires accurate unwind tables, it is
part of the ABI.  x86_64 is tending towards requiring accurate CFI
data.

Without valid unwind tables, backtraces are flakey at best.  The lack
of decent kernel unwind for i386 is one of the reasons that kdb
backtrace for i386 is so horrible.

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Ingo Molnar]

* Linus Torvalds <torvalds@osdl.org> wrote:

> 
> On Wed, 21 Dec 2005, Ingo Molnar wrote:
> >
> > add two new atomic ops to i386: atomic_dec_call_if_negative() and
> > atomic_inc_call_if_nonpositive(), which are conditional-call-if
> > atomic operations. Needed by the new mutex code.
> 
> Umm. This asm is broken. It doesn't mark %eax as changed, [...]

hm, i thought gcc treats all explicitly used register in the asm as 
clobbered - and i'm using %%eax explicitly for that reason. Or is that 
only the case if that's an input/output register as well?

	Ingo

Re: [patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Linus Torvalds]

On Wed, 21 Dec 2005, Ingo Molnar wrote:
> 
> hm, i thought gcc treats all explicitly used register in the asm as 
> clobbered - and i'm using %%eax explicitly for that reason.

Nope, they are totally invisible to gcc afaik. It just sees "%%" and 
changes it into "%", and ignores the rest.

		Linus

[patch 3/8] mutex subsystem, add atomic_*_call_if_*() to i386

[Ingo Molnar]

add two new atomic ops to i386: atomic_dec_call_if_negative() and
atomic_inc_call_if_nonpositive(), which are conditional-call-if
atomic operations. Needed by the new mutex code.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

----

 include/asm-i386/atomic.h |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+)

Index: linux/include/asm-i386/atomic.h
===================================================================
--- linux.orig/include/asm-i386/atomic.h
+++ linux/include/asm-i386/atomic.h
@@ -240,6 +240,63 @@ static __inline__ int atomic_sub_return(
 #define atomic_inc_return(v)  (atomic_add_return(1,v))
 #define atomic_dec_return(v)  (atomic_sub_return(1,v))
 
+/**
+ * atomic_dec_call_if_negative - decrement and call function if negative
+ * @v: pointer of type atomic_t
+ * @fn: function to call if the result is negative
+ *
+ * Atomically decrements @v and calls a function if the result is negative.
+ */
+#define atomic_dec_call_if_negative(v, fn_name)				\
+do {									\
+	fastcall void (*__tmp)(atomic_t *) = fn_name;			\
+	unsigned int dummy;						\
+									\
+	(void)__tmp;							\
+	typecheck(atomic_t *, v);					\
+									\
+	__asm__ __volatile__(						\
+		LOCK "decl (%%eax)\n"  					\
+		"js 2f\n"						\
+		"1:\n"							\
+		LOCK_SECTION_START("")					\
+		"2: call "#fn_name"\n\t"				\
+		"jmp 1b\n"						\
+		LOCK_SECTION_END					\
+		:"=a"(dummy)						\
+		:"a" (v)						\
+		:"memory", "ecx", "edx");				\
+} while (0)
+
+/**
+ * atomic_inc_call_if_nonpositive - increment and call function if nonpositive
+ * @v: pointer of type atomic_t
+ * @fn: function to call if the result is nonpositive
+ *
+ * Atomically increments @v and calls a function if the result is nonpositive.
+ */
+#define atomic_inc_call_if_nonpositive(v, fn_name)			\
+do {									\
+	fastcall void (*__tmp)(atomic_t *) = fn_name;			\
+	unsigned int dummy;						\
+									\
+	(void)__tmp;							\
+	typecheck(atomic_t *, v);					\
+									\
+	__asm__ __volatile__(						\
+		LOCK "incl (%%eax)\n"  					\
+		"jle 2f\n"						\
+		"1:\n"							\
+		LOCK_SECTION_START("")					\
+		"2: call "#fn_name"\n\t"				\
+		"jmp 1b\n"						\
+		LOCK_SECTION_END					\
+		:"=a" (dummy)						\
+		:"a" (v)						\
+		:"memory", "ecx", "edx");				\
+} while (0)
+
+
 /* These are x86-specific, used by some header files */
 #define atomic_clear_mask(mask, addr) \
 __asm__ __volatile__(LOCK "andl %0,%1" \