* Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)
From: Eric Dumazet @ 2011-03-03 9:07 UTC (permalink / raw)
To: Fabio Checconi
Cc: Stephen Hemminger, David Miller, Luigi Rizzo, netdev,
Paolo Valente
In-Reply-To: <20110303081940.GA29685@gandalf.sssup.it>
Le jeudi 03 mars 2011 à 09:19 +0100, Fabio Checconi a écrit :
> The same bug you identified in qfq_reset_qdisc() is present also in
> qfq_drop(), both loops need to be corrected...
I dont think so, because in qfq_drop() we exit from qfq_drop() right
after call to qfq_deactivate_class()
But I agree code should be same ;)
^ permalink raw reply
* [PATCH] bonding 802.3ad: Fix the state machine locking
From: Nils Carlson @ 2011-03-03 9:05 UTC (permalink / raw)
To: bonding-devel, netdev; +Cc: Nils Carlson
The current implementation only locked around the rx state machine,
but the rx state machine reads data touched by other state machines
as well, so in a very ugly scenario we would see the rx state machine
reading bad values as data was being overwritten by other state
machines. This patch moves the bond_3ad locking to protect all the
state machines from concurrency issues.
Signed-off-by: Nils Carlson <nils.carlson@ericsson.com>
---
drivers/net/bonding/bond_3ad.c | 32 +++++++++++++++++++-------------
drivers/net/bonding/bond_3ad.h | 2 +-
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 1024ae1..bbc473b 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -281,23 +281,23 @@ static inline int __check_agg_selection_timer(struct port *port)
}
/**
- * __get_rx_machine_lock - lock the port's RX machine
+ * __get_state_machine_lock - lock the port's state machine
* @port: the port we're looking at
*
*/
-static inline void __get_rx_machine_lock(struct port *port)
+static inline void __get_state_machine_lock(struct port *port)
{
- spin_lock_bh(&(SLAVE_AD_INFO(port->slave).rx_machine_lock));
+ spin_lock_bh(&(SLAVE_AD_INFO(port->slave).state_machine_lock));
}
/**
- * __release_rx_machine_lock - unlock the port's RX machine
+ * __release_state_machine_lock - unlock the port's state machine
* @port: the port we're looking at
*
*/
-static inline void __release_rx_machine_lock(struct port *port)
+static inline void __release_state_machine_lock(struct port *port)
{
- spin_unlock_bh(&(SLAVE_AD_INFO(port->slave).rx_machine_lock));
+ spin_unlock_bh(&(SLAVE_AD_INFO(port->slave).state_machine_lock));
}
/**
@@ -388,14 +388,14 @@ static u8 __get_duplex(struct port *port)
}
/**
- * __initialize_port_locks - initialize a port's RX machine spinlock
+ * __initialize_port_locks - initialize a port's state machine spinlock
* @port: the port we're looking at
*
*/
static inline void __initialize_port_locks(struct port *port)
{
// make sure it isn't called twice
- spin_lock_init(&(SLAVE_AD_INFO(port->slave).rx_machine_lock));
+ spin_lock_init(&(SLAVE_AD_INFO(port->slave).state_machine_lock));
}
//conversions
@@ -1025,9 +1025,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
{
rx_states_t last_state;
- // Lock to prevent 2 instances of this function to run simultaneously(rx interrupt and periodic machine callback)
- __get_rx_machine_lock(port);
-
// keep current State Machine state to compare later if it was changed
last_state = port->sm_rx_state;
@@ -1133,7 +1130,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
pr_err("%s: An illegal loopback occurred on adapter (%s).\n"
"Check the configuration to verify that all adapters are connected to 802.3ad compliant switch ports\n",
port->slave->dev->master->name, port->slave->dev->name);
- __release_rx_machine_lock(port);
return;
}
__update_selected(lacpdu, port);
@@ -1153,7 +1149,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
break;
}
}
- __release_rx_machine_lock(port);
}
/**
@@ -2155,6 +2150,12 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
goto re_arm;
}
+ /* Lock around all the state machines to protect data that may
+ * be accessed also from the ad_rx_machine running in the
+ * interrupt handler.
+ */
+ __get_state_machine_lock(port);
+
ad_rx_machine(NULL, port);
ad_periodic_machine(port);
ad_port_selection_logic(port);
@@ -2164,6 +2165,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
// turn off the BEGIN bit, since we already handled it
if (port->sm_vars & AD_PORT_BEGIN)
port->sm_vars &= ~AD_PORT_BEGIN;
+
+ __release_state_machine_lock(port);
}
re_arm:
@@ -2200,7 +2203,10 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
case AD_TYPE_LACPDU:
pr_debug("Received LACPDU on port %d\n",
port->actor_port_number);
+ /* Protect against concurrent state machines */
+ __get_state_machine_lock(port);
ad_rx_machine(lacpdu, port);
+ __release_state_machine_lock(port);
break;
case AD_TYPE_MARKER:
diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
index 2c46a15..9182506 100644
--- a/drivers/net/bonding/bond_3ad.h
+++ b/drivers/net/bonding/bond_3ad.h
@@ -264,7 +264,7 @@ struct ad_bond_info {
struct ad_slave_info {
struct aggregator aggregator; // 802.3ad aggregator structure
struct port port; // 802.3ad port structure
- spinlock_t rx_machine_lock; // To avoid race condition between callback and receive interrupt
+ spinlock_t state_machine_lock; // To avoid race condition between callback and receive interrupt
u16 id;
};
--
1.7.1
^ permalink raw reply related
* Re: [PATCH (sh-2.6) 1/4] clksource: Generic timer infrastructure
From: Arnd Bergmann @ 2011-03-03 8:45 UTC (permalink / raw)
To: Peppe CAVALLARO
Cc: Thomas Gleixner, Stuart MENEFY, linux-sh@vger.kernel.org,
netdev@vger.kernel.org, John Stultz, linux-kernel@vger.kernel.org,
linux@arm.linux.org.uk
In-Reply-To: <4D6E7FD7.1060408@st.com>
On Wednesday 02 March 2011, Peppe CAVALLARO wrote:
> At any rate, I am happy to use the stmmac as experimental
> driver to do this kind tests.
> Indeed, in the past, on old Kernel (IIRC 2.6.23), I tried to use
> the kernel timers but I removed the code from it because
> I had noticed packets loss and a strange phenomenon with cyclesoak
> (that showed broken sysload % during the heavy network activities).
>
> Let me know how to proceed:
>
> 1) experiment with stmmac and hrtimer for handling rx/tx?
> 2) rework the patches for the Generic Timer Infra?
I'd suggest doing the first. I'm surprised that using an unrelated
timer for processing interrupts even helps you on stmmac.
The timers that you'd normally use for rx interrupt mitigation
are not periodic timers but are started when a packet arrives
from the outside.
Doing periodic wakeups for RX instead of just waiting for
packets to come in should have a significant impact on power
management on an otherwise idle system.
For tx resource reclaim, a relatively slow oneshot timer (not
even hrtimer) should be good enough, since it only needs to be
active when there is no other way to clean up. E.g. when you
are in napi polling mode (interrupt disabled), you know that
stmmac_poll gets called soon, and you can also do the reclaim
from stmmac_xmit() in order to prevent the timer from triggering
when you are constantly transmitting.
Arnd
^ permalink raw reply
* Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame
From: Nicolas de Pesloüan @ 2011-03-03 8:37 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Andy Gospodarek, netdev, davem, fubar, eric.dumazet
In-Reply-To: <20110303061433.GA2835@psychotron.redhat.com>
Le 03/03/2011 07:14, Jiri Pirko a écrit :
> Wed, Mar 02, 2011 at 10:54:03PM CET, nicolas.2p.debian@gmail.com wrote:
[snip]
>> All those handlers that call netif_rx() or __netif_receive_skb()
>> sound horrible to me. Can you imagine the global overhead of the
>> above receive path?
>>
>> The reason why I suggested you introduce the goto another_round is
>> because most - if not all - stacking configurations could/should be
>> handled simply by returning the right skb from the rx_handler and let
>> __netif_receive_skb() loop. And by having the right orig_dev logic
>> inside __netif_receive_skb(), it could be possible to remove the
>> current vlan_on_bond_hook hack.
>
> Well that wouldn't solve the problem. if we just got anorther_round the
> packed would not be delivered to bond0.100 but only to bond0. That's
> also unwanted. Well, this think shouldn't have been added here in the
> first place :(
Yes, the packet would be delivered "exact match" to bond0, and this is what the bonding ARP handler
expects, because it registered at this level, then deliver "exact match or NULL" to bond0.100, if
appropriate, to whatever other protocol handlers.
There is a subtile difference between ptype_all and ptype_base delivery:
- The ptype_all handlers will receive the frame between two call to rx_handlers.
- The ptype_base handlers will receive the frame after all rx_handlers (and hard coded handlers like
vlan) were called.
So a ptype_all handler might receive vlan tagged frame, if it register below the interface that
untag the frame, while a ptype_base handler will always receive the untagged frame, because
ptype_base delivery is done at the end of __netif_receive_skb().
I don't know whether it is desirable, but it used to be required for the bonding ARP monitor to work.
May be we should add some extra properties to protocol handler, so they can tell
__netif_receive_skb() what they expect :
- a property to tell whether the protocol handler want the exact frame that cross the interface they
registered on or the final frame, after all rx_handlers.
- a property to tell what they expect a the orig_dev value : the lowest interface known to
__netif_receive_skb() (f_packet) or the interface one level below the interface they registered on
(bonding ARP monitor).
Having those properties, I consider we would be in a position to remove most special hacks we
currently have to handle particular stacking configuration.
Nicolas.
^ permalink raw reply
* Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)
From: Eric Dumazet @ 2011-03-03 8:27 UTC (permalink / raw)
To: Fabio Checconi
Cc: Stephen Hemminger, David Miller, Luigi Rizzo, netdev,
Paolo Valente
In-Reply-To: <20110303081940.GA29685@gandalf.sssup.it>
Le jeudi 03 mars 2011 à 09:19 +0100, Fabio Checconi a écrit :
> Hi,
>
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Wed, Mar 02, 2011 06:31:27PM +0100
> >
> > Le mercredi 02 mars 2011 à 17:18 +0100, Eric Dumazet a écrit :
> > > Le mercredi 02 mars 2011 à 08:11 -0800, Stephen Hemminger a écrit :
> > >
> > > > I put the iproute2 code into the repository in the experimental branch.
> > > >
> > >
> > > Thanks
> > >
> > > It seems as soon as packets are dropped, qdisc is frozen (no more
> > > packets dequeued)
>
> I've been able to reproduce this problem using netem, and it seems to
> be due to this check:
>
> > + /* If the new skb is not the head of queue, then done here. */
> > + if (skb != qdisc_peek_head(cl->qdisc))
> > + return err;
>
> changing it to:
>
> if (cl->qdisc->q.qlen != 1)
> return err;
>
> seems to make things work. I think this is because qdisc_peek_head()
> looks at the wrong list, as the skb is not queued directly in q, but
> ends up in the child qdisc attached to cl->qdisc.
>
Strange, I used the default qdisc (pfifo) created in qfq classes
>
> > >
> > > Hmm...
> > >
> >
> > It seems class deletes are buggy.
> >
> > After one "tc class del dev $ETH classid 11:1 ..."
> >
> > a "tc -s -d qdisc show dev $ETH" triggers an Oops
> >
>
> This seems to be due to:
>
> > +static void qfq_destroy_class(struct Qdisc *sch, struct qfq_class *cl)
> > +{
> > + struct qfq_sched *q = (struct qfq_sched *)sch;
>
> it should be:
>
> struct qfq_sched *q = sched_priv(sch);
>
> The same bug you identified in qfq_reset_qdisc() is present also in
> qfq_drop(), both loops need to be corrected...
>
> It should also be noted that this scheduler (like HFSC, IIRC) depends
> on the child qdisc not to reorder packets for the guarantees to be met,
> as the timestamps need to be in sync with the length of the packet at the
> head of the queue. If this can't be guaranteed, to preserve the formal
> correctness it should be changed to always use the maximum packet size
> to calculate the timestamps.
>
> @Stephen: not that I'm proud of that, but all the bugs found so far are mine...
I am going to test an updated version, thanks for all these hints !
^ permalink raw reply
* Re: [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Eric Dumazet @ 2011-03-03 8:35 UTC (permalink / raw)
To: Changli Gao; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <AANLkTimNb-ZPFuhhdsSroFzaHKFauR3pTBXL3HMmnECf@mail.gmail.com>
Le jeudi 03 mars 2011 à 16:21 +0800, Changli Gao a écrit :
> On Thu, Mar 3, 2011 at 3:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> >
> > Thats minor, but using :
> >
> > u16 *dest = eth_hdr(skb)->h_dest;
> >
> > memcpy(dest, ptr, ETH_ALEN);
> >
> > Is better because compiler knows both destination and source are at
> > least aligned on shorts.
> >
> > On some arches, it helps to not using 6 bytes copy, but 3 shorts.
> >
> >
>
> Is it still true if ptr isn't aligned on shorts? And
> net_device.dev_addr is an unsigned char *pointer. Thanks.
>
dev_addr[] was aligned to word boundaries (because of natural structure
alignment), but the recent changes made it a char *pointer, so gcc is
not able to make this true anymore.
This could change if dev_addr was a pointer to struct netdev_hw_addr
^ permalink raw reply
* Re: inetpeer with create==0
From: David Miller @ 2011-03-03 8:34 UTC (permalink / raw)
To: xiaosuo; +Cc: eric.dumazet, netdev
In-Reply-To: <AANLkTinz4ppTi=580K9O=RZ6zbuBrTtjBDhEEnWM3Txg@mail.gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Thu, 3 Mar 2011 16:07:32 +0800
> After routing cache is removed totally, Linux is still unsuitable for
> use as a router on the core internet. Because it isn't a realtime OS,
> and the forwarding delay isn't bounded. With routing cache, the memory
> cost isn't bounded too. :)
With many cores, pipe can be filled no problem, because with multi-core
machine routing lookup proceeds just like Cisco router makes in hardware.
I wrote about this in my blog a few years ago.
^ permalink raw reply
* Re: inetpeer with create==0
From: David Miller @ 2011-03-03 8:32 UTC (permalink / raw)
To: eric.dumazet; +Cc: xiaosuo, netdev
In-Reply-To: <1299137977.2456.15.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 03 Mar 2011 08:39:37 +0100
> Le mercredi 02 mars 2011 à 22:42 -0800, David Miller a écrit :
>> Actually, back to the original topic, I wonder how bad it is to simply
>> elide the recheck in the create==0 case anyways. Except for the ipv4
>> fragmentation wraparound protection values, perfect inetpeer finding
>> is not necessary for correctness. And IPv4 fragmentation always calls
>> inetpeer with create!=0.
>
> We could use a seqlock, to detect that a writer might have changed
> things while we did our RCU lookup ?
That would certainly work.
^ permalink raw reply
* Re: [GIT PULL nf-2.6] IPVS
From: David Miller @ 2011-03-03 8:31 UTC (permalink / raw)
To: kaber; +Cc: horms, lvs-devel, netdev, netfilter-devel, netfilter, hans, ja
In-Reply-To: <4D6F3AD2.30502@trash.net>
From: Patrick McHardy <kaber@trash.net>
Date: Thu, 03 Mar 2011 07:53:06 +0100
> Yes. If you need it as base for further work, the fastest way
> is to ask Dave to pull net-2.6 into net-next-2.6, I can then
> pull it into nf-next.
I plan to do a net-2.6 into net-next-2.6 merge for another reason
tomorrow, so this should be taken care of soon.
^ permalink raw reply
* Re: inetpeer with create==0
From: David Miller @ 2011-03-03 8:30 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1299135107.2456.7.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 03 Mar 2011 07:51:47 +0100
> Hmm... I'm curious you send this hack to me ;)
Ok, it's so simple :-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index 48f8d45..a194e91 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -492,6 +492,8 @@ struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create)
unlink_from_unused(p);
return p;
}
+ if (!create)
+ return NULL;
/* retry an exact lookup, taking the lock before.
* At least, nodes should be hot in our cache.
@@ -505,7 +507,7 @@ struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create)
unlink_from_unused(p);
return p;
}
- p = create ? kmem_cache_alloc(peer_cachep, GFP_ATOMIC) : NULL;
+ p = kmem_cache_alloc(peer_cachep, GFP_ATOMIC);
if (p) {
p->daddr = *daddr;
atomic_set(&p->refcnt, 1);
^ permalink raw reply related
* Re: [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Changli Gao @ 2011-03-03 8:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <1299138901.2456.32.camel@edumazet-laptop>
On Thu, Mar 3, 2011 at 3:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> Thats minor, but using :
>
> u16 *dest = eth_hdr(skb)->h_dest;
>
> memcpy(dest, ptr, ETH_ALEN);
>
> Is better because compiler knows both destination and source are at
> least aligned on shorts.
>
> On some arches, it helps to not using 6 bytes copy, but 3 shorts.
>
>
Is it still true if ptr isn't aligned on shorts? And
net_device.dev_addr is an unsigned char *pointer. Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)
From: Fabio Checconi @ 2011-03-03 8:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: Stephen Hemminger, David Miller, Luigi Rizzo, netdev,
Paolo Valente
In-Reply-To: <1299087087.2920.27.camel@edumazet-laptop>
Hi,
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, Mar 02, 2011 06:31:27PM +0100
>
> Le mercredi 02 mars 2011 à 17:18 +0100, Eric Dumazet a écrit :
> > Le mercredi 02 mars 2011 à 08:11 -0800, Stephen Hemminger a écrit :
> >
> > > I put the iproute2 code into the repository in the experimental branch.
> > >
> >
> > Thanks
> >
> > It seems as soon as packets are dropped, qdisc is frozen (no more
> > packets dequeued)
I've been able to reproduce this problem using netem, and it seems to
be due to this check:
> + /* If the new skb is not the head of queue, then done here. */
> + if (skb != qdisc_peek_head(cl->qdisc))
> + return err;
changing it to:
if (cl->qdisc->q.qlen != 1)
return err;
seems to make things work. I think this is because qdisc_peek_head()
looks at the wrong list, as the skb is not queued directly in q, but
ends up in the child qdisc attached to cl->qdisc.
> >
> > Hmm...
> >
>
> It seems class deletes are buggy.
>
> After one "tc class del dev $ETH classid 11:1 ..."
>
> a "tc -s -d qdisc show dev $ETH" triggers an Oops
>
This seems to be due to:
> +static void qfq_destroy_class(struct Qdisc *sch, struct qfq_class *cl)
> +{
> + struct qfq_sched *q = (struct qfq_sched *)sch;
it should be:
struct qfq_sched *q = sched_priv(sch);
The same bug you identified in qfq_reset_qdisc() is present also in
qfq_drop(), both loops need to be corrected...
It should also be noted that this scheduler (like HFSC, IIRC) depends
on the child qdisc not to reorder packets for the guarantees to be met,
as the timestamps need to be in sync with the length of the packet at the
head of the queue. If this can't be guaranteed, to preserve the formal
correctness it should be changed to always use the maximum packet size
to calculate the timestamps.
@Stephen: not that I'm proud of that, but all the bugs found so far are mine...
^ permalink raw reply
* Re: inetpeer with create==0
From: Changli Gao @ 2011-03-03 8:07 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev
In-Reply-To: <20110302.224220.179921025.davem@davemloft.net>
On Thu, Mar 3, 2011 at 2:42 PM, David Miller <davem@davemloft.net> wrote:
>
> Thats why I am working hard to remove it :-)
>
> The %99 percentile performance of our routing cache is absolutely
> terrible. The routing cache does nothing except get in the way.
>
> It is the reason why Linux is still completely unsuitable for use as a
> router on the core internet.
>
After routing cache is removed totally, Linux is still unsuitable for
use as a router on the core internet. Because it isn't a realtime OS,
and the forwarding delay isn't bounded. With routing cache, the memory
cost isn't bounded too. :)
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Eric Dumazet @ 2011-03-03 7:55 UTC (permalink / raw)
To: Changli Gao; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <1299136034-5549-1-git-send-email-xiaosuo@gmail.com>
Le jeudi 03 mars 2011 à 15:07 +0800, Changli Gao a écrit :
> When there is a ptype handler holding a clone of this skb, whose
> destination MAC addresse is overwritten, the owner of this handler may
> get a corrupted packet.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> v2: fix the bug in the previous one. Thank him.
> drivers/net/bonding/bond_main.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 912b416..7b7ca97 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1511,9 +1511,13 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
> if (bond_dev->priv_flags & IFF_MASTER_ALB &&
> bond_dev->priv_flags & IFF_BRIDGE_PORT &&
> skb->pkt_type == PACKET_HOST) {
> - u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
>
> - memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
> + if (unlikely(skb_cow_head(skb,
> + skb->data - skb_mac_header(skb)))) {
> + kfree_skb(skb);
> + return NULL;
> + }
> + memcpy(eth_hdr(skb)->h_dest, bond_dev->dev_addr, ETH_ALEN);
> }
>
> return skb;
Thats minor, but using :
u16 *dest = eth_hdr(skb)->h_dest;
memcpy(dest, ptr, ETH_ALEN);
Is better because compiler knows both destination and source are at
least aligned on shorts.
On some arches, it helps to not using 6 bytes copy, but 3 shorts.
^ permalink raw reply
* Re: [GIT PULL nf-2.6] IPVS
From: Simon Horman @ 2011-03-03 7:47 UTC (permalink / raw)
To: Patrick McHardy
Cc: lvs-devel, netdev, netfilter-devel, netfilter, Hans Schillstrom,
Julian Anastasov
In-Reply-To: <4D6F3AD2.30502@trash.net>
On Thu, Mar 03, 2011 at 07:53:06AM +0100, Patrick McHardy wrote:
> On 02.03.2011 23:06, Simon Horman wrote:
> > On Wed, Mar 02, 2011 at 11:58:18AM +0100, Patrick McHardy wrote:
> >> Am 01.03.2011 23:59, schrieb Simon Horman:
> >>> Hi Patrick,
> >>>
> >>> please consider pulling
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/horms/lvs-test-2.6.git for-patrick
> >>> to get the following change from Julian. Please note that it is an nf-2.6
> >>> (that is 2.6.38-rc) change.
> >>>
> >>> Julian Anastasov (1):
> >>> ipvs: fix dst_lock locking on dest update
> >>
> >> Pulled, thanks Simon.
> >
> > Thanks Patrick.
> >
> > This change is also needed in nf-next-2.6 but I assume that
> > will automatically happen when nf-2.6 is merged into nf-next-2.6,
> > possibly via a merge of net-2.6 into net-next-2.6.
>
> Yes. If you need it as base for further work, the fastest way
> is to ask Dave to pull net-2.6 into net-next-2.6, I can then
> pull it into nf-next.
There is no rush at this time.
^ permalink raw reply
* Re: inetpeer with create==0
From: Eric Dumazet @ 2011-03-03 7:39 UTC (permalink / raw)
To: David Miller; +Cc: xiaosuo, netdev
In-Reply-To: <20110302.224220.179921025.davem@davemloft.net>
Le mercredi 02 mars 2011 à 22:42 -0800, David Miller a écrit :
> Actually, back to the original topic, I wonder how bad it is to simply
> elide the recheck in the create==0 case anyways. Except for the ipv4
> fragmentation wraparound protection values, perfect inetpeer finding
> is not necessary for correctness. And IPv4 fragmentation always calls
> inetpeer with create!=0.
>
We could use a seqlock, to detect that a writer might have changed
things while we did our RCU lookup ?
^ permalink raw reply
* Re: [PATCH 5/7] x25: remove the BKL
From: Andrew Hendry @ 2011-03-03 7:38 UTC (permalink / raw)
To: David Miller; +Cc: arnd, linux-kernel, linux-x25, netdev, eric.dumazet
In-Reply-To: <20110301.151622.200374696.davem@davemloft.net>
Looks good, put 8gig through it over the past few days and system stable.
Tested-by: Andrew Hendry <andrew.hendry@gmail.com>
On Wed, Mar 2, 2011 at 10:16 AM, David Miller <davem@davemloft.net> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Wed, 2 Mar 2011 00:13:09 +0100
>
>> This replaces all instances of lock_kernel in x25
>> with lock_sock, taking care to release the socket
>> lock around sleeping functions (sock_alloc_send_skb
>> and skb_recv_datagram). It is not clear whether
>> this is a correct solution, but it seem to be what
>> other protocols do in the same situation.
>>
>> Includes a fix suggested by Eric Dumazet.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Acked-by: David S. Miller <davem@davemloft.net>
>
^ permalink raw reply
* Re: Kernel panic nf_nat_setup_info+0x5b3/0x6e0
From: Changli Gao @ 2011-03-03 7:33 UTC (permalink / raw)
To: Oleg A. Arkhangelsky
Cc: Patrick McHardy, netfilter-devel, netdev, Paul E McKenney
In-Reply-To: <124481299095426@web67.yandex.ru>
[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]
On Thu, Mar 3, 2011 at 3:50 AM, "Oleg A. Arkhangelsky"
<sysoleg@yandex.ru> wrote:
> 02.03.2011, 17:37, "Changli Gao" <xiaosuo@gmail.com>:
>
>> t should be NULL here, as offsetof(struct nf_conn, dst.protonum) == 0x36.
>> We should free the nf_ct_extend with call_rcu(), since nat ext is
>> referenced in the rcu read context.
>
> Yes, I think the problem is triggered when nf_conntrack_free() is called by
> different CPU during net->ipv4.nat_bysource hash traversal. Extensions
> framework doesn't have any SLAB_DESTROY_BY_RCU magic.
>
> I'm not sure, but couldn't this problem be introduced by:
>
> ea781f197d6a835cbb93a0bf88ee1696296ed8aa
> netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of call_rcu()
>
> ?
>
There is nothing to do with SLAB_DESTROY_BY_RCU. Here is the comment
for this flag:
/*
* SLAB_DESTROY_BY_RCU - **WARNING** READ THIS!
*
* This delays freeing the SLAB page by a grace period, it does _NOT_
* delay object freeing. This means that if you do kmem_cache_free()
* that memory location is free to be reused at any time. Thus it may
* be possible to see another object there in the same RCU grace period.
*
* This feature only ensures the memory location backing the object
* stays valid, the trick to using this is relying on an independent
* object validation pass. Something like:
...
Please try the patch attached and test if the problem is solved or not. Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
[-- Attachment #2: nf_ext_rcu.diff --]
[-- Type: text/plain, Size: 1384 bytes --]
diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index 2dcf317..354cccb9 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -66,13 +66,15 @@ static inline void nf_ct_ext_destroy(struct nf_conn *ct)
__nf_ct_ext_destroy(ct);
}
+void __nf_ct_ext_free_rcu(struct rcu_head *head);
+
/* Free operation. If you want to free a object referred from private area,
* please implement __nf_ct_ext_free() and call it.
*/
static inline void nf_ct_ext_free(struct nf_conn *ct)
{
if (ct->ext)
- kfree(ct->ext);
+ call_rcu(&ct->ext->rcu, __nf_ct_ext_free_rcu);
}
/* Add this type, returns pointer to data or NULL. */
diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
index 80a23ed..3a47b76 100644
--- a/net/netfilter/nf_conntrack_extend.c
+++ b/net/netfilter/nf_conntrack_extend.c
@@ -68,11 +68,12 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
return (void *)(*ext) + off;
}
-static void __nf_ct_ext_free_rcu(struct rcu_head *head)
+void __nf_ct_ext_free_rcu(struct rcu_head *head)
{
struct nf_ct_ext *ext = container_of(head, struct nf_ct_ext, rcu);
kfree(ext);
}
+EXPORT_SYMBOL_GPL(__nf_ct_ext_free_rcu);
void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
{
^ permalink raw reply related
* [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Changli Gao @ 2011-03-03 7:07 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: David S. Miller, Eric Dumazet, netdev, Changli Gao
When there is a ptype handler holding a clone of this skb, whose
destination MAC addresse is overwritten, the owner of this handler may
get a corrupted packet.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
v2: fix the bug in the previous one. Thank him.
drivers/net/bonding/bond_main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 912b416..7b7ca97 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1511,9 +1511,13 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
if (bond_dev->priv_flags & IFF_MASTER_ALB &&
bond_dev->priv_flags & IFF_BRIDGE_PORT &&
skb->pkt_type == PACKET_HOST) {
- u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
- memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
+ if (unlikely(skb_cow_head(skb,
+ skb->data - skb_mac_header(skb)))) {
+ kfree_skb(skb);
+ return NULL;
+ }
+ memcpy(eth_hdr(skb)->h_dest, bond_dev->dev_addr, ETH_ALEN);
}
return skb;
^ permalink raw reply related
* Re: [PATCH 2/2] bonding: COW before overwriting the destination MAC address
From: Changli Gao @ 2011-03-03 6:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <1299134742.2456.1.camel@edumazet-laptop>
On Thu, Mar 3, 2011 at 2:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 03 mars 2011 à 12:25 +0800, Changli Gao a écrit :
>> When there is a ptype handler holding a clone of this skb, whose
>> destination MAC addresse is overwritten, the owner of this handler may
>> get a corrupted packet.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>> drivers/net/bonding/bond_main.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 912b416..211be52 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1513,6 +1513,11 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>> skb->pkt_type == PACKET_HOST) {
>> u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
>>
>> + if (unlikely(skb_cow_head(skb,
>> + skb->data - skb_mac_header(skb)))) {
>> + kfree_skb(skb);
>> + return NULL;
>> + }
>> memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
>> }
>>
>
> This seems buggy.
>
> dest points to old skb head content.
>
>
Oh, thanks. I'll respin it.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [GIT PULL nf-2.6] IPVS
From: Patrick McHardy @ 2011-03-03 6:53 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, netfilter-devel, netfilter, Hans Schillstrom,
Julian Anastasov
In-Reply-To: <20110302220637.GD8193@verge.net.au>
On 02.03.2011 23:06, Simon Horman wrote:
> On Wed, Mar 02, 2011 at 11:58:18AM +0100, Patrick McHardy wrote:
>> Am 01.03.2011 23:59, schrieb Simon Horman:
>>> Hi Patrick,
>>>
>>> please consider pulling
>>> git://git.kernel.org/pub/scm/linux/kernel/git/horms/lvs-test-2.6.git for-patrick
>>> to get the following change from Julian. Please note that it is an nf-2.6
>>> (that is 2.6.38-rc) change.
>>>
>>> Julian Anastasov (1):
>>> ipvs: fix dst_lock locking on dest update
>>
>> Pulled, thanks Simon.
>
> Thanks Patrick.
>
> This change is also needed in nf-next-2.6 but I assume that
> will automatically happen when nf-2.6 is merged into nf-next-2.6,
> possibly via a merge of net-2.6 into net-next-2.6.
Yes. If you need it as base for further work, the fastest way
is to ask Dave to pull net-2.6 into net-next-2.6, I can then
pull it into nf-next.
^ permalink raw reply
* Re: inetpeer with create==0
From: Eric Dumazet @ 2011-03-03 6:51 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110302.204545.193730647.davem@davemloft.net>
Le mercredi 02 mars 2011 à 20:45 -0800, David Miller a écrit :
> Eric, I was profiling the non-routing-cache case and something that stuck
> out is the case of calling inet_getpeer() with create==0.
>
> If an entry is not found, we have to redo the lookup under a spinlock
> to make certain that a concurrent writer rebalancing the tree does
> not "hide" an existing entry from us.
>
> This makes the case of a create==0 lookup for a not-present entry
> really expensive. It is on the order of 600 cpu cycles on my
> Niagara2.
>
Well, your test assumes all data is already on cpu caches ?
I'll take a look, but my reasoning was that the real cost in DDOS
situation is to bring data into caches. With a 20 depth, cache misses
costs are the problem.
The second lookup was basically free.
> I added a hack to not do the relookup under the lock when create==0
> and it now costs less than 300 cycles.
>
Hmm... I'm curious you send this hack to me ;)
> This is now a pretty common operation with the way we handle COW'd
> metrics, so I think it's definitely worth optimizing.
>
> I looked at the generic radix tree implementation, and it supports
> full RCU lookups in parallel with insert/delete. It handles the race
> case without the relookup under lock because it creates fixed paths
> to "slots" where nodes live using shifts and masks. So if a path
> to a slot ever existed, it will always exist.
>
> Take a look at lib/radix-tree.c and include/linux/radix-tree.h if
> you are curious.
>
> I think we should do something similar for inetpeer. Currently we
> cannot just use the existing generic radix-tree code because it only
> supports indexes as large as "unsigned long" and we need to handle
> 128-bit ipv6 addresses.
^ permalink raw reply
* Re: [PATCH 2/2] bonding: COW before overwriting the destination MAC address
From: Eric Dumazet @ 2011-03-03 6:45 UTC (permalink / raw)
To: Changli Gao; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <1299126321-9300-1-git-send-email-xiaosuo@gmail.com>
Le jeudi 03 mars 2011 à 12:25 +0800, Changli Gao a écrit :
> When there is a ptype handler holding a clone of this skb, whose
> destination MAC addresse is overwritten, the owner of this handler may
> get a corrupted packet.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> drivers/net/bonding/bond_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 912b416..211be52 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1513,6 +1513,11 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
> skb->pkt_type == PACKET_HOST) {
> u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
>
> + if (unlikely(skb_cow_head(skb,
> + skb->data - skb_mac_header(skb)))) {
> + kfree_skb(skb);
> + return NULL;
> + }
> memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
> }
>
This seems buggy.
dest points to old skb head content.
^ permalink raw reply
* Re: inetpeer with create==0
From: David Miller @ 2011-03-03 6:42 UTC (permalink / raw)
To: xiaosuo; +Cc: eric.dumazet, netdev
In-Reply-To: <AANLkTi=Ty+C3b9U+151dUMpgGiwG3AE4dMmc3djWmp6v@mail.gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Thu, 3 Mar 2011 14:27:40 +0800
> On Thu, Mar 3, 2011 at 1:36 PM, David Miller <davem@davemloft.net> wrote:
>>
>> Because trie eliminates all of the issues of having to size a hash
>> table, dynamically resize it, etc.
>>
>> Trie gives well bounded performance dependent solely upon size of
>> the table, rather than access patterns, distribution of keys, and
>> how perfect hash function is.
>
> Thanks for your explaination. Routing cache has all of these issues. :)
Thats why I am working hard to remove it :-)
The %99 percentile performance of our routing cache is absolutely
terrible. The routing cache does nothing except get in the way.
It is the reason why Linux is still completely unsuitable for use as a
router on the core internet.
> Radix tree may cost lots of memory than a rbtree, avl tree or hash
> table. Here is a case: turning to rbtree from radix tree.
> http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commit;h=8549164143a5431f9d9ea846acaa35a862410d9c
I would not be against the use of rbtree or similar if it could be
done with with fully RCU non-locked lookups even in the not-present
case.
> Hash table + jhash have been proven a safe and efficient data
> structure for large data sets(conntrack and ipvs), although the size
> of the hash table may have to be adjusted by an administrator.
If anything is proven, it is that hashing based collections of cached
information are nothing but trouble when the contents are controlled in
some way by external entities.
Actually, back to the original topic, I wonder how bad it is to simply
elide the recheck in the create==0 case anyways. Except for the ipv4
fragmentation wraparound protection values, perfect inetpeer finding
is not necessary for correctness. And IPv4 fragmentation always calls
inetpeer with create!=0.
Eric?
^ permalink raw reply
* RE: [Bugme-new] [Bug 29712] New: Bonding Driver(version : 3.5.0) - Problem with ARP monitoring in active backup mode
From: Harsha R02 @ 2011-03-03 6:31 UTC (permalink / raw)
To: Jay Vosburgh
Cc: Brian Haley, Andrew Morton, bugzilla-daemon, bugme-daemon, netdev
In-Reply-To: <17444.1298660550@death>
Hi Jay,
We found that the patch that is presented here has some issues and we
cannot go with this solution.
In function "bond_ab_arp_probe" in addition to sending arp probes for
the currently active slave we should also
be sending arp probes for the primary_slave if the link status of the
primary slave is up correct ?
I have made changes as below :
static void bond_ab_arp_probe(struct bonding *bond)
{
struct slave *slave;
int i;
read_lock(&bond->curr_slave_lock);
if (bond->current_arp_slave && bond->curr_active_slave)
pr_info(DRV_NAME "PROBE: c_arp %s && cas %s BAD\n",
bond->current_arp_slave->dev->name,
bond->curr_active_slave->dev->name);
if (bond->curr_active_slave) {
+ if((bond->curr_active_slave != bond->primary_slave) &&
+ (IS_UP(bond->primary_slave->dev))) {
+ bond_arp_send_all(bond, bond->primary_slave);
+ }
bond_arp_send_all(bond, bond->curr_active_slave);
read_unlock(&bond->curr_slave_lock);
return;
}
Please let us know if this can help us ? or if you see any side effects
?
Thanks,
Harsha
-----Original Message-----
From: Jay Vosburgh [mailto:fubar@us.ibm.com]
Sent: Saturday, February 26, 2011 12:33 AM
To: Harsha R02
Cc: Brian Haley; Andrew Morton; bugzilla-daemon@bugzilla.kernel.org;
bugme-daemon@bugzilla.kernel.org; netdev@vger.kernel.org
Subject: Re: [Bugme-new] [Bug 29712] New: Bonding Driver(version :
3.5.0) - Problem with ARP monitoring in active backup mode
Harsha R02 <Harsha.R02@mphasis.com> wrote:
>diff --git a/drivers/net/bonding/bond_main.c
b/drivers/net/bonding/bond_main.c
>index 40fb5ee..0413917 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3020,11 +3020,16 @@ static void bond_ab_arp_probe(struct bonding
*bond)
> bond->curr_active_slave->dev->name);
> if (bond->curr_active_slave) {
>+ if((bond->curr_active_slave != bond->primary_slave) &&
>+ (IS_UP(bond->primary_slave->dev)))
>+ goto failover;
>+
> bond_arp_send_all(bond, bond->curr_active_slave);
> read_unlock(&bond->curr_slave_lock);
> return;
> }
>+failover:
> read_unlock(&bond->curr_slave_lock);
> /* if we don't have a curr_active_slave, search for the next
available
I'm not sure this is the proper place to put the "failover:"
label, as it will go through the "search for any peer" logic that's
normally used when there are no available slaves. That will likely take
longer than simply switching to the primary.
It should be possible to simply call bond_change_active_slave
with the appropriate arguments; did you try this?
-J
>-----------------------------------------------------------------------
--------
>From: Harsha R02
>Sent: Fri 2/25/2011 6:14 PM
>To: Brian Haley; Andrew Morton
>Cc: bugzilla-daemon@bugzilla.kernel.org;
bugme-daemon@bugzilla.kernel.org;
>netdev@vger.kernel.org; Jay Vosburgh
>Subject: RE: [Bugme-new] [Bug 29712] New: Bonding Driver(version :
3.5.0) -
>Problem with ARP monitoring in active backup mode
>
>Attached patch resolves the issue. Failover happened back to primary
when it
>was up again in both the point to point and switch configuration.
>
>Please let us know if this change can be included.
>
>Thanks,
>
>- Harsha
>
>-----Original Message-----
>From: Brian Haley [mailto:brian.haley@hp.com]
>Sent: Friday, February 25, 2011 9:12 AM
>To: Andrew Morton
>Cc: Harsha R02; bugzilla-daemon@bugzilla.kernel.org;
>bugme-daemon@bugzilla.kernel.org; netdev@vger.kernel.org; Jay Vosburgh
>Subject: Re: [Bugme-new] [Bug 29712] New: Bonding Driver(version :
3.5.0) -
>Problem with ARP monitoring in active backup mode
>
>On 02/24/2011 05:51 PM, Andrew Morton wrote:
>> (switched to email. Please respond via emailed reply-to-all, not via
the
>> bugzilla web interface).
>>
>> On Wed, 23 Feb 2011 10:41:34 GMT
>> bugzilla-daemon@bugzilla.kernel.org wrote:
>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=29712
>>>
>>> Summary: Bonding Driver(version : 3.5.0) - Problem with
ARP
>>> monitoring in active backup mode
>>> Product: Drivers
>>> Version: 2.5
>>> Kernel Version: 2.6.32
>>
>> That's a paleolithic kernel you have there. This problem might have
>> been fixed already. Can you test a more recent kernel?
>
>I can add some more info since I originally looked at the problem.
This
>happens on 2.6.38 as well, and on this 2.6.32 kernel with a backported
>3.7.0 bonding driver (with the primary_reselect option). Harsha has a
>prototype patch that's being tested, but wanted to log the bug to see
>if one of the bonding maintainers had a better solution.
>
>I'll let him respond as I'm now out of the loop...
>
>Thanks,
>
>-Brian
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
Information transmitted by this e-mail is proprietary to MphasiS, its associated companies and/ or its customers and is intended
for use only by the individual or entity to which it is addressed, and may contain information that is privileged, confidential or
exempt from disclosure under applicable law. If you are not the intended recipient or it appears that this mail has been forwarded
to you without proper authority, you are notified that any use or dissemination of this information in any manner is strictly
prohibited. In such cases, please notify us immediately at mailmaster@mphasis.com and delete this mail from your records.
^ 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