* Re: [PATCH] phylib: Add support for the LXT973 phy.
From: Richard Cochran @ 2010-06-07 15:39 UTC (permalink / raw)
To: David Miller; +Cc: afleming, netdev
In-Reply-To: <20100607.011835.55846752.davem@davemloft.net>
On Mon, Jun 07, 2010 at 01:18:35AM -0700, David Miller wrote:
>
> Richard, please just resubmit your original patch and I will apply it.
Okay, here it is.
Thanks,
Richard
This patch implements a work around for Erratum 5, "3.3 V Fiber Speed
Selection." If the hardware wiring does not respect this erratum, then
fiber optic mode will not work properly.
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
drivers/net/phy/lxt.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 50 insertions(+), 1 deletions(-)
diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 057ecaa..c3de582 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -53,6 +53,9 @@
#define MII_LXT971_ISR 19 /* Interrupt Status Register */
+/* register definitions for the 973 */
+#define MII_LXT973_PCR 16 /* Port Configuration Register */
+#define PCR_FIBER_SELECT 1
MODULE_DESCRIPTION("Intel LXT PHY driver");
MODULE_AUTHOR("Andy Fleming");
@@ -119,6 +122,33 @@ static int lxt971_config_intr(struct phy_device *phydev)
return err;
}
+static int lxt973_probe(struct phy_device *phydev)
+{
+ int val = phy_read(phydev, MII_LXT973_PCR);
+
+ if (val & PCR_FIBER_SELECT) {
+ /*
+ * If fiber is selected, then the only correct setting
+ * is 100Mbps, full duplex, and auto negotiation off.
+ */
+ val = phy_read(phydev, MII_BMCR);
+ val |= (BMCR_SPEED100 | BMCR_FULLDPLX);
+ val &= ~BMCR_ANENABLE;
+ phy_write(phydev, MII_BMCR, val);
+ /* Remember that the port is in fiber mode. */
+ phydev->priv = lxt973_probe;
+ } else {
+ phydev->priv = NULL;
+ }
+ return 0;
+}
+
+static int lxt973_config_aneg(struct phy_device *phydev)
+{
+ /* Do nothing if port is in fiber mode. */
+ return phydev->priv ? 0 : genphy_config_aneg(phydev);
+}
+
static struct phy_driver lxt970_driver = {
.phy_id = 0x78100000,
.name = "LXT970",
@@ -146,6 +176,18 @@ static struct phy_driver lxt971_driver = {
.driver = { .owner = THIS_MODULE,},
};
+static struct phy_driver lxt973_driver = {
+ .phy_id = 0x00137a10,
+ .name = "LXT973",
+ .phy_id_mask = 0xfffffff0,
+ .features = PHY_BASIC_FEATURES,
+ .flags = 0,
+ .probe = lxt973_probe,
+ .config_aneg = lxt973_config_aneg,
+ .read_status = genphy_read_status,
+ .driver = { .owner = THIS_MODULE,},
+};
+
static int __init lxt_init(void)
{
int ret;
@@ -157,9 +199,15 @@ static int __init lxt_init(void)
ret = phy_driver_register(&lxt971_driver);
if (ret)
goto err2;
+
+ ret = phy_driver_register(&lxt973_driver);
+ if (ret)
+ goto err3;
return 0;
- err2:
+ err3:
+ phy_driver_unregister(&lxt971_driver);
+ err2:
phy_driver_unregister(&lxt970_driver);
err1:
return ret;
@@ -169,6 +217,7 @@ static void __exit lxt_exit(void)
{
phy_driver_unregister(&lxt970_driver);
phy_driver_unregister(&lxt971_driver);
+ phy_driver_unregister(&lxt973_driver);
}
module_init(lxt_init);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH net-next-2.6] pkt_sched: gen_estimator: kill est_lock rwlock
From: Eric Dumazet @ 2010-06-07 15:30 UTC (permalink / raw)
To: Changli Gao; +Cc: David Miller, netdev, Stephen Hemminger, Jarek Poplawski
In-Reply-To: <AANLkTimgmYjA7Xp19gycRHDXmwP1w1twn92cfT08tCv2@mail.gmail.com>
Le lundi 07 juin 2010 à 22:53 +0800, Changli Gao a écrit :
> Hmm. I don't think it is correct.
>
> Look at this code:
>
> void xt_rateest_put(struct xt_rateest *est)
> {
> mutex_lock(&xt_rateest_mutex);
> if (--est->refcnt == 0) {
> hlist_del(&est->list);
> gen_kill_estimator(&est->bstats, &est->rstats);
> kfree(est);
> }
> mutex_unlock(&xt_rateest_mutex);
> }
>
> est will be released after gen_kill_estimator() returns. After est is
> freed, there may still be some other CPUs running in the branch:
> if (bstats) {
> ....
> }
>
Oh well, I think I knew this from a previous attempt, but then I
forgot :)
I'll provide an updated patch, so that no other attempt is tried next
yer, thanks !
^ permalink raw reply
* Re: [Linux-ATM-General] RX/close vcc race with solos/atmtcp/usbatm/he
From: Chas Williams (CONTRACTOR) @ 2010-06-07 15:10 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-atm-general, netdev
In-Reply-To: <1275920035.17903.4998.camel@macbook.infradead.org>
In message <1275920035.17903.4998.camel@macbook.infradead.org>,David Woodhouse
writes:
>On Mon, 2010-06-07 at 09:44 -0400, Chas Williams (CONTRACTOR) wrote:
>> vcc's are really sockets, so you could just increase the refcount --
>> sock_hold().
>
>There are rules about where we're allowed to call sock_hold(), and I
>don't think our find_vcc() functions can be made to meet them.
if you are using find_vcc() then you should already have a lock on the
hash table for the vccs. you can safely increment the ref count at
this point.
>In that case I think we might as well stick with the RCU-like solution I
>already implemented in the vcc close function -- which is just to wait
>for the tasklet to complete, thus ensuring that it's no longer using the
>defunct vcc.
the driver shouldnt close the vcc while the tasklet is running and
using the vcc in question. i guess the safest thing is to simply
do as you are doing and not close while running the tasklet.
^ permalink raw reply
* Re: no reassembly for outgoing packets on RAW socket
From: Jiri Olsa @ 2010-06-07 14:55 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev
In-Reply-To: <4C08EB85.3050900@trash.net>
On Fri, Jun 04, 2010 at 02:03:17PM +0200, Patrick McHardy wrote:
> Jiri Olsa wrote:
> > hi,
> >
> > I'd like to be able to sendout a single IP packet with MF flag set.
> >
> > When using RAW sockets the packet will get stuck in the
> > netfilter (NF_INET_LOCAL_OUT nf_defrag_ipv4 reassembly unit)
> > and wont ever make it out..
> >
> > I made a change which bypass the outgoing reassembly for
> > RAW sockets, but I'm not sure wether it's too invasive..
>
> That would break reassembly (and thus connection tracking) for cases
> where its really intended.
>
> > Is there any standard for RAW sockets behaviour?
> > Or another way around? :)
>
> You could use the NOTRACK target to bypass connection tracking.
ok,
I tried the NOTRACK target, but the packet is still going
throught reassembly, because the RAW filter has lower priority
then the connection track defragmentation..
I was able to get it bypassed by attached patch and following
command:
iptables -v -t raw -A OUTPUT -p icmp -j NOTRACK
again, not sure if this is too invasive ;)
If this is not the way, I'd appreciatte any hint.. my goal is
to put malformed packet on the wire (more frags bit set for a
non fragmented packet)
thanks for help,
jirka
---
diff --git a/include/linux/netfilter_ipv4.h b/include/linux/netfilter_ipv4.h
index 29c7727..d249b6a 100644
--- a/include/linux/netfilter_ipv4.h
+++ b/include/linux/netfilter_ipv4.h
@@ -53,8 +53,8 @@
enum nf_ip_hook_priorities {
NF_IP_PRI_FIRST = INT_MIN,
- NF_IP_PRI_CONNTRACK_DEFRAG = -400,
- NF_IP_PRI_RAW = -300,
+ NF_IP_PRI_RAW = -400,
+ NF_IP_PRI_CONNTRACK_DEFRAG = -300,
NF_IP_PRI_SELINUX_FIRST = -225,
NF_IP_PRI_CONNTRACK = -200,
NF_IP_PRI_MANGLE = -150,
diff --git a/net/ipv4/netfilter/nf_defrag_ipv4.c b/net/ipv4/netfilter/nf_defrag_ipv4.c
index cb763ae..cb865d1 100644
--- a/net/ipv4/netfilter/nf_defrag_ipv4.c
+++ b/net/ipv4/netfilter/nf_defrag_ipv4.c
@@ -74,6 +74,9 @@ static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
return NF_ACCEPT;
#endif
#endif
+ if (nf_ct_is_untracked(skb))
+ return NF_ACCEPT;
+
/* Gather fragments. */
if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
enum ip_defrag_users user = nf_ct_defrag_user(hooknum, skb);
^ permalink raw reply related
* Re: [PATCH net-next-2.6] pkt_sched: gen_estimator: kill est_lock rwlock
From: Changli Gao @ 2010-06-07 14:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Stephen Hemminger, Jarek Poplawski
In-Reply-To: <1275921171.2545.102.camel@edumazet-laptop>
On Mon, Jun 7, 2010 at 10:32 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> We can use RCU in est_timer() and kill est_lock rwlock.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> net/core/gen_estimator.c | 45 ++++++++++++++-----------------------
> 1 file changed, 18 insertions(+), 27 deletions(-)
>
> diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> index cf8e703..406d880 100644
> --- a/net/core/gen_estimator.c
> +++ b/net/core/gen_estimator.c
> @@ -102,9 +102,6 @@ struct gen_estimator_head
>
> static struct gen_estimator_head elist[EST_MAX_INTERVAL+1];
>
> -/* Protects against NULL dereference */
> -static DEFINE_RWLOCK(est_lock);
> -
> /* Protects against soft lockup during large deletion */
> static struct rb_root est_root = RB_ROOT;
>
> @@ -115,29 +112,25 @@ static void est_timer(unsigned long arg)
>
> rcu_read_lock();
> list_for_each_entry_rcu(e, &elist[idx].list, list) {
> - u64 nbytes;
> - u64 brate;
> - u32 npackets;
> - u32 rate;
> + struct gnet_stats_basic_packed *bstats;
>
> spin_lock(e->stats_lock);
> - read_lock(&est_lock);
> - if (e->bstats == NULL)
> - goto skip;
> -
> - nbytes = e->bstats->bytes;
> - npackets = e->bstats->packets;
> - brate = (nbytes - e->last_bytes)<<(7 - idx);
> - e->last_bytes = nbytes;
> - e->avbps += (brate >> e->ewma_log) - (e->avbps >> e->ewma_log);
> - e->rate_est->bps = (e->avbps+0xF)>>5;
> -
> - rate = (npackets - e->last_packets)<<(12 - idx);
> - e->last_packets = npackets;
> - e->avpps += (rate >> e->ewma_log) - (e->avpps >> e->ewma_log);
> - e->rate_est->pps = (e->avpps+0x1FF)>>10;
> -skip:
> - read_unlock(&est_lock);
> + bstats = rcu_dereference(e->bstats);
> + if (bstats) {
> + u64 nbytes = ACCESS_ONCE(bstats->bytes);
> + u32 npackets = ACCESS_ONCE(bstats->packets);
> + u64 brate = (nbytes - e->last_bytes)<<(7 - idx);
> + u32 rate;
> +
> + e->last_bytes = nbytes;
> + e->avbps += (brate >> e->ewma_log) - (e->avbps >> e->ewma_log);
> + e->rate_est->bps = (e->avbps+0xF)>>5;
> +
> + rate = (npackets - e->last_packets)<<(12 - idx);
> + e->last_packets = npackets;
> + e->avpps += (rate >> e->ewma_log) - (e->avpps >> e->ewma_log);
> + e->rate_est->pps = (e->avpps+0x1FF)>>10;
> + }
> spin_unlock(e->stats_lock);
> }
>
> @@ -271,9 +264,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
> while ((e = gen_find_node(bstats, rate_est))) {
> rb_erase(&e->node, &est_root);
>
> - write_lock_bh(&est_lock);
> - e->bstats = NULL;
> - write_unlock_bh(&est_lock);
> + rcu_assign_pointer(e->bstats, NULL);
>
> list_del_rcu(&e->list);
> call_rcu(&e->e_rcu, __gen_kill_estimator);
>
Hmm. I don't think it is correct.
Look at this code:
void xt_rateest_put(struct xt_rateest *est)
{
mutex_lock(&xt_rateest_mutex);
if (--est->refcnt == 0) {
hlist_del(&est->list);
gen_kill_estimator(&est->bstats, &est->rstats);
kfree(est);
}
mutex_unlock(&xt_rateest_mutex);
}
est will be released after gen_kill_estimator() returns. After est is
freed, there may still be some other CPUs running in the branch:
if (bstats) {
....
}
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* [patch] caif: fix a couple range checks
From: Dan Carpenter @ 2010-06-07 14:51 UTC (permalink / raw)
To: Sjur Braendeland; +Cc: David S. Miller, netdev, kernel-janitors
The extra ! character means that these conditions are always false.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/net/caif/cfveil.c b/net/caif/cfveil.c
index 0fd827f..e04f7d9 100644
--- a/net/caif/cfveil.c
+++ b/net/caif/cfveil.c
@@ -84,7 +84,7 @@ static int cfvei_transmit(struct cflayer *layr, struct cfpkt *pkt)
return ret;
caif_assert(layr->dn != NULL);
caif_assert(layr->dn->transmit != NULL);
- if (!cfpkt_getlen(pkt) > CAIF_MAX_PAYLOAD_SIZE) {
+ if (cfpkt_getlen(pkt) > CAIF_MAX_PAYLOAD_SIZE) {
pr_warning("CAIF: %s(): Packet too large - size=%d\n",
__func__, cfpkt_getlen(pkt));
return -EOVERFLOW;
diff --git a/net/caif/cfrfml.c b/net/caif/cfrfml.c
index cd2830f..fd27b17 100644
--- a/net/caif/cfrfml.c
+++ b/net/caif/cfrfml.c
@@ -83,7 +83,7 @@ static int cfrfml_transmit(struct cflayer *layr, struct cfpkt *pkt)
if (!cfsrvl_ready(service, &ret))
return ret;
- if (!cfpkt_getlen(pkt) > CAIF_MAX_PAYLOAD_SIZE) {
+ if (cfpkt_getlen(pkt) > CAIF_MAX_PAYLOAD_SIZE) {
pr_err("CAIF: %s():Packet too large - size=%d\n",
__func__, cfpkt_getlen(pkt));
return -EOVERFLOW;
^ permalink raw reply related
* [PATCH net-next-2.6] pkt_sched: gen_estimator: kill est_lock rwlock
From: Eric Dumazet @ 2010-06-07 14:32 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Stephen Hemminger, Jarek Poplawski
We can use RCU in est_timer() and kill est_lock rwlock.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/core/gen_estimator.c | 45 ++++++++++++++-----------------------
1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index cf8e703..406d880 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -102,9 +102,6 @@ struct gen_estimator_head
static struct gen_estimator_head elist[EST_MAX_INTERVAL+1];
-/* Protects against NULL dereference */
-static DEFINE_RWLOCK(est_lock);
-
/* Protects against soft lockup during large deletion */
static struct rb_root est_root = RB_ROOT;
@@ -115,29 +112,25 @@ static void est_timer(unsigned long arg)
rcu_read_lock();
list_for_each_entry_rcu(e, &elist[idx].list, list) {
- u64 nbytes;
- u64 brate;
- u32 npackets;
- u32 rate;
+ struct gnet_stats_basic_packed *bstats;
spin_lock(e->stats_lock);
- read_lock(&est_lock);
- if (e->bstats == NULL)
- goto skip;
-
- nbytes = e->bstats->bytes;
- npackets = e->bstats->packets;
- brate = (nbytes - e->last_bytes)<<(7 - idx);
- e->last_bytes = nbytes;
- e->avbps += (brate >> e->ewma_log) - (e->avbps >> e->ewma_log);
- e->rate_est->bps = (e->avbps+0xF)>>5;
-
- rate = (npackets - e->last_packets)<<(12 - idx);
- e->last_packets = npackets;
- e->avpps += (rate >> e->ewma_log) - (e->avpps >> e->ewma_log);
- e->rate_est->pps = (e->avpps+0x1FF)>>10;
-skip:
- read_unlock(&est_lock);
+ bstats = rcu_dereference(e->bstats);
+ if (bstats) {
+ u64 nbytes = ACCESS_ONCE(bstats->bytes);
+ u32 npackets = ACCESS_ONCE(bstats->packets);
+ u64 brate = (nbytes - e->last_bytes)<<(7 - idx);
+ u32 rate;
+
+ e->last_bytes = nbytes;
+ e->avbps += (brate >> e->ewma_log) - (e->avbps >> e->ewma_log);
+ e->rate_est->bps = (e->avbps+0xF)>>5;
+
+ rate = (npackets - e->last_packets)<<(12 - idx);
+ e->last_packets = npackets;
+ e->avpps += (rate >> e->ewma_log) - (e->avpps >> e->ewma_log);
+ e->rate_est->pps = (e->avpps+0x1FF)>>10;
+ }
spin_unlock(e->stats_lock);
}
@@ -271,9 +264,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
while ((e = gen_find_node(bstats, rate_est))) {
rb_erase(&e->node, &est_root);
- write_lock_bh(&est_lock);
- e->bstats = NULL;
- write_unlock_bh(&est_lock);
+ rcu_assign_pointer(e->bstats, NULL);
list_del_rcu(&e->list);
call_rcu(&e->e_rcu, __gen_kill_estimator);
^ permalink raw reply related
* [RFC] lunar manet routing module
From: Christophe Jelger @ 2010-06-07 14:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1275917415.2545.79.camel@edumazet-laptop>
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
Eric Dumazet wrote:
>
> Christophe,
>
> Unless patch is really huge, its ok to send it on netdev, with a RFC
> label, so that only people with free time take a look, eventually.
>
> [RFC] lunar: ....
[not sure what 'huge' means, I'm sending 60k -- sorry for the pollution]
Eric: thanks for the advice. Instead of a patch I attach a .tgz (hope
it's ok) of the module code with a README explaining everything: it
compiles for 2.6.31 and 2.6.32, didn't try more recent kernels because I
actually want to deploy the whole thing on OpenWRT 2.6.32 on Linksys
devices.
To all: the lunar module crashes the kernel, so be careful. I tried
debugging with qemu and gdb server but could not find the bug(s) -- my
experience for kernel debugging is in fact limited.
thanks in advance for any help,
Christophe
[-- Attachment #2: lunar.tgz --]
[-- Type: application/x-compressed-tar, Size: 59498 bytes --]
^ permalink raw reply
* Re: [Linux-ATM-General] RX/close vcc race with solos/atmtcp/usbatm/he
From: David Woodhouse @ 2010-06-07 14:13 UTC (permalink / raw)
To: chas3; +Cc: linux-atm-general, netdev
In-Reply-To: <201006071344.o57DiiCx018593@thirdoffive.cmf.nrl.navy.mil>
On Mon, 2010-06-07 at 09:44 -0400, Chas Williams (CONTRACTOR) wrote:
> vcc's are really sockets, so you could just increase the refcount --
> sock_hold().
There are rules about where we're allowed to call sock_hold(), and I
don't think our find_vcc() functions can be made to meet them.
> however, i think the close routines dont really expect this behavior
> so the card driver's vcc close might need to be changed to wait around
> if the refcount on the vcc is > 1.
In that case I think we might as well stick with the RCU-like solution I
already implemented in the vcc close function -- which is just to wait
for the tasklet to complete, thus ensuring that it's no longer using the
defunct vcc.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* [PATCH net-next-2.6] net: avoid two atomic ops in ip_rcv_options()
From: Eric Dumazet @ 2010-06-07 13:54 UTC (permalink / raw)
To: David Miller; +Cc: netdev
in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv4/ip_input.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index d52c9da..27b5a09 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -298,18 +298,16 @@ static inline int ip_rcv_options(struct sk_buff *skb)
}
if (unlikely(opt->srr)) {
- struct in_device *in_dev = in_dev_get(dev);
+ struct in_device *in_dev = __in_dev_get_rcu(dev);
+
if (in_dev) {
if (!IN_DEV_SOURCE_ROUTE(in_dev)) {
if (IN_DEV_LOG_MARTIANS(in_dev) &&
net_ratelimit())
printk(KERN_INFO "source route option %pI4 -> %pI4\n",
&iph->saddr, &iph->daddr);
- in_dev_put(in_dev);
goto drop;
}
-
- in_dev_put(in_dev);
}
if (ip_options_rcv_srr(skb))
^ permalink raw reply related
* Re: [Linux-ATM-General] RX/close vcc race with solos/atmtcp/usbatm/he
From: Chas Williams (CONTRACTOR) @ 2010-06-07 13:44 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-atm-general, netdev
In-Reply-To: <1274872584.20576.13579.camel@macbook.infradead.org>
In message <1274872584.20576.13579.camel@macbook.infradead.org>,David Woodhouse
writes:
>The problem is that the 'find_vcc' functions in these drivers are
>returning a vcc with the ATM_VF_READY bit cleared, because it's already
>in the process of being destroyed. If we fix that simple oversight,
>there's still a race condition because the socket can still be closed
>(and completely freed, afaict) between our call to find_vcc() and our
>call to vcc->push() in the RX tasklet.
...
>Can anyone see a better approach -- short of rewriting the whole ATM
>layer to make the locking saner?
vcc's are really sockets, so you could just increase the refcount --
sock_hold(). after you push the packet, drop the refcount, sock_put()
and hopefully things will be well. however, i think the close routines
dont really expect this behavior so the card driver's vcc close might
need to be changed to wait around if the refcount on the vcc is > 1.
or perhaps the driver independent part needs to do this.
the he driver works around this issue by holding vcc_sklist_lock around
the find_vcc and ->push() which happen to occur in the same tasklet.
^ permalink raw reply
* Re: [PATCH net-next-2.6] phonet: use call_rcu for phonet device free
From: Eric Dumazet @ 2010-06-07 13:49 UTC (permalink / raw)
To: Rémi Denis-Courmont; +Cc: Jiri Pirko, netdev, davem
In-Reply-To: <474f08fed4a406e929af3d4142d3e185@chewa.net>
Le lundi 07 juin 2010 à 15:43 +0200, Rémi Denis-Courmont a écrit :
> On Mon, 7 Jun 2010 15:27:39 +0200, Jiri Pirko <jpirko@redhat.com> wrote:
> > Use call_rcu rather than synchronize_rcu.
> >
> > Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>
> This looks fine to me, but what is the goal here? The RCU documentation
> seems to imply that synchronize_rcu() is preferable over call_rcu() when at
> all possible.
>
Thats not exactly that.
synchronize_rcu() is easier, in respect of memory use.
But its drawback is current thread is blocked for several milli seconds.
In the end, call_rcu() is more scalable.
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next-2.6] phonet: use call_rcu for phonet device free
From: Rémi Denis-Courmont @ 2010-06-07 13:43 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem
In-Reply-To: <20100607132738.GB2730@psychotron.lab.eng.brq.redhat.com>
On Mon, 7 Jun 2010 15:27:39 +0200, Jiri Pirko <jpirko@redhat.com> wrote:
> Use call_rcu rather than synchronize_rcu.
>
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
This looks fine to me, but what is the goal here? The RCU documentation
seems to imply that synchronize_rcu() is preferable over call_rcu() when at
all possible.
--
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis
^ permalink raw reply
* Re: Virtual device and ARP table
From: Eric Dumazet @ 2010-06-07 13:30 UTC (permalink / raw)
To: Christophe Jelger; +Cc: netdev
In-Reply-To: <4C0CEE3E.50100@unibas.ch>
Le lundi 07 juin 2010 à 15:03 +0200, Christophe Jelger a écrit :
>
> Eric: thanks for the forward to the netdev list. Regarding the code, I
> of course welcome any help but didn't want to pollute the list with
> unsollicited code: I can of course of course send it directly to anyone
> who is willing to help (I can easily reproduce the problem on different
> machines).
>
Christophe,
Unless patch is really huge, its ok to send it on netdev, with a RFC
label, so that only people with free time take a look, eventually.
[RFC] lunar: ....
^ permalink raw reply
* [PATCH net-next-2.6] phonet: use call_rcu for phonet device free
From: Jiri Pirko @ 2010-06-07 13:27 UTC (permalink / raw)
To: netdev; +Cc: davem, remi.denis-courmont
Use call_rcu rather than synchronize_rcu.
Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
include/net/phonet/pn_dev.h | 1 +
net/phonet/pn_dev.c | 15 +++++++++++----
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h
index d7b989c..2d16783 100644
--- a/include/net/phonet/pn_dev.h
+++ b/include/net/phonet/pn_dev.h
@@ -34,6 +34,7 @@ struct phonet_device {
struct list_head list;
struct net_device *netdev;
DECLARE_BITMAP(addrs, 64);
+ struct rcu_head rcu;
};
int phonet_device_init(void);
diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c
index c33da65..b18e48f 100644
--- a/net/phonet/pn_dev.c
+++ b/net/phonet/pn_dev.c
@@ -162,6 +162,14 @@ int phonet_address_add(struct net_device *dev, u8 addr)
return err;
}
+static void phonet_device_rcu_free(struct rcu_head *head)
+{
+ struct phonet_device *pnd;
+
+ pnd = container_of(head, struct phonet_device, rcu);
+ kfree(pnd);
+}
+
int phonet_address_del(struct net_device *dev, u8 addr)
{
struct phonet_device_list *pndevs = phonet_device_list(dev_net(dev));
@@ -179,10 +187,9 @@ int phonet_address_del(struct net_device *dev, u8 addr)
pnd = NULL;
mutex_unlock(&pndevs->lock);
- if (pnd) {
- synchronize_rcu();
- kfree(pnd);
- }
+ if (pnd)
+ call_rcu(&pnd->rcu, phonet_device_rcu_free);
+
return err;
}
--
1.7.0.1
^ permalink raw reply related
* [PATCH net-next-2.6] ipv4: avoid two atomic ops in ip_rt_redirect()
From: Eric Dumazet @ 2010-06-07 13:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev
in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv4/route.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 7b8eacd..0c30534 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1343,11 +1343,12 @@ static void rt_del(unsigned hash, struct rtable *rt)
spin_unlock_bh(rt_hash_lock_addr(hash));
}
+/* called in rcu_read_lock() section */
void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
__be32 saddr, struct net_device *dev)
{
int i, k;
- struct in_device *in_dev = in_dev_get(dev);
+ struct in_device *in_dev = __in_dev_get_rcu(dev);
struct rtable *rth, **rthp;
__be32 skeys[2] = { saddr, 0 };
int ikeys[2] = { dev->ifindex, 0 };
@@ -1383,7 +1384,6 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
rthp=&rt_hash_table[hash].chain;
- rcu_read_lock();
while ((rth = rcu_dereference(*rthp)) != NULL) {
struct rtable *rt;
@@ -1405,12 +1405,10 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
break;
dst_hold(&rth->u.dst);
- rcu_read_unlock();
rt = dst_alloc(&ipv4_dst_ops);
if (rt == NULL) {
ip_rt_put(rth);
- in_dev_put(in_dev);
return;
}
@@ -1463,12 +1461,10 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
ip_rt_put(rt);
goto do_next;
}
- rcu_read_unlock();
do_next:
;
}
}
- in_dev_put(in_dev);
return;
reject_redirect:
@@ -1479,7 +1475,6 @@ reject_redirect:
&old_gw, dev->name, &new_gw,
&saddr, &daddr);
#endif
- in_dev_put(in_dev);
}
static struct dst_entry *ipv4_negative_advice(struct dst_entry *dst)
^ permalink raw reply related
* [PATCH net-next-2.6] igmp: avoid two atomic ops in igmp_rcv()
From: Eric Dumazet @ 2010-06-07 13:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv4/igmp.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 250cb5e..3294f54 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -916,18 +916,19 @@ static void igmp_heard_query(struct in_device *in_dev, struct sk_buff *skb,
read_unlock(&in_dev->mc_list_lock);
}
+/* called in rcu_read_lock() section */
int igmp_rcv(struct sk_buff *skb)
{
/* This basically follows the spec line by line -- see RFC1112 */
struct igmphdr *ih;
- struct in_device *in_dev = in_dev_get(skb->dev);
+ struct in_device *in_dev = __in_dev_get_rcu(skb->dev);
int len = skb->len;
if (in_dev == NULL)
goto drop;
if (!pskb_may_pull(skb, sizeof(struct igmphdr)))
- goto drop_ref;
+ goto drop;
switch (skb->ip_summed) {
case CHECKSUM_COMPLETE:
@@ -937,7 +938,7 @@ int igmp_rcv(struct sk_buff *skb)
case CHECKSUM_NONE:
skb->csum = 0;
if (__skb_checksum_complete(skb))
- goto drop_ref;
+ goto drop;
}
ih = igmp_hdr(skb);
@@ -957,7 +958,6 @@ int igmp_rcv(struct sk_buff *skb)
break;
case IGMP_PIM:
#ifdef CONFIG_IP_PIMSM_V1
- in_dev_put(in_dev);
return pim_rcv_v1(skb);
#endif
case IGMPV3_HOST_MEMBERSHIP_REPORT:
@@ -971,8 +971,6 @@ int igmp_rcv(struct sk_buff *skb)
break;
}
-drop_ref:
- in_dev_put(in_dev);
drop:
kfree_skb(skb);
return 0;
^ permalink raw reply related
* Re: Virtual device and ARP table
From: Christophe Jelger @ 2010-06-07 13:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1275913320.2545.53.camel@edumazet-laptop>
Eric Dumazet wrote:
> Le lundi 07 juin 2010 à 12:21 +0200, Christophe Jelger a écrit :
>> Hello,
>>
>> I am currently "resurrecting" a Linux module (called LUNAR) which I
>> co-developed in 2007 and I'm having a weird kernel crash. This code
>> basically used to work fine up to 2.6.18 which was the latest version
>> before we stopped our development. I quickly ported it to 2.6.{31,32}:
>> it compiles fine and loads fine, but it crashes/hangs the kernel when
>> it's really being used.
>>
>> The module is a virtual device used for MANET routing: with the current
>> version, it basically "captures" DNS requests sent to the virtual
>> interface --> this triggers the sending of a fake DNS reply (see below)
>> and the creation of an ARP table entry for the destination (the MANET
>> route is built at the same time). Packets can then be sent to the
>> destination.
>>
>> The problem I'm having is that the kernel quickly hangs after I create a
>> new ARP entry (actually only if it's being used). If the entry I create
>> is set to NUD_PERMANENT, then everything works fine! I use
>> __neigh_lookup_errno to lookup/create the entry and neigh_lookup to
>> set/update the MAC address. Note that the ARP entry is created without
>> problem, but typically even just doing a userspace "arp -a" command can
>> crash the kernel (it also hangs the userspace command!). Doing "arp -na"
>> usually does NOT crash the kernel!
>>
>> I guess the problem comes from a combination of ARP + DNS
>> lookups/replies. Note that my kernel module has its own internal fake
>> DNS server which captures lookups and sends replies directly back to the
>> stack. What is amazing: if the ARP entry I create is set to
>> NUD_PERMANENT, then I don't get any crash (however I cannot develop my
>> module with permanent ARP entries).
>>
>> I'm wondering if there were any major changes to the neighbor and arp
>> code (between 2.6.18 and 2.6.31) that are somehow causing this problem ?...
>>
>> Any hint is very welcome.
>>
>> thanks in advance,
>> Christophe
>>
>> PS: I can easily reproduce the problem, and was trying to debug with
>> qemu and gdb server but so fra no success to clearly identify the
>> problem. Last point: it seems the kernel does not really "crash" but
>> rather ends up in some unstable state and maybe in a loop.
>> --
>
> Hi Christophe
>
> You should ask these kind of questions on netdev instead of lkml.
>
> And of course, post your patch, or send us a crystal ball ;)
>
> Yes, many things changed between 2.6.18 and 2.6.34
>
Eric: thanks for the forward to the netdev list. Regarding the code, I
of course welcome any help but didn't want to pollute the list with
unsollicited code: I can of course of course send it directly to anyone
who is willing to help (I can easily reproduce the problem on different
machines).
Christophe
^ permalink raw reply
* [PATCH net-next-2.6] ip: Router Alert RCU conversion
From: Eric Dumazet @ 2010-06-07 13:12 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Straightforward conversion to RCU.
One rwlock becomes a spinlock, and is static.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/ip.h | 2 +-
net/ipv4/ip_input.c | 11 +++--------
net/ipv4/ip_sockglue.c | 23 ++++++++++++++---------
3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/net/ip.h b/include/net/ip.h
index 452f229..9982c97 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -62,10 +62,10 @@ struct ip_ra_chain {
struct ip_ra_chain *next;
struct sock *sk;
void (*destructor)(struct sock *);
+ struct rcu_head rcu;
};
extern struct ip_ra_chain *ip_ra_chain;
-extern rwlock_t ip_ra_lock;
/* IP flags. */
#define IP_CE 0x8000 /* Flag: "Congestion" */
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index d52c9da..08a3b12 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -146,7 +146,7 @@
#include <linux/netlink.h>
/*
- * Process Router Attention IP option
+ * Process Router Attention IP option (RFC 2113)
*/
int ip_call_ra_chain(struct sk_buff *skb)
{
@@ -155,8 +155,7 @@ int ip_call_ra_chain(struct sk_buff *skb)
struct sock *last = NULL;
struct net_device *dev = skb->dev;
- read_lock(&ip_ra_lock);
- for (ra = ip_ra_chain; ra; ra = ra->next) {
+ for (ra = rcu_dereference(ip_ra_chain); ra; ra = rcu_dereference(ra->next)) {
struct sock *sk = ra->sk;
/* If socket is bound to an interface, only report
@@ -167,10 +166,8 @@ int ip_call_ra_chain(struct sk_buff *skb)
sk->sk_bound_dev_if == dev->ifindex) &&
net_eq(sock_net(sk), dev_net(dev))) {
if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
- if (ip_defrag(skb, IP_DEFRAG_CALL_RA_CHAIN)) {
- read_unlock(&ip_ra_lock);
+ if (ip_defrag(skb, IP_DEFRAG_CALL_RA_CHAIN))
return 1;
- }
}
if (last) {
struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
@@ -183,10 +180,8 @@ int ip_call_ra_chain(struct sk_buff *skb)
if (last) {
raw_rcv(last, skb);
- read_unlock(&ip_ra_lock);
return 1;
}
- read_unlock(&ip_ra_lock);
return 0;
}
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index ce23178..08b9519 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -239,7 +239,12 @@ int ip_cmsg_send(struct net *net, struct msghdr *msg, struct ipcm_cookie *ipc)
sent to multicast group to reach destination designated router.
*/
struct ip_ra_chain *ip_ra_chain;
-DEFINE_RWLOCK(ip_ra_lock);
+static DEFINE_SPINLOCK(ip_ra_lock);
+
+static void ip_ra_free_rcu(struct rcu_head *head)
+{
+ kfree(container_of(head, struct ip_ra_chain, rcu));
+}
int ip_ra_control(struct sock *sk, unsigned char on,
void (*destructor)(struct sock *))
@@ -251,35 +256,35 @@ int ip_ra_control(struct sock *sk, unsigned char on,
new_ra = on ? kmalloc(sizeof(*new_ra), GFP_KERNEL) : NULL;
- write_lock_bh(&ip_ra_lock);
+ spin_lock_bh(&ip_ra_lock);
for (rap = &ip_ra_chain; (ra = *rap) != NULL; rap = &ra->next) {
if (ra->sk == sk) {
if (on) {
- write_unlock_bh(&ip_ra_lock);
+ spin_unlock_bh(&ip_ra_lock);
kfree(new_ra);
return -EADDRINUSE;
}
- *rap = ra->next;
- write_unlock_bh(&ip_ra_lock);
+ rcu_assign_pointer(*rap, ra->next);
+ spin_unlock_bh(&ip_ra_lock);
if (ra->destructor)
ra->destructor(sk);
sock_put(sk);
- kfree(ra);
+ call_rcu(&ra->rcu, ip_ra_free_rcu);
return 0;
}
}
if (new_ra == NULL) {
- write_unlock_bh(&ip_ra_lock);
+ spin_unlock_bh(&ip_ra_lock);
return -ENOBUFS;
}
new_ra->sk = sk;
new_ra->destructor = destructor;
new_ra->next = ra;
- *rap = new_ra;
+ rcu_assign_pointer(*rap, new_ra);
sock_hold(sk);
- write_unlock_bh(&ip_ra_lock);
+ spin_unlock_bh(&ip_ra_lock);
return 0;
}
^ permalink raw reply related
* Re: [Patch 2/2] mlx4: add dynamic LRO disable support
From: Cong Wang @ 2010-06-07 13:15 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Ben Hutchings, netdev, herbert.xu, nhorman, davem
In-Reply-To: <20100607130038.11581e28@dhcp-lab-109.englab.brq.redhat.com>
On 06/07/10 19:00, Stanislaw Gruszka wrote:
> On Mon, 07 Jun 2010 16:51:49 +0800
> Cong Wang<amwang@redhat.com> wrote:
>
>>> Now that I look at the patch again, I see you're using a static (i.e.
>>> global) variable to 'back up' the non-zero (enabled) value of num_lro.
>>> This is introducing a bug! The correct value is apparently set in
>>> mlx4_en_get_profile(); you would need to replicate that.
>>>
>>
>> Oh, probably, but unfortunately 'num_lro' is static so only visible
>> in en_main.c.
>
> So just remove "static" and make it global :-)
>
Not that easy, it is defined with a macro which is also used
by other parameters. :-/
^ permalink raw reply
* Re: [v5 Patch 1/3] netpoll: add generic support for bridge and bonding devices
From: Andy Gospodarek @ 2010-06-07 13:03 UTC (permalink / raw)
To: Cong Wang
Cc: Andy Gospodarek, Jay Vosburgh, Flavio Leitner, linux-kernel,
Matt Mackall, netdev, bridge, Andy Gospodarek, Neil Horman,
Jeff Moyer, Stephen Hemminger, bonding-devel, David Miller
In-Reply-To: <4C0CC29D.9070507@redhat.com>
On Mon, Jun 07, 2010 at 05:57:49PM +0800, Cong Wang wrote:
> On 06/05/10 03:18, Andy Gospodarek wrote:
>> On Wed, Jun 02, 2010 at 06:04:45PM +0800, Cong Wang wrote:
>>> On 06/02/10 02:42, Jay Vosburgh wrote:
>>>> Cong Wang<amwang@redhat.com> wrote:
>>>>
>>>>> On 06/01/10 03:08, Flavio Leitner wrote:
>>>>>> On Mon, May 31, 2010 at 01:56:52PM +0800, Cong Wang wrote:
>>>>>>> Hi, Flavio,
>>>>>>>
>>>>>>> Please use the attached patch instead, try to see if it solves
>>>>>>> all your problems.
>>>>>>
>>>>>> I tried and it hangs. No backtraces this time.
>>>>>> The bond_change_active_slave() prints before NETDEV_BONDING_FAILOVER
>>>>>> notification, so I think it won't work.
>>>>>
>>>>> Ah, I thought the same.
>>>>>
>>>>>>
>>>>>> Please, correct if I'm wrong, but when a failover happens with your
>>>>>> patch applied, the netconsole would be disabled forever even with
>>>>>> another healthy slave, right?
>>>>>>
>>>>>
>>>>> Yes, this is an easy solution, because bonding has several modes,
>>>>> it is complex to make netpoll work in different modes.
>>>>
>>>> If I understand correctly, the root cause of the problem with
>>>> netconsole and bonding is that bonding is, ultimately, performing
>>>> printks with a write lock held, and when netconsole recursively calls
>>>> into bonding to send the printk over the netconsole, there is a deadlock
>>>> (when the bonding xmit function attempts to acquire the same lock for
>>>> read).
>>>
>>>
>>> Yes.
>>>
>>>>
>>>> You're trying to avoid the deadlock by shutting off netconsole
>>>> (permanently, it looks like) for one problem case: a failover, which
>>>> does some printks with a write lock held.
>>>>
>>>> This doesn't look to me like a complete solution, there are
>>>> other cases in bonding that will do printk with write locks held. I
>>>> suspect those will also hang netconsole as things exist today, and won't
>>>> be affected by your patch below.
>>>
>>>
>>> I can expect that, bonding modes are complex.
>>>
>>>>
>>>> For example:
>>>>
>>>> The sysfs functions to set the primary (bonding_store_primary)
>>>> or active (bonding_store_active_slave) options: a pr_info is called to
>>>> provide a log message of the results. These could be tested by setting
>>>> the primary or active options via sysfs, e.g.,
>>>>
>>>> echo eth0> /sys/class/net/bond0/bonding/primary
>>>> echo eth0> /sys/class/net/bond0/bonding/active
>>>>
>>>> If the kernel is defined with DEBUG, there are a few pr_debug
>>>> calls within write_locks (bond_del_vlan, for example).
>>>>
>>>> If the slave's underlying device driver's ndo_vlan_rx_register
>>>> or ndo_vlan_rx_kill_vid functions call printk (and it looks like some do
>>>> for error cases, e.g., igbvf, ehea, enic), those would also presumably
>>>> deadlock (because bonding holds its write_lock when calling the ndo_
>>>> vlan functions).
>>>>
>>>> It also appears that (with the patch below) some nominally
>>>> normal usage patterns will immediately disable netconsole. The one that
>>>> comes to mind is if the primary= option is set (to "eth1" for this
>>>> example), but that slave not enslaved first (the slaves are added, say,
>>>> eth0 then eth1). In that situation, when the primary slave (eth1 here)
>>>> is added, the first thing that will happen is a failover, and that will
>>>> disable netconsole.
>>>>
>>>
>>> Thanks for your detailed explanation!
>>>
>>> This is why I said bonding is complex. I guess we would have to adjust
>>> netpoll code for different bonding cases, one solution seems not fix all.
>>> I am not sure how much work to do, since I am not familiar with bonding
>>> code. Maybe Andy can help?
>>>
>>
>> Sorry I've been silent until now. This does seem quite similar to a
>> problem I've previously encountered when dealing with bonding+netpoll on
>> some old 2.6.9-based kernels. There is no guarantee the methods used
>> there will apply here, but I'll talk about them anyway.
>>
>> As Flavio noticed, recursive calls into the bond transmit routines were
>> not a good idea. I discovered the same and worked around this issue by
>> checking to see if we could take the bond->lock for writing before
>> continuing. If we could not get, I wanted to signal that this should be
>> queued for transmission later. Based on the flow of netpoll_send_skb
>> (or possibly for another reason that is escaping me right now) I added
>> one of these checks in bond_poll_controller too. These aren't the
>> prettiest fixes, but seemed to work well for me when I did this work in
>> the past. I realize the differences are not that great compared to some
>> of the patches posted by Flavio, but I think they are worth trying.
>
>
> Hmm, I still feel like this way is ugly, although it may work.
> I guess David doesn't like it either.
>
Notice how I referred to it as a work-around? :)
It certainly isn't a great way to resolve the issue, but I wanted to
offer my opinon on the issue since you asked.
> Anyway, Flavio, could you try the following patch as well?
>
> Thanks a lot!
>
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index ef60244..d7b9b99 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1290,6 +1290,12 @@ static bool slaves_support_netpoll(struct net_device *bond_dev)
>> static void bond_poll_controller(struct net_device *bond_dev)
>> {
>> struct net_device *dev = bond_dev->npinfo->netpoll->real_dev;
>> + struct bonding *bond = netdev_priv(bond_dev);
>> +
>> + if (!write_trylock(&bond->lock))
>> + return;
>> + write_unlock(&bond->lock);
>> +
>> if (dev != bond_dev)
>> netpoll_poll_dev(dev);
>> }
>> @@ -4418,7 +4424,11 @@ static void bond_set_xmit_hash_policy(struct bonding *bond)
>>
>> static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> {
>> - const struct bonding *bond = netdev_priv(dev);
>> + struct bonding *bond = netdev_priv(dev);
>> +
>> + if (!write_trylock(&bond->lock))
>> + return NETDEV_TX_BUSY;
>> + write_unlock(&bond->lock);
>>
>> switch (bond->params.mode) {
>> case BOND_MODE_ROUNDROBIN:
>>
>> The other key to all of this is to make sure that queuing is done
>> correctly now that we expect to queue these frames and have them sent at
>> some point when there is a member of the bond that is actually capable
>> of sending them out.
>>
>> The new style of sending queued skbs in a workqueue is much better than
>> what was done in the 2.6.9 timeframe, but careful attention should still
>> be paid to txq lock and which processor is the owner. Returning
>> something other than NETDEV_TX_OK from bond_start_xmit and checking for
>> locks being held there should also help with any deadlocks that show up
>> while running in queue_process (though they would not be recursive).
>>
>> I'm not in a good spot to test this right now, but I can take a look at
>> next week and we can try and track down any of the other deadlocks that
>> currently exist as I suspect this will not resolve all of the issues.
>
> --
> 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: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Kay Sievers @ 2010-06-07 12:54 UTC (permalink / raw)
To: Johannes Berg; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <1275914205.29978.10.camel@jlt3.sipsolutions.net>
On Mon, Jun 7, 2010 at 14:36, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2010-06-07 at 14:26 +0200, Kay Sievers wrote:
>> On Mon, Jun 7, 2010 at 13:41, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > (mind you, I think we probably need to have the bus/driver assignment,
>> > but I wanted to try out your suggestion first)
>>
>> Class device can never have a driver. And unregistered drivers can
>> not be used, what you try to do here. That all should just be removed or
>> properly registered with the core if needed.
>
> Yeah but it shouldn't influence the operation?
It does. You can not use "dead static" drivers. They need to be
properly registered, and can only work for bus devices. Class devices
are not allowed to have drivers.
This worked only because the core ignored the value you assigned
behind its back. :)
>> Here is something that seems to work for me.
>
> I see you remove the driver. Does this mean that in sysfs these devices
> wouldn't have a driver symlink? ISTR that we needed that for userspace,
> but I'm not entirely sure, and I don't have all the relevant userspace
> in my test setup.
Yeah, if you need the driver, you need to use bus device, as you do
now. The driver needs to be registered with the core, and the probe
function needs to tell to bind to the devices while registered. You
can not manually assign this.
Kay
^ permalink raw reply
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Johannes Berg @ 2010-06-07 12:36 UTC (permalink / raw)
To: Kay Sievers; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <1275913563.1823.1.camel@yio.site>
On Mon, 2010-06-07 at 14:26 +0200, Kay Sievers wrote:
> On Mon, Jun 7, 2010 at 13:41, Johannes Berg <johannes@sipsolutions.net> wrote:
> > (mind you, I think we probably need to have the bus/driver assignment,
> > but I wanted to try out your suggestion first)
>
> Class device can never have a driver. And unregistered drivers can
> not be used, what you try to do here. That all should just be removed or
> properly registered with the core if needed.
Yeah but it shouldn't influence the operation? Anyway I see from your
patch that I should have assigned the release function and that would've
helped I guess.
> > So I removed bus/driver assignment from the above code just to try it
> > out, and got
> >
> > Device 'hwsim0' does not have a release() function, it is broken and
> > must be fixed.
>
> The driver core expects the resources of the device to be freed on
> release. You can provide an empty release function because you do this
> from a global list of devices.
Ok, makes sense.
> > This has evolved far too much for me right now. Can we just apply the
> > initial patch from Eric and be happier for a while? I can't justify
> > spending this much time on it right now. Alternatively, you could look
> > at hwsim too, since it's all virtual, nothing special is required, I do
> > testing in a virtual machine ...
> >
> > current patch is at
> > http://johannes.sipsolutions.net/patches/kernel/all/2010-06-07-11:41/hwsim-bus.patch
>
> Here is something that seems to work for me.
I see you remove the driver. Does this mean that in sysfs these devices
wouldn't have a driver symlink? ISTR that we needed that for userspace,
but I'm not entirely sure, and I don't have all the relevant userspace
in my test setup.
johannes
^ permalink raw reply
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Kay Sievers @ 2010-06-07 12:26 UTC (permalink / raw)
To: Johannes Berg; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <1275910905.29978.7.camel@jlt3.sipsolutions.net>
On Mon, Jun 7, 2010 at 13:41, Johannes Berg <johannes@sipsolutions.net> wrote:
> (mind you, I think we probably need to have the bus/driver assignment,
> but I wanted to try out your suggestion first)
Class device can never have a driver. And unregistered drivers can
not be used, what you try to do here. That all should just be removed or
properly registered with the core if needed.
> So I removed bus/driver assignment from the above code just to try it
> out, and got
>
> Device 'hwsim0' does not have a release() function, it is broken and
> must be fixed.
The driver core expects the resources of the device to be freed on
release. You can provide an empty release function because you do this
from a global list of devices.
> This has evolved far too much for me right now. Can we just apply the
> initial patch from Eric and be happier for a while? I can't justify
> spending this much time on it right now. Alternatively, you could look
> at hwsim too, since it's all virtual, nothing special is required, I do
> testing in a virtual machine ...
>
> current patch is at
> http://johannes.sipsolutions.net/patches/kernel/all/2010-06-07-11:41/hwsim-bus.patch
Here is something that seems to work for me.
Cheers,
Kay
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 6f8cb3e..d210ce6 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -194,7 +194,13 @@ static inline void hwsim_clear_sta_magic(struct ieee80211_sta *sta)
sp->magic = 0;
}
-static struct class *hwsim_class;
+static struct bus_type hwsim_bus = {
+ .name = "mac80211_hwsim",
+};
+
+static struct device hwsim_parent = {
+ .init_name = "mac80211_hwsim",
+};
static struct net_device *hwsim_mon; /* global monitor netdev */
@@ -1071,14 +1077,10 @@ static void mac80211_hwsim_free(void)
device_unregister(data->dev);
ieee80211_free_hw(data->hw);
}
- class_destroy(hwsim_class);
+ device_unregister(&hwsim_parent);
+ bus_unregister(&hwsim_bus);
}
-
-static struct device_driver mac80211_hwsim_driver = {
- .name = "mac80211_hwsim"
-};
-
static const struct net_device_ops hwsim_netdev_ops = {
.ndo_start_xmit = hwsim_mon_xmit,
.ndo_change_mtu = eth_change_mtu,
@@ -1231,6 +1233,9 @@ DEFINE_SIMPLE_ATTRIBUTE(hwsim_fops_group,
hwsim_fops_group_read, hwsim_fops_group_write,
"%llx\n");
+/* all devices are kept in out own list and the ressources are freed on exit */
+static void hwsim_dev_release(struct device* dev) {}
+
static int __init init_mac80211_hwsim(void)
{
int i, err = 0;
@@ -1251,9 +1256,15 @@ static int __init init_mac80211_hwsim(void)
spin_lock_init(&hwsim_radio_lock);
INIT_LIST_HEAD(&hwsim_radios);
- hwsim_class = class_create(THIS_MODULE, "mac80211_hwsim");
- if (IS_ERR(hwsim_class))
- return PTR_ERR(hwsim_class);
+ err = bus_register(&hwsim_bus);
+ if (err)
+ return err;
+
+ err = device_register(&hwsim_parent);
+ if (err) {
+ bus_unregister(&hwsim_bus);
+ return err;
+ }
memset(addr, 0, ETH_ALEN);
addr[0] = 0x02;
@@ -1271,16 +1282,24 @@ static int __init init_mac80211_hwsim(void)
data = hw->priv;
data->hw = hw;
- data->dev = device_create(hwsim_class, NULL, 0, hw,
- "hwsim%d", i);
- if (IS_ERR(data->dev)) {
- printk(KERN_DEBUG
- "mac80211_hwsim: device_create "
- "failed (%ld)\n", PTR_ERR(data->dev));
+ data->dev = kzalloc(sizeof(struct device), GFP_KERNEL);
+ if (!data->dev) {
err = -ENOMEM;
goto failed_drvdata;
}
- data->dev->driver = &mac80211_hwsim_driver;
+
+ dev_set_name(data->dev, "hwsim%d", i);
+ data->dev->parent = &hwsim_parent;
+ data->dev->bus = &hwsim_bus;
+ data->dev->release = hwsim_dev_release;
+
+ err = device_register(data->dev);
+ if (err) {
+ printk(KERN_DEBUG
+ "mac80211_hwsim: device_register failed (%d)\n",
+ err);
+ goto failed_drvdata;
+ }
SET_IEEE80211_DEV(hw, data->dev);
addr[3] = i >> 8;
^ permalink raw reply related
* Re: Virtual device and ARP table
From: Eric Dumazet @ 2010-06-07 12:22 UTC (permalink / raw)
To: Christophe Jelger; +Cc: linux-kernel, netdev
In-Reply-To: <4C0CC810.7030501@unibas.ch>
Le lundi 07 juin 2010 à 12:21 +0200, Christophe Jelger a écrit :
> Hello,
>
> I am currently "resurrecting" a Linux module (called LUNAR) which I
> co-developed in 2007 and I'm having a weird kernel crash. This code
> basically used to work fine up to 2.6.18 which was the latest version
> before we stopped our development. I quickly ported it to 2.6.{31,32}:
> it compiles fine and loads fine, but it crashes/hangs the kernel when
> it's really being used.
>
> The module is a virtual device used for MANET routing: with the current
> version, it basically "captures" DNS requests sent to the virtual
> interface --> this triggers the sending of a fake DNS reply (see below)
> and the creation of an ARP table entry for the destination (the MANET
> route is built at the same time). Packets can then be sent to the
> destination.
>
> The problem I'm having is that the kernel quickly hangs after I create a
> new ARP entry (actually only if it's being used). If the entry I create
> is set to NUD_PERMANENT, then everything works fine! I use
> __neigh_lookup_errno to lookup/create the entry and neigh_lookup to
> set/update the MAC address. Note that the ARP entry is created without
> problem, but typically even just doing a userspace "arp -a" command can
> crash the kernel (it also hangs the userspace command!). Doing "arp -na"
> usually does NOT crash the kernel!
>
> I guess the problem comes from a combination of ARP + DNS
> lookups/replies. Note that my kernel module has its own internal fake
> DNS server which captures lookups and sends replies directly back to the
> stack. What is amazing: if the ARP entry I create is set to
> NUD_PERMANENT, then I don't get any crash (however I cannot develop my
> module with permanent ARP entries).
>
> I'm wondering if there were any major changes to the neighbor and arp
> code (between 2.6.18 and 2.6.31) that are somehow causing this problem ?...
>
> Any hint is very welcome.
>
> thanks in advance,
> Christophe
>
> PS: I can easily reproduce the problem, and was trying to debug with
> qemu and gdb server but so fra no success to clearly identify the
> problem. Last point: it seems the kernel does not really "crash" but
> rather ends up in some unstable state and maybe in a loop.
> --
Hi Christophe
You should ask these kind of questions on netdev instead of lkml.
And of course, post your patch, or send us a crystal ball ;)
Yes, many things changed between 2.6.18 and 2.6.34
^ 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