[Linux-ia64] rwlock and atomic_sub on ia64

[Linux-ia64] rwlock and atomic_sub on ia64

[Noah Romer]

I'm in the process of adding support for the ia64 architecture to the 
message/fusion/mptlan.c driver (it's previously only been tested with 
i386 and sparc64 systems) and have come across a couple of issues:

1) there doesn't appear to be support for rwlocks under ia64. At least, 
I can't find any way to make the above driver link when it's got code 
that uses rwlocks.

and

2) atomic_sub looks to require that I cast a u8 to (int) when I pass it 
in as a parameter. Not a major issue, I just found it odd.

I'm most likely missing something (didn't see anything about either of 
the above when I looked through the list archive), but I was wondering 
if I could get a pointer or two from those with more knowledge of 
ia64/linux issues.

Thanks.

I'm not currently subscribed to linux-ia64, so please cc: me on replies.

-- 
Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[Jes Sorensen]

>>>>> "Noah" = Noah Romer <nromer@lsil.com> writes:

Noah> I'm in the process of adding support for the ia64 architecture
Noah> to the message/fusion/mptlan.c driver (it's previously only been
Noah> tested with i386 and sparc64 systems) and have come across a
Noah> couple of issues:

Is this a kernel driver or a userland application? If it's the latter
then you are not allowed to rely on kernel definitions etc.

Noah> 2) atomic_sub looks to require that I cast a u8 to (int) when I
Noah> pass it in as a parameter. Not a major issue, I just found it
Noah> odd.

Noah> I'm most likely missing something (didn't see anything about
Noah> either of the above when I looked through the list archive), but
Noah> I was wondering if I could get a pointer or two from those with
Noah> more knowledge of ia64/linux issues.

You are talking about the subtracted argument I take it? It's int on
most architectures, including Linux/i386:

static __inline__ void atomic_sub(int i, atomic_t *v)
{
	__asm__ __volatile__(
		LOCK "subl %1,%0"
		:"=m" (v->counter)
		:"ir" (i), "m" (v->counter));
}

Jes

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[Noah Romer]

Jes Sorensen wrote:
>>>>>>"Noah" = Noah Romer <nromer@lsil.com> writes:
>>>>>>
> 
> Noah> I'm in the process of adding support for the ia64 architecture
> Noah> to the message/fusion/mptlan.c driver (it's previously only been
> Noah> tested with i386 and sparc64 systems) and have come across a
> Noah> couple of issues:
> 
> Is this a kernel driver or a userland application? If it's the latter
> then you are not allowed to rely on kernel definitions etc.

This is a kernel driver (linux/drivers/message/fusion). The version in 
2.4.7 isn't up to date (and doesn't contain the code that uses rwlocks), 
but it's there, finally.

I've been poking around in the kernel with Source Navigator and have 
found implementations of rwlocks in Alpha, i386, Mips, Mips64, PArisc 
and Sparc, but not for ia64, yet. If it's not there yet, I can just 
ifdef around the code for ia64 (it's in a bit of code that works around 
QLogic's "alternative" view of RFC 2625 and the Fibre Channel protocol 
specs), but I thought I'd check first.

> Noah> 2) atomic_sub looks to require that I cast a u8 to (int) when I
> Noah> pass it in as a parameter. Not a major issue, I just found it
> Noah> odd.
> 
> Noah> I'm most likely missing something (didn't see anything about
> Noah> either of the above when I looked through the list archive), but
> Noah> I was wondering if I could get a pointer or two from those with
> Noah> more knowledge of ia64/linux issues.
> 
> You are talking about the subtracted argument I take it? It's int on

Correct.

> most architectures, including Linux/i386:
> 
> static __inline__ void atomic_sub(int i, atomic_t *v)
> {
> 	__asm__ __volatile__(
> 		LOCK "subl %1,%0"
> 		:"=m" (v->counter)
> 		:"ir" (i), "m" (v->counter));
> }

Yep, but, for some reason, I've never gotten any complaints from the 
compiler until I tried compiling on ia64. It's not a big issue 
(especially since the subtracted value in this instance should never be 
big enough that signed/unsigned will make a diff), as I can just cast it 
to int. It just struck me as strange that the compiler would now decide 
to issue a warning. The rwlock bit is what's got me hung up.

Thanks.

-- 
Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[Jes Sorensen]

>>>>> "Noah" = Noah Romer <nromer@lsil.com> writes:

Noah> This is a kernel driver (linux/drivers/message/fusion). The
Noah> version in 2.4.7 isn't up to date (and doesn't contain the code
Noah> that uses rwlocks), but it's there, finally.

Ok, I saw Dave Miller checked it into his CVS tree yesterday or so.

Noah> Yep, but, for some reason, I've never gotten any complaints from
Noah> the compiler until I tried compiling on ia64. It's not a big
Noah> issue (especially since the subtracted value in this instance
Noah> should never be big enough that signed/unsigned will make a
Noah> diff), as I can just cast it to int. It just struck me as
Noah> strange that the compiler would now decide to issue a
Noah> warning. The rwlock bit is what's got me hung up.

