Why does test_bit() take a volatile addr?

Why does test_bit() take a volatile addr?

[Rusty Russell]

Predates git, does anyone remember the rationale?

ie:
        int test_bit(int nr, const volatile unsigned long *addr)

I noticed because gcc failed to elimiate some code in a patch I was
playing with.

I'm nervous about subtle bugs involved in ripping it out, even if noone
knows why.  Should I add __test_bit()?

Thanks,
Rusty.

Re: Why does test_bit() take a volatile addr?

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

Hi Rusty,

On Mon, 16 Sep 2013 13:38:35 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Predates git, does anyone remember the rationale?
> 
> ie:
>         int test_bit(int nr, const volatile unsigned long *addr)

Because we sometimes pass volatile pointers to it and gcc will complain
if you pass a volatile to a non volatile  (I think).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Why does test_bit() take a volatile addr?

[Michael S. Tsirkin]

On Mon, Sep 16, 2013 at 04:53:44PM +1000, Stephen Rothwell wrote:
> Hi Rusty,
> 
> On Mon, 16 Sep 2013 13:38:35 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
> >
> > Predates git, does anyone remember the rationale?
> > 
> > ie:
> >         int test_bit(int nr, const volatile unsigned long *addr)
> 
> Because we sometimes pass volatile pointers to it and gcc will complain
> if you pass a volatile to a non volatile  (I think).

Where are these? I did git grep -W test_bit and looked for volatile,
couldn't find anything.

> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au

Re: Why does test_bit() take a volatile addr?

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

Hi Michael,

On Mon, 16 Sep 2013 10:26:03 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> On Mon, Sep 16, 2013 at 04:53:44PM +1000, Stephen Rothwell wrote:
> > 
> > On Mon, 16 Sep 2013 13:38:35 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
> > >
> > > Predates git, does anyone remember the rationale?
> > > 
> > > ie:
> > >         int test_bit(int nr, const volatile unsigned long *addr)
> > 
> > Because we sometimes pass volatile pointers to it and gcc will complain
> > if you pass a volatile to a non volatile  (I think).
> 
> Where are these? I did git grep -W test_bit and looked for volatile,
> couldn't find anything.

OK, so it was a bit of a guess.  Have you really checked the type of
every address passed to every call of test_bit()?

Second guess:  we wanted to make the test_bit access volatile (as opposed
to the datatypes of the objects being tested) so that things like

	while (testbit(bit, addr)) {
		do_very_little();
	}

don't get over optimised (since we are operating in a very threaded
environment that the compiler not might expect).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Why does test_bit() take a volatile addr?

[Michael S. Tsirkin]

On Mon, Sep 16, 2013 at 06:02:31PM +1000, Stephen Rothwell wrote:
> Hi Michael,
> 
> On Mon, 16 Sep 2013 10:26:03 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > On Mon, Sep 16, 2013 at 04:53:44PM +1000, Stephen Rothwell wrote:
> > > 
> > > On Mon, 16 Sep 2013 13:38:35 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > >
> > > > Predates git, does anyone remember the rationale?
> > > > 
> > > > ie:
> > > >         int test_bit(int nr, const volatile unsigned long *addr)
> > > 
> > > Because we sometimes pass volatile pointers to it and gcc will complain
> > > if you pass a volatile to a non volatile  (I think).
> > 
> > Where are these? I did git grep -W test_bit and looked for volatile,
> > couldn't find anything.
> 
> OK, so it was a bit of a guess.  Have you really checked the type of
> every address passed to every call of test_bit()?

Yea, I have this magic tool called gcc :)

Change
-static __always_inline int constant_test_bit(long nr, const volatile unsigned long *addr)
+static __always_inline int constant_test_bit(long nr, const unsigned long *addr)

and watch for new warnings.

I didn't see any.

> Second guess:  we wanted to make the test_bit access volatile (as opposed
> to the datatypes of the objects being tested) so that things like
> 
> 	while (testbit(bit, addr)) {
> 		do_very_little();
> 	}
> 
> don't get over optimised (since we are operating in a very threaded
> environment that the compiler not might expect).
> 
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au

Re: Why does test_bit() take a volatile addr?

[Geert Uytterhoeven]

On Mon, Sep 16, 2013 at 6:08 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Predates git, does anyone remember the rationale?
>
> ie:
>         int test_bit(int nr, const volatile unsigned long *addr)

