* Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered
From: Gerrit Renker @ 2006-06-12 7:22 UTC (permalink / raw)
To: David Miller; +Cc: netdev, kaber, jmorris
In-Reply-To: <20060611.231300.95505265.davem@davemloft.net>
The code below implements the discussed solution of decrementing
InDatagrams if a datagram fails the checksum within udp_recvmsg().
I have given it a quick test / build and checked the outcome against
previous results: I now obtained correct counter values, i.e. the application
counted exactly InDatagrams datagrams, whereas with the same settings before
this was not the case (datagrams with checksum errors were counted both as
InErrors and as InDatagrams).
Will add this patch to http://bugzilla.kernel.org/show_bug.cgi?id=6660
where this problem is also described. Patches under 2.6.16 with no complaints.
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
include/net/snmp.h | 2 ++
include/net/udp.h | 1 +
net/ipv4/udp.c | 1 +
3 files changed, 4 insertions(+)
diff -Nurp a/include/net/snmp.h b/include/net/snmp.h
--- a/include/net/snmp.h 2006-06-05 21:52:55.000000000 +0100
+++ b/include/net/snmp.h 2006-06-12 07:38:11.000000000 +0100
@@ -137,6 +137,8 @@ struct linux_mib {
(per_cpu_ptr(mib[!in_softirq()], raw_smp_processor_id())->mibs[field]++)
#define SNMP_DEC_STATS(mib, field) \
(per_cpu_ptr(mib[!in_softirq()], raw_smp_processor_id())->mibs[field]--)
+#define SNMP_DEC_STATS_BH(mib, field) \
+ (per_cpu_ptr(mib[0], raw_smp_processor_id())->mibs[field]--)
#define SNMP_ADD_STATS_BH(mib, field, addend) \
(per_cpu_ptr(mib[0], raw_smp_processor_id())->mibs[field] += addend)
#define SNMP_ADD_STATS_USER(mib, field, addend) \
diff -Nurp a/include/net/udp.h b/include/net/udp.h
--- a/include/net/udp.h 2006-06-06 18:04:36.000000000 +0100
+++ b/include/net/udp.h 2006-06-12 07:39:29.000000000 +0100
@@ -78,6 +78,7 @@ DECLARE_SNMP_STAT(struct udp_mib, udp_st
#define UDP_INC_STATS(field) SNMP_INC_STATS(udp_statistics, field)
#define UDP_INC_STATS_BH(field) SNMP_INC_STATS_BH(udp_statistics, field)
#define UDP_INC_STATS_USER(field) SNMP_INC_STATS_USER(udp_statistics, field)
+#define UDP_DEC_STATS_BH(field) SNMP_DEC_STATS_BH(udp_statistics, field)
/* /proc */
struct udp_seq_afinfo {
diff -Nurp a/net/ipv4/udp.c b/net/ipv4/udp.c
--- a/net/ipv4/udp.c 2006-06-07 20:44:13.000000000 +0100
+++ b/net/ipv4/udp.c 2006-06-12 07:40:02.000000000 +0100
@@ -846,6 +846,7 @@ out:
csum_copy_err:
UDP_INC_STATS_BH(UDP_MIB_INERRORS);
+ UDP_DEC_STATS_BH(UDP_MIB_INDATAGRAMS);
skb_kill_datagram(sk, skb, flags);
^ permalink raw reply
* Re: [PATCH 3/5] ehea: queue management
From: Jan-Bernd Themann @ 2006-06-12 7:14 UTC (permalink / raw)
To: John Rose; +Cc: netdev, Marcus Eder, Christoph Raisch, themann, tklein
In-Reply-To: <1149878470.28483.6.camel@sinatra.austin.ibm.com>
John Rose wrote:
>> +#define EHEA_MEM_START 0xc000000000000000
>
> You probably don't want to hardcode this. Maybe KERNELBASE from page.h?
Yes, we want to use KERNELBASE
>
>> +
>> +int ehea_reg_mr_adapter(struct ehea_adapter *adapter)
>> +{
>> + }
>
> This creates DMA mappings for the entirety of kernel memory, right? Has
> this been run by the ppc64 folks for possible impacts?
Yes, that is correct. We have tested it on our ppc64 without running into any
problems. There should be no impacts on other kernel parts, but we haven't
talked to the ppc64 folks yet.
Jan-Bernd
^ permalink raw reply
* Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered
From: Herbert Xu @ 2006-06-12 7:43 UTC (permalink / raw)
To: David Miller; +Cc: gerrit, netdev, kaber, jmorris
In-Reply-To: <20060611.234905.35014718.davem@davemloft.net>
On Sun, Jun 11, 2006 at 11:49:05PM -0700, David Miller wrote:
>
> Yeah. Good point. But how much protocol internals do we want to
> slide into the ->data_ready() callbacks of such layers? That's ugly
> and something we should try to avoid.
I agree with the objective of minimising the exposure of internals.
However, in this particular instance we're already exposing much more
than a couple of UDP SNMP counters in the sunrpc code. This makes
adjusting the counters there the most expedient course of action.
Longer term we probably want to restructure the code a bit so that
more if it moves to udp.c.
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: MIB "ipInHdrErrors" error
From: Wei Dong @ 2006-06-12 8:49 UTC (permalink / raw)
To: netdev; +Cc: davem
On Mon, 2006-06-12 at 15:24 +0800, Wei Dong wrote:
> > Also, when kernel receives an IP packet and need to forward, but
> > TTL=1 or TTL=0, kernel just sends an ICMP packet to inform the
> > sender TTL count exceeded, and doesn't increase this counter.
>
> This part of your change seems correct, please just resubmit
> this part.
The following is the latest patch for MIB ipIHdrErrors.
diff -ruN old/net/ipv4/ip_forward.c new/net/ipv4/ip_forward.c
--- old/net/ipv4/ip_forward.c 2006-06-06 13:56:48.000000000 +0800
+++ new/net/ipv4/ip_forward.c 2006-06-12 15:11:04.000000000 +0800
@@ -120,6 +120,7 @@
too_many_hops:
/* Tell the sender its packet died... */
+ IP_INC_STATS_BH(IPSTATS_MIB_INHDRERRORS);
icmp_send(skb, ICMP_TIME_EXCEEDED, ICMP_EXC_TTL, 0);
drop:
kfree_skb(skb);
Signed-off-by: Weidong <weid@nanjing-fnst.com>
^ permalink raw reply
* Re: [patch] undo AF_UNIX _bh locking changes and split lock-type instead
From: Ingo Molnar @ 2006-06-12 8:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Andrew Morton, linux1394-devel, Valdis.Kletnieks, Jiri Slaby,
netdev, linux-kernel, mingo, Stefan Richter, David S. Miller,
arjan
In-Reply-To: <20060612065701.GA24213@elte.hu>
* Ingo Molnar <mingo@elte.hu> wrote:
> (the #ifdef LOCKDEP should probably be converted to some sort of
> lockdep_split_lock_key(&sk->sk_receive_queue.lock) op - i'll do that
> later)
i've added such an op, lockdep_reinit_lock_key() - this makes the patch
cleaner:
----------------------
Subject: undo AF_UNIX _bh locking changes and split lock-type
From: Ingo Molnar <mingo@elte.hu>
this cleans up lock-validator-special-locking-af_unix.patch: instead
of adding _bh locking to AF_UNIX, this patch splits their
sk_receive_queue.lock type from the other networking skb-queue locks.
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
net/unix/af_unix.c | 18 ++++++++++++++----
net/unix/garbage.c | 8 ++++----
2 files changed, 18 insertions(+), 8 deletions(-)
Index: linux/net/unix/af_unix.c
===================================================================
--- linux.orig/net/unix/af_unix.c
+++ linux/net/unix/af_unix.c
@@ -542,6 +542,14 @@ static struct proto unix_proto = {
.obj_size = sizeof(struct unix_sock),
};
+/*
+ * AF_UNIX sockets do not interact with hardware, hence they
+ * dont trigger interrupts - so it's safe for them to have
+ * bh-unsafe locking for their sk_receive_queue.lock. Split off
+ * this special lock-type by reinitializing the spinlock key:
+ */
+static struct lockdep_type_key af_unix_sk_receive_queue_lock_key;
+
static struct sock * unix_create1(struct socket *sock)
{
struct sock *sk = NULL;
@@ -557,6 +565,8 @@ static struct sock * unix_create1(struct
atomic_inc(&unix_nr_socks);
sock_init_data(sock,sk);
+ lockdep_reinit_lock_key(&sk->sk_receive_queue.lock,
+ &af_unix_sk_receive_queue_lock_key);
sk->sk_write_space = unix_write_space;
sk->sk_max_ack_backlog = sysctl_unix_max_dgram_qlen;
@@ -1073,12 +1083,12 @@ restart:
unix_state_wunlock(sk);
/* take ten and and send info to listening sock */
- spin_lock_bh(&other->sk_receive_queue.lock);
+ spin_lock(&other->sk_receive_queue.lock);
__skb_queue_tail(&other->sk_receive_queue, skb);
/* Undo artificially decreased inflight after embrion
* is installed to listening socket. */
atomic_inc(&newu->inflight);
- spin_unlock_bh(&other->sk_receive_queue.lock);
+ spin_unlock(&other->sk_receive_queue.lock);
unix_state_runlock(other);
other->sk_data_ready(other, 0);
sock_put(other);
@@ -1843,7 +1853,7 @@ static int unix_ioctl(struct socket *soc
break;
}
- spin_lock_bh(&sk->sk_receive_queue.lock);
+ spin_lock(&sk->sk_receive_queue.lock);
if (sk->sk_type == SOCK_STREAM ||
sk->sk_type == SOCK_SEQPACKET) {
skb_queue_walk(&sk->sk_receive_queue, skb)
@@ -1853,7 +1863,7 @@ static int unix_ioctl(struct socket *soc
if (skb)
amount=skb->len;
}
- spin_unlock_bh(&sk->sk_receive_queue.lock);
+ spin_unlock(&sk->sk_receive_queue.lock);
err = put_user(amount, (int __user *)arg);
break;
}
Index: linux/net/unix/garbage.c
===================================================================
--- linux.orig/net/unix/garbage.c
+++ linux/net/unix/garbage.c
@@ -235,7 +235,7 @@ void unix_gc(void)
struct sock *x = pop_stack();
struct sock *sk;
- spin_lock_bh(&x->sk_receive_queue.lock);
+ spin_lock(&x->sk_receive_queue.lock);
skb = skb_peek(&x->sk_receive_queue);
/*
@@ -270,7 +270,7 @@ void unix_gc(void)
maybe_unmark_and_push(skb->sk);
skb=skb->next;
}
- spin_unlock_bh(&x->sk_receive_queue.lock);
+ spin_unlock(&x->sk_receive_queue.lock);
sock_put(x);
}
@@ -283,7 +283,7 @@ void unix_gc(void)
if (u->gc_tree == GC_ORPHAN) {
struct sk_buff *nextsk;
- spin_lock_bh(&s->sk_receive_queue.lock);
+ spin_lock(&s->sk_receive_queue.lock);
skb = skb_peek(&s->sk_receive_queue);
while (skb &&
skb != (struct sk_buff *)&s->sk_receive_queue) {
@@ -298,7 +298,7 @@ void unix_gc(void)
}
skb = nextsk;
}
- spin_unlock_bh(&s->sk_receive_queue.lock);
+ spin_unlock(&s->sk_receive_queue.lock);
}
u->gc_tree = GC_ORPHAN;
}
^ permalink raw reply
* Re[1]:Aloha
From: Galya B. @ 2006-06-12 10:05 UTC (permalink / raw)
To: linux-x25
Greeting!
I think we had correspondence a long time ago if it was not you I am sorry.
If it was I could not answer you because my Mozilla mail manager was down for a
long time and I could not fix it only with my friend's help I got the emails
address out for me ..:)
I hope it was whom we were corresponded with you are still interested, as I am,
though I realize much time has passed since then...
I really don't know where to start ....
Maybe you could tell me a little about yourself since I lost our early letters,
your appearance,age , hobbies, and are you still in the search?
If it was you I wrote to and you are interested to get to know me better, I have
a profile at :
http://www.5HUMbcTu8XpXQKDf.im-waiting-4you.net/
Don't really know what else to say for now I hope this is the right address
Let me know if you are interested, And I hope
you won't run when you see my picture :-)
Best regards,
Galya B.
^ permalink raw reply
* Re: [RFT] Realtek 8168 ethernet support
From: Mourad De Clerck @ 2006-06-12 10:30 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev
In-Reply-To: <20060611233022.GA11776@electric-eye.fr.zoreil.com>
On 12/06/06 01:30, Francois Romieu wrote:
> The patch below agaisnt 2.6.17-rc6 includes the following changes:
Just FYI:
I just tried this patch set, but it doesn't do anything for the "freeze
at high speed" I mentioned on 2006-06-09. It still locks up. (As an
additional data point: I installed win2k on this machine, and it seems
to have no problems transferring at high speeds. Just wanted to try to
rule out faulty hardware.)
Thanks,
-- Mourad DC
^ permalink raw reply
* Re: Using netconsole for debugging suspend/resume
From: Andi Kleen @ 2006-06-12 11:21 UTC (permalink / raw)
To: Mark Lord
Cc: Jeremy Fitzhardinge, Matt Mackall, Linux Kernel Mailing List,
netdev
In-Reply-To: <448992B7.1050906@rtr.ca>
On Friday 09 June 2006 17:24, Mark Lord wrote:
> Andi Kleen wrote:
> >
> > If your laptop has firewire you can also use firescope.
> > (ftp://ftp.suse.com/pub/people/ak/firescope/)
> ..
> > FW keeps running as long as nobody resets the ieee1394 chip.
>
> This looks interesting. But how does one set it up for use
> on the *other* end of that firewire cable? The Quickstart and
> manpage don't seem to describe this fully.
It's in the manpage:
>.SH NOTES
>The target must have the ohci1394 driver loaded. This implies
>that firescope cannot be used in early boot.
That's it.
-Andi
^ permalink raw reply
* Re: link-local address via ifconfig
From: Ingo Oeser @ 2006-06-12 14:02 UTC (permalink / raw)
To: Anand Kumria; +Cc: netdev
In-Reply-To: <20060603025743.GL549@progsoc.uts.edu.au>
Hi Anand,
Anand Kumria wrote:
> There are plenty of people who still use ifconfig to list the addresses
> assigned to their network interfaces (I know, ifconfig is broken) and
> who then parse the output.
>
> However the kernel puts link-local scoped address first if the address
> list of an interface, so an interface like:
> Is there any reason to put the link-local address first in the list?
>
> I've had a number of bugreports (or outright panic attacks) where the
> problem turned out to be that ifconfig was reporting the link-local
> address first, rather than the global/site one.
This can be worked around by defining an "alias interface" this way
"ip address add 169.254.182.108/16 brd 169.254.255.255 scope link dev eth0 label eth0:0"
(see: "label eth0:0" is appended)
So in reality this is no problem for users.
Regards
Ingo Oeser
^ permalink raw reply
* [PATCH] bcm43xx: suspend MAC while executing long pwork
From: Michael Buesch @ 2006-06-12 15:02 UTC (permalink / raw)
To: John Linville; +Cc: bcm43xx-dev, netdev
Hi John,
Please queue this for 2.6.18.
--
Suspend MAC (and make MAC-suspend refcounting) when doing
long periodic work.
On long periodic work, we disable IRQs on the device, so
we don't want the MAC to stay operating and probably miss
packets due do non-delivery of interrupts.
Signed-off-by: Michael Buesch <mb@bu3sch.de>
Index: wireless-dev/drivers/net/wireless/bcm43xx/bcm43xx.h
===================================================================
--- wireless-dev.orig/drivers/net/wireless/bcm43xx/bcm43xx.h 2006-06-05 18:19:59.000000000 +0200
+++ wireless-dev/drivers/net/wireless/bcm43xx/bcm43xx.h 2006-06-12 16:20:28.000000000 +0200
@@ -723,6 +723,8 @@
u32 irq_savedstate;
/* Link Quality calculation context. */
struct bcm43xx_noise_calculation noisecalc;
+ /* if > 0 MAC is suspended. if == 0 MAC is enabled. */
+ int mac_suspended;
/* Threshold values. */
//TODO: The RTS thr has to be _used_. Currently, it is only set via WX.
Index: wireless-dev/drivers/net/wireless/bcm43xx/bcm43xx_main.c
===================================================================
--- wireless-dev.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.c 2006-06-05 20:17:36.000000000 +0200
+++ wireless-dev/drivers/net/wireless/bcm43xx/bcm43xx_main.c 2006-06-12 16:41:41.000000000 +0200
@@ -2284,13 +2284,17 @@
/* http://bcm-specs.sipsolutions.net/EnableMac */
void bcm43xx_mac_enable(struct bcm43xx_private *bcm)
{
- bcm43xx_write32(bcm, BCM43xx_MMIO_STATUS_BITFIELD,
- bcm43xx_read32(bcm, BCM43xx_MMIO_STATUS_BITFIELD)
- | BCM43xx_SBF_MAC_ENABLED);
- bcm43xx_write32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON, BCM43xx_IRQ_READY);
- bcm43xx_read32(bcm, BCM43xx_MMIO_STATUS_BITFIELD); /* dummy read */
- bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON); /* dummy read */
- bcm43xx_power_saving_ctl_bits(bcm, -1, -1);
+ bcm->mac_suspended--;
+ assert(bcm->mac_suspended >= 0);
+ if (bcm->mac_suspended == 0) {
+ bcm43xx_write32(bcm, BCM43xx_MMIO_STATUS_BITFIELD,
+ bcm43xx_read32(bcm, BCM43xx_MMIO_STATUS_BITFIELD)
+ | BCM43xx_SBF_MAC_ENABLED);
+ bcm43xx_write32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON, BCM43xx_IRQ_READY);
+ bcm43xx_read32(bcm, BCM43xx_MMIO_STATUS_BITFIELD); /* dummy read */
+ bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON); /* dummy read */
+ bcm43xx_power_saving_ctl_bits(bcm, -1, -1);
+ }
}
/* http://bcm-specs.sipsolutions.net/SuspendMAC */
@@ -2299,18 +2303,23 @@
int i;
u32 tmp;
- bcm43xx_power_saving_ctl_bits(bcm, -1, 1);
- bcm43xx_write32(bcm, BCM43xx_MMIO_STATUS_BITFIELD,
- bcm43xx_read32(bcm, BCM43xx_MMIO_STATUS_BITFIELD)
- & ~BCM43xx_SBF_MAC_ENABLED);
- bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON); /* dummy read */
- for (i = 100000; i; i--) {
- tmp = bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON);
- if (tmp & BCM43xx_IRQ_READY)
- return;
- udelay(10);
+ assert(bcm->mac_suspended >= 0);
+ if (bcm->mac_suspended == 0) {
+ bcm43xx_power_saving_ctl_bits(bcm, -1, 1);
+ bcm43xx_write32(bcm, BCM43xx_MMIO_STATUS_BITFIELD,
+ bcm43xx_read32(bcm, BCM43xx_MMIO_STATUS_BITFIELD)
+ & ~BCM43xx_SBF_MAC_ENABLED);
+ bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON); /* dummy read */
+ for (i = 100000; i; i--) {
+ tmp = bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON);
+ if (tmp & BCM43xx_IRQ_READY)
+ goto out;
+ udelay(10);
+ }
+ printkl(KERN_ERR PFX "MAC suspend failed\n");
}
- printkl(KERN_ERR PFX "MAC suspend failed\n");
+out:
+ bcm->mac_suspended++;
}
void bcm43xx_set_iwmode(struct bcm43xx_private *bcm,
@@ -3168,8 +3177,10 @@
/* Periodic work will take a long time, so we want it to
* be preemtible.
*/
- bcm43xx_lock_irqonly(bcm, flags);
netif_stop_queue(bcm->net_dev);
+ synchronize_net();
+ bcm43xx_lock_irqonly(bcm, flags);
+ bcm43xx_mac_suspend(bcm);
if (bcm43xx_using_pio(bcm))
bcm43xx_pio_freeze_txqueues(bcm);
savedirqs = bcm43xx_interrupt_disable(bcm, BCM43xx_IRQ_ALL);
@@ -3192,6 +3203,7 @@
bcm43xx_interrupt_enable(bcm, savedirqs);
if (bcm43xx_using_pio(bcm))
bcm43xx_pio_thaw_txqueues(bcm);
+ bcm43xx_mac_enable(bcm);
}
netif_wake_queue(bcm->net_dev);
mmiowb();
@@ -3774,6 +3786,7 @@
bcm->softmac->set_channel = bcm43xx_ieee80211_set_chan;
bcm->irq_savedstate = BCM43xx_IRQ_INITIAL;
+ bcm->mac_suspended = 1;
bcm->pci_dev = pci_dev;
bcm->net_dev = net_dev;
bcm->bad_frames_preempt = modparam_bad_frames_preempt;
--
Greetings Michael.
^ permalink raw reply
* Re: [PATCH 2.6.17-rc6] Remove Prism II support from Orinoco
From: John W. Linville @ 2006-06-12 15:24 UTC (permalink / raw)
To: Faidon Liambotis; +Cc: Dave Jones, netdev, proski, hermes
In-Reply-To: <20060611224954.GA2880@divinity>
On Mon, Jun 12, 2006 at 01:49:54AM +0300, Faidon Liambotis wrote:
> Having two drivers supporting the same set of hardware seems pretty
> pointless to me. Plus, it confuses hotplugging/automatic detection.
This subject comes-up from time to time. In fact, I'm pretty sure
it came-up very recently w.r.t. orinoco and hostap.
The consensus seems to be that drivers should have IDs for all devices
they support, even if that means that some devices are supported by
multiple drivers. This leaves the choice of which driver to use in
the hands of the user and/or distro.
If the Orinoco guys want this patch, I'll consider it. Otherwise,
I'm not inclined to take it.
John
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply
* Re: Using netconsole for debugging suspend/resume
From: Mark Lord @ 2006-06-12 15:38 UTC (permalink / raw)
To: Andi Kleen
Cc: Jeremy Fitzhardinge, Matt Mackall, Linux Kernel Mailing List,
netdev
In-Reply-To: <200606121321.30388.ak@suse.de>
Andi Kleen wrote:
> On Friday 09 June 2006 17:24, Mark Lord wrote:
>> Andi Kleen wrote:
>>> If your laptop has firewire you can also use firescope.
>>> (ftp://ftp.suse.com/pub/people/ak/firescope/)
>> ..
>>> FW keeps running as long as nobody resets the ieee1394 chip.
>> This looks interesting. But how does one set it up for use
>> on the *other* end of that firewire cable? The Quickstart and
>> manpage don't seem to describe this fully.
>
> It's in the manpage:
>
>> .SH NOTES
>> The target must have the ohci1394 driver loaded. This implies
>> that firescope cannot be used in early boot.
>
> That's it.
Okay, so I'm daft. But.. *what* is "it" ??
We have two machines: target (being debugged), and host (anything).
Sure, the target has to have ohci1394 loaded, and firescope running.
But what about the *other* end of the connection? What commands?
Thanks
^ permalink raw reply
* [PATCH] netpoll: break recursive loop in netpoll rx path
From: Neil Horman @ 2006-06-12 15:40 UTC (permalink / raw)
To: netdev; +Cc: mpm, jmoyer
Hey there-
the netpoll system currently has a rx to tx path via:
netpoll_rx
__netpoll_rx
arp_reply
netpoll_send_skb
dev->hard_start_tx
This rx->tx loop places network drivers at risk of inadvertently causing
a deadlock or BUG halt by recursively trying to acquire a spinlock that is used
in both their rx and tx paths (this problem was origionally reported to me in
the 3c59x driver, which shares a spinlock between the boomerang_interrupt and
boomerang_start_xmit routines). This patch breaks this loop, by queueing arp
frames, so that they can be responded to after all receive operations have been
completed. Tested by myself and the reported with successful results.
Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
include/linux/netpoll.h | 1 +
net/core/netpoll.c | 17 +++++++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
--- linux-2.6/include/linux/netpoll.h.orig 2006-06-12 09:11:01.000000000 -0400
+++ linux-2.6/include/linux/netpoll.h 2006-06-12 09:34:17.000000000 -0400
@@ -31,6 +31,7 @@ struct netpoll_info {
int rx_flags;
spinlock_t rx_lock;
struct netpoll *rx_np; /* netpoll that registered an rx_hook */
+ struct sk_buff_head arp_tx; /* list of arp requests to reply to */
};
void netpoll_poll(struct netpoll *np);
--- linux-2.6/net/core/netpoll.c.orig 2006-06-12 09:11:01.000000000 -0400
+++ linux-2.6/net/core/netpoll.c 2006-06-12 11:10:13.000000000 -0400
@@ -54,6 +54,7 @@ static atomic_t trapped;
sizeof(struct iphdr) + sizeof(struct ethhdr))
static void zap_completion_queue(void);
+static void arp_reply(struct sk_buff *skb);
static void queue_process(void *p)
{
@@ -155,14 +156,23 @@ static void poll_napi(struct netpoll *np
void netpoll_poll(struct netpoll *np)
{
+ struct sk_buff *skb;
+ struct netpoll_info *npi;
+
if(!np->dev || !netif_running(np->dev) || !np->dev->poll_controller)
return;
+ npi = np->dev->npinfo;
+
/* Process pending work on NIC */
np->dev->poll_controller(np->dev);
if (np->dev->poll)
poll_napi(np);
+ if (likely(npi))
+ while ((skb = skb_dequeue(&npi->arp_tx)) != NULL)
+ arp_reply(skb);
+
zap_completion_queue();
}
@@ -449,7 +459,9 @@ int __netpoll_rx(struct sk_buff *skb)
int proto, len, ulen;
struct iphdr *iph;
struct udphdr *uh;
- struct netpoll *np = skb->dev->npinfo->rx_np;
+ struct netpoll_info *npi = skb->dev->npinfo;
+ struct netpoll *np = npi->rx_np;
+
if (!np)
goto out;
@@ -459,7 +471,7 @@ int __netpoll_rx(struct sk_buff *skb)
/* check if netpoll clients need ARP */
if (skb->protocol == __constant_htons(ETH_P_ARP) &&
atomic_read(&trapped)) {
- arp_reply(skb);
+ skb_queue_tail(&npi->arp_tx, skb);
return 1;
}
@@ -654,6 +666,7 @@ int netpoll_setup(struct netpoll *np)
npinfo->poll_owner = -1;
npinfo->tries = MAX_RETRIES;
spin_lock_init(&npinfo->rx_lock);
+ skb_queue_head_init(&npinfo->arp_tx);
} else
npinfo = ndev->npinfo;
--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/
^ permalink raw reply
* Re: Using netconsole for debugging suspend/resume
From: Andi Kleen @ 2006-06-12 15:46 UTC (permalink / raw)
To: Mark Lord
Cc: Jeremy Fitzhardinge, Matt Mackall, Linux Kernel Mailing List,
netdev
In-Reply-To: <448D8A90.3040508@rtr.ca>
On Monday 12 June 2006 17:38, Mark Lord wrote:
> Andi Kleen wrote:
> > On Friday 09 June 2006 17:24, Mark Lord wrote:
> >> Andi Kleen wrote:
> >>> If your laptop has firewire you can also use firescope.
> >>> (ftp://ftp.suse.com/pub/people/ak/firescope/)
> >> ..
> >>> FW keeps running as long as nobody resets the ieee1394 chip.
> >> This looks interesting. But how does one set it up for use
> >> on the *other* end of that firewire cable? The Quickstart and
> >> manpage don't seem to describe this fully.
> >
> > It's in the manpage:
> >
> >> .SH NOTES
> >> The target must have the ohci1394 driver loaded. This implies
> >> that firescope cannot be used in early boot.
> >
> > That's it.
>
> Okay, so I'm daft. But.. *what* is "it" ??
>
> We have two machines: target (being debugged), and host (anything).
> Sure, the target has to have ohci1394 loaded, and firescope running.
> But what about the *other* end of the connection? What commands?
>From the same manpage:
"The raw1394 module must be loaded and its device node
be writable (this normally requires root)"
Ok it doesn't say you need ohci1394 too and doesn't say that's the target.
If I do a new revision I'll perhaps expand the docs a bit.
So load ohci1394/raw1394 and run firescope as root. Your distribution
will hopefully take care of the device nodes. Usually you want
something like firescope -Au System.map
-Andi
>
^ permalink raw reply
* Re: [PATCH v2 1/2] iWARP Connection Manager.
From: Tom Tucker @ 2006-06-12 15:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Steve Wise, rdreier, mshefty, linux-kernel, netdev,
openib-general
In-Reply-To: <20060608005452.087b34db.akpm@osdl.org>
Andrew, thanks for the review, comments inline...
On Thu, 2006-06-08 at 00:54 -0700, Andrew Morton wrote:
> On Wed, 07 Jun 2006 15:06:05 -0500
> Steve Wise <swise@opengridcomputing.com> wrote:
>
> >
> > This patch provides the new files implementing the iWARP Connection
> > Manager.
> >
> > Review Changes:
> >
> > - sizeof -> sizeof()
> >
> > - removed printks
> >
> > - removed TT debug code
> >
> > - cleaned up lock/unlock around switch statements.
> >
> > - waitqueue -> completion for destroy path.
> >
> > ...
> >
> > +/*
> > + * This function is called on interrupt context. Schedule events on
> > + * the iwcm_wq thread to allow callback functions to downcall into
> > + * the CM and/or block. Events are queued to a per-CM_ID
> > + * work_list. If this is the first event on the work_list, the work
> > + * element is also queued on the iwcm_wq thread.
> > + *
> > + * Each event holds a reference on the cm_id. Until the last posted
> > + * event has been delivered and processed, the cm_id cannot be
> > + * deleted.
> > + */
> > +static void cm_event_handler(struct iw_cm_id *cm_id,
> > + struct iw_cm_event *iw_event)
> > +{
> > + struct iwcm_work *work;
> > + struct iwcm_id_private *cm_id_priv;
> > + unsigned long flags;
> > +
> > + work = kmalloc(sizeof(*work), GFP_ATOMIC);
> > + if (!work)
> > + return;
>
> This allocation _will_ fail sometimes. The driver must recover from it.
> Will it do so?
Er...no. It will lose this event. Depending on the event...the carnage
varies. We'll take a look at this.
>
> > +EXPORT_SYMBOL(iw_cm_init_qp_attr);
>
> This file exports a ton of symbols. It's usual to provide some justifying
> commentary in the changelog when this happens.
This module is a logical instance of the xx_cm where xx is the transport
type. I think there is some discussion warranted on whether or not these
should all be built into and exported by rdma_cm. One rationale would be
that the rdma_cm is the only client for many of these functions (this
being a particularly good example) and doing so would reduce the export
count. Others would be reasonably needed for any application (connect,
etc...)
All that said, we'll be sure to document the exported symbols in a
follow-up patch.
>
> > +/*
> > + * Copyright (c) 2005 Network Appliance, Inc. All rights reserved.
> > + * Copyright (c) 2005 Open Grid Computing, Inc. All rights reserved.
> > + *
> > + * This software is available to you under a choice of one of two
> > + * licenses. You may choose to be licensed under the terms of the GNU
> > + * General Public License (GPL) Version 2, available from the file
> > + * COPYING in the main directory of this source tree, or the
> > + * OpenIB.org BSD license below:
> > + *
> > + * Redistribution and use in source and binary forms, with or
> > + * without modification, are permitted provided that the following
> > + * conditions are met:
> > + *
> > + * - Redistributions of source code must retain the above
> > + * copyright notice, this list of conditions and the following
> > + * disclaimer.
> > + *
> > + * - Redistributions in binary form must reproduce the above
> > + * copyright notice, this list of conditions and the following
> > + * disclaimer in the documentation and/or other materials
> > + * provided with the distribution.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + */
> > +#if !defined(IW_CM_PRIVATE_H)
> > +#define IW_CM_PRIVATE_H
>
> We normally use #ifndef here.
We'll change this..
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 4/7] AMSO1100 Memory Management.
From: Tom Tucker @ 2006-06-12 16:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Steve Wise, rdreier, mshefty, linux-kernel, netdev,
openib-general
In-Reply-To: <20060608011744.1a66e85a.akpm@osdl.org>
On Thu, 2006-06-08 at 01:17 -0700, Andrew Morton wrote:
> On Wed, 07 Jun 2006 15:06:55 -0500
> Steve Wise <swise@opengridcomputing.com> wrote:
>
> >
> > +void c2_free(struct c2_alloc *alloc, u32 obj)
> > +{
> > + spin_lock(&alloc->lock);
> > + clear_bit(obj, alloc->table);
> > + spin_unlock(&alloc->lock);
> > +}
>
> The spinlock is unneeded here.
Good point.
>
>
> What does all the code in this file do, anyway? It looks totally generic
> (and hence inappropriate for drivers/infiniband/hw/amso1100/) and somewhat
> similar to idr trees, perhaps.
>
We mimicked the mthca driver. It may be code that should be replaced
with Linux core services for new drivers. We'll investigate.
> > +int c2_array_set(struct c2_array *array, int index, void *value)
> > +{
> > + int p = (index * sizeof(void *)) >> PAGE_SHIFT;
> > +
> > + /* Allocate with GFP_ATOMIC because we'll be called with locks held. */
> > + if (!array->page_list[p].page)
> > + array->page_list[p].page =
> > + (void **) get_zeroed_page(GFP_ATOMIC);
> > +
> > + if (!array->page_list[p].page)
> > + return -ENOMEM;
>
> This _will_ happen under load. What will the result of that be, in the
> context of thise driver?
A higher level object allocation will fail. In this case, a kernel
application request will fail and the application must handle the error.
>
> This function is incorrectly designed - it should receive a gfp_t argument.
> Because you don't *know* that the caller will always hold a spinlock. And
> GFP_KERNEL is far, far stronger than GFP_ATOMIC.
This service is allocating a page that the adapter will DMA 2B message
indices into.
>
> > +static int c2_alloc_mqsp_chunk(gfp_t gfp_mask, struct sp_chunk **head)
> > +{
> > + int i;
> > + struct sp_chunk *new_head;
> > +
> > + new_head = (struct sp_chunk *) __get_free_page(gfp_mask | GFP_DMA);
>
> Why is __GFP_DMA in there? Unless you've cornered the ISA bus infiniband
> market, it's likely to be wrong.
>
Flag confusion about what GFP_DMA means. We'll revisit this whole
file ...
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] netpoll: break recursive loop in netpoll rx path
From: Matt Mackall @ 2006-06-12 15:51 UTC (permalink / raw)
To: Neil Horman; +Cc: netdev, jmoyer
In-Reply-To: <20060612154029.GA3790@hmsreliant.homelinux.net>
On Mon, Jun 12, 2006 at 11:40:29AM -0400, Neil Horman wrote:
> Hey there-
> the netpoll system currently has a rx to tx path via:
> netpoll_rx
> __netpoll_rx
> arp_reply
> netpoll_send_skb
> dev->hard_start_tx
>
> This rx->tx loop places network drivers at risk of
> inadvertently causing a deadlock or BUG halt by recursively trying
> to acquire a spinlock that is used in both their rx and tx paths
> (this problem was origionally reported to me in the 3c59x driver,
> which shares a spinlock between the boomerang_interrupt and
> boomerang_start_xmit routines).
Grumble.
> This patch breaks this loop, by queueing arp frames, so that they
> can be responded to after all receive operations have been
> completed. Tested by myself and the reported with successful
> results.
Tested how? kgdb?
> + if (likely(npi))
> + while ((skb = skb_dequeue(&npi->arp_tx)) != NULL)
> + arp_reply(skb);
> +
Assignment inside tests is frowned upon. I'd prefer pulling this out
to its own function too.
Mathematics is the supreme nostalgia of our time.
^ permalink raw reply
* Re: [PATCH 1/2] PHYLIB: Add get_link ethtool helper function
From: Nathaniel Case @ 2006-06-12 16:12 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Andy Fleming, netdev, galak
In-Reply-To: <448C4131.3000504@garzik.org>
On Sun, 2006-06-11 at 12:13 -0400, Jeff Garzik wrote:
> > This adds a phy_ethtool_get_link() function along the same lines as
> > phy_ethtool_gset(). This provides drivers utilizing PHYLIB an
> > alternative to using ethtool_op_get_link(). This is more desirable
> > since the "Link detected" field in ethtool would actually reflect the
> > state of the PHY register.
> >
[snip]
> NAK, needs an EXPORT
In that case, shouldn't we EXPORT phy_ethtool_sset() and
phy_ethtool_gset() in drivers/net/phy/phy.c as well?
- Nate Case <ncase@xes-inc.com>
^ permalink raw reply
* Re: [PATCH 1/2] e1000: fix netpoll with NAPI
From: Mitch Williams @ 2006-06-12 16:42 UTC (permalink / raw)
To: Neil Horman
Cc: Jeff Moyer, Kok, Auke-jan H, Matt Mackall, Garzik, Jeff, netdev,
Brandeburg, Jesse, Kok, Auke
In-Reply-To: <20060612001356.GA5112@localhost.localdomain>
On Sun, 2006-06-11 at 17:13 -0700, Neil Horman wrote:
> Any further thoughts on this guys? I still think my last solution
> solves all of
> the netpoll problems, and isn't going to have any noticable impact on
> performance.
>
I haven't had time to evaluate performance on your patch (sorry!), but
after thinking about it, I agree that it should not have any noticeable
impact. OTOH, performance tuning is a funny thing, and things you think
won't cause problems often do.
Anyway, I'm still not quite ready to ACK this because it's just not
future-proof. Eventually, we will need to support multiple RX queues,
and this solution will not work in that situation.
A simpler short-term solution is just to schedule our NAPI polling on
the "real" netdev instead of our polling netdev. This is a trivial
change and works correctly with a single queue. But, like your patch,
it isn't future-proof.
So, I'm still thinking and pondering on this one.
If we get a patch in to fix the recursive loop in netpoll, my original
patch will work, right? Or is there still another issue?
-Mitch
^ permalink raw reply
* Heads up: additional work on SELinux xfrm
From: Venkat Yekkirala @ 2006-06-12 16:47 UTC (permalink / raw)
To: netdev; +Cc: Chad Hanson, Darrel Goeddel
Hello,
This is just a heads up for anyone interested that I have submitted a patch
set that involves core networking and the xfrm subsystem to the selinux
list. I plan on submitting it to this list as soon as it gets properly
reviewed on the SELinux side.
I am shooting for the 2.6.18 window.
Broad description/overview of changes at:
http://marc.theaimsgroup.com/?l=selinux&m=115012769919108&w=2
Core networking changes at:
http://marc.theaimsgroup.com/?l=selinux&m=115012770004292&w=2
xfrm subsystem changes at:
http://marc.theaimsgroup.com/?l=selinux&m=115012769903910&w=2
Thanks,
venkat
^ permalink raw reply
* Re: The AI parameter of tcp_highspeed.c (in 2.6.18)
From: Xiaoliang (David) Wei @ 2006-06-12 17:10 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: John Heffner, netdev
In-Reply-To: <20060602120507.7e1206d8@dxpl.pdx.osdl.net>
Thanks, Stephen. But I think there is still a problem with the AIMD
parameter update in HighSpeed TCP code.
Line 125~138 of the code (net/ipv4/tcp_highspeed.c):
/* Update AIMD parameters */
if (tp->snd_cwnd > hstcp_aimd_vals[ca->ai].cwnd) {
while (tp->snd_cwnd > hstcp_aimd_vals[ca->ai].cwnd &&
ca->ai < HSTCP_AIMD_MAX - 1)
ca->ai++;
} else if (tp->snd_cwnd < hstcp_aimd_vals[ca->ai].cwnd) {
while (tp->snd_cwnd > hstcp_aimd_vals[ca->ai].cwnd &&
ca->ai > 0)
ca->ai--;
In fact, the second part (decreasing ca->ai) never decreases since the
while loop's inequality is in the reverse direction. This leads to
unfairness with multiple flows (once a flow happens to enjoy a higher
ca->ai, it keeps enjoying that even its cwnd decreases)
Here is a tentative fix (I also added a comment, trying to keep the
change clear):
--- /home/weixl/linux-2.6.16.18/net/ipv4/tcp_highspeed.c 2006-05-22
11:04:35.000000000 -0700
+++ /home/weixl/linux-2.6.16.18/net/ipv4/tcp_highspeed.c.new 2006-06-12
09:56:40.000000000 -0700
@@ -123,13 +123,13 @@
tcp_slow_start(tp);
else {
/* Update AIMD parameters */
+ /* We want to guarantee that hstcp_aimd_vals[ca->ai-1].cwnd <
snd_cwnd <= hstcp_aimd_vals[ca->ai].cwnd */
if (tp->snd_cwnd > hstcp_aimd_vals[ca->ai].cwnd) {
while (tp->snd_cwnd > hstcp_aimd_vals[ca->ai].cwnd &&
ca->ai < HSTCP_AIMD_MAX - 1)
ca->ai++;
- } else if (tp->snd_cwnd < hstcp_aimd_vals[ca->ai].cwnd) {
- while (tp->snd_cwnd > hstcp_aimd_vals[ca->ai].cwnd &&
- ca->ai > 0)
+ } else if (ca->ai && tp->snd_cwnd <= hstcp_aimd_vals[ca->ai-1].cwnd) {
+ while (ca->ai && tp->snd_cwnd <= hstcp_aimd_vals[ca->ai-1].cwnd)
ca->ai--;
}
Thanks.
-David
On 6/2/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> Went backed and looked at the RFC. The problem was just a simple
> translation of table to C array (0 based). Added this to the TCP testing repository.
>
> Subject: [PATCH] Problem observed by Xiaoliang (David) Wei:
>
> When snd_cwnd is smaller than 38 and the connection is in
> congestion avoidance phase (snd_cwnd > snd_ssthresh), the snd_cwnd
> seems to stop growing.
>
> The additive increase was confused because C array's are 0 based.
>
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
>
> ---
>
> net/ipv4/tcp_highspeed.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> 121685b7b61c8faeb87e6c6f0c346b0fe1c46fd2
> diff --git a/net/ipv4/tcp_highspeed.c b/net/ipv4/tcp_highspeed.c
> index b72fa55..ba7c63c 100644
> --- a/net/ipv4/tcp_highspeed.c
> +++ b/net/ipv4/tcp_highspeed.c
> @@ -135,7 +135,8 @@ static void hstcp_cong_avoid(struct sock
>
> /* Do additive increase */
> if (tp->snd_cwnd < tp->snd_cwnd_clamp) {
> - tp->snd_cwnd_cnt += ca->ai;
> + /* cwnd = cwnd + a(w) / cwnd */
> + tp->snd_cwnd_cnt += ca->ai + 1;
> if (tp->snd_cwnd_cnt >= tp->snd_cwnd) {
> tp->snd_cwnd_cnt -= tp->snd_cwnd;
> tp->snd_cwnd++;
> --
> 1.3.3
>
>
--
Xiaoliang (David) Wei Graduate Student, CS@Caltech
http://davidwei.org
***********************************************
^ permalink raw reply
* Re: [PATCH] wan/sdla section fixes
From: Krzysztof Halasa @ 2006-06-12 17:50 UTC (permalink / raw)
To: Randy.Dunlap; +Cc: netdev, mike.mclagan, akpm
In-Reply-To: <20060608203616.16796ec3.rdunlap@xenotime.net>
Hi,
"Randy.Dunlap" <rdunlap@xenotime.net> writes:
> Priority: tossup.
> netdev->set_config can be called at any time, so these references
> to __initdata would be a real problem.
> However, problem has not been observed AFAIK.
>
> Fix section mismatch warnings:
> WARNING: drivers/net/wan/sdla.o - Section mismatch: reference to .init.data: from .text between 'sdla_set_config' (at offset 0x1b8e) and 'sdla_stats'
> WARNING: drivers/net/wan/sdla.o - Section mismatch: reference to .init.data: from .text between 'sdla_set_config' (at offset 0x1e76) and 'sdla_stats'
Is sdla.c still in use? I remember someone mentioned that Sangoma
drivers are now outside the kernel but I don't know how do they
relate to sdla.c and friend(s).
> static const char* version = "SDLA driver v0.30, 12 Sep 1996,
> mike.mclagan@linux.org";
1996 doesn't look encouraging but it may be misleading.
--
Krzysztof Halasa
^ permalink raw reply
* Re: [PATCH] wan/sdla section fixes
From: Randy.Dunlap @ 2006-06-12 18:05 UTC (permalink / raw)
To: Krzysztof Halasa; +Cc: netdev, mike.mclagan, akpm
In-Reply-To: <m3d5de461t.fsf@defiant.localdomain>
On Mon, 12 Jun 2006 19:50:22 +0200 Krzysztof Halasa wrote:
> Hi,
>
> "Randy.Dunlap" <rdunlap@xenotime.net> writes:
>
> > Priority: tossup.
> > netdev->set_config can be called at any time, so these references
> > to __initdata would be a real problem.
> > However, problem has not been observed AFAIK.
> >
> > Fix section mismatch warnings:
> > WARNING: drivers/net/wan/sdla.o - Section mismatch: reference to .init.data: from .text between 'sdla_set_config' (at offset 0x1b8e) and 'sdla_stats'
> > WARNING: drivers/net/wan/sdla.o - Section mismatch: reference to .init.data: from .text between 'sdla_set_config' (at offset 0x1e76) and 'sdla_stats'
>
> Is sdla.c still in use? I remember someone mentioned that Sangoma
> drivers are now outside the kernel but I don't know how do they
> relate to sdla.c and friend(s).
No idea.
> > static const char* version = "SDLA driver v0.30, 12 Sep 1996,
> > mike.mclagan@linux.org";
>
> 1996 doesn't look encouraging but it may be misleading.
Yep. You could ignore it :) or rm drivers/net/wan/slda* :)
---
~Randy
^ permalink raw reply
* Re: [PATCH 1/2] e1000: fix netpoll with NAPI
From: Neil Horman @ 2006-06-12 18:06 UTC (permalink / raw)
To: Mitch Williams
Cc: Jeff Moyer, Kok, Auke-jan H, Matt Mackall, Garzik, Jeff, netdev,
Brandeburg, Jesse, Kok, Auke
In-Reply-To: <1150130534.2879.9.camel@strongmad>
On Mon, Jun 12, 2006 at 09:42:14AM -0700, Mitch Williams wrote:
> On Sun, 2006-06-11 at 17:13 -0700, Neil Horman wrote:
> > Any further thoughts on this guys? I still think my last solution
> > solves all of
> > the netpoll problems, and isn't going to have any noticable impact on
> > performance.
> >
> I haven't had time to evaluate performance on your patch (sorry!), but
> after thinking about it, I agree that it should not have any noticeable
> impact. OTOH, performance tuning is a funny thing, and things you think
> won't cause problems often do.
>
Thats ok, I just didn't hear out of anyone on friday, so I was curious as to
where we were on this. I don't have the ability to do any real world
performance testing here, but I'll try to record the run time of the interrupt
routine on a limited number of frames here.
> Anyway, I'm still not quite ready to ACK this because it's just not
> future-proof. Eventually, we will need to support multiple RX queues,
> and this solution will not work in that situation.
>
Not sure I understand this. My patch:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114970506406155&w=2
still allows for the use of multiple rx
queues in the nominal case. Only when we have to use a relatively slow netpoll
driven operation (kgdb, netdump, etc), do we need to receive on the same
interface that we transmit on.
> A simpler short-term solution is just to schedule our NAPI polling on
> the "real" netdev instead of our polling netdev. This is a trivial
> change and works correctly with a single queue. But, like your patch,
> it isn't future-proof.
>
Again, not sure what you mean here. My last patch proposal:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114970807606096&w=2
Does precisely what you describe, but it still allows for multiple rx queues in
the nominal (non poll_controller driven) receive case.
> So, I'm still thinking and pondering on this one.
>
> If we get a patch in to fix the recursive loop in netpoll, my original
> patch will work, right? Or is there still another issue?
>
I assume you are referring to this patch:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114970506406155&w=2
If so, then no, that patch is still broken for the reasons Jeff outlined
previously, The recursion patch that I proposed earlier today is related to a
different recursion problem, and while the e1000 driver might be suceptable to
it, your patch is also suceptible to the data corruption that arises from when
the poll_controller calls adapter->clean_rx at the same that that dev->poll is
called for the same adapter on another cpu. If that happens we can have two
cpu's writing to the same private net_device data without the benefit of a
spinlock to protect them. And yes, you can add a spin lock to protect the case
where you have a dev->poll_controller and a dev->poll operation at the same
time, but that seems to me like it will also re-serialize all the parallel
operations that you could otherwise do with multiple rx queues
I'll post again if I get a chance to do some performance measurements
Regrds
Neil
> -Mitch
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/
^ permalink raw reply
* Re: [PATCH 2.6.17-rc6] Remove Prism II support from Orinoco
From: Faidon Liambotis @ 2006-06-12 15:39 UTC (permalink / raw)
To: John W. Linville; +Cc: Dave Jones, netdev, proski, hermes
In-Reply-To: <20060612152434.GA14851@tuxdriver.com>
On Mon, Jun 12, 2006 at 11:24:39AM -0400, John W. Linville wrote:
> On Mon, Jun 12, 2006 at 01:49:54AM +0300, Faidon Liambotis wrote:
>
> > Having two drivers supporting the same set of hardware seems pretty
> > pointless to me. Plus, it confuses hotplugging/automatic detection.
>
> This subject comes-up from time to time. In fact, I'm pretty sure
> it came-up very recently w.r.t. orinoco and hostap.
I remember a patch that added all of Orinoco PCI IDs to HostAP. I'm not
sure if you're referring to that, but that's pretty different (and
obviously wrong).
> The consensus seems to be that drivers should have IDs for all devices
> they support, even if that means that some devices are supported by
> multiple drivers. This leaves the choice of which driver to use in
> the hands of the user and/or distro.
I'd mostly agree to that if distributors had a way to enable/disable
Prism2 support on the orinoco_cs driver based on a build-time
configuration option.
Should I prepare such a patch?
FWIW, I think we've experienced a similar situation like this in the
past in the networking land and the consensus was to completely remove
the other driver. I'm referring to e100/eepro100, of course.
Regards,
Faidon
^ 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