[patch] s390: missing ifdef in bitops.h

[patch] s390: missing ifdef in bitops.h

[Heiko Carstens]

From: Cedric Le Goater <clg@fr.ibm.com>

Add missing #ifdef __KERNEL__ to asm-s390/bitops.h

Signed-off-by: Cedric Le Goater <clg@fr.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

Patch is for -mm tree only and resolves a merge conflict.

 include/asm-s390/bitops.h |    3 +++
 1 file changed, 3 insertions(+)

diff -purN a/include/asm-s390/bitops.h b/include/asm-s390/bitops.h
--- a/include/asm-s390/bitops.h	2006-06-13 07:38:53.000000000 +0200
+++ b/include/asm-s390/bitops.h	2006-06-13 07:41:20.000000000 +0200
@@ -12,6 +12,9 @@
  *    Copyright (C) 1992, Linus Torvalds
  *
  */
+
+#ifdef __KERNEL__
+
 #include <linux/compiler.h>
 
 /*

Re: [patch] s390: missing ifdef in bitops.h

[David Woodhouse]

On Tue, 2006-06-13 at 14:09 +0200, Heiko Carstens wrote:
> Add missing #ifdef __KERNEL__ to asm-s390/bitops.h

But asm/bitops.h isn't suitable for userspace _anyway_, is it?
We should be able to drop all instances of __KERNEL__ from it.

Which means that asm-s390/posix_types.h probably ought never to be
trying to include asm/bitops.h in the !__KERNEL__ case... we never had
libc5 on S390 anyway, did we?

Martin, is it OK for me to add this to the hdrcleanup-2.6.git tree?

diff --git a/include/asm-s390/posix_types.h b/include/asm-s390/posix_types.h
index 61788de..18344dc 100644
--- a/include/asm-s390/posix_types.h
+++ b/include/asm-s390/posix_types.h
@@ -76,7 +76,7 @@ #endif                       /* !defined
 } __kernel_fsid_t;
 
 
-#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
+#ifdef __KERNEL__
 
 #ifndef _S390_BITOPS_H
 #include <asm/bitops.h>
@@ -94,6 +94,6 @@ #define __FD_ISSET(fd,fdsetp)  test_bit(
 #undef  __FD_ZERO
 #define __FD_ZERO(fdsetp) (memset ((fdsetp), 0, sizeof(*(fd_set *)(fdsetp))))
 
-#endif     /* defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)*/
+#endif     /* __KERNEL__ */
 
 #endif


 

-- 
dwmw2

Re: [patch] s390: missing ifdef in bitops.h

[Arnd Bergmann]

Am Tuesday 13 June 2006 17:17 schrieb David Woodhouse:
> -#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
> +#ifdef __KERNEL__
>  
>  #ifndef _S390_BITOPS_H
>  #include <asm/bitops.h>
> @@ -94,6 +94,6 @@ #define __FD_ISSET(fd,fdsetp)  test_bit(
>  #undef  __FD_ZERO
>  #define __FD_ZERO(fdsetp) (memset ((fdsetp), 0, sizeof(*(fd_set
> *)(fdsetp)))) 
> -#endif     /* defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ <
> 2)*/ +#endif     /* __KERNEL__ */
>  

Erm, I don't think the kernel even uses those definitions, the only reason
to keep them is old user space.

Now that is true for all architectures, although they all handle things
slightly differently: 

cris, v850, s390: use bitops, which can be considered broken
arm, arm26, frv, h8300, m68k: use a trivial C macro
i386: has an inline assembly
sparc64, x86_64, alpha, xtensa, sparc, ia64, powerpc, sh, parisc, mips:
   use variations of the same unrolled C inline functions

I'd suggest using only one version. In doubt, I would move the parisc
version to asm-generic/fd_set.h or similar and include that from
everywhere.

	Arnd <><

Re: [patch] s390: missing ifdef in bitops.h

[David Woodhouse]

On Tue, 2006-06-13 at 22:33 +0200, Arnd Bergmann wrote:
> I'd suggest using only one version. In doubt, I would move the parisc
> version to asm-generic/fd_set.h or similar and include that from
> everywhere. 