That's why we have full-history-linux ;-)

Unfortunately it doesn't show the rationale, as this change also predates
bitkeeper.

Since 2.1.30, volatile is used unconditionally:

commit 84d59b7dda1092a22663f4e2da77a6ce581b539e
Author: linus1 <torvalds@linuxfoundation.org>
Date:   Mon Mar 10 12:00:00 1997 -0600

    Import 2.1.30

 #ifdef __SMP__
 #define LOCK_PREFIX "lock ; "
-#define SMPVOL volatile
 #else
 #define LOCK_PREFIX ""
-#define SMPVOL
 #endif

 /*
  * This routine doesn't need to be atomic.
  */
-extern __inline__ int test_bit(int nr, const SMPVOL void * addr)
+extern __inline__ int __constant_test_bit(int nr, const volatile void * addr)
 {
-       return ((1UL << (nr & 31)) & (((const unsigned int *) addr)[nr >> 5])) !
+       return ((1UL << (nr & 31)) & (((const volatile unsigned int *) addr)[nr
 }

+extern __inline__ int __test_bit(int nr, volatile void * addr)
+{
+       int oldbit;
+
+       __asm__ __volatile__(
+               "btl %2,%1\n\tsbbl %0,%0"
+               :"=r" (oldbit)
+               :"m" (ADDR),"ir" (nr));
+       return oldbit;
+}
+
+#define test_bit(nr,addr) \
+(__builtin_constant_p(nr) ? \
+ __constant_test_bit((nr),(addr)) : \
+ __test_bit((nr),(addr)))
+
 /*


Between 1.3.75 and 2.1.30, volatile was used on SMP only:

commit 08c5b9d698e6ca2233aec0f7d7f0fdd6eb3ad235
Author: linus1 <torvalds@linuxfoundation.org>
Date:   Thu Mar 7 12:00:00 1996 -0600

    Import 1.3.75


 #ifdef __SMP__
 #define LOCK_PREFIX "lock ; "
+#define SMPVOL volatile
 #else
 #define LOCK_PREFIX ""
+#define SMPVOL
 #endif


 /*
  * This routine doesn't need to be atomic.
  */
-extern __inline__ int test_bit(int nr, const void * addr)
+extern __inline__ int test_bit(int nr, const SMPVOL void * addr)
 {
        return 1UL & (((const unsigned int *) addr)[nr >> 5] >> (nr & 31));
 }


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Why does test_bit() take a volatile addr?

[Michael S. Tsirkin]

On Mon, Sep 16, 2013 at 01:38:35PM +0930, Rusty Russell wrote:
> Predates git, does anyone remember the rationale?
> 
> ie:
>         int test_bit(int nr, const volatile unsigned long *addr)
> 
> I noticed because gcc failed to elimiate some code in a patch I was
> playing with.
> 
> I'm nervous about subtle bugs involved in ripping it out, even if noone
> knows why.  Should I add __test_bit()?
> 
> Thanks,
> Rusty.

So looking at this some more, e.g. on x86 I see:

static inline int variable_test_bit(long nr, volatile const unsigned
long *addr)
{
        int oldbit;

        asm volatile("bt %2,%1\n\t"
                     "sbb %0,%0"
                     : "=r" (oldbit)
                     : "m" (*(unsigned long *)addr), "Ir" (nr));

        return oldbit;
}

and I have a vague memory that (at least for some old versions) gcc
would assume (*(unsigned long *)addr) only refers to addr[0].

OTOH constant_test_bit is
static __always_inline int constant_test_bit(long nr, const volatile unsigned long *addr)
{
        return ((1UL << (nr & (BITS_PER_LONG-1))) &
                (addr[nr >> _BITOPS_LONG_SHIFT])) != 0;
}

So there's a chance that we can drop volatile here.

I'll look at it some more.

-- 
MST

Re: Why does test_bit() take a volatile addr?

[Oliver Neukum]

On Mon, 2013-09-16 at 13:38 +0930, Rusty Russell wrote:
> Predates git, does anyone remember the rationale?
> 
> ie:
>         int test_bit(int nr, const volatile unsigned long *addr)
> 
> I noticed because gcc failed to elimiate some code in a patch I was
> playing with.
> 
> I'm nervous about subtle bugs involved in ripping it out, even if noone
> knows why.  Should I add __test_bit()?

It seems to me that if you do

b = *ptr & 0xf;
c = b << 2;
if (test_bit(1, ptr))

the compiler could optimize away the memory access on ptr without
the volatile. We'd have to add a lot of mb().

	Regards
		Oliver

Re: Why does test_bit() take a volatile addr?

[Michael S. Tsirkin]

On Mon, Sep 16, 2013 at 10:40:00AM +0200, Oliver Neukum wrote:
> On Mon, 2013-09-16 at 13:38 +0930, Rusty Russell wrote:
> > Predates git, does anyone remember the rationale?
> > 
> > ie:
> >         int test_bit(int nr, const volatile unsigned long *addr)
> > 
> > I noticed because gcc failed to elimiate some code in a patch I was
> > playing with.
> > 
> > I'm nervous about subtle bugs involved in ripping it out, even if noone
> > knows why.  Should I add __test_bit()?
> 
> It seems to me that if you do
> 
> b = *ptr & 0xf;
> c = b << 2;
> if (test_bit(1, ptr))
> 
> the compiler could optimize away the memory access on ptr without
> the volatile. We'd have to add a lot of mb().
> 
> 	Regards
> 		Oliver

What is this code supposed to do?
Any specific examples?

-- 
MST

Re: Why does test_bit() take a volatile addr?

[Oliver Neukum]

On Mon, 2013-09-16 at 11:44 +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 16, 2013 at 10:40:00AM +0200, Oliver Neukum wrote:
> > On Mon, 2013-09-16 at 13:38 +0930, Rusty Russell wrote:
> > > Predates git, does anyone remember the rationale?
> > > 
> > > ie:
> > >         int test_bit(int nr, const volatile unsigned long *addr)
> > > 
> > > I noticed because gcc failed to elimiate some code in a patch I was
> > > playing with.
> > > 
> > > I'm nervous about subtle bugs involved in ripping it out, even if noone
> > > knows why.  Should I add __test_bit()?
> > 
> > It seems to me that if you do
> > 
> > b = *ptr & 0xf;
> > c = b << 2;
> > if (test_bit(1, ptr))
> > 
> > the compiler could optimize away the memory access on ptr without
> > the volatile. We'd have to add a lot of mb().
> > 
> > 	Regards
> > 		Oliver
> 
> What is this code supposed to do?
> Any specific examples?
> 

Often you see

while (test_bit(...) && condition) ... ;

If the compiler can show that you don't change the memory you
do the test_bit on, it can change this to:

if (test_bit(...))
	while (condition) ...;

That must not happen.

	Regards
		Oliver

Re: Why does test_bit() take a volatile addr?

[Linus Torvalds]

On Mon, Sep 16, 2013 at 12:08 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Predates git, does anyone remember the rationale?
>
> ie:
>         int test_bit(int nr, const volatile unsigned long *addr)

Both of Stephen Rothwell's guesses are correct.

One reason is that we used to use "volatile" a lot more than we do
now, and "const volatile *" is the most permissive pointer that allows
any use without warnings.

We've largely stopped using "volatile" in favor of explicit barriers
and locks (ie "cpu_relax()" and "barrier()") and explicit volatility
in code (ACCESS_ONCE() and "rcu_access_pointer()" etc).

The other reasons is for fear of having some old code that does effectively

   while (condition) /* nothing */

using "test_bit()", and forcing a reload. They used to happen. They
were rare even before, and I'd hope they are nonexistent now, but they
were real.

We could try to see what happens if we remove "volatile" from the
bitops these days. But the scary part is all the random drivers
potentially doing that second thing. So it's not exactly easily
testable. It would need to be worth it to bother.

                Linus

Re: Why does test_bit() take a volatile addr?

[Rob Landley]

On 09/15/2013 11:08:35 PM, Rusty Russell wrote:
> Predates git, does anyone remember the rationale?

Depends which git: http://landley.net/kdocs/fullhist/ :)

Rob

Re: Why does test_bit() take a volatile addr?

[Rusty Russell]

Rob Landley <rob@landley.net> writes:
> On 09/15/2013 11:08:35 PM, Rusty Russell wrote:
>> Predates git, does anyone remember the rationale?
>
> Depends which git: http://landley.net/kdocs/fullhist/ :)

Not useful.  See Geert's more helpful response.

Cheers,
Rusty.