user-mode port 0.44-2.4.7

The user-mode port of 2.4.7 is available.

In a minor packaging breakthrogh, a .deb for UML is now available.

The UML block driver now supports a read-write COW layer above a shared read-only filesystem. This allows multiple UMLs to boot off the same filesystem. See http://user-mode-linux.sourceforge.net/shared_fs.html for more information.

The ppc port is now fully merged.

The pid file and mconsole socket are now located in a directory defined by the UML umid.

There is now IO memory emulation. This allows a host file to be mapped by a UML driver, which can provide whatever interface it wants to that file to UML processes. This is a first step towards doing hardware driver development under UML.

gdbs are now killed properly.

A nasty bug involving a misunderstanding with FASTCALL was fixed.

Block devices and network devices are now pluggable from the mconsole - they can be added to and removed from a running system. See
http://user-mode-linux.sourceforge.net/mconsole.html for more information.

SIGHUP no longer causes UML to go crazy.

The project's home page is http://user-mode-linux.sourceforge.net

Downloads are available at

	http://sourceforge.net/project/showfiles.php?group_id=429
	ftp://ftp.nl.linux.org/pub/uml/
	http://uml-pub.ists.dartmouth.edu/uml/

				Jeff

in my tree I did some further cleanup, here the ones that you can interested about:

diff -urN uml-ref/arch/um/include/kern_util.h uml/arch/um/include/kern_util.h
@@ -28,7 +28,6 @@
 extern long execute_syscall(struct sys_pt_regs regs);
 extern void syscall_segv(int sig);
 extern int current_pid(void);
-extern void do_bh(void);
 extern void set_init_pid(int pid);
 extern unsigned long alloc_stack(void);
 extern int do_signal(unsigned long *error, int *again_out);

diff -urN uml-ref/arch/um/kernel/process_kern.c uml/arch/um/kernel/process_kern.c
@@ -217,25 +217,12 @@
 	return(current->thread.request.u.cswitch.from);
 }
 
-void do_bh(void)
-{
-#ifndef CONFIG_SMP
-	if (softirq_pending(0)){
-		do_softirq();
-		unblock_signals();
-	}
-#else
-#error Need to update do_bh
-#endif
-}
-
 void ret_from_sys_call(void *t)
 {
 	struct task_struct *task;
 
 	task = t;
 	if(task == NULL) task = current;
-	do_bh();
 	if(task->need_resched) schedule();
 	if(task->sigpending != 0) do_signal(NULL, NULL);
 }

diff -urN uml-ref/arch/um/kernel/time.c uml/arch/um/kernel/time.c
@@ -16,7 +16,7 @@
 #include "user.h"
 #include "process.h"
 