That makes sense. It's not _entirely_ trivial though, so I don't want to
do it in my hdrcleanup-2.6.git tree. I'll leave it for later.

-- 
dwmw2

Re: [patch] s390: missing ifdef in bitops.h

[Heiko Carstens]

On Tue, Jun 13, 2006 at 10:39:36PM +0100, David Woodhouse wrote:
> On Tue, 2006-06-13 at 22:33 +0200, Arnd Bergmann wrote:
> > I'd suggest using only one version. In doubt, I would move the parisc
> > version to asm-generic/fd_set.h or similar and include that from
> > everywhere. 
> 
> That makes sense. It's not _entirely_ trivial though, so I don't want to
> do it in my hdrcleanup-2.6.git tree. I'll leave it for later.

In that case it would be good to get Cedric's bitops patch merged, since it
fixes compilation.

Re: [patch] s390: missing ifdef in bitops.h

[Arnd Bergmann]

Am Tuesday 13 June 2006 22:33 schrieb Arnd Bergmann:
> Erm, I don't think the kernel even uses those definitions, the only reason
> to keep them is old user space.

Sorry, misinformation on my side. The kernel uses the FD_foo versions,
not the __FD_foo ones, so I did not find them at first.

It's only used in fs/open.c though, and I wonder if that could just
use the bitops versions directly, as these are more likely to be
optimized well for a given architecture.

	Arnd <><

Re: [patch] s390: missing ifdef in bitops.h

[Martin Schwidefsky]

On Tue, 2006-06-13 at 22:43 +0200, Arnd Bergmann wrote:
> Am Tuesday 13 June 2006 22:33 schrieb Arnd Bergmann:
> > Erm, I don't think the kernel even uses those definitions, the only reason
> > to keep them is old user space.
> 
> Sorry, misinformation on my side. The kernel uses the FD_foo versions,
> not the __FD_foo ones, so I did not find them at first.
> 
> It's only used in fs/open.c though, and I wonder if that could just
> use the bitops versions directly, as these are more likely to be
> optimized well for a given architecture.

Using set_bit and clear_bit is actually overkill since they use compare
and swap to do an atomic update. That is not needed for __FD_foo. How
about the attached patch ?

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

--

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

[S390] __FD_foo definitions.

Make the definitions of __FD_SET, __FD_CLR and __FD_ISSET independent
from asm/bitops.h and remove the macro magic that tests for __GLIBC__.
Use simple C inline functions instead of set_bit, clear_bit and test_bit.

Spotted by David Woodhouse <dwmw2@infradead.org>

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

 include/asm-s390/posix_types.h |   42 ++++++++++++++++++++++++++---------------
 1 files changed, 27 insertions(+), 15 deletions(-)

diff -urpN linux-2.6/include/asm-s390/posix_types.h linux-2.6-patched/include/asm-s390/posix_types.h
--- linux-2.6/include/asm-s390/posix_types.h	2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6-patched/include/asm-s390/posix_types.h	2006-06-14 12:37:54.000000000 +0200
@@ -76,24 +76,36 @@ typedef struct {
 } __kernel_fsid_t;
 
 
-#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
+#ifdef __KERNEL__
 
-#ifndef _S390_BITOPS_H
-#include <asm/bitops.h>
-#endif
-
-#undef  __FD_SET
-#define __FD_SET(fd,fdsetp)  set_bit((fd),(fdsetp)->fds_bits)
-
-#undef  __FD_CLR
-#define __FD_CLR(fd,fdsetp)  clear_bit((fd),(fdsetp)->fds_bits)
-
-#undef  __FD_ISSET
-#define __FD_ISSET(fd,fdsetp)  test_bit((fd),(fdsetp)->fds_bits)
+#undef __FD_SET
+static inline void __FD_SET(unsigned long fd, __kernel_fd_set *fdsetp)
+{
+	unsigned long _tmp = fd / __NFDBITS;
+	unsigned long _rem = fd % __NFDBITS;
+	fdsetp->fds_bits[_tmp] |= (1UL<<_rem);
+}
+
+#undef __FD_CLR
+static inline void __FD_CLR(unsigned long fd, __kernel_fd_set *fdsetp)
+{
+	unsigned long _tmp = fd / __NFDBITS;
+	unsigned long _rem = fd % __NFDBITS;
+	fdsetp->fds_bits[_tmp] &= ~(1UL<<_rem);
+}
+
+#undef __FD_ISSET
+static inline int __FD_ISSET(unsigned long fd, const __kernel_fd_set *fdsetp)
+{
+	unsigned long _tmp = fd / __NFDBITS;
+	unsigned long _rem = fd % __NFDBITS;
+	return (fdsetp->fds_bits[_tmp] & (1UL<<_rem)) != 0;
+}
 
 #undef  __FD_ZERO