The reason for this is most likely the compiler versions. For
Linux/ia64 you have to use at least gcc-2.96 to compile the
kernel. This compiler is a lot more picky about data types than the
older versions such as egcs-1.1.2/gcc-2.95 etc.

I'll leave the question about rwsem's to David ;)

Cheers
Jes

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[Noah Romer]

Jes Sorensen wrote:
>>>>>>"Noah" = Noah Romer <nromer@lsil.com> writes:
>>>>>>
> 
> Noah> This is a kernel driver (linux/drivers/message/fusion). The
> Noah> version in 2.4.7 isn't up to date (and doesn't contain the code
> Noah> that uses rwlocks), but it's there, finally.
> 
> Ok, I saw Dave Miller checked it into his CVS tree yesterday or so.
> 
> Noah> Yep, but, for some reason, I've never gotten any complaints from
> Noah> the compiler until I tried compiling on ia64. It's not a big
> Noah> issue (especially since the subtracted value in this instance
> Noah> should never be big enough that signed/unsigned will make a
> Noah> diff), as I can just cast it to int. It just struck me as
> Noah> strange that the compiler would now decide to issue a
> Noah> warning. The rwlock bit is what's got me hung up.
> 
> The reason for this is most likely the compiler versions. For
> Linux/ia64 you have to use at least gcc-2.96 to compile the
> kernel. This compiler is a lot more picky about data types than the
> older versions such as egcs-1.1.2/gcc-2.95 etc.

Ok, I wondered if it had anything to do w/ the newer compiler that came 
w/ Redhat's ia64 beta. Most of my i386 compiles have been done w/, um, 
egcs 2.91.66. I guess it's time to update those systems.

> I'll leave the question about rwsem's to David ;)
> 
> Cheers
> Jes

Thanks.
-- 
Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[David Mosberger]

>>>>> On 13 Aug 2001 22:48:11 +0200, Jes Sorensen <jes@sunsite.dk> said:

  Jes> I'll leave the question about rwsem's to David ;)

We're using the generic implementation on ia64.

	--david

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[Noah Romer]

David Mosberger wrote:
 >>>>>>On 13 Aug 2001 22:48:11 +0200, Jes Sorensen <jes@sunsite.dk> said:
 >>>>>>
 >
 >   Jes> I'll leave the question about rwsem's to David ;)
 >
 > We're using the generic implementation on ia64.
 >
 > 	--david
 >

Yes, but how do I get to it? I finally found the ia64 implementation of
write_lock and read_lock for SMP systems (which is what I'm compiling
on).(1) However, when I try to compile the driver I get a

   warning: implicit declaration of function `rwlock_init'

message. I have <linux/spinlock.h> included, and that's been enough up 
until now. Is there someother include file I need under the ia64 arch?

Thanks.


(1) Source Navigator doesn't seem to find it when I just have it look
for functions/macros w/ those names, but it does find the rwlock_t
declaration in include/asm-ia64/spinlock.h.

-- 
Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[David Mosberger]

>>>>> On Tue, 14 Aug 2001 16:00:39 -0500, Noah Romer <nromer@lsil.com> said:

  Noah> David Mosberger wrote:

  Jes> I'll leave the question about rwsem's to David ;)

  >>  We're using the generic implementation on ia64.

Oops, Noah was actually talking about the rwlocks, not about rwsem.
For rwlocks, we have our own implementation.

  Noah> Yes, but how do I get to it? I finally found the ia64
  Noah> implementation of write_lock and read_lock for SMP systems
  Noah> (which is what I'm compiling on).(1) However, when I try to
  Noah> compile the driver I get a

  Noah>    warning: implicit declaration of function `rwlock_init'

  Noah> message. I have <linux/spinlock.h> included, and that's been
  Noah> enough up until now. Is there someother include file I need
  Noah> under the ia64 arch?

No, that seems to be a genuine omission.  rwlock_init() was added in
the 2.4.0 test series and I seem to have missed that.  There aren't
too many drivers that use it.  Anyhow, the fix is trivial.  Something
along the lines of:

--- include/asm-ia64/spinlock.h~	Tue Aug 14 00:06:43 2001
+++ include/asm-ia64/spinlock.h	Wed Aug 15 11:31:24 2001
@@ -108,6 +108,8 @@
 } rwlock_t;
 #define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
 
