* Re: [PATCH RFC]: napi_struct V5
From: Roland Dreier @ 2007-08-09 17:49 UTC (permalink / raw)
To: David Miller; +Cc: xma, hadi, jgarzik, netdev, netdev-owner, rusty, shemminger
In-Reply-To: <20070808.152059.101495015.davem@davemloft.net>
> > Dave, could you please hold this portion of the patch for a moment. I will
> > test this patch ASAP. According to our previous experience, this changes
> > significant changes some IPoIB driver performance.
>
> I reverted everything Roland had an issue with, I got tired of arguing
> my position and doing all of the coding too. He won.
Why does this have to become a contest of wills where someone wins? I
wish we could just have a technical discussion and get to the best
solution. You posted an RFC, and I commented on the parts where I
believe I have some expertise. We're all on the same side here.
Shirley, I think it would still be useful to run benchmarks of IPoIB
on ehca with Dave's NAPI patches, both V5 that changed the "missed
event" behavior and V6 that didn't. At least I'm curious to know how
much the difference is.
- R.
^ permalink raw reply
* Re: [PATCH] make atomic_t volatile on all architectures
From: Chuck Ebbert @ 2007-08-09 17:36 UTC (permalink / raw)
To: Chris Snook
Cc: Linus Torvalds, akpm, ak, heiko.carstens, davem, linux-kernel,
netdev, schwidefsky, wensong, horms, wjiang, cfriesen, zlynx
In-Reply-To: <46BAC2BE.1090106@redhat.com>
On 08/09/2007 03:31 AM, Chris Snook wrote:
>
> Fair enough. Casting to (volatile int *) will give us the behavior
> people expect when using atomic_t without needing to use inefficient
> barriers.
>
You can use this forget() macro to make the compiler reread a variable:
#define forget(var) asm volatile ("" : "=m"(var))
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Paul E. McKenney @ 2007-08-09 17:41 UTC (permalink / raw)
To: Chris Snook
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <46BB4B7B.4070007@redhat.com>
On Thu, Aug 09, 2007 at 01:14:35PM -0400, Chris Snook wrote:
> Paul E. McKenney wrote:
> >On Thu, Aug 09, 2007 at 12:36:17PM -0400, Chris Snook wrote:
> >>Paul E. McKenney wrote:
> >>>The compiler is within its rights to read a 32-bit quantity 16 bits at
> >>>at time, even on a 32-bit machine. I would be glad to help pummel any
> >>>compiler writer that pulls such a dirty trick, but the C standard really
> >>>does permit this.
> >>Yes, but we don't write code for these compilers. There are countless
> >>pieces of kernel code which would break in this condition, and there
> >>doesn't seem to be any interest in fixing this.
> >>
> >>>Use of volatile does in fact save you from the compiler pushing stores
> >>>out
> >>>of loops regardless of whether you are also doing reads. The C standard
> >>>has the notion of sequence points, which occur at various places
> >>>including
> >>>the ends of statements and the control expressions for "if" and "while"
> >>>statements. The compiler is not permitted to move volatile references
> >>>across a sequence point. Therefore, the compiler is not allowed to
> >>>push a volatile store out of a loop. Now the CPU might well do such a
> >>>reordering, but that is a separate issue to be dealt with via memory
> >>>barriers. Note that it is the CPU and I/O system, not the compiler,
> >>>that is forcing you to use reads to flush writes to MMIO registers.
> >>Sequence points enforce read-after-write ordering, not write-after-write.
> >>We flush writes with reads for MMIO because of this effect as well as the
> >>CPU/bus effects.
> >
> >Neither volatile reads nor volatile writes may be moved across sequence
> >points.
>
> By the compiler, or by the CPU?
As mentioned in earlier emails, by the compiler. The CPU knows nothing
of C sequence points.
> If you're depending on volatile writes
> being visible to other CPUs, you're screwed either way, because the CPU can
> hold that data in cache as long as it wants before it writes it to memory.
> When this finally does happen, it will happen atomically, which is all that
> atomic_set guarantees. If you need to guarantee that the value is written
> to memory at a particular time in your execution sequence, you either have
> to read it from memory to force the compiler to store it first (and a
> volatile cast in atomic_read will suffice for this) or you have to use
> LOCK_PREFIX instructions which will invalidate remote cache lines
> containing the same variable. This patch doesn't change either of these
> cases.
The case that it -can- change is interactions with interrupt handlers.
And NMI/SMI handlers, for that matter.
> >>>And you would be amazed at what compiler writers will do in order to
> >>>get an additional fraction of a percent out of SpecCPU...
> >>Probably not :)
> >>
> >>>In short, please retain atomic_set()'s volatility, especially on those
> >>>architectures that declared the atomic_t's counter to be volatile.
> >>Like i386 and x86_64? These used to have volatile in the atomic_t
> >>declaration. We removed it, and the sky did not fall.
> >
> >Interesting. You tested all possible configs on all possible hardware?
>
> No, but I can reason about it and be confident that this won't break
> anything that isn't already broken. At worst, this patch will make any
> existing subtly incorrect uses of atomic_t much more obvious and easier to
> track down.
You took interrupt and NMI/SMI handlers into account?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 3/14] nes: connection manager routines
From: Roland Dreier @ 2007-08-09 17:37 UTC (permalink / raw)
To: Jeff Garzik; +Cc: ggrundstrom, ewg, netdev
In-Reply-To: <46B9207D.6000905@garzik.org>
> > +atomic_t cm_connects;
> > +atomic_t cm_accepts;
> > +atomic_t cm_disconnects;
> > +atomic_t cm_closes;
> > +atomic_t cm_connecteds;
> > +atomic_t cm_connect_reqs;
> > +atomic_t cm_rejects;
>
> do you really want to take the hit of a LOCK prefix each time you
> increment a stat???
I think these are once-per-connection things, so the overhead is not
that bad. On the other hand there's probably another lock you have to
take anyway so maybe you can just define that these are protected by
some lock you take anyway.
Also, these names seem kind of generic to be global -- a nes_ prefix
is probably appropriate.
- R.
^ permalink raw reply
* Re: [ewg] [PATCH 14/14] nes: kernel build infrastructure
From: Roland Dreier @ 2007-08-09 17:34 UTC (permalink / raw)
To: ggrundstrom; +Cc: netdev, ewg
In-Reply-To: <200708080125.l781PVIM004915@neteffect.com>
> +ifdef CONFIG_INFINIBAND_NES_DEBUG
> +EXTRA_CFLAGS += -DNES_DEBUG
> +endif
I would just use the CONFIG_INFINIBAND_NES_DEBUG symbol directly in
your code.
> +EXTRA_CFLAGS += -DNES_MINICM
If you're defining this unconditionally, can you just get rid of all
the tests of this symbol?
- R.
^ permalink raw reply
* Re: [ewg] [PATCH 13/14] nes: kernel build infrastructure
From: Roland Dreier @ 2007-08-09 17:32 UTC (permalink / raw)
To: ggrundstrom; +Cc: netdev, ewg
In-Reply-To: <200708080123.l781NpKJ004903@neteffect.com>
> +config INFINIBAND_NES
> + tristate "NetEffect RNIC Driver"
> + depends on PCI && INET && INFINIBAND
You don't need the dependency on INFINIBAND any more, because the
whole drivers/infiniband/Kconfig file is guarded by an "if INFINIBAND"
- R.
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Chris Snook @ 2007-08-09 17:14 UTC (permalink / raw)
To: paulmck
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809165853.GD8424@linux.vnet.ibm.com>
Paul E. McKenney wrote:
> On Thu, Aug 09, 2007 at 12:36:17PM -0400, Chris Snook wrote:
>> Paul E. McKenney wrote:
>>> The compiler is within its rights to read a 32-bit quantity 16 bits at
>>> at time, even on a 32-bit machine. I would be glad to help pummel any
>>> compiler writer that pulls such a dirty trick, but the C standard really
>>> does permit this.
>> Yes, but we don't write code for these compilers. There are countless
>> pieces of kernel code which would break in this condition, and there
>> doesn't seem to be any interest in fixing this.
>>
>>> Use of volatile does in fact save you from the compiler pushing stores out
>>> of loops regardless of whether you are also doing reads. The C standard
>>> has the notion of sequence points, which occur at various places including
>>> the ends of statements and the control expressions for "if" and "while"
>>> statements. The compiler is not permitted to move volatile references
>>> across a sequence point. Therefore, the compiler is not allowed to
>>> push a volatile store out of a loop. Now the CPU might well do such a
>>> reordering, but that is a separate issue to be dealt with via memory
>>> barriers. Note that it is the CPU and I/O system, not the compiler,
>>> that is forcing you to use reads to flush writes to MMIO registers.
>> Sequence points enforce read-after-write ordering, not write-after-write.
>> We flush writes with reads for MMIO because of this effect as well as the
>> CPU/bus effects.
>
> Neither volatile reads nor volatile writes may be moved across sequence
> points.
By the compiler, or by the CPU? If you're depending on volatile writes being
visible to other CPUs, you're screwed either way, because the CPU can hold that
data in cache as long as it wants before it writes it to memory. When this
finally does happen, it will happen atomically, which is all that atomic_set
guarantees. If you need to guarantee that the value is written to memory at a
particular time in your execution sequence, you either have to read it from
memory to force the compiler to store it first (and a volatile cast in
atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions
which will invalidate remote cache lines containing the same variable. This
patch doesn't change either of these cases.
>>> And you would be amazed at what compiler writers will do in order to
>>> get an additional fraction of a percent out of SpecCPU...
>> Probably not :)
>>
>>> In short, please retain atomic_set()'s volatility, especially on those
>>> architectures that declared the atomic_t's counter to be volatile.
>> Like i386 and x86_64? These used to have volatile in the atomic_t
>> declaration. We removed it, and the sky did not fall.
>
> Interesting. You tested all possible configs on all possible hardware?
No, but I can reason about it and be confident that this won't break anything
that isn't already broken. At worst, this patch will make any existing subtly
incorrect uses of atomic_t much more obvious and easier to track down.
-- Chris
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Arnd Bergmann @ 2007-08-09 15:30 UTC (permalink / raw)
To: Chris Snook
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <46BB24CB.1040405@redhat.com>
On Thursday 09 August 2007, Chris Snook wrote:
> a) chicken and egg: asm-generic/atomic.h depends on definitions in asm/atomic.h
Ok, I see.
> If you can find a way to reshuffle the code and make it simpler, I personally am
> all for it. I'm skeptical that you'll get much to show for the effort.
I guess it could be done using more macros or new headers, but I don't see
a way that would actually improve the situation.
> b) The definitions aren't precisely identical between all architectures, so it
> would be a mess of special cases, which gets us right back to where we are now.
Why are they not identical? Anything beyond the 32/64 bit difference should
be the same afaics, or it might cause more bugs.
Arnd <><
^ permalink raw reply
* Re: 2.6.23-rc2-mm1: e1000e global symbols must be renamed
From: Kok, Auke @ 2007-08-09 17:06 UTC (permalink / raw)
To: Adrian Bunk; +Cc: e1000-devel, netdev, Andrew Morton, jgarzik, linux-kernel
In-Reply-To: <20070809135131.GN6997@stusta.de>
Adrian Bunk wrote:
> On Thu, Aug 09, 2007 at 01:51:06AM -0700, Andrew Morton wrote:
>> ...
>> - There is a new e1000 driver in git-netdev-all, called e1000e. I'm sure
>> the developers would like it tested. Please cc netdev@vger.kernel.org on
>> any reports.
>> ...
>> Changes since 2.6.23-rc2-mm1:
>> ...
>> git-netdev-all.patch
>> ...
>> git trees
>> ...
>
> <-- snip -->
>
> ...
> LD drivers/net/built-in.o
> drivers/net/e1000e/built-in.o: In function `e1000_read_mac_addr':
> (.text+0x3470): multiple definition of `e1000_read_mac_addr'
> drivers/net/e1000/built-in.o:(.text+0xb6cc): first defined here
> drivers/net/e1000e/built-in.o: In function `e1000_set_ethtool_ops':
> (.text+0x594d): multiple definition of `e1000_set_ethtool_ops'
> drivers/net/e1000/built-in.o:(.text+0xc97a): first defined here
> ...
> make[3]: *** [drivers/net/built-in.o] Error 1
ack, I'll step on that and make it go away :)
Auke
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
^ permalink raw reply
* Re: [PATCH 6/24] make atomic_read() behave consistently on frv
From: Chris Snook @ 2007-08-09 16:54 UTC (permalink / raw)
To: linux-kernel, linux-arch, torvalds
Cc: netdev, akpm, ak, heiko.carstens, davem, schwidefsky, wensong,
horms, wjiang, cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809134150.GA14890@shell.boston.redhat.com>
Chris Snook wrote:
> From: Chris Snook <csnook@redhat.com>
>
> Make atomic_read() volatile on frv. This ensures that busy-waiting
> for an interrupt handler to change an atomic_t won't get compiled to an
> infinite loop, consistent with SMP architectures.
To head off the criticism, I admit this is an oversimplification, and true
busy-waiters should be using cpu_relax(), which contains a barrier. As
discussed in recent threads, there are other cases which can be optimized by
removing the need for a barrier, and having behavior consistent with
architectures where the benefit is more profound is also valuable.
-- Chris
^ permalink raw reply
* Re: [PATCH RFC]: napi_struct V5
From: Roland Dreier @ 2007-08-09 16:58 UTC (permalink / raw)
To: hadi
Cc: Shirley Ma, David Miller, jgarzik, netdev, netdev-owner, rusty,
shemminger
In-Reply-To: <1186587154.5155.43.camel@localhost>
> > Dave, could you please hold this portion of the patch for a moment. I
> > will test this patch ASAP. According to our previous experience, this
> > changes significant changes some IPoIB driver performance.
> If you adjust your quantum while doing that testing you may find an
> optimal value.
> Think of a box where you have other network interfaces, the way you
> are implementing currently implies you are going to be very unfair to
> the other interfaces on the box.
Could you explain why this is unfair? This is an honest question: I'm
not trying to be difficult, I just don't see how this implementation
leads to unfairness. If a driver uses *less* than its full budget in
the poll routine, requests that the poll routine be rescheduled and
then returns, it seems to me that the effect on other interfaces would
be to give them more than their fair share of NAPI processing time.
Also, perhaps it would be a good idea to explain exactly what the
ipoib driver is doing in its NAPI poll routine. The difficultly is
that the IB "interrupt" semantics are not a perfect fit for NAPI -- in
effect, IB just gives us an edge-triggered one-shot interrupt, and so
there is an unadvoidable race between detecting that there is no more
work to do and enabling the interrupt. It's not worth going into the
details of why things are this way, but IB can return a hint that says
"you may have missed an event" when enabling the interrupt, which can
be used to close the race. So the two implementations being discussed
are roughly:
if (may_have_missed_event &&
netif_rx_reschedule(napi))
goto poll_more;
versus
if (may_have_missed_event) {
netif_rx_reschedule(napi))
return done;
}
The second one seems to perform better because in the missed event
case, it gives a few more packets a chance to arrive so that we can
amortize the polling overhead a little more. To be honest, I've never
been able to come up with a good story of why the IBM hardware where
this makes a measurable difference hits the missed event case enough
for it to matter.
The other thing that confuses me about why this is a fairness issue is
that if this were a driver where the missed event race didn't happen,
when we detected no more work to do, we would just do:
netif_rx_complete(napi);
enable_hw_interrupts();
return done;
and if a packet arrived between netif_rx_complete and enabling
interrupts, we would still get an interrupt and so the effect would be
to reschedule polling, just via a more inefficient route (going
through the HW interrupt handler).
So clarification on this point would be appreciated -- not because I
want to continue an argument, but just to improve my understanding of NAPI.
- Roland
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Paul E. McKenney @ 2007-08-09 16:58 UTC (permalink / raw)
To: Chris Snook
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <46BB4281.7010803@redhat.com>
On Thu, Aug 09, 2007 at 12:36:17PM -0400, Chris Snook wrote:
> Paul E. McKenney wrote:
> >The compiler is within its rights to read a 32-bit quantity 16 bits at
> >at time, even on a 32-bit machine. I would be glad to help pummel any
> >compiler writer that pulls such a dirty trick, but the C standard really
> >does permit this.
>
> Yes, but we don't write code for these compilers. There are countless
> pieces of kernel code which would break in this condition, and there
> doesn't seem to be any interest in fixing this.
>
> >Use of volatile does in fact save you from the compiler pushing stores out
> >of loops regardless of whether you are also doing reads. The C standard
> >has the notion of sequence points, which occur at various places including
> >the ends of statements and the control expressions for "if" and "while"
> >statements. The compiler is not permitted to move volatile references
> >across a sequence point. Therefore, the compiler is not allowed to
> >push a volatile store out of a loop. Now the CPU might well do such a
> >reordering, but that is a separate issue to be dealt with via memory
> >barriers. Note that it is the CPU and I/O system, not the compiler,
> >that is forcing you to use reads to flush writes to MMIO registers.
>
> Sequence points enforce read-after-write ordering, not write-after-write.
> We flush writes with reads for MMIO because of this effect as well as the
> CPU/bus effects.
Neither volatile reads nor volatile writes may be moved across sequence
points.
> >And you would be amazed at what compiler writers will do in order to
> >get an additional fraction of a percent out of SpecCPU...
>
> Probably not :)
>
> >In short, please retain atomic_set()'s volatility, especially on those
> >architectures that declared the atomic_t's counter to be volatile.
>
> Like i386 and x86_64? These used to have volatile in the atomic_t
> declaration. We removed it, and the sky did not fall.
Interesting. You tested all possible configs on all possible hardware?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 14/24] make atomic_read() behave consistently on parisc
From: Kyle McMartin @ 2007-08-09 16:45 UTC (permalink / raw)
To: Chris Snook
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809140154.GA17489@shell.boston.redhat.com>
On Thu, Aug 09, 2007 at 10:01:54AM -0400, Chris Snook wrote:
> From: Chris Snook <csnook@redhat.com>
>
> Purify volatile use for atomic[64]_t on parisc.
>
> Signed-off-by: Chris Snook <csnook@redhat.com>
>
Sure, why not.
ACKed-by: Kyle McMartin <kyle@parisc-linux.org>
^ permalink raw reply
* [PATCH] ethtool: Add LRO support
From: Auke Kok @ 2007-08-09 16:41 UTC (permalink / raw)
To: davem, jeff; +Cc: netdev, ossthema
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---
ethtool-copy.h | 8 ++++++++
ethtool.8 | 8 ++++++--
ethtool.c | 39 +++++++++++++++++++++++++++++++++------
3 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/ethtool-copy.h b/ethtool-copy.h
index 3a63224..ab9d688 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -274,6 +274,8 @@ int ethtool_op_get_perm_addr(struct net_device *dev,
struct ethtool_perm_addr *addr, u8 *data);
u32 ethtool_op_get_ufo(struct net_device *dev);
int ethtool_op_set_ufo(struct net_device *dev, u32 data);
+u32 ethtool_op_get_lro(struct net_device *dev);
+int ethtool_op_set_lro(struct net_device *dev, u32 data);
/**
* ðtool_ops - Alter and report network device settings
@@ -305,6 +307,8 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data);
* set_tso: Turn TCP segmentation offload on or off
* get_ufo: Report whether UDP fragmentation offload is enabled
* set_ufo: Turn UDP fragmentation offload on or off
+ * get_lro: Report whether large receive offload is enabled
+ * set_lro: Turn large receive offload on or off
* self_test: Run specified self-tests
* get_strings: Return a set of strings that describe the requested objects
* phys_id: Identify the device
@@ -373,6 +377,8 @@ struct ethtool_ops {
void (*complete)(struct net_device *);
u32 (*get_ufo)(struct net_device *);
int (*set_ufo)(struct net_device *, u32);
+ u32 (*get_lro)(struct net_device *);
+ int (*set_lro)(struct net_device *, u32);
};
#endif /* __KERNEL__ */
@@ -414,6 +420,8 @@ struct ethtool_ops {
#define ETHTOOL_SUFO 0x00000022 /* Set UFO enable (ethtool_value) */
#define ETHTOOL_GGSO 0x00000023 /* Get GSO enable (ethtool_value) */
#define ETHTOOL_SGSO 0x00000024 /* Set GSO enable (ethtool_value) */
+#define ETHTOOL_GLRO 0x00000025 /* Get LRO enable (ethtool_value) */
+#define ETHTOOL_SLRO 0x00000026 /* Set LRO enable (ethtool_value) */
/* compatibility with older code */
#define SPARC_ETH_GSET ETHTOOL_GSET
diff --git a/ethtool.8 b/ethtool.8
index af51056..89abf08 100644
--- a/ethtool.8
+++ b/ethtool.8
@@ -158,6 +158,7 @@ ethtool \- Display or change ethernet card settings
.B2 tso on off
.B2 ufo on off
.B2 gso on off
+.B2 lro on off
.B ethtool \-p|\-\-blink
.I ethX
@@ -289,10 +290,13 @@ Specifies whether scatter-gather should be enabled.
Specifies whether TCP segmentation offload should be enabled.
.TP
.A2 ufo on off
-Specifies whether UDP fragmentation offload should be enabled
+Specifies whether UDP fragmentation offload should be enabled.
.TP
.A2 gso on off
-Specifies whether generic segmentation offload should be enabled
+Specifies whether generic segmentation offload should be enabled.
+.TP
+.A2 lro on off
+Specifies whether large receive offload should be enabled.
.TP
.B \-p \-\-identify
Initiates adapter-specific action intended to enable an operator to
diff --git a/ethtool.c b/ethtool.c
index b04f747..4c9844a 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -151,7 +151,8 @@ static struct option {
" [ sg on|off ]\n"
" [ tso on|off ]\n"
" [ ufo on|off ]\n"
- " [ gso on|off ]\n" },
+ " [ gso on|off ]\n"
+ " [ lro on|off ]\n" },
{ "-i", "--driver", MODE_GDRV, "Show driver information" },
{ "-d", "--register-dump", MODE_GREGS, "Do a register dump",
" [ raw on|off ]\n"
@@ -200,6 +201,7 @@ static int off_sg_wanted = -1;
static int off_tso_wanted = -1;
static int off_ufo_wanted = -1;
static int off_gso_wanted = -1;
+static int off_lro_wanted = -1;
static struct ethtool_pauseparam epause;
static int gpause_changed = 0;
@@ -310,6 +312,7 @@ static struct cmdline_info cmdline_offload[] = {
{ "tso", CMDL_BOOL, &off_tso_wanted, NULL },
{ "ufo", CMDL_BOOL, &off_ufo_wanted, NULL },
{ "gso", CMDL_BOOL, &off_gso_wanted, NULL },
+ { "lro", CMDL_BOOL, &off_lro_wanted, NULL },
};
static struct cmdline_info cmdline_pause[] = {
@@ -1207,7 +1210,7 @@ static int dump_coalesce(void)
return 0;
}
-static int dump_offload (int rx, int tx, int sg, int tso, int ufo, int gso)
+static int dump_offload (int rx, int tx, int sg, int tso, int ufo, int gso, int lro)
{
fprintf(stdout,
"rx-checksumming: %s\n"
@@ -1215,13 +1218,15 @@ static int dump_offload (int rx, int tx, int sg, int tso, int ufo, int gso)
"scatter-gather: %s\n"
"tcp segmentation offload: %s\n"
"udp fragmentation offload: %s\n"
- "generic segmentation offload: %s\n",
+ "generic segmentation offload: %s\n"
+ "large receive offload: %s\n",
rx ? "on" : "off",
tx ? "on" : "off",
sg ? "on" : "off",
tso ? "on" : "off",
ufo ? "on" : "off",
- gso ? "on" : "off");
+ gso ? "on" : "off",
+ lro ? "on" : "off");
return 0;
}
@@ -1485,7 +1490,8 @@ static int do_scoalesce(int fd, struct ifreq *ifr)
static int do_goffload(int fd, struct ifreq *ifr)
{
struct ethtool_value eval;
- int err, allfail = 1, rx = 0, tx = 0, sg = 0, tso = 0, ufo = 0, gso = 0;
+ int err, allfail = 1;
+ int rx = 0, tx = 0, sg = 0, tso = 0, ufo = 0, gso = 0, lro = 0;
fprintf(stdout, "Offload parameters for %s:\n", devname);
@@ -1549,12 +1555,22 @@ static int do_goffload(int fd, struct ifreq *ifr)
allfail = 0;
}
+ eval.cmd = ETHTOOL_GLRO;
+ ifr->ifr_data = (caddr_t)&eval;
+ err = ioctl(fd, SIOCETHTOOL, ifr);
+ if (err)
+ perror("Cannot get device generic large receive offload settings");
+ else {
+ gso = eval.data;
+ allfail = 0;
+ }
+
if (allfail) {
fprintf(stdout, "no offload info available\n");
return 83;
}
- return dump_offload(rx, tx, sg, tso, ufo, gso);
+ return dump_offload(rx, tx, sg, tso, ufo, gso, lro);
}
static int do_soffload(int fd, struct ifreq *ifr)
@@ -1631,6 +1647,17 @@ static int do_soffload(int fd, struct ifreq *ifr)
return 90;
}
}
+ if (off_lro_wanted >= 0) {
+ changed = 1;
+ eval.cmd = ETHTOOL_SLRO;
+ eval.data = (off_gso_wanted == 1);
+ ifr->ifr_data = (caddr_t)&eval;
+ err = ioctl(fd, SIOCETHTOOL, ifr);
+ if (err) {
+ perror("Cannot set device large receive offload settings");
+ return 91;
+ }
+ }
if (!changed) {
fprintf(stdout, "no offload settings changed\n");
}
^ permalink raw reply related
* [PATCH] [NET] ethtool: Add LRO support
From: Auke Kok @ 2007-08-09 16:41 UTC (permalink / raw)
To: davem, jeff; +Cc: netdev, ossthema
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---
include/linux/ethtool.h | 8 +++++++
include/linux/netdevice.h | 1 +
net/core/ethtool.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 0 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 23ccea8..a97248e 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -272,6 +272,8 @@ u32 ethtool_op_get_tso(struct net_device *dev);
int ethtool_op_set_tso(struct net_device *dev, u32 data);
u32 ethtool_op_get_ufo(struct net_device *dev);
int ethtool_op_set_ufo(struct net_device *dev, u32 data);
+u32 ethtool_op_get_lro(struct net_device *dev);
+int ethtool_op_set_lro(struct net_device *dev, u32 data);
/**
* ðtool_ops - Alter and report network device settings
@@ -303,6 +305,8 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data);
* set_tso: Turn TCP segmentation offload on or off
* get_ufo: Report whether UDP fragmentation offload is enabled
* set_ufo: Turn UDP fragmentation offload on or off
+ * get_lro: Report whether large receive offload is enabled
+ * set_lro: Turn large receive offload on or off
* self_test: Run specified self-tests
* get_strings: Return a set of strings that describe the requested objects
* phys_id: Identify the device
@@ -369,6 +373,8 @@ struct ethtool_ops {
void (*complete)(struct net_device *);
u32 (*get_ufo)(struct net_device *);
int (*set_ufo)(struct net_device *, u32);
+ u32 (*get_lro)(struct net_device *);
+ int (*set_lro)(struct net_device *, u32);
};
#endif /* __KERNEL__ */
@@ -410,6 +416,8 @@ struct ethtool_ops {
#define ETHTOOL_SUFO 0x00000022 /* Set UFO enable (ethtool_value) */
#define ETHTOOL_GGSO 0x00000023 /* Get GSO enable (ethtool_value) */
#define ETHTOOL_SGSO 0x00000024 /* Set GSO enable (ethtool_value) */
+#define ETHTOOL_GLRO 0x00000025 /* Get LRO enable (ethtool_value) */
+#define ETHTOOL_SLRO 0x00000026 /* Set LRO enable (ethtool_value) */
/* compatibility with older code */
#define SPARC_ETH_GSET ETHTOOL_GSET
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4a616d7..4863ffc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -341,6 +341,7 @@ struct net_device
#define NETIF_F_GSO 2048 /* Enable software GSO. */
#define NETIF_F_LLTX 4096 /* LockLess TX */
#define NETIF_F_MULTI_QUEUE 16384 /* Has multiple TX/RX queues */
+#define NETIF_F_LRO 32768 /* Has large receive offload */
/* Segmentation offload features */
#define NETIF_F_GSO_SHIFT 16
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 2ab0a60..65f751b 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -109,6 +109,20 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data)
return 0;
}
+u32 ethtool_op_get_lro(struct net_device *dev)
+{
+ return (dev->features & NETIF_F_LRO) != 0;
+}
+
+int ethtool_op_set_lro(struct net_device *dev, u32 data)
+{
+ if (data)
+ dev->features |= NETIF_F_LRO;
+ else
+ dev->features &= ~NETIF_F_LRO;
+ return 0;
+}
+
/* Handlers for each ethtool command */
static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
@@ -504,6 +518,13 @@ static int __ethtool_set_sg(struct net_device *dev, u32 data)
if (err)
return err;
}
+
+ if (!data && dev->ethtool_ops->set_lro) {
+ err = dev->ethtool_ops->set_lro(dev, 0);
+ if (err)
+ return err;
+ }
+
return dev->ethtool_ops->set_sg(dev, data);
}
@@ -615,6 +636,29 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
return dev->ethtool_ops->set_ufo(dev, edata.data);
}
+static int ethtool_get_lro(struct net_device *dev, char __user *useraddr)
+{
+ struct ethtool_value edata = { ETHTOOL_GLRO };
+
+ edata.data = dev->features & NETIF_F_LRO;
+ if (copy_to_user(useraddr, &edata, sizeof(edata)))
+ return -EFAULT;
+ return 0;
+}
+
+static int ethtool_set_lro(struct net_device *dev, char __user *useraddr)
+{
+ struct ethtool_value edata;
+
+ if (copy_from_user(&edata, useraddr, sizeof(edata)))
+ return -EFAULT;
+ if (edata.data)
+ dev->features |= NETIF_F_LRO;
+ else
+ dev->features &= ~NETIF_F_LRO;
+ return 0;
+}
+
static int ethtool_get_gso(struct net_device *dev, char __user *useraddr)
{
struct ethtool_value edata = { ETHTOOL_GGSO };
@@ -816,6 +860,7 @@ int dev_ethtool(struct ifreq *ifr)
case ETHTOOL_GTSO:
case ETHTOOL_GPERMADDR:
case ETHTOOL_GUFO:
+ case ETHTOOL_GLRO:
case ETHTOOL_GGSO:
break;
default:
@@ -929,6 +974,12 @@ int dev_ethtool(struct ifreq *ifr)
case ETHTOOL_SUFO:
rc = ethtool_set_ufo(dev, useraddr);
break;
+ case ETHTOOL_GLRO:
+ rc = ethtool_get_lro(dev, useraddr);
+ break;
+ case ETHTOOL_SLRO:
+ rc = ethtool_set_lro(dev, useraddr);
+ break;
case ETHTOOL_GGSO:
rc = ethtool_get_gso(dev, useraddr);
break;
@@ -960,3 +1011,5 @@ EXPORT_SYMBOL(ethtool_op_set_tx_hw_csum);
EXPORT_SYMBOL(ethtool_op_set_tx_ipv6_csum);
EXPORT_SYMBOL(ethtool_op_set_ufo);
EXPORT_SYMBOL(ethtool_op_get_ufo);
+EXPORT_SYMBOL(ethtool_op_set_lro);
+EXPORT_SYMBOL(ethtool_op_get_lro);
^ permalink raw reply related
* Re: [PATCH 17/24] make atomic_read() behave consistently on sh64
From: Paul Mundt @ 2007-08-09 16:40 UTC (permalink / raw)
To: Chris Snook
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809140908.GA18208@shell.boston.redhat.com>
On Thu, Aug 09, 2007 at 10:09:08AM -0400, Chris Snook wrote:
> Purify volatile use for atomic_t on sh64.
>
> Signed-off-by: Chris Snook <csnook@redhat.com>
On Thu, Aug 09, 2007 at 10:10:34AM -0400, Chris Snook wrote:
> Purify volatile use for atomic_t on sh.
>
> Signed-off-by: Chris Snook <csnook@redhat.com>
Acked-by: Paul Mundt <lethal@linux-sh.org>
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Chris Snook @ 2007-08-09 16:36 UTC (permalink / raw)
To: paulmck
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809161024.GC8424@linux.vnet.ibm.com>
Paul E. McKenney wrote:
> The compiler is within its rights to read a 32-bit quantity 16 bits at
> at time, even on a 32-bit machine. I would be glad to help pummel any
> compiler writer that pulls such a dirty trick, but the C standard really
> does permit this.
Yes, but we don't write code for these compilers. There are countless pieces of
kernel code which would break in this condition, and there doesn't seem to be
any interest in fixing this.
> Use of volatile does in fact save you from the compiler pushing stores out
> of loops regardless of whether you are also doing reads. The C standard
> has the notion of sequence points, which occur at various places including
> the ends of statements and the control expressions for "if" and "while"
> statements. The compiler is not permitted to move volatile references
> across a sequence point. Therefore, the compiler is not allowed to
> push a volatile store out of a loop. Now the CPU might well do such a
> reordering, but that is a separate issue to be dealt with via memory
> barriers. Note that it is the CPU and I/O system, not the compiler,
> that is forcing you to use reads to flush writes to MMIO registers.
Sequence points enforce read-after-write ordering, not write-after-write. We
flush writes with reads for MMIO because of this effect as well as the CPU/bus
effects.
> And you would be amazed at what compiler writers will do in order to
> get an additional fraction of a percent out of SpecCPU...
Probably not :)
> In short, please retain atomic_set()'s volatility, especially on those
> architectures that declared the atomic_t's counter to be volatile.
Like i386 and x86_64? These used to have volatile in the atomic_t declaration.
We removed it, and the sky did not fall.
-- Chris
^ permalink raw reply
* Re: [PATCH 24/24] document volatile atomic_read() behavior
From: Chris Snook @ 2007-08-09 16:26 UTC (permalink / raw)
To: Segher Boessenkool
Cc: wjiang, wensong, heiko.carstens, linux-kernel, ak, cfriesen,
netdev, horms, akpm, linux-arch, torvalds, schwidefsky, davem,
zlynx, rpjday, jesper.juhl
In-Reply-To: <8f6bb8a9e4f2819a161d732bdb6c70c0@kernel.crashing.org>
Segher Boessenkool wrote:
>> Historically this has been
>> +accomplished by declaring the counter itself to be volatile, but the
>> +ambiguity of the C standard on the semantics of volatile make this
>> practice
>> +vulnerable to overly creative interpretation by compilers.
>
> It's even worse when accessing through a volatile casted pointer;
> see for example the recent(*) GCC bugs in that area.
>
> (*) Well, not _all_ that recent. No one should be using the 3.x
> series anymore, right?
>
>> Explicit
>> +casting in atomic_read() ensures consistent behavior across
>> architectures
>> +and compilers.
>
> Even modulo compiler bugs, what makes you believe that?
When you declare a variable volatile, you don't actually tell the compiler where
you want to override its default optimization behavior, giving it some freedom
to guess your intentions incorrectly. When you put the cast on the data access
itself, there is no question about precisely where in the code you want to
override the compiler's default optimization behavior. If the compiler doesn't
do what you want with a volatile declaration, it might have a plausible excuse
in the ambiguity of the C standard. If the compiler doesn't do what you want in
a cast specific to a single dereference, it's just plain broken. We try to be
compatible with plausibly correct compilers, but if they're completely broken,
we're screwed no matter what.
-- Chris
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Chris Snook @ 2007-08-09 16:20 UTC (permalink / raw)
To: Segher Boessenkool
Cc: wjiang, rpjday, wensong, heiko.carstens, linux-kernel, ak, netdev,
paulmck, horms, akpm, linux-arch, jesper.juhl, torvalds,
schwidefsky, davem, cfriesen, zlynx
In-Reply-To: <3bfabd7472d6f019aa1880b14013f7a1@kernel.crashing.org>
Segher Boessenkool wrote:
>> We can't have split stores because we don't use atomic64_t on 32-bit
>> architectures.
>
> That's not true; the compiler is free to split all stores
> (and reads) from memory however it wants. It is debatable
> whether "volatile" would prevent this as well, certainly
> it is unsafe if you want to be portable. GCC will do its
> best to not split volatile memory accesses, but bugs in
> this area do happen a lot (because the compiler code for
> volatile isn't as well exercised as most other compiler
> code, and because it is simply a hard subject; and there
> is no real formalised model for what GCC should do).
>
> The only safe way to get atomic accesses is to write
> assembler code. Are there any downsides to that? I don't
> see any.
The assumption that aligned word reads and writes are atomic, and that words are
aligned unless explicitly packed otherwise, is endemic in the kernel. No sane
compiler violates this assumption. It's true that we're not portable to insane
compilers after this patch, but we never were in the first place.
-- Chris
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Paul E. McKenney @ 2007-08-09 16:10 UTC (permalink / raw)
To: Chris Snook
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <46BB31A6.4080507@redhat.com>
On Thu, Aug 09, 2007 at 11:24:22AM -0400, Chris Snook wrote:
> Paul E. McKenney wrote:
> >On Thu, Aug 09, 2007 at 10:53:14AM -0400, Chris Snook wrote:
> >>Paul E. McKenney wrote:
> >>>Why not the same access-once semantics for atomic_set() as
> >>>for atomic_read()? As this patch stands, it might introduce
> >>>architecture-specific compiler-induced bugs due to the fact that
> >>>atomic_set() used to imply volatile behavior but no longer does.
> >>When we make the volatile cast in atomic_read(), we're casting an rvalue
> >>to volatile. This unambiguously tells the compiler that we want to
> >>re-load that register from memory. What's "volatile behavior" for an
> >>lvalue?
> >
> >I was absolutely -not- suggesting volatile behavior for lvalues.
> >
> >Instead, I am asking for volatile behavior from an -rvalue-. In the
> >case of atomic_read(), it is the atomic_t being read from. In the case
> >of atomic_set(), it is the atomic_t being written to. As suggested in
> >my previous email:
> >
> >#define atomic_set(v,i) ((*(volatile int *)&(v)->counter) =
> >(i))
> >#define atomic64_set(v,i) ((*(volatile long *)&(v)->counter) = (i))
>
> That looks like a volatile lvalue to me. I confess I didn't exactly ace
> compilers. Care to explain this?
OK, so I am dylexic this morning. Never could tell left from right...
This is indeed an lvalue, as is any non-function expression that you
can apply the "&" prefix operator to. Including the expression in
your proposed definitions of atomic_set() and atomic_set64().
An lvalue is any expression that -could- appear on the left-hand side
of an assignment operator, regardless of where it actually appears.
More precisely, an lvalue is an expression that refers to a variable in
such a way that the variable might be both loaded from and stored to,
ignoring things like "const" for the moment.
Because "(v)->counter" could be either loaded from or stored to, it
is an lvalue, which is why the compiler didn't scream bloody murder
at you when you took the address of it.
> >Again, the architectures that used to have their "counter" declared
> >as volatile will lose volatile semantics on atomic_set() with your
> >patch, which might result in bugs due to overly imaginative compiler
> >optimizations. The above would prevent any such bugs from appearing.
> >
> >> A
> >>write to an lvalue already implies an eventual write to memory, so this
> >>would be a no-op. Maybe you'll write to the register a few times before
> >>flushing it to memory, but it will happen eventually. With an rvalue,
> >>there's no guarantee that it will *ever* load from memory, which is what
> >>volatile fixes.
> >>
> >>I think what you have in mind is LOCK_PREFIX behavior, which is not the
> >>purpose of atomic_set. We use LOCK_PREFIX in the inline assembly for the
> >>atomic_* operations that read, modify, and write a value, only because it
> >>is necessary to perform that entire transaction atomically.
> >
> >No LOCK_PREFIX, thank you!!! I just want to make sure that the compiler
> >doesn't push the store down out of a loop, split the store, allow the
> >store to happen twice (e.g., to allow different code paths to be merged),
> >and all the other tricks that the C standard permits compilers to pull.
>
> We can't have split stores because we don't use atomic64_t on 32-bit
> architectures. volatile won't save you from pushing stores out of loops
> unless you're also doing reads. This is why we use reads to flush writes
> to mmio registers. In this case, an atomic_read() with volatile in it will
> suffice. Storing twice is perfectly legal here, though it's unlikely that
> an optimizing compiler smart enough to create the problem we're addressing
> would ever do that.
The compiler is within its rights to read a 32-bit quantity 16 bits at
at time, even on a 32-bit machine. I would be glad to help pummel any
compiler writer that pulls such a dirty trick, but the C standard really
does permit this.
Use of volatile does in fact save you from the compiler pushing stores out
of loops regardless of whether you are also doing reads. The C standard
has the notion of sequence points, which occur at various places including
the ends of statements and the control expressions for "if" and "while"
statements. The compiler is not permitted to move volatile references
across a sequence point. Therefore, the compiler is not allowed to
push a volatile store out of a loop. Now the CPU might well do such a
reordering, but that is a separate issue to be dealt with via memory
barriers. Note that it is the CPU and I/O system, not the compiler,
that is forcing you to use reads to flush writes to MMIO registers.
And you would be amazed at what compiler writers will do in order to
get an additional fraction of a percent out of SpecCPU...
In short, please retain atomic_set()'s volatility, especially on those
architectures that declared the atomic_t's counter to be volatile.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 2/24] make atomic_read() behave consistently on arm
From: Russell King @ 2007-08-09 16:06 UTC (permalink / raw)
To: Chris Snook
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809133028.GA13241@shell.boston.redhat.com>
On Thu, Aug 09, 2007 at 09:30:28AM -0400, Chris Snook wrote:
> From: Chris Snook <csnook@redhat.com>
>
> Purify volatile use for atomic_t on arm.
>
> Signed-off-by: Chris Snook <csnook@redhat.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
^ permalink raw reply
* Re: [PATCH 24/24] document volatile atomic_read() behavior
From: Segher Boessenkool @ 2007-08-09 15:59 UTC (permalink / raw)
To: Chris Snook
Cc: wjiang, wensong, heiko.carstens, linux-kernel, ak, cfriesen,
netdev, horms, akpm, linux-arch, torvalds, schwidefsky, davem,
zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809142430.GA19817@shell.boston.redhat.com>
> Historically this has been
> +accomplished by declaring the counter itself to be volatile, but the
> +ambiguity of the C standard on the semantics of volatile make this
> practice
> +vulnerable to overly creative interpretation by compilers.
It's even worse when accessing through a volatile casted pointer;
see for example the recent(*) GCC bugs in that area.
(*) Well, not _all_ that recent. No one should be using the 3.x
series anymore, right?
> Explicit
> +casting in atomic_read() ensures consistent behavior across
> architectures
> +and compilers.
Even modulo compiler bugs, what makes you believe that?
Segher
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Segher Boessenkool @ 2007-08-09 15:50 UTC (permalink / raw)
To: Chris Snook
Cc: wjiang, rpjday, wensong, heiko.carstens, linux-kernel, ak, netdev,
paulmck, horms, akpm, linux-arch, jesper.juhl, torvalds,
schwidefsky, davem, cfriesen, zlynx
In-Reply-To: <46BB31A6.4080507@redhat.com>
> We can't have split stores because we don't use atomic64_t on 32-bit
> architectures.
That's not true; the compiler is free to split all stores
(and reads) from memory however it wants. It is debatable
whether "volatile" would prevent this as well, certainly
it is unsafe if you want to be portable. GCC will do its
best to not split volatile memory accesses, but bugs in
this area do happen a lot (because the compiler code for
volatile isn't as well exercised as most other compiler
code, and because it is simply a hard subject; and there
is no real formalised model for what GCC should do).
The only safe way to get atomic accesses is to write
assembler code. Are there any downsides to that? I don't
see any.
Segher
^ permalink raw reply
* Re: 2.6.23-rc2: WARNING: at kernel/irq/resend.c:70 check_irq_resend()
From: Jarek Poplawski @ 2007-08-09 15:54 UTC (permalink / raw)
To: John Stoffel
Cc: linux-kernel, shemminger, vignaud, marcin.slusarz, tglx, mingo,
torvalds, akpm, alan, linux-net, netdev
In-Reply-To: <18107.11431.838905.331157@stoffel.org>
On Thu, Aug 09, 2007 at 11:03:03AM -0400, John Stoffel wrote:
>
> Hi,
Hi, read below, please...
>
> I'm opening this ticket as a new subject, even though it looks like it
> might be related to the thread "Networking dies after random time".
> Sorry for the wide CC list, but since my network hasn't died since I
> rebooted into 2.6.23-rc2 (after 30+ days at 2.6.22-rc7), I'm wondering
> if the problem is more than networking related.
>
> Honestly, I haven't gone back over the previous thread in detail, so I
> might be missing info here.
>
> System details: Dell Precision 610MT, Intel 440GX chipset, Dual PIII
> Xeon, 550Mhz, 2gb RAM (upgraded from 768Mb last night), a mix of IDE,
> SCSI and SATA disks in the system. My poor PCI bus! Just upgraded to
> 2.6.23-rc2. Interrupts looks like this:
>
> > cat /proc/interrupts
> CPU0 CPU1
> 0: 280 1 IO-APIC-edge timer
> 1: 788 0 IO-APIC-edge i8042
> 6: 1 4 IO-APIC-edge floppy
> 8: 0 1 IO-APIC-edge rtc
> 9: 0 0 IO-APIC-fasteoi acpi
> 11: 82410 1239 IO-APIC-edge Cyclom-Y
> 12: 279 106 IO-APIC-edge i8042
> 14: 440901 4266 IO-APIC-edge libata
> 15: 0 0 IO-APIC-edge libata
> 16: 2394727 42983 IO-APIC-fasteoi ohci_hcd:usb3, Ensoniq
> AudioPCI, mga@pci:0000:01:00.0
> 17: 2237362 1110 IO-APIC-fasteoi sata_sil,
> ehci_hcd:usb1, eth0
> 18: 126520 31978 IO-APIC-fasteoi aic7xxx, aic7xxx, ide2,
> ide3, ohci1394
> 19: 0 0 IO-APIC-fasteoi ohci_hcd:usb2,
> uhci_hcd:usb4
> NMI: 0 0
> LOC: 40672484 40672246
> ERR: 0
> MIS: 0
>
> I've only seen the one Warning oops, and backups and other system
> processes have been running for the past 12 hours without a problem.
>
>
> [ 187.747442] Probing IDE interface ide2...
> [ 188.011634] hde: WDC WD1200JB-00CRA1, ATA DISK drive
> [ 188.623038] WARNING: at kernel/irq/resend.c:70 check_irq_resend()
> [ 188.623105] [<c0149e38>] check_irq_resend+0xa8/0xc0
> [ 188.623204] [<c01499d3>] enable_irq+0xc3/0xd0
> [ 188.623295] [<f8867280>] probe_hwif+0x670/0x7c0 [ide_core]
> [ 188.623448] [<f8869f04>] do_ide_setup_pci_device+0x154/0x480
> [ide_core]
> [ 188.623571] [<f8867d6c>] probe_hwif_init_with_fixup+0xc/0x90
> [ide_core]
> [ 188.623690] [<f88817d0>] init_setup_hpt302+0x0/0x30 [hpt366]
> [ 188.623791] [<f886a39b>] ide_setup_pci_device+0x7b/0xc0 [ide_core]
> [ 188.623909] [<f88817d0>] init_setup_hpt302+0x0/0x30 [hpt366]
> [ 188.624004] [<f88811ed>] hpt366_init_one+0x8d/0xa0 [hpt366]
> [ 188.624095] [<f88817d0>] init_setup_hpt302+0x0/0x30 [hpt366]
> [ 188.624187] [<f8881e50>] init_chipset_hpt366+0x0/0x680 [hpt366]
> [ 188.624281] [<f8882680>] init_hwif_hpt366+0x0/0x380 [hpt366]
> [ 188.624372] [<f8881800>] init_dma_hpt366+0x0/0xe0 [hpt366]
> [ 188.624466] [<c0265fc6>] pci_device_probe+0x56/0x80
> [ 188.624565] [<c02d0f8e>] driver_probe_device+0x8e/0x190
> [ 188.624669] [<c02d11fe>] __driver_attach+0x9e/0xa0
> [ 188.624756] [<c02d038a>] bus_for_each_dev+0x3a/0x60
> [ 188.624845] [<c02d0e06>] driver_attach+0x16/0x20
> [ 188.624932] [<c02d1160>] __driver_attach+0x0/0xa0
> [ 188.625017] [<c02d075a>] bus_add_driver+0x8a/0x1b0
> [ 188.625107] [<c0266173>] __pci_register_driver+0x53/0xa0
> [ 188.625197] [<c0144d5d>] sys_init_module+0x13d/0x1820
> [ 188.625315] [<f8844000>] snd_timer_find+0x0/0x90 [snd_timer]
> [ 188.625424] [<c0149530>] disable_irq+0x0/0x30
> [ 188.625513] [<c0108b7d>] sys_mmap2+0xcd/0xd0
> [ 188.625612] [<c0104266>] syscall_call+0x7/0xb
> [ 188.625701] [<c0410000>] rpc_get_inode+0x0/0x80
> [ 188.625798] =======================
> [ 188.625871] hde: selected mode 0x45
> [ 188.626817] ide2 at 0xecf8-0xecff,0xecf2 on irq 18
> [ 188.627080] Probing IDE interface ide3...
> [ 188.891165] hdg: WDC WD1200JB-00EVA0, ATA DISK drive
> [ 189.502580] hdg: selected mode 0x45
> [ 189.503698] ide3 at 0xece0-0xece7,0xecda on irq 18
>
>
> Let
I'm not sure I don't miss anything (a little in hurry now), but this
warning's aim was purely diagnostical and nothing wrong is meant!
Unless there is something wrong... Then please try to be more explicit.
If you prefer to not see this, there is my patch proposal somewhere
in this older thread:
Subject: [patch] genirq: temporary fix for level-triggered IRQ resend
Date: Wed, 8 Aug 2007 13:00:37 +0200
On the other hand, if it works OK, it would be better to let it be
tested more like this...
Regards,
Jarek P.
^ permalink raw reply
* Re: [PATCH 1/24] make atomic_read() behave consistently on alpha
From: Chris Snook @ 2007-08-09 15:24 UTC (permalink / raw)
To: paulmck
Cc: linux-kernel, linux-arch, torvalds, netdev, akpm, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl
In-Reply-To: <20070809150445.GB8424@linux.vnet.ibm.com>
Paul E. McKenney wrote:
> On Thu, Aug 09, 2007 at 10:53:14AM -0400, Chris Snook wrote:
>> Paul E. McKenney wrote:
>>> Why not the same access-once semantics for atomic_set() as
>>> for atomic_read()? As this patch stands, it might introduce
>>> architecture-specific compiler-induced bugs due to the fact that
>>> atomic_set() used to imply volatile behavior but no longer does.
>> When we make the volatile cast in atomic_read(), we're casting an rvalue to
>> volatile. This unambiguously tells the compiler that we want to re-load
>> that register from memory. What's "volatile behavior" for an lvalue?
>
> I was absolutely -not- suggesting volatile behavior for lvalues.
>
> Instead, I am asking for volatile behavior from an -rvalue-. In the
> case of atomic_read(), it is the atomic_t being read from. In the case
> of atomic_set(), it is the atomic_t being written to. As suggested in
> my previous email:
>
> #define atomic_set(v,i) ((*(volatile int *)&(v)->counter) = (i))
> #define atomic64_set(v,i) ((*(volatile long *)&(v)->counter) = (i))
That looks like a volatile lvalue to me. I confess I didn't exactly ace
compilers. Care to explain this?
> Again, the architectures that used to have their "counter" declared
> as volatile will lose volatile semantics on atomic_set() with your
> patch, which might result in bugs due to overly imaginative compiler
> optimizations. The above would prevent any such bugs from appearing.
>
>> A
>> write to an lvalue already implies an eventual write to memory, so this
>> would be a no-op. Maybe you'll write to the register a few times before
>> flushing it to memory, but it will happen eventually. With an rvalue,
>> there's no guarantee that it will *ever* load from memory, which is what
>> volatile fixes.
>>
>> I think what you have in mind is LOCK_PREFIX behavior, which is not the
>> purpose of atomic_set. We use LOCK_PREFIX in the inline assembly for the
>> atomic_* operations that read, modify, and write a value, only because it
>> is necessary to perform that entire transaction atomically.
>
> No LOCK_PREFIX, thank you!!! I just want to make sure that the compiler
> doesn't push the store down out of a loop, split the store, allow the
> store to happen twice (e.g., to allow different code paths to be merged),
> and all the other tricks that the C standard permits compilers to pull.
We can't have split stores because we don't use atomic64_t on 32-bit
architectures. volatile won't save you from pushing stores out of loops unless
you're also doing reads. This is why we use reads to flush writes to mmio
registers. In this case, an atomic_read() with volatile in it will suffice.
Storing twice is perfectly legal here, though it's unlikely that an optimizing
compiler smart enough to create the problem we're addressing would ever do that.
-- Chris
^ 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