-#define __FD_ZERO(fdsetp) (memset ((fdsetp), 0, sizeof(*(fd_set *)(fdsetp))))
+#define __FD_ZERO(fdsetp) \
+	((void) memset ((__ptr_t) (fdsetp), 0, sizeof (__kernel_fd_set)))
 
-#endif     /* defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)*/
+#endif     /* __KERNEL__ */
 
 #endif

Re: [patch] s390: missing ifdef in bitops.h

[David Woodhouse]

On Wed, 2006-06-14 at 12:50 +0200, Martin Schwidefsky wrote:
> Using set_bit and clear_bit is actually overkill since they use compare
> and swap to do an atomic update. That is not needed for __FD_foo. How
> about the attached patch ? 

Looks good. I'll stick that in the hdrcleanup-2.6.git tree right now, if
that's OK?

-- 
dwmw2

Re: [patch] s390: missing ifdef in bitops.h

[Martin Schwidefsky]

On Wed, 2006-06-14 at 14:04 +0100, David Woodhouse wrote:
> On Wed, 2006-06-14 at 12:50 +0200, Martin Schwidefsky wrote:
> > Using set_bit and clear_bit is actually overkill since they use compare
> > and swap to do an atomic update. That is not needed for __FD_foo. How
> > about the attached patch ? 
> 
> Looks good. I'll stick that in the hdrcleanup-2.6.git tree right now, if
> that's OK?

Fine with me.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

Re: [patch] s390: missing ifdef in bitops.h

[Martin Schwidefsky]

On Tue, 2006-06-13 at 16:17 +0100, David Woodhouse wrote:
> On Tue, 2006-06-13 at 14:09 +0200, Heiko Carstens wrote:
> > Add missing #ifdef __KERNEL__ to asm-s390/bitops.h
> 
> But asm/bitops.h isn't suitable for userspace _anyway_, is it?
> We should be able to drop all instances of __KERNEL__ from it.

Nothing from asm/bitops.h should ever be used in userspace.

> Which means that asm-s390/posix_types.h probably ought never to be
> trying to include asm/bitops.h in the !__KERNEL__ case... we never had
> libc5 on S390 anyway, did we?

Correct. And the depedency to bitops.h in case of __KERNEL__ = 1 should
not be there either, in particular since __FD_SET/__FD_CLR do not have
to be atomic. A simple piece of C code should be used instead of
set_bit/clear_bit. 

> Martin, is it OK for me to add this to the hdrcleanup-2.6.git tree?
> 
> diff --git a/include/asm-s390/posix_types.h b/include/asm-s390/posix_types.h
> index 61788de..18344dc 100644
> --- a/include/asm-s390/posix_types.h
> +++ b/include/asm-s390/posix_types.h
> @@ -76,7 +76,7 @@ #endif                       /* !defined
>  } __kernel_fsid_t;
>  
> 
> -#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
> +#ifdef __KERNEL__
>  
>  #ifndef _S390_BITOPS_H
>  #include <asm/bitops.h>
> @@ -94,6 +94,6 @@ #define __FD_ISSET(fd,fdsetp)  test_bit(
>  #undef  __FD_ZERO
>  #define __FD_ZERO(fdsetp) (memset ((fdsetp), 0, sizeof(*(fd_set *)(fdsetp))))
>  
> -#endif     /* defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)*/
> +#endif     /* __KERNEL__ */
>  
>  #endif

It should not hurt. s390 always used a glibc with a major >= 2.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.