+#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+
 #define read_lock(rw)								\
 do {										\
 	int tmp = 0;								\

should do the trick.

	--david

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[Noah Romer]

David Mosberger wrote:
>On Tue, 14 Aug 2001 16:00:39 -0500, Noah Romer <nromer@lsil.com> said:
>   Noah> Yes, but how do I get to it? I finally found the ia64
>   Noah> implementation of write_lock and read_lock for SMP systems
>   Noah> (which is what I'm compiling on).(1) However, when I try to
>   Noah> compile the driver I get a
> 
>   Noah>    warning: implicit declaration of function `rwlock_init'
> 
>   Noah> message. I have <linux/spinlock.h> included, and that's been
>   Noah> enough up until now. Is there someother include file I need
>   Noah> under the ia64 arch?
> 
> No, that seems to be a genuine omission.  rwlock_init() was added in
> the 2.4.0 test series and I seem to have missed that.  There aren't
> too many drivers that use it.  Anyhow, the fix is trivial.  Something
> along the lines of:
> 
> --- include/asm-ia64/spinlock.h~	Tue Aug 14 00:06:43 2001
> +++ include/asm-ia64/spinlock.h	Wed Aug 15 11:31:24 2001
> @@ -108,6 +108,8 @@
>  } rwlock_t;
>  #define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
>  
> +#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
> +
>  #define read_lock(rw)					\
>  do {								\
>  	int tmp = 0;						\
> 
> should do the trick.

Yep, that fixes it. I never even noticed that rwlock_init wasn't 
defined, just saw read_lock, write_lock and rwlock_t and assumed 
rwlock_init was there. My bad. Thanks.

On another note, I'm still a little puzzled by the warnings I got when 
using atomic_sub. I was passing a u8 value in as the amount to be 
subtracted, and got four

    mptlan.c:1135: warning: comparison is always false due to limited 
range of data type

Now, I can make the warning go away by changing the variable to an int 
(but not by casting the u8 to an int), but it seems odd that gcc would 
complain about the size of a u8 variable making the comparison always 
false. I started poking around in the output for other drivers, and 
found the above warning for all sorts of code (a lot of it involving bh 
data types). This is probably a question for the gcc folks, but . . . ;)

-- 
Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[KOCHI Takayoshi]

On Wed, 15 Aug 2001 15:30:17 -0500
Noah Romer <nromer@lsil.com> wrote:

> On another note, I'm still a little puzzled by the warnings I got when 
> using atomic_sub. I was passing a u8 value in as the amount to be 
> subtracted, and got four
> 
>     mptlan.c:1135: warning: comparison is always false due to limited 
> range of data type
> 
> Now, I can make the warning go away by changing the variable to an int 
> (but not by casting the u8 to an int), but it seems odd that gcc would 
> complain about the size of a u8 variable making the comparison always 
> false. I started poking around in the output for other drivers, and 
> found the above warning for all sorts of code (a lot of it involving bh 
> data types). This is probably a question for the gcc folks, but . . . ;)

This is because atomic_sub is actually a macro defined in
asm-ia64/atomic.h and it compares value against negative values
to get optimized code.

So, four comparison against negative numbers makes four warnings
for the single atomic_sub.

Here is the quote of atomic_sub

#define atomic_sub_return(i,v)						\
	((__builtin_constant_p(i) &&					\
	  (    ( i =  1) || ( i =  4) || ( i =  8) || ( i = 16)	\
	    || ( i = -1) || ( i = -4) || ( i = -8) || ( i = -16)))	\
	 ? ia64_fetch_and_add(-(i), &(v)->counter)			\
	 : ia64_atomic_sub(i, v))

#define atomic_sub(i,v)		atomic_sub_return((i), (v))

-- 
KOCHI Takayoshi <t-kouchi@cq.jp.nec.com/t-kouchi@mvf.biglobe.ne.jp>

Re: [Linux-ia64] rwlock and atomic_sub on ia64

[David Mosberger]

>>>>> On Wed, 15 Aug 2001 15:29:19 -0700, KOCHI Takayoshi <t-kouchi@mvf.biglobe.ne.jp> said:

  Kochi> This is because atomic_sub is actually a macro defined in
  Kochi> asm-ia64/atomic.h and it compares value against negative values
  Kochi> to get optimized code.

  Kochi> So, four comparison against negative numbers makes four warnings
  Kochi> for the single atomic_sub.

  Kochi> Here is the quote of atomic_sub

  Kochi> #define atomic_sub_return(i,v)						\
  Kochi> ((__builtin_constant_p(i) &&					\
  Kochi> (    ( i =  1) || ( i =  4) || ( i =  8) || ( i = 16)	\
  Kochi> || ( i = -1) || ( i = -4) || ( i = -8) || ( i = -16)))	\
  Kochi> ? ia64_fetch_and_add(-(i), &(v)->counter)			\
  Kochi> : ia64_atomic_sub(i, v))

  Kochi> #define atomic_sub(i,v)		atomic_sub_return((i), (v))

That's correct.  We could fix this with appropriate casting and then
you could use fetchadd1 to increment unsigned variables by 224, 240,
252, 255.  However, getting this 100% is rather tricky and I'm not
sure I want to play with this in the 2.4 series.  For now, I'd
recommend to just live with the warning (it's harmless, other than
being annoying).

	--david