-extern struct timeval xtime;
+extern volatile struct timeval xtime;
 
 void timer_handler(int sig, void *sc, int usermode)
 {

this one for compiling uml with the gcc-3_0-branch of yesterday:

@@ -21,7 +21,7 @@
  * Allocate and free page tables.
  */
 
-extern __inline__ pgd_t *get_pgd_slow(void)
+static __inline__ pgd_t *get_pgd_slow(void)
 {
 	pgd_t *pgd = (pgd_t *)__get_free_page(GFP_KERNEL);
 

BTW, Linus the _below_ patches against mainline are needed to compile the x86 port with gcc-3_0-branch of yesterday, it is safe to include it in mainline:

@@ -23,7 +23,7 @@
 extern void *kmalloc(size_t, int);
 extern void kfree(const void *);
 
-extern __inline__ pgd_t *get_pgd_slow(void)
+static __inline__ pgd_t *get_pgd_slow(void)
 {
 	int i;
 	pgd_t *pgd = kmalloc(PTRS_PER_PGD * sizeof(pgd_t), GFP_KERNEL);

@@ -48,7 +48,7 @@
 
 #else
 
-extern __inline__ pgd_t *get_pgd_slow(void)
+static __inline__ pgd_t *get_pgd_slow(void)
 {
 	pgd_t *pgd = (pgd_t *)__get_free_page(GFP_KERNEL);
 

@@ -216,7 +216,7 @@
 #ifdef __KERNEL__
 #include <linux/string.h>
 
-extern inline void copy_siginfo(siginfo_t *to, siginfo_t *from)
+static inline void copy_siginfo(siginfo_t *to, siginfo_t *from)
 {
 	if (from->si_code < 0)
 		memcpy(to, from, sizeof(siginfo_t));

@@ -274,7 +274,7 @@
 
 /* Process one rtnetlink message. */
 
-extern __inline__ int
+static __inline__ int
 rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, int *errp)
 {
 	struct rtnetlink_link *link;

@@ -541,7 +541,7 @@
 extern unsigned long volatile jiffies;
 extern unsigned long itimer_ticks;
 extern unsigned long itimer_next;
-extern struct timeval xtime;
+extern volatile struct timeval xtime;
 extern void do_timer(struct pt_regs *);
 
 extern unsigned int * prof_buffer;

Andrea

Andrea, please drop the "volatile" from xtime. It's bogus.

There's no reason why gcc couldn't make one or many accesses to that variable, and volatile is an evil keyword that is badly specified and only makes the compiler generate worse code without ever fixing any real bugs.

If you need a stable value, use a lock.

Linus

it's the other way around, it's needed and gcc trapped a kernel bug.

If the contents of memory not declared volatile changes under GCC (like it can happen right now for xtime since it's declared non volatile), gcc has the full rights to crash the kernel at runtime.

I know there are other bugs like this one in the kernel, but this is not a good reason to fix the known ones IMHO.

Andrea

No it's not.

Absolutely not.

If we care abotu the thing always having the same value, we HAVE to use a lock. "volatile" is not the answer.

Show me a place where we care.

Linus

The problem is not at the source code level, of course all places where we read the xtime cannot crash the kernel as far as the kernel is concerned, but the problem is instead at the gcc level: and when the logic implemented by the asm generated by gcc chokes we can also get a dangling poitner and crash in the kernel.

GCC internally is allowed to generate code that relies on the contents of the memory to stay constant, this because of the C standard, the usual example is a fast path jump table for the "case" statement.

So in short having non volatile memory that changes under gcc gives gcc the full rights to crash the kernel at runtime anytime.

Andrea

Bah.

If we have a case where we _really_ care about the value being stable, I _still_ claim that that is the one that we need to protect. Not waving some magic wand over the thing, and saying that "volatile" makes an unstable value ok.

"volatile" (in the data sense, not the "asm volatile" sense) is almost always a sign of a bug. If you have an algorithm that depends on the (compiler-specifi) behaviour of "volatile", you need to change the algorithm, not try to hide the fact that you have a bug.

Now, the change of algorithm might be something like

	/*
	 * We need to get _one_ value here, because our
	 * state machine ....
	 */
	unsigned long stable = *(volatile unsigned long *)ptr;

	switch (stable) {
	....
	}

where the volatile is in the place where we care, and the volatile is commented on WHY it actually is the correct thing to do.

The C standard doesn't say crap about volatile. It leaves it up to the compiler to do something about it.

This is _doubly_ true of multi-word data structures like "xtime". The meaning of "volatile" on the data structure is so unclear that it's not even funny.

Linus

gcc can assume 'state' stays constant in memory not just during the 'case'.

The C folks definitely say it is a kernel bug if you don't apply my patch and I agree with them. Ask Jan.

Andrea

The point is that if the kernel has _any_ algorithm where it cares, it's a kernel bug. With volatile or without.

SHOW ME THE CASE WHERE IT CARES. Let's fix it. Let's not just hide it with "volatile".

Linus

in arch/i386/kernel/io_apic.c:

If jiffies were not volatile, this initializing assignment and the test at the end could be optimized away, leaving an unconditional "return 0". A lock is of no help.

/Jonathan Lundell.

Right.

We want optimization barriers, and there's an explicit "barrier()" thing for Linux exactly for this reason.

For historical reasons "jiffies" is actually marked volatile, but at least it has reasonably good semantics with a single-data item. Which is not to say I like it. But grep for "barrier()" to see how many times we make this explicit in the algorithms.

And really, THAT is my whole point. Notice in the previous mail how I used "volatile" when it was part of the _algorithm_. You should not hide algorithmic choices in your data structures. You should make them explicit, so that when you read the code you _see_ what assumptions people make.

For example, if you fix the code by adding an explicit barrier(), people see that (a) you're aware of the fact that you expect the values to change and (b) they see that it is taken care of.

In contrast, if your data structure is marked volatile, how is anybody reading the code every going to be sure that the code is correct? You have to look at the header file defining all the data structures. That kind of thing is NOT GOOD.

So make the algorithm be correct. Then you will notice that there is _never_ any reason (except for being lazy with tons of existing code) to add "volatile" to data structures.

Ponder. Understand.

Linus

OK, sure, that's fine. Better than barrier() in some respects, too. Namely, 1) volatile is portable C; barrier() isn't (not that that's much of an issue for compiling Linux), and 2) volatile can be specific to a variable, unlike the indiscriminate barrier(), which forces a reload of everything that might be cached (OK, not a big deal for IA32, but nontrivial for many-register architectures). One could imagine a more specific barrier(jiffies) syntax, I suppose, but the volatile cast is nice, restricting the effect not only to the single variable but to the single reference to a single variable.

/Jonathan Lundell.

One more thing, with volatile you specify it one time ( declaration time ), while with barrier() you've to spread inside the code tons of such macro everywhere you touch the variable.

- Davide

That's the whole point, damnit. Syntax (or semantics) sugar is a Bad Thing(tm). If your algorithm depends on something in a nontrivial way - _spell_ _it_ _out_.

I would not call, to pretend the compiler to issue memory loads every time it access a variable, a nontrivial way.
It sounds pretty clear to me.

- Davide

You know, one of the nice things about C is that unless you abuse preprocessor, reading code doesn't require doing far lookups. Most of it can be read and understood with very little context. "Do it once when you declare a variable" goes against that and that's not a good thing.

Look, you're not going to request any kind of black magic over that variable. You're simply telling the compiler the way it has to ( not ) optimize the code. This is IMHO a declaration time issue.
Looking at this code :

while (jiffies < ...) {
        ...
}

the "natural" behaviour that a reader expects is that the "content" of the memory pointed by jiffied is loaded and compared.
That content, not the content of a register loaded 100 asm instructions before the load. If you like this code more :

for (;;) {
        barrier();
        if (jiffies >= ...)
                break;
        ...
}

It's clear that a declaration like :

__locked_access__ struct pio {
        int a, b, c;
};

for (;;) {
        ++a;
        if (a > b && c < a)
        ...
}

sounds a "Bad Thing"(tm) even to me.

- Davide

Ehh.

But it shouldn't optimize it that way _every_ time. You only want the specific optimizations in specific places. Which is why you use "barrier()" or volatile in the _code_, not the data declaration.

For example, if you're holding a lock that protects it or you otherwise know that nothing is touching it at the same time, you do NOT want to have the compiler generate bad code.

And trust me, "volatile" generates _bad_ code a lot more often than it generates correct code.

Look at this:

	volatile int i;
	int j;

	int main()
	{
	        i++;
	        j++;
	}

turning into this:

main:
movl i,%eax
incl %eax
movl %eax,i
incl j
ret

Now, ask yourself why? The two _should_ be the same. Both do a read-modify-write cycle. But the fact is, that when you add "volatile" to the register, it really tells gcc "Be afraid. Be very afraid. This user expects some random behaviour that is not actually covered by any standard, so just don't ever use this variable for any optimizations, even if they are obviously correct. That way he can't complain".

See? "volatile" is evil. It has _no_ standard semantics, which makes it really hard to implement sanely for the compiler. It also means that the compiler can change the semantics of what "volatile" means, without you really being able to complain.

Also note how the "incl j" instruction is actually _better_ from a "atomicity" standpoint than the "load+inc+store" instruction. In this case, adding a "volatile" actually made the accesses to "i" be _less_ likely to be correct - you could have had an interrupt happen in between that also updated "i", and got lost when we wrote the value back.

Moral of the story: don't use volatile. If you want to have a counter that is updated in interrupts etc, use "atomic_t" or locking. "volatile" makes things worse or better based on completely random criteria that don't necessarily have anything to do with what you _want_ to do.

Linus

This is a too simple case, this is maybe better :

        mov homer, %edx
        ...
        ...
        ...
        ... ( 101 asm ins )
loop:
        cmp %edx, ...
        ja out
        ...
        inc %edx
        ...
        jmp loop

You're right, it might be optimized with a barrier() but it's all kind of how much times you're going to need one behaviour or the other.
When I'll need most of my access to be "strict" I'd like to have a way that avoid me to spread the code with barries()s.

Not that much if you look at how incl is "decomposed" internally ( w/o LOCK ) by the CPU. If you really care about j you need an atomic op here, in any case.

- Davide

Yes, but the "inc" is atomic at least on a UP system. So here "volatile" might actually _show_ bugs instead of hiding them.

The real isssue, though, is that that is all volatile ever does. It can show or hide bugs, but it can't fix them.

Of course, some people consider hidden bugs to _be_ fixed. I don't believe in that particulat philosophy myself.

Linus

Well, that depends on your definition of "natural". In my definition, it would be absolutely normal in this example for the compiler to cache jiffies because it considers it as a non-changing variable if none of the code inside the while loop refers to jiffies again. But that's just me...

Er, what is wrong with:

while (barrier(), jiffies < ...) {
        ...
}

It is just as clean as the starting point but tells both the compiler at compile time and _me_ when reading the code that jiffies is expected to change under me.

That is _way_ better than declaring it volatile in some obsure header file which, chances are, I have never looked at, or looked at and long forgotten about...

Just my 2p.

Anton

  "Nothing succeeds like success." - Alexandre Dumas
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

If I understand correctly, xtime is updated asynchronously. If it isn't, then ignore this message totally. However, if it is, then *not* specifying it as volatile could easily cause problems in technically correct but poorly written code.

Suppose I loop against xtime reaching a particular value. While this is definately not good practice, if xtime is not specified as volatile then since I never modify it within the loop the compiler is free to move the initial load out of the loop when optimizing. In this example the case where it is marked as volatile will run (though inefficiently), but the non-volatile case can hang totally.

Do we want to get ourselves into something like this?

Chris

Chris Friesen                    | MailStop: 043/33/F10  
Nortel Networks                  | work: (613) 765-0557
3500 Carling Avenue              | fax:  (613) 765-2986
Nepean, ON K2H 8E9 Canada        | email: [email protected]

Yes.

Adding "volatile" often helps poorly written code.

In fact, the one AND ONLY reason to add volatile to data structures is in fact poorly written code.

Now, think about that for a minute, and maybe you'll understand why I don't want more volatiles in the kernel.

Linus

PS. This has come up before. The old pre-Alan networking code had "volatile" on just about every single network data structure. Every damn single one of them was a bug. Without exception.

With due respect to Fred and Ross most of them were there because the gcc 2.4.5 compiler was buggy.

Nowdays we still have some volatile users - notably jiffies, and one or two of them make sense, but not many

Alan

This is actually another issue, not really the problem I was talking about. The problem I was talking about was just about the C language and about writing correct C code (whith correct C code I mean something that cannot break by using a future release of gcc). Without my patch you never know. About the loop problem you mentioned you can know it won't break instead if you write it carefully. Of course I understand in most cases if the code breaks in the actual usages of xtime it is likely that gcc is doing something stupid in terms of performance. but GCC if it wants to is allowed to compile this code:

	printf("%lx\n", xtime.tv_sec);

as:

	unsigned long sec = xtime.tv_sec;
	if (sec != xtime.tv_sec)
		BUG();
	printf("%lx\n", sec);

Andrea

And if it does that, it's stupid. Why on earth would GCC add extra code to check if a value hasn't changed? I want it to produce efficient code. What's next? Wrap checking?

    printk ("You've just wrapped an integer: press [ENTER] to confirm,
	    [NT] to ignore   ");

				Regards,

					Richard....
Permanent: [email protected]
Current:   [email protected]

GCC will obviously _never_ introduce a BUG(), I never said that, the above example is only meant to show what GCC is _allowed_ to do and what we have to do to write correct C code.

The real life case of the BUG() is when gcc optimize `case' with a jump table the equivalent of BUG() will be you derferencing a dangling pointer at runtime. Same can happen in other cases but don't ask me the other cases as I'm not a gcc developer and I've no idea what they plans to do for other things (ask Honza for those details).

Andrea

I don't think it should be allowed to do that. That's a whipping offence. I think that example is a red herring.

So grab a local snapshot of the variable, as Linus suggested. In fact, the switch example is interesting, because one could argue the opposite way, that declaring the switch variable as "volatile" means that if GCC needs to internally re-"get" the variable, it should grab it from memory, and thus definately fail. Without "volatile", GCC is implicitely allowed to cache the variable (which is of course safe).

So, really, the switch example requires an "antivolatile" declaration. Oh, wait! I get the same with " ".

				Regards,

					Richard....
Permanent: [email protected]
Current:   [email protected]

"Correct" C code as in portable C code? Standards compliant so it's portable to other compilers?

The linux kernel is compiled on gcc. It always has been. Specific versions of the kernel have a specific range of "known working" gcc versions that in the past have produced a functional result. gcc is -ALLOWED- to compile code the way Borland, Watcom, or Microsoft C compilers do. Assuming somebody wants to rewrite gcc from scratch and still call the result gcc...

Standards are fun, but we don't even thread posixly yet...

Rob

correct C code _mainly_ for gcc which is very aggressive.

Andrea

it is allowed to do that, period. This is not your choice or my choice. You may ask gcc folks not to do that and I think they just do.

If gcc caches there's no problem indeed, the problem is when it doesn't cache it which can happen, with volatile it will understand it must not make assumption for the variable to not change under it. Anyways as just said in another email in this thread I'm been told it wasn't just for 'case'. I think tomorrow Honza will comment this since he's the gcc developer who asked those kernel bugs to be fixed for gcc.

Andrea

Stop this stupid argument.

A C compiler is "allowed" to do just about anything. The introduction of "volatile" does not change that in any really meaningful way. It doesn't change the fact that gcc is "allowed" to do a really shitty job on _any_ code it is given.

	int i = *int_ptr;

into the equivalent of (assuming a little-endian machine with only byte load/store instructions)

	unsigned char *tmp = (unsigned char *)int_ptr + 3;
	int j = 4;
	int i = 0;

	do {
		i <<= 8;
		i += *tmp;
		tmp--;
	} while (--j);

The fact that a C compiler is _allowed_ to create code like just about anything is not an argument at all.

The above, btw, is NOT as ridiculous as it sounds. We've had the exact opposite problem on alpha: byte stores are not "atomic", and would "corrupt" the bytes around it due to the non-atomic nature of having to do

load word
mask value
insert byte
store word

Did it help to mark things "volatile" there? No. We had to change the code to (a) either use locking so that nobody would ever touch adjacent bytes concurrently or (b) stop using byte values.

Linus

I will if Honza assures me that no future version of gcc will cause me to crash if I don't declare xtime volatile and I play with it while it can change under me (which seems not the case from his last email).

Of course I know that this is more a theorical thing and that if we consider that a bug we have also many other bugs of the same kind.

Honza? Do you assure me that? In case you don't, could you suggest another way besides volatile and spinlocks around the access to the variable to avoid gcc to get confused?

Andrea

WHY DO YOU NOT ADD THE "VOLATILE" TO THE PLACES THAT _CARE_?

This is not a gcc issue. Even if gcc _were_ to generate bad code, the global volatile _still_ wouldn't be the correct answer.

Linus

I think his worry is the pedantic reason that without the volatile gcc is allowed to write code that chokes and dies if xtime happens to change in a volatile manner. The example given earlier was:

code as written:

	kprintf("%d\n",xtime.tv_usec);

code as compiled by gcc (with xtime not specified as volatile):

	suseconds_t temp = xtime.tv_usec;
	if (temp != xtime.tv_usec)
		BUG();
	kprintf"%d\n",temp);

I hope that gcc would not do such a thing, but I think a case could be made that it could do it and still comply with the language standard.

Of course, since the linux kernel is an important thing for gcc to compile, I can't imagine them doing something that would break it on purpose without a really good reason.

Chris

Chris Friesen                    | MailStop: 043/33/F10  
Nortel Networks                  | work: (613) 765-0557
3500 Carling Avenue              | fax:  (613) 765-2986
Nepean, ON K2H 8E9 Canada        | email: [email protected]

Looking at the code, it really looks as perfect candidate for volatile. GCC has definitly right to assume that the memory location won't change and use it for optimization. On the other hand, from usual usage of time I guess gcc won't be able to do so, at least not today.

Basically most optimizations comes from that compiler recognizes that value is equivalent to something (constant) at given code path and promotes it futher.

So it is probably up to kernel folks if you want to follow C standard and blame gcc developers for breaking it, or stay in the unsafe ground and fix kernel each time gcc will introduce new nasty optimizations. This may become tricky later - for instance as making kernel codebase -fstrict-aliasing ready.

Honza

Until the C language improves its ability to describe aliasing safety I don't think its going to be practical to fix the issue. I've seen patches to make slhc.c strict aliasing safe for example, and they are too ugly to live

Very aggressive and very known.

If we know how GCC is going to behave in a given situation (and have a fairly good idea of how it's going to behave in the future), we don't have to worry about theoretical stardards loopholes like it calling BUG(), do we?

Is there something in the current behavior that's causing trouble (in which case, what exactly is it), or are the gcc guys (who you mentioned earlier*) warning that the behavior of upcoming versions of gcc is likely to break known constructs in the current kernel?

Rob

(Has the english language officially held a funeral for the word "whom" yet?)

What I was concerned about is for example scenario:
1) cse realizes that given variable is constnat in some region of program (by seeing an conjump).
2) it replaces occurences of other same constants in the program by that variable (as costs says register is cheaper)
3) register allocator runs out of registers
4) reload optimizes and re-loads the variable from original place instead of spilling it.

This way you can effectivly change one constant to another for some region of program.

At the moment gcc rarely exploits knowledge about the variable value, but this is going to improve, as gcc envolve and we are riscing hard to debug problems in kernel.

I am not sure volatile is sensible solution tought.

For instance gcc 3.1 now does load store motion that makes gcc to keep global variables in registers much longer than it did originally making such risk greater.

Honza

Note that just about _every_ single global in the whole kernel is "volatile" in this sense.

In fact, I can't think of a single (interesting) global that cannot be changed by another thread at any time.

What makes this even more interesting, of course, is that with SMP you also have various memory ordering models, so many classic algorithms simply will not work: one classic algorithm is to know about update order and avoid locking by using optimistic algorithms:

	do {
		x = (volatile) value1;
		y = (volatile) value2;
	} while (x != (volatile) value1);

the above is a completely valid algorithm in a strictly ordered system, but it simply WILL NOT WORK on SMP.

How do we handle globals in the kernel? We use locking.

It's really that simple. We _always_ use locking to a first approximation. And the locking code takes care to tell gcc that gcc cannot optimize memory movements of assume that memory is constant.

This is just a basic fact of life.

There are other valid algorithms, but they have to be secondary if only because they are a lot harder to think about and validate.

One is to use memory ordering constraints. Note that in this one "volatile" simply doesn't help. Even with "volatile" maybe (and this is debatable - gcc does not actually have a very good definition of what volatile means even for a purely gcc implementation standpoint) forcing ordering with the compiler, it doesn't force any ordering in the hardware.

In fact, there have been CPU's at least designed that simply will not be guaranteed to pick up changes by another CPU without some kind of memory barrier. On such a CPU it is simply not _possible_ to use

	while ((volatile) jiffies < x)
		/* do nothing */;

because if "jiffies" is updated on another CPU, the first CPU will not necessarily see it AT ALL. Even though the compiler causes the code to re-load the value constantly.

An example of this kind of machine? Imagine a P4 that didn't have some backwards compatibility cruft.. And wonder about some of the reasons that Intel _really_ wants people to start using "rep ; nop" in these kinds of loops.

Do we potentially have these bugs? Yes. In the case of "jiffies", we'd be ok on these kinds of machines simply because all places that do this do so with interrupts enabled (it wouldn't work on UP otherwise), and interrupts would end up working as memory barriers in any case.

Ok, so memory ordering is one valid algorithm. And clearly "volatile" is useless for memory ordering.

Another valid algorithm: "we don't care what the value is, old or new". This is the one most often used for "jiffies". We simply don't care enough. We know that jiffies can change at any time, and the code had better not do anything strange about it anyway. And the code had better be such that even the compiler cannot do anything really strange with it either.

"xtime" simply isn't this kind of value. Why? Because it's not even atomic. It's two 32-bit words, and the operations gcc would use on it aren't things that we could improve with "volatile".

For "xtime", we're ok with (for example) only looking at one of the fields (usually xtime.sec), and just not caring _what_ the value is, because we just copy it into "inode->i_mtime" or something.

But if we have an algorithm that really _does_ care, then "volatile" really doesn't help. See above on memory ordering etc constraints anyway.

I bet that we'll find kernel bugs thanks to it. And I look forward to it, as that is just a sign of gcc doing a better job. And we'll have to make sure that we add the locks or the memory ordering things that we missed.

Looking back a few years, we used to have lots of code of the type

	while (!SCpnt->ready) {
		.. do some waiting ..
	}

and gcc was simply too stupid to notice that it could hoist the load of "SCpnt->ready" outside the loop because the loop did nothing.

Then gcc improved, and for a while we had a _lot_ of "barrier()" calls added. And we've been fine with this ever since.

Maybe we'll need to add some more. Maybe for xtime. Maybe for something we haven't even thought about. Bugs are nothing new, and nothing to be really afraid of.

Linus

Thanks, it's in my queue.

Although I think I'll hold off on the xtime change until you and Linus have decided whether it needs volatile or locking :-)

Jeff

here another one for reiserfs:

@@ -828,7 +828,7 @@
 
 /* compose directory item containing "." and ".." entries (entries are
    not aligned to 4 byte boundary) */
-extern inline void make_empty_dir_item_v1 (char * body, __u32 dirid, __u32 objid,
+static inline void make_empty_dir_item_v1 (char * body, __u32 dirid, __u32 objid,
 					   __u32 par_dirid, __u32 par_objid)
 {
     struct reiserfs_de_head * deh;

@@ -859,7 +859,7 @@
 }
 
 /* compose directory item containing "." and ".." entries */
-extern inline void make_empty_dir_item (char * body, __u32 dirid, __u32 objid,
+static inline void make_empty_dir_item (char * body, __u32 dirid, __u32 objid,
 					__u32 par_dirid, __u32 par_objid)
 {
     struct reiserfs_de_head * deh;

Andrea

here another one:

@@ -492,7 +492,7 @@
 
 #define RCV_SKB_FAIL(err) do { netlink_ack(skb, nlh, (err)); return; } while (0);
 
-extern __inline__ void netlink_receive_user_skb(struct sk_buff *skb)
+static __inline__ void netlink_receive_user_skb(struct sk_buff *skb)
 {
 	int status, type;
 	struct nlmsghdr *nlh;

Andrea

another one:

@@ -1435,7 +1435,7 @@
 	ARLAN_DEBUG_EXIT("arlan_queue_retransmit");
 };
 
-extern inline void RetryOrFail(struct net_device *dev)
+static inline void RetryOrFail(struct net_device *dev)
 {
 	struct arlan_private *priv = ((struct arlan_private *) dev->priv);
 

Andrea