* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul E. McKenney @ 2007-08-16 0:49 UTC (permalink / raw)
To: Herbert Xu
Cc: David Howells, Satyam Sharma, Stefan Richter, Christoph Lameter,
Chris Snook, Linux Kernel Mailing List, linux-arch,
Linus Torvalds, netdev, Andrew Morton, ak, heiko.carstens, davem,
schwidefsky, wensong, horms, wjiang, cfriesen, zlynx, rpjday,
jesper.juhl, segher
In-Reply-To: <20070816003023.GA29447@gondor.apana.org.au>
On Thu, Aug 16, 2007 at 08:30:23AM +0800, Herbert Xu wrote:
> On Wed, Aug 15, 2007 at 05:23:10PM -0700, Paul E. McKenney wrote:
> > On Thu, Aug 16, 2007 at 08:12:48AM +0800, Herbert Xu wrote:
> > > On Wed, Aug 15, 2007 at 04:53:35PM -0700, Paul E. McKenney wrote:
> > > >
> > > > > > Communicating between process context and interrupt/NMI handlers using
> > > > > > per-CPU variables.
> > > > >
> > > > > Remeber we're talking about atomic_read/atomic_set. Please
> > > > > cite the actual file/function name you have in mind.
> > > >
> > > > Yep, we are indeed talking about atomic_read()/atomic_set().
> > > >
> > > > We have been through this issue already in this thread.
> > >
> > > Sorry, but I must've missed it. Could you cite the file or
> > > function for my benefit?
> >
> > I might summarize the thread if there is interest, but I am not able to
> > do so right this minute.
>
> Thanks. But I don't need a summary of the thread, I'm asking
> for an extant code snippet in our kernel that benefits from
> the volatile change and is not part of a busy-wait.
Sorry, can't help you there. I really do believe that the information
you need (as opposed to the specific item you are asking for) really
has been put forth in this thread.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Satyam Sharma @ 2007-08-16 0:58 UTC (permalink / raw)
To: Herbert Xu
Cc: Segher Boessenkool, horms, Stefan Richter,
Linux Kernel Mailing List, Paul E. McKenney, ak, netdev, cfriesen,
Heiko Carstens, rpjday, jesper.juhl, linux-arch, Andrew Morton,
zlynx, clameter, schwidefsky, Chris Snook, davem, Linus Torvalds,
wensong, wjiang
In-Reply-To: <20070816003226.GA29491@gondor.apana.org.au>
[ Sorry for empty subject line in previous mail. I intended to make
a patch so cleared it to change it, but ultimately neither made
a patch nor restored subject line. Done that now. ]
On Thu, 16 Aug 2007, Herbert Xu wrote:
> On Thu, Aug 16, 2007 at 06:06:00AM +0530, Satyam Sharma wrote:
> >
> > that are:
> >
> > while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {
> > mdelay(1);
> > msecs--;
> > }
> >
> > where mdelay() becomes __const_udelay() which happens to be in another
> > translation unit (arch/i386/lib/delay.c) and hence saves this callsite
> > from being a bug :-)
>
> The udelay itself certainly should have some form of cpu_relax in it.
Yes, a form of barrier() must be present in mdelay() or udelay() itself
as you say, having it in __const_udelay() is *not* enough (superflous
actually, considering it is already a separate translation unit and
invisible to the compiler).
However, there are no compiler barriers on the macro-definition-path
between mdelay(1) and __const_udelay(), so the only thing that saves us
from being a bug here is indeed the different-translation-unit concept.
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Christoph Lameter @ 2007-08-16 0:42 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Paul Mackerras, Satyam Sharma, Stefan Richter, Chris Snook,
Linux Kernel Mailing List, linux-arch, Linus Torvalds, netdev,
Andrew Morton, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher,
Herbert Xu
In-Reply-To: <20070816003948.GY9645@linux.vnet.ibm.com>
On Wed, 15 Aug 2007, Paul E. McKenney wrote:
> Seems to me that we face greater chance of confusion without the
> volatile than with, particularly as compiler optimizations become
> more aggressive. Yes, we could simply disable optimization, but
> optimization can be quite helpful.
A volatile default would disable optimizations for atomic_read.
atomic_read without volatile would allow for full optimization by the
compiler. Seems that this is what one wants in many cases.
^ permalink raw reply
* Re: drivers/infiniband/mlx/mad.c misplaced ;
From: Joe Perches @ 2007-08-16 0:40 UTC (permalink / raw)
To: Dave Jones
Cc: Linux Kernel, rolandd, Chas Williams, Paul Mundt, isdn4linux,
mikep, netdev, swen, linux390, linux-s390, jdike,
user-mode-linux-devel, user-mode-linux-user, netfilter-devel,
coreteam
In-Reply-To: <20070815235825.GB17004@redhat.com>
On Wed, 2007-08-15 at 19:58 -0400, Dave Jones wrote:
> Signed-off-by: Dave Jones <davej@redhat.com>
>
> diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
> index 3330917..0ed02b7 100644
> --- a/drivers/infiniband/hw/mlx4/mad.c
> +++ b/drivers/infiniband/hw/mlx4/mad.c
> @@ -109,7 +109,7 @@ int mlx4_MAD_IFC(struct mlx4_ib_dev *dev, int ignore_mkey, int ignore_bkey,
> in_modifier, op_modifier,
> MLX4_CMD_MAD_IFC, MLX4_CMD_TIME_CLASS_C);
>
> - if (!err);
> + if (!err)
There's more than a few of these (not inspected).
$ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" *
arch/sh/boards/se/7343/io.c: if (0) ;
drivers/atm/iphase.c: if (!desc1) ;
drivers/infiniband/hw/mlx4/mad.c: if (!err);
drivers/isdn/capi/capiutil.c: else if (c <= 0x0f);
drivers/net/tokenring/ibmtr.c: else if (ti->shared_ram_paging == 0xf); /* No paging in adapter */
drivers/s390/scsi/zfcp_erp.c: if (status == ZFCP_ERP_SUCCEEDED) ; /* no further action */
fs/hostfs/hostfs_user.c: if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
net/netfilter/xt_u32.c: if (skb->len < 4 || pos > skb->len - 4);
sound/pci/au88x0/au88x0_synth.c: if (eax == 0) ;
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Christoph Lameter @ 2007-08-16 0:40 UTC (permalink / raw)
To: Paul Mackerras
Cc: Satyam Sharma, Stefan Richter, Chris Snook,
Linux Kernel Mailing List, linux-arch, Linus Torvalds, netdev,
Andrew Morton, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher,
Herbert Xu, Paul E. McKenney
In-Reply-To: <18115.39815.730873.346526@cargo.ozlabs.ibm.com>
On Thu, 16 Aug 2007, Paul Mackerras wrote:
> Those barriers are for when we need ordering between atomic variables
> and other memory locations. An atomic variable by itself doesn't and
> shouldn't need any barriers for other CPUs to be able to see what's
> happening to it.
It does not need any barriers. As soon as one cpu acquires the
cacheline for write it will be invalidated in the caches of the others. So
the other cpu will have to refetch. No need for volatile.
The issue here may be that the compiler has fetched the atomic variable
earlier and put it into a register. However, that prefetching is limited
because it cannot cross functions calls etc. The only problem could be
loops where the compiler does not refetch the variable since it assumes
that it does not change and there are no function calls in the body of the
loop. But AFAIK these loops need cpu_relax and other measures anyways to
avoid bad effects from busy waiting.
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul E. McKenney @ 2007-08-16 0:39 UTC (permalink / raw)
To: Christoph Lameter
Cc: Paul Mackerras, Satyam Sharma, Stefan Richter, Chris Snook,
Linux Kernel Mailing List, linux-arch, Linus Torvalds, netdev,
Andrew Morton, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher,
Herbert Xu
In-Reply-To: <Pine.LNX.4.64.0708151725130.10497@schroedinger.engr.sgi.com>
On Wed, Aug 15, 2007 at 05:26:34PM -0700, Christoph Lameter wrote:
> On Thu, 16 Aug 2007, Paul Mackerras wrote:
>
> > In the kernel we use atomic variables in precisely those situations
> > where a variable is potentially accessed concurrently by multiple
> > CPUs, and where each CPU needs to see updates done by other CPUs in a
> > timely fashion. That is what they are for. Therefore the compiler
> > must not cache values of atomic variables in registers; each
> > atomic_read must result in a load and each atomic_set must result in a
> > store. Anything else will just lead to subtle bugs.
>
> This may have been the intend. However, today the visibility is controlled
> using barriers. And we have barriers that we use with atomic operations.
> Having volatile be the default just lead to confusion. Atomic read should
> just read with no extras. Extras can be added by using variants like
> atomic_read_volatile or so.
Seems to me that we face greater chance of confusion without the
volatile than with, particularly as compiler optimizations become
more aggressive. Yes, we could simply disable optimization, but
optimization can be quite helpful.
Thanx, Paul
^ permalink raw reply
* (unknown)
From: Satyam Sharma @ 2007-08-16 0:36 UTC (permalink / raw)
To: Segher Boessenkool
Cc: horms, Stefan Richter, Linux Kernel Mailing List,
Paul E. McKenney, ak, netdev, cfriesen, Heiko Carstens, rpjday,
jesper.juhl, linux-arch, Andrew Morton, zlynx, clameter,
schwidefsky, Chris Snook, Herbert Xu, davem, Linus Torvalds,
wensong, wjiang
In-Reply-To: <2d2eeab6276cab2e6cc5830d36a43b98@kernel.crashing.org>
On Wed, 15 Aug 2007, Segher Boessenkool wrote:
> > > > What you probably mean is that the compiler has to assume any code
> > > > it cannot currently see can do anything (insofar as allowed by the
> > > > relevant standards etc.)
> >
> > I think this was just terminology confusion here again. Isn't "any code
> > that it cannot currently see" the same as "another compilation unit",
>
> It is not; try gcc -combine or the upcoming link-time optimisation
> stuff, for example.
>
> > and wouldn't the "compilation unit" itself expand if we ask gcc to
> > compile more than one unit at once? Or is there some more specific
> > "definition" for "compilation unit" (in gcc lingo, possibly?)
>
> "compilation unit" is a C standard term. It typically boils down
> to "single .c file".
As you mentioned later, "single .c file with all the other files (headers
or other .c files) that it pulls in via #include" is actually "translation
unit", both in the C standard as well as gcc docs. "Compilation unit"
doesn't seem to be nearly as standard a term, though in most places it
is indeed meant to be same as "translation unit", but with the new gcc
inter-module-analysis stuff that you referred to above, I suspect one may
reasonably want to call a "compilation unit" as all that the compiler sees
at a given instant.
BTW I did some auditing (only inside include/asm-{i386,x86_64}/ and
arch/{i386,x86_64}/) and found a couple more callsites that don't use
cpu_relax():
arch/i386/kernel/crash.c:101
arch/x86_64/kernel/crash.c:97
that are:
while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {
mdelay(1);
msecs--;
}
where mdelay() becomes __const_udelay() which happens to be in another
translation unit (arch/i386/lib/delay.c) and hence saves this callsite
from being a bug :-)
Curiously, __const_udelay() is still marked as "inline" where it is
implemented in lib/delay.c which is weird, considering it won't ever
be inlined, would it? With the kernel presently being compiled one
translation unit at a time, I don't see how the implementation would
be visible to any callsite out there to be able to inline it.
Satyam
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul Mackerras @ 2007-08-16 0:34 UTC (permalink / raw)
To: Christoph Lameter
Cc: Satyam Sharma, Stefan Richter, Chris Snook,
Linux Kernel Mailing List, linux-arch, Linus Torvalds, netdev,
Andrew Morton, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher,
Herbert Xu, Paul E. McKenney
In-Reply-To: <Pine.LNX.4.64.0708151725130.10497@schroedinger.engr.sgi.com>
Christoph Lameter writes:
> On Thu, 16 Aug 2007, Paul Mackerras wrote:
>
> > In the kernel we use atomic variables in precisely those situations
> > where a variable is potentially accessed concurrently by multiple
> > CPUs, and where each CPU needs to see updates done by other CPUs in a
> > timely fashion. That is what they are for. Therefore the compiler
> > must not cache values of atomic variables in registers; each
> > atomic_read must result in a load and each atomic_set must result in a
> > store. Anything else will just lead to subtle bugs.
>
> This may have been the intend. However, today the visibility is controlled
> using barriers. And we have barriers that we use with atomic operations.
Those barriers are for when we need ordering between atomic variables
and other memory locations. An atomic variable by itself doesn't and
shouldn't need any barriers for other CPUs to be able to see what's
happening to it.
Paul.
^ permalink raw reply
* Re: your mail
From: Herbert Xu @ 2007-08-16 0:32 UTC (permalink / raw)
To: Satyam Sharma
Cc: Segher Boessenkool, horms, Stefan Richter,
Linux Kernel Mailing List, Paul E. McKenney, ak, netdev, cfriesen,
Heiko Carstens, rpjday, jesper.juhl, linux-arch, Andrew Morton,
zlynx, clameter, schwidefsky, Chris Snook, davem, Linus Torvalds,
wensong, wjiang
In-Reply-To: <alpine.LFD.0.999.0708160338520.22701@enigma.security.iitk.ac.in>
On Thu, Aug 16, 2007 at 06:06:00AM +0530, Satyam Sharma wrote:
>
> that are:
>
> while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {
> mdelay(1);
> msecs--;
> }
>
> where mdelay() becomes __const_udelay() which happens to be in another
> translation unit (arch/i386/lib/delay.c) and hence saves this callsite
> from being a bug :-)
The udelay itself certainly should have some form of cpu_relax in it.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Herbert Xu @ 2007-08-16 0:30 UTC (permalink / raw)
To: Paul E. McKenney
Cc: David Howells, Satyam Sharma, Stefan Richter, Christoph Lameter,
Chris Snook, Linux Kernel Mailing List, linux-arch,
Linus Torvalds, netdev, Andrew Morton, ak, heiko.carstens, davem,
schwidefsky, wensong, horms, wjiang, cfriesen, zlynx, rpjday,
jesper.juhl, segher
In-Reply-To: <20070816002310.GW9645@linux.vnet.ibm.com>
On Wed, Aug 15, 2007 at 05:23:10PM -0700, Paul E. McKenney wrote:
> On Thu, Aug 16, 2007 at 08:12:48AM +0800, Herbert Xu wrote:
> > On Wed, Aug 15, 2007 at 04:53:35PM -0700, Paul E. McKenney wrote:
> > >
> > > > > Communicating between process context and interrupt/NMI handlers using
> > > > > per-CPU variables.
> > > >
> > > > Remeber we're talking about atomic_read/atomic_set. Please
> > > > cite the actual file/function name you have in mind.
> > >
> > > Yep, we are indeed talking about atomic_read()/atomic_set().
> > >
> > > We have been through this issue already in this thread.
> >
> > Sorry, but I must've missed it. Could you cite the file or
> > function for my benefit?
>
> I might summarize the thread if there is interest, but I am not able to
> do so right this minute.
Thanks. But I don't need a summary of the thread, I'm asking
for an extant code snippet in our kernel that benefits from
the volatile change and is not part of a busy-wait.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH] i386: Fix a couple busy loops in mach_wakecpu.h:wait_for_init_deassert()
From: Satyam Sharma @ 2007-08-16 0:39 UTC (permalink / raw)
To: Heiko Carstens
Cc: Herbert Xu, Chris Snook, clameter, Linux Kernel Mailing List,
linux-arch, Linus Torvalds, netdev, Andrew Morton, ak, davem,
schwidefsky, wensong, horms, wjiang, cfriesen, zlynx, rpjday,
jesper.juhl, segher
In-Reply-To: <20070815081841.GA16551@osiris.boeblingen.de.ibm.com>
On Wed, 15 Aug 2007, Heiko Carstens wrote:
> [...]
> Btw.: we still have
>
> include/asm-i386/mach-es7000/mach_wakecpu.h: while (!atomic_read(deassert));
> include/asm-i386/mach-default/mach_wakecpu.h: while (!atomic_read(deassert));
>
> Looks like they need to be fixed as well.
[PATCH] i386: Fix a couple busy loops in mach_wakecpu.h:wait_for_init_deassert()
Use cpu_relax() in the busy loops, as atomic_read() doesn't automatically
imply volatility for i386 and x86_64. x86_64 doesn't have this issue because
it open-codes the while loop in smpboot.c:smp_callin() itself that already
uses cpu_relax().
For i386, however, smpboot.c:smp_callin() calls wait_for_init_deassert()
which is buggy for mach-default and mach-es7000 cases.
[ I test-built a kernel -- smp_callin() itself got inlined in its only
callsite, smpboot.c:start_secondary() -- and the relevant piece of
code disassembles to the following:
0xc1019704 <start_secondary+12>: mov 0xc144c4c8,%eax
0xc1019709 <start_secondary+17>: test %eax,%eax
0xc101970b <start_secondary+19>: je 0xc1019709 <start_secondary+17>
init_deasserted (at 0xc144c4c8) gets fetched into %eax only once and
then we loop over the test of the stale value in the register only,
so these look like real bugs to me. With the fix below, this becomes:
0xc1019706 <start_secondary+14>: pause
0xc1019708 <start_secondary+16>: cmpl $0x0,0xc144c4c8
0xc101970f <start_secondary+23>: je 0xc1019706 <start_secondary+14>
which looks nice and healthy. ]
Thanks to Heiko Carstens for noticing this.
Signed-off-by: Satyam Sharma <satyam@infradead.org>
---
include/asm-i386/mach-default/mach_wakecpu.h | 3 ++-
include/asm-i386/mach-es7000/mach_wakecpu.h | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/asm-i386/mach-default/mach_wakecpu.h b/include/asm-i386/mach-default/mach_wakecpu.h
index 673b85c..3ebb178 100644
--- a/include/asm-i386/mach-default/mach_wakecpu.h
+++ b/include/asm-i386/mach-default/mach_wakecpu.h
@@ -15,7 +15,8 @@
static inline void wait_for_init_deassert(atomic_t *deassert)
{
- while (!atomic_read(deassert));
+ while (!atomic_read(deassert))
+ cpu_relax();
return;
}
diff --git a/include/asm-i386/mach-es7000/mach_wakecpu.h b/include/asm-i386/mach-es7000/mach_wakecpu.h
index efc903b..84ff583 100644
--- a/include/asm-i386/mach-es7000/mach_wakecpu.h
+++ b/include/asm-i386/mach-es7000/mach_wakecpu.h
@@ -31,7 +31,8 @@ wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
static inline void wait_for_init_deassert(atomic_t *deassert)
{
#ifdef WAKE_SECONDARY_VIA_INIT
- while (!atomic_read(deassert));
+ while (!atomic_read(deassert))
+ cpu_relax();
#endif
return;
}
^ permalink raw reply related
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Christoph Lameter @ 2007-08-16 0:26 UTC (permalink / raw)
To: Paul Mackerras
Cc: Satyam Sharma, Stefan Richter, Chris Snook,
Linux Kernel Mailing List, linux-arch, Linus Torvalds, netdev,
Andrew Morton, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher,
Herbert Xu, Paul E. McKenney
In-Reply-To: <18115.35524.56393.347841@cargo.ozlabs.ibm.com>
On Thu, 16 Aug 2007, Paul Mackerras wrote:
> In the kernel we use atomic variables in precisely those situations
> where a variable is potentially accessed concurrently by multiple
> CPUs, and where each CPU needs to see updates done by other CPUs in a
> timely fashion. That is what they are for. Therefore the compiler
> must not cache values of atomic variables in registers; each
> atomic_read must result in a load and each atomic_set must result in a
> store. Anything else will just lead to subtle bugs.
This may have been the intend. However, today the visibility is controlled
using barriers. And we have barriers that we use with atomic operations.
Having volatile be the default just lead to confusion. Atomic read should
just read with no extras. Extras can be added by using variants like
atomic_read_volatile or so.
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul E. McKenney @ 2007-08-16 0:23 UTC (permalink / raw)
To: Herbert Xu
Cc: David Howells, Satyam Sharma, Stefan Richter, Christoph Lameter,
Chris Snook, Linux Kernel Mailing List, linux-arch,
Linus Torvalds, netdev, Andrew Morton, ak, heiko.carstens, davem,
schwidefsky, wensong, horms, wjiang, cfriesen, zlynx, rpjday,
jesper.juhl, segher
In-Reply-To: <20070816001248.GA29214@gondor.apana.org.au>
On Thu, Aug 16, 2007 at 08:12:48AM +0800, Herbert Xu wrote:
> On Wed, Aug 15, 2007 at 04:53:35PM -0700, Paul E. McKenney wrote:
> >
> > > > Communicating between process context and interrupt/NMI handlers using
> > > > per-CPU variables.
> > >
> > > Remeber we're talking about atomic_read/atomic_set. Please
> > > cite the actual file/function name you have in mind.
> >
> > Yep, we are indeed talking about atomic_read()/atomic_set().
> >
> > We have been through this issue already in this thread.
>
> Sorry, but I must've missed it. Could you cite the file or
> function for my benefit?
I might summarize the thread if there is interest, but I am not able to
do so right this minute.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Herbert Xu @ 2007-08-16 0:12 UTC (permalink / raw)
To: Paul E. McKenney
Cc: David Howells, Satyam Sharma, Stefan Richter, Christoph Lameter,
Chris Snook, Linux Kernel Mailing List, linux-arch,
Linus Torvalds, netdev, Andrew Morton, ak, heiko.carstens, davem,
schwidefsky, wensong, horms, wjiang, cfriesen, zlynx, rpjday,
jesper.juhl, segher
In-Reply-To: <20070815235335.GU9645@linux.vnet.ibm.com>
On Wed, Aug 15, 2007 at 04:53:35PM -0700, Paul E. McKenney wrote:
>
> > > Communicating between process context and interrupt/NMI handlers using
> > > per-CPU variables.
> >
> > Remeber we're talking about atomic_read/atomic_set. Please
> > cite the actual file/function name you have in mind.
>
> Yep, we are indeed talking about atomic_read()/atomic_set().
>
> We have been through this issue already in this thread.
Sorry, but I must've missed it. Could you cite the file or
function for my benefit?
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul E. McKenney @ 2007-08-15 23:53 UTC (permalink / raw)
To: Herbert Xu
Cc: David Howells, Satyam Sharma, Stefan Richter, Christoph Lameter,
Chris Snook, Linux Kernel Mailing List, linux-arch,
Linus Torvalds, netdev, Andrew Morton, ak, heiko.carstens, davem,
schwidefsky, wensong, horms, wjiang, cfriesen, zlynx, rpjday,
jesper.juhl, segher
In-Reply-To: <20070815234146.GB28775@gondor.apana.org.au>
On Thu, Aug 16, 2007 at 07:41:46AM +0800, Herbert Xu wrote:
> On Wed, Aug 15, 2007 at 11:45:20AM -0700, Paul E. McKenney wrote:
> > On Wed, Aug 15, 2007 at 07:19:57PM +0100, David Howells wrote:
> > > Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >
> > > > Let's turn this around. Can you give a single example where
> > > > the volatile semantics is needed in a legitimate way?
> > >
> > > Accessing H/W registers? But apart from that...
> >
> > Communicating between process context and interrupt/NMI handlers using
> > per-CPU variables.
>
> Remeber we're talking about atomic_read/atomic_set. Please
> cite the actual file/function name you have in mind.
Yep, we are indeed talking about atomic_read()/atomic_set().
We have been through this issue already in this thread.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH] [IPv6]: Invalid semicolon after if statement
From: Dave Jones @ 2007-08-15 23:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20070815.150814.82381935.davem@davemloft.net>
On Wed, Aug 15, 2007 at 03:08:14PM -0700, David Miller wrote:
> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Thu, 16 Aug 2007 00:57:00 +0300 (EEST)
>
> > A similar fix to netfilter from Eric Dumazet inspired me to
> > look around a bit by using some grep/sed stuff as looking for
> > this kind of bugs seemed easy to automate. This is one of them
> > I found where it looks like this semicolon is not valid.
> >
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
>
> Yikes! Makes you want to audit the entire tree for these
> things :-)))
Indeed. Here's another one.
Signed-off-by: Dave Jones <davej@redhat.com>
diff --git a/net/netfilter/xt_u32.c b/net/netfilter/xt_u32.c
index 74f9b14..bec4279 100644
--- a/net/netfilter/xt_u32.c
+++ b/net/netfilter/xt_u32.c
@@ -36,7 +36,7 @@ static bool u32_match_it(const struct xt_u32 *data,
at = 0;
pos = ct->location[0].number;
- if (skb->len < 4 || pos > skb->len - 4);
+ if (skb->len < 4 || pos > skb->len - 4)
return false;
ret = skb_copy_bits(skb, pos, &n, sizeof(n));
--
http://www.codemonkey.org.uk
^ permalink raw reply related
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul E. McKenney @ 2007-08-15 23:51 UTC (permalink / raw)
To: Herbert Xu
Cc: Chris Snook, satyam, clameter, linux-kernel, linux-arch, torvalds,
netdev, akpm, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher
In-Reply-To: <20070815234021.GA28775@gondor.apana.org.au>
On Thu, Aug 16, 2007 at 07:40:21AM +0800, Herbert Xu wrote:
> On Wed, Aug 15, 2007 at 12:13:12PM -0400, Chris Snook wrote:
> >
> > Part of the motivation here is to fix heisenbugs. If I knew where they
>
> By the same token we should probably disable optimisations
> altogether since that too can create heisenbugs.
Precisely the point -- use of volatile (whether in casts or on asms)
in these cases are intended to disable those optimizations likely to
result in heisenbugs. But they are also intended to leave other
valuable optimizations in force.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Herbert Xu @ 2007-08-15 23:41 UTC (permalink / raw)
To: Paul E. McKenney
Cc: David Howells, Satyam Sharma, Stefan Richter, Christoph Lameter,
Chris Snook, Linux Kernel Mailing List, linux-arch,
Linus Torvalds, netdev, Andrew Morton, ak, heiko.carstens, davem,
schwidefsky, wensong, horms, wjiang, cfriesen, zlynx, rpjday,
jesper.juhl, segher
In-Reply-To: <20070815184520.GG9645@linux.vnet.ibm.com>
On Wed, Aug 15, 2007 at 11:45:20AM -0700, Paul E. McKenney wrote:
> On Wed, Aug 15, 2007 at 07:19:57PM +0100, David Howells wrote:
> > Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > > Let's turn this around. Can you give a single example where
> > > the volatile semantics is needed in a legitimate way?
> >
> > Accessing H/W registers? But apart from that...
>
> Communicating between process context and interrupt/NMI handlers using
> per-CPU variables.
Remeber we're talking about atomic_read/atomic_set. Please
cite the actual file/function name you have in mind.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Herbert Xu @ 2007-08-15 23:40 UTC (permalink / raw)
To: Chris Snook
Cc: satyam, clameter, linux-kernel, linux-arch, torvalds, netdev,
akpm, ak, heiko.carstens, davem, schwidefsky, wensong, horms,
wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher
In-Reply-To: <46C32618.2080108@redhat.com>
On Wed, Aug 15, 2007 at 12:13:12PM -0400, Chris Snook wrote:
>
> Part of the motivation here is to fix heisenbugs. If I knew where they
By the same token we should probably disable optimisations
altogether since that too can create heisenbugs.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul Mackerras @ 2007-08-15 23:22 UTC (permalink / raw)
To: Satyam Sharma
Cc: Stefan Richter, Christoph Lameter, Chris Snook,
Linux Kernel Mailing List, linux-arch, Linus Torvalds, netdev,
Andrew Morton, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl, segher,
Herbert Xu, Paul E. McKenney
In-Reply-To: <alpine.LFD.0.999.0708151713080.16414@enigma.security.iitk.ac.in>
Satyam Sharma writes:
> > Doesn't "atomic WRT all processors" require volatility?
>
> No, it definitely doesn't. Why should it?
>
> "Atomic w.r.t. all processors" is just your normal, simple "atomicity"
> for SMP systems (ensure that that object is modified / set / replaced
> in main memory atomically) and has nothing to do with "volatile"
> behaviour.
Atomic variables are "volatile" in the sense that they are liable to
be changed at any time by mechanisms that are outside the knowledge of
the C compiler, namely, other CPUs, or this CPU executing an interrupt
routine.
In the kernel we use atomic variables in precisely those situations
where a variable is potentially accessed concurrently by multiple
CPUs, and where each CPU needs to see updates done by other CPUs in a
timely fashion. That is what they are for. Therefore the compiler
must not cache values of atomic variables in registers; each
atomic_read must result in a load and each atomic_set must result in a
store. Anything else will just lead to subtle bugs.
I have no strong opinion about whether or not the best way to achieve
this is through the use of the "volatile" C keyword. Segher's idea of
using asm instead seems like a good one to me.
Paul.
^ permalink raw reply
* Re: Please pull 'upstream-davem' branch of wireless-2.6
From: David Miller @ 2007-08-15 23:09 UTC (permalink / raw)
To: linville-2XuSBdqkA4R54TAoqtyWWQ
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20070815003410.GJ7198-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
From: "John W. Linville" <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Tue, 14 Aug 2007 20:34:10 -0400
> Individual patches available here:
>
> http://www.kernel.org/pub/linux/kernel/people/linville/wireless-2.6/upstream-davem
John, what I'm going to do is wait for Linus to pull in the
2.6.23 mac80211 fixes you submitted yesterday, then rebase
the net-2.6.24 tree and add these fixes on top.
Take care.
^ permalink raw reply
* Re: [PATCH for 2.6.24] SCTP: Rewrite of sctp buffer management code
From: David Miller @ 2007-08-15 23:08 UTC (permalink / raw)
To: vladislav.yasevich; +Cc: netdev, lksctp-developers, nhorman
In-Reply-To: <11870114401993-git-send-email-vladislav.yasevich@hp.com>
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Mon, 13 Aug 2007 09:24:00 -0400
> Sorry about that. Not sure what happened to that patch. Here is
> the good patch with witespace cleanups.
Applied to net-2.6.24, thanks for fixing this patch up.
^ permalink raw reply
* Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls
From: David Miller @ 2007-08-15 23:05 UTC (permalink / raw)
To: jeff; +Cc: netdev, linux-kernel
In-Reply-To: <46BCD0F1.8040006@garzik.org>
From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 10 Aug 2007 16:56:17 -0400
> All this is currently checked into the 'eflags' branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git
>
> But when everybody is happy with it, IMO we should get it into
> net-2.6.24.git, as it enables LRO.
I've backed out the ETHTOOL LRO stuff from Auke, and applied your
patches to net-2.6.24
We can get rid of that NETIF_F_LRO thing, since as you observed
it is indeed superfluous.
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul E. McKenney @ 2007-08-15 22:44 UTC (permalink / raw)
To: Segher Boessenkool
Cc: horms, Stefan Richter, Satyam Sharma, Linux Kernel Mailing List,
rpjday, netdev, ak, cfriesen, Heiko Carstens, jesper.juhl,
linux-arch, Andrew Morton, zlynx, clameter, schwidefsky,
Chris Snook, Herbert Xu, davem, Linus Torvalds, wensong, wjiang
In-Reply-To: <2c907d70a2267b887de346891758983d@kernel.crashing.org>
On Wed, Aug 15, 2007 at 11:05:35PM +0200, Segher Boessenkool wrote:
> >>No; compilation units have nothing to do with it, GCC can optimise
> >>across compilation unit boundaries just fine, if you tell it to
> >>compile more than one compilation unit at once.
> >
> >Last I checked, the Linux kernel build system did compile each .c file
> >as a separate compilation unit.
>
> I have some patches to use -combine -fwhole-program for Linux.
> Highly experimental, you need a patched bleeding edge toolchain.
> If there's interest I'll clean it up and put it online.
>
> David Woodhouse had some similar patches about a year ago.
Sounds exciting... ;-)
> >>>In many cases, the compiler also has to assume that
> >>>msleep_interruptible()
> >>>might call back into a function in the current compilation unit, thus
> >>>possibly modifying global static variables.
> >>
> >>It most often is smart enough to see what compilation-unit-local
> >>variables might be modified that way, though :-)
> >
> >Yep. For example, if it knows the current value of a given such local
> >variable, and if all code paths that would change some other variable
> >cannot be reached given that current value of the first variable.
>
> Or the most common thing: if neither the address of the translation-
> unit local variable nor the address of any function writing to that
> variable can "escape" from that translation unit, nothing outside
> the translation unit can write to the variable.
But there is usually at least one externally callable function in
a .c file.
> >At least given that gcc doesn't know about multiple threads of
> >execution!
>
> Heh, only about the threads it creates itself (not relevant to
> the kernel, for sure :-) )
;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Paul E. McKenney @ 2007-08-15 22:42 UTC (permalink / raw)
To: Segher Boessenkool
Cc: horms, Stefan Richter, Satyam Sharma, Linux Kernel Mailing List,
rpjday, netdev, ak, cfriesen, Heiko Carstens, jesper.juhl,
linux-arch, Andrew Morton, zlynx, clameter, schwidefsky,
Chris Snook, Herbert Xu, davem, Linus Torvalds, wensong, wjiang
In-Reply-To: <96f3fa3929d873263891fefadb222431@kernel.crashing.org>
On Wed, Aug 15, 2007 at 10:52:53PM +0200, Segher Boessenkool wrote:
> >>I think this was just terminology confusion here again. Isn't "any
> >>code
> >>that it cannot currently see" the same as "another compilation unit",
> >>and wouldn't the "compilation unit" itself expand if we ask gcc to
> >>compile more than one unit at once? Or is there some more specific
> >>"definition" for "compilation unit" (in gcc lingo, possibly?)
> >
> >This is indeed my understanding -- "compilation unit" is whatever the
> >compiler looks at in one go. I have heard the word "module" used for
> >the minimal compilation unit covering a single .c file and everything
> >that it #includes, but there might be a better name for this.
>
> Yes, that's what's called "compilation unit" :-)
>
> [/me double checks]
>
> Erm, the C standard actually calls it "translation unit".
>
> To be exact, to avoid any more confusion:
>
> 5.1.1.1/1:
> A C program need not all be translated at the same time. The
> text of the program is kept in units called source files, (or
> preprocessing files) in this International Standard. A source
> file together with all the headers and source files included
> via the preprocessing directive #include is known as a
> preprocessing translation unit. After preprocessing, a
> preprocessing translation unit is called a translation unit.
I am OK with "translation" and "compilation" being near-synonyms. ;-)
Thanx, Paul
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox