* Re: [PATCH 7/7] [TFRC] New rx history code
From: Gerrit Renker @ 2007-12-05 9:35 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, netdev, dccp
In-Reply-To: <20071204115939.GA18084@ghostprotocols.net>
Quoting Arnaldo:
| Em Tue, Dec 04, 2007 at 06:55:17AM +0000, Gerrit Renker escreveu:
| > NAK. You have changed the control flow of the algorithm and the underlying
| > data structure. Originally it had been an array of pointers, and this had
| > been a design decision right from the first submissions in March. From I
| > of http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/ccid3_packet_reception/loss_detection/loss_detection_algorithm_notes.txt
| >
| > "1. 'ring' is an array of pointers rather than a contiguous area in
| > memory. The reason is that, when having to swap adjacent entries
| > to sort the history, it is easier to swap pointers than to copy
| > (heavy-weight) history-entry data structs."
| >
| > But in your algorithm it becomes a normal linear array.
| >
| > As a consequence all subsequent code needs to be rewritten to use
| > copying instead of swapping pointers. And there is a lot of that, since
| > the loss detection code makes heavy use of swapping entries in order to
| > cope with reordered and/or delayed packets.
|
| So lets not do that, the decision was made based on the RX history patch
| alone, where, as pointed out, no swapping occurs.
|
| > This is not what I would call "entirely equivalent" as you claim. Your
| > algorithm is fundamentally different, the control flow is not the same,
| > nor are the underlying data structures.
|
| Its is equivalent up to what is in the tree, thank you for point out
| that in subsequent patches it will be used.
|
| Had this design decision been made explicit in the changeset comment
| that introduces the data structure I wouldn't have tried to reduce the
| number of allocations.
|
| And you even said that it was a good decision when first reacting to
| this change, which I saw as positive feedback for the RFC I sent.
|
That was my fault. Your solution "looked" much better to me but even I had forgotten
that the algorithm is based on an array of pointers. When I finally sat down
and looked through the patch set I found the original notes from March and
saw that using a linear array instead of an array of pointers will require quite
a few changes and means changing the algorithm. I then thought through whether there
could be any advantage in using a linear array as in this patch, but could find none.
In the end this went back to the same questions and issues that were tackled between
February and March this year. I wish we could have had this dicussion then; it would
have made this email unnecessary.
I thank you for your replay and I am sorry if you took offense at my perhaps a little strong
reaction, here is the essence what I tried to convey but what maybe did not succeed in conveying:
* I am absolutely ok with you making changes to variable names, namespaces, file organisation,
overall arrangement etc (as per previous email). You have experience and this will often be a
benefit. For instance, you combined tfrc_entry_from_skb() with tfrc_rx_hist_update() to give
a new function, tfrc_rx_hist_add_packet(). This is ok since entry_from_skb() is not otherwise
used (and I only found this out when reading your patch). (On the other hand, since this is a
4-element ring buffer, it is no real adding, the same entry will frequently be overwritten,
this is why it was initially called hist_update().)
* I am also not so much concerned about credit. As long as the (changed) end result is as
least as good as or hopefully better than the submitted input, the author name is a side
issue; so I do think that your approach is sound and fair. The only place where credits
are needed is for the SatSix project (www.ist-satsix.org) which funded this work. This is
why some of the copyrights now included "University of Aberdeen". What in any case I'd like
to add is at least the Signed-off-by: if it turns out to be a mess I want to contribute my
part of responsibility.
* But where I need to interact is when changes are made to the algorithm, even if these may
seem small. The underlying reasons that lead to the code may not be immediately clear,
but since this is a solution for a specific problem, there are also specific reasons; which
is why for algorithm changes (and underlying data structure) I'd ask you to discuss this first
before committing the patch.
* In part you are already doing this (message subject); it may be necessary to adapt the way
of discussion/presentation. The subject is complex (spent a week with the flow chart only)
and it is a lot - initially this had been 40 small patches.
| > | I modified it just to try to make it closer to the existing API, hide details from
| > | the CCIDs and fix a couple bugs found in the initial implementation.
| > What is "a couple bugs"? So far you have pointed out only one problem and that was
| > agreed, it can be fixed. But it remains a side issue, it is not a failure of the
|
| I pointed two problems one ended up being just the fact that tfrc_ewma
| checks if the average is zero for every calculation.
|
With regard to the tfrc_ewma we need to consider the discussion as I think we are having a
misunderstanding. It is necessary to
(a) deal with the case that a peer may be sending zero-sized packets:
- one solution is to simply omit the check "if (len > 0)" and declare 0-sized data
- I think you have a point in that the len check is indeed redundant when we are
already checking that this is a data packet
packets as pathological (which is probably what they are);
- a BUG_ON would be not such a good solution since 0-sized packets are legal;
- my suggestion is to remove the "len > 0" test - then tfrc_ewma() can also be
used directly, without the surrounding inline function wrapper.
(b) reconsider the control flow: in your patch set you introduced a different control flow
in the rx_packet_recv() function. I tried to figure out why this was introduced, there
are two problems, one not immediately obvious at this moment:
- it does not consider where and how loss detection is done (which is part of a later
patch). This is a key point why the flow may seem strange: loss detection must be
done on both non-data and data packets; there are several cases depending on whether
loss previously occurred; the accounting must also be done in parallel for data packets
(for instance s and bytes_recvd) and non-data packets (highest-received seqno is always
- it is not equivalent with the initial one, when comparing this to the flowchart:
http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/ccid3_packet_reception/reception-main-flowchart.png
| > I have provided documentation, written test modules, and am able to prove
| > that the algorithm as submitted works reasonably correct. In addition, the
| > behaviour has been verified using regression tests.
| >
| > I am not prepared to take your claims and expressions of "deepest
| > respect" at face value since your actions - not your words - show that
| > you are, in fact, not dealing respectfully with work which has taken
| > months to complete and verify.
| >
| > During 9 months on dccp@vger you did not provide so much as a paragraph
| > of feedback. Instead you mopped up code from the test tree, modified it
| > according to your own whims and now, in the circle of your
| > invitation-only buddies, start to talk about having discussions and
| > iterations. The only iteration I can see is in fixing up the things you
| > introduced, so it is not you who has to pay the price.
| >
| > Sorry, unless you can offer a more mature model of collaboration,
| > consider me out of this and the patches summarily withdrawn. I am not
| > prepared to throw away several months of work just because you feel
| > inspired to do as you please with the work of others.
|
| Again you want to have your patches accepted as-is. Pointed to one case
| where I gave you credit while improving on your work (TX history) and
| another where the changes were up for review. I don't consider this a
| warm welcome for me to finally dedicate time to this effort. Would you
| prefer to continue working without help? I think we should encourage
| more people to work on DCCP, you seem to think that changes to your work
| are not acceptable.
|
Yes and no. The internal algorithm I would rather have accepted as-is, unless
there are good reasons to change it. But this by no means means I want this
waved through like a diplomat's car at the border: there are reasons why
it is as it is, and I am happy to discuss these reasons; and if someone
can point out a better solution, will accept that. It is just that this
is a solution to a specific problem which so far has proven to work, and
I therefore think that we can spent the time more efficiently by not
fixing things that have already been fixed once: there are many more
problems in DCCP (just recently you mentioned robustness of memory usage,
then there is the ongoing discussion of how to integrate CCID4 as well);
I think we could divide up work time with more benefit.
| Perhaps others can comment on what is happening and help us find, as you
| say, to find a more mature model of collaboration.
|
No need to be cynical, you have replied gracefully and with substance.
For that I thank you. The topic is complicated, adapting how it is
treated may improve the discussion - for instance, if it helps, we could
switch back to smaller patch format as original, which you have already
been introducing.
Best regards
Gerrit
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Herbert Xu @ 2007-12-05 9:26 UTC (permalink / raw)
To: Wang Chen; +Cc: David S. Miller, netdev
In-Reply-To: <47566D59.1080905@cn.fujitsu.com>
On Wed, Dec 05, 2007 at 05:20:25PM +0800, Wang Chen wrote:
>
> Maybe AF_UNSPEC? It's joke:)
> I just more like switch to avoid false positive problem.
We should check for things like that when a socket is constructed
and not when it's used.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Wang Chen @ 2007-12-05 9:20 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev
In-Reply-To: <20071205091028.GA15006@gondor.apana.org.au>
Herbert Xu said the following on 2007-12-5 17:10:
> On Wed, Dec 05, 2007 at 05:04:48PM +0800, Wang Chen wrote:
>> Yes, now, sunrpc only supports INET and INET6.
>> But UDPX_INC_STATS_BH is defined in udp.h, in case others use it :)
>
> Which other family would that be :)
>
Maybe AF_UNSPEC? It's joke:)
I just more like switch to avoid false positive problem.
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Herbert Xu @ 2007-12-05 9:10 UTC (permalink / raw)
To: Wang Chen; +Cc: David S. Miller, netdev
In-Reply-To: <475669B0.4030501@cn.fujitsu.com>
On Wed, Dec 05, 2007 at 05:04:48PM +0800, Wang Chen wrote:
>
> Yes, now, sunrpc only supports INET and INET6.
> But UDPX_INC_STATS_BH is defined in udp.h, in case others use it :)
Which other family would that be :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Wang Chen @ 2007-12-05 9:04 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev
In-Reply-To: <20071205090228.GA14659@gondor.apana.org.au>
Herbert Xu said the following on 2007-12-5 17:02:
> On Wed, Dec 05, 2007 at 04:39:48PM +0800, Wang Chen wrote:
>> Herbert Xu said the following on 2007-12-5 10:15:
>>> +#define UDPX_INC_STATS_BH(sk, field) \
>>> + do { \
>>> + if ((sk)->sk_family == AF_INET) \
>>> + UDP_INC_STATS_BH(field, 0); \
>>> + else \
>>> + UDP6_INC_STATS_BH(field, 0); \
>>> + } while (0);
>>> +
>> Because there are lot of address families, not only AF_INET
>> and AF_INET6.
>
> There may be a lot of families but only two run UDP, or rather, only
> two are supported by sunrpc :)
>
Yes, now, sunrpc only supports INET and INET6.
But UDPX_INC_STATS_BH is defined in udp.h, in case others use it :)
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Herbert Xu @ 2007-12-05 9:02 UTC (permalink / raw)
To: Wang Chen; +Cc: David S. Miller, netdev
In-Reply-To: <475663D4.9040806@cn.fujitsu.com>
On Wed, Dec 05, 2007 at 04:39:48PM +0800, Wang Chen wrote:
> Herbert Xu said the following on 2007-12-5 10:15:
> > +#define UDPX_INC_STATS_BH(sk, field) \
> > + do { \
> > + if ((sk)->sk_family == AF_INET) \
> > + UDP_INC_STATS_BH(field, 0); \
> > + else \
> > + UDP6_INC_STATS_BH(field, 0); \
> > + } while (0);
> > +
>
> Because there are lot of address families, not only AF_INET
> and AF_INET6.
There may be a lot of families but only two run UDP, or rather, only
two are supported by sunrpc :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH] bonding: Fix time comparison
From: David Sterba @ 2007-12-05 8:52 UTC (permalink / raw)
To: linux-kernel; +Cc: dsterba, bonding-devel, fubar, ctindel, netdev
bonding: Fix time comparison
Use macros for comparing jiffies. Jiffies' wrap caused missed events and hangs.
Module reinsert was needed to make bonding work again.
Signed-off-by: David Sterba <dsterba@suse.cz>
---
drivers/net/bonding/bond_main.c | 25 +++++++++++++------------
1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 423298c..e4a4714 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -74,6 +74,7 @@
#include <linux/ethtool.h>
#include <linux/if_vlan.h>
#include <linux/if_bonding.h>
+#include <linux/jiffies.h>
#include <net/route.h>
#include <net/net_namespace.h>
#include "bonding.h"
@@ -2722,8 +2723,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
*/
bond_for_each_slave(bond, slave, i) {
if (slave->link != BOND_LINK_UP) {
- if (((jiffies - slave->dev->trans_start) <= delta_in_ticks) &&
- ((jiffies - slave->dev->last_rx) <= delta_in_ticks)) {
+ if (time_before_eq(jiffies, slave->dev->trans_start + delta_in_ticks) &&
+ time_before_eq(jiffies, slave->dev->last_rx + delta_in_ticks)) {
slave->link = BOND_LINK_UP;
slave->state = BOND_STATE_ACTIVE;
@@ -2754,8 +2755,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
* when the source ip is 0, so don't take the link down
* if we don't know our ip yet
*/
- if (((jiffies - slave->dev->trans_start) >= (2*delta_in_ticks)) ||
- (((jiffies - slave->dev->last_rx) >= (2*delta_in_ticks)) &&
+ if (time_after_eq(jiffies, slave->dev->trans_start + 2*delta_in_ticks) ||
+ (time_after_eq(jiffies, slave->dev->last_rx + 2*delta_in_ticks) &&
bond_has_ip(bond))) {
slave->link = BOND_LINK_DOWN;
@@ -2848,8 +2849,8 @@ void bond_activebackup_arp_mon(struct work_struct *work)
*/
bond_for_each_slave(bond, slave, i) {
if (slave->link != BOND_LINK_UP) {
- if ((jiffies - slave_last_rx(bond, slave)) <=
- delta_in_ticks) {
+ if (time_before_eq(jiffies,
+ slave_last_rx(bond, slave) + delta_in_ticks)) {
slave->link = BOND_LINK_UP;
@@ -2858,7 +2859,7 @@ void bond_activebackup_arp_mon(struct work_struct *work)
write_lock_bh(&bond->curr_slave_lock);
if ((!bond->curr_active_slave) &&
- ((jiffies - slave->dev->trans_start) <= delta_in_ticks)) {
+ time_before_eq(jiffies, slave->dev->trans_start + delta_in_ticks)) {
bond_change_active_slave(bond, slave);
bond->current_arp_slave = NULL;
} else if (bond->curr_active_slave != slave) {
@@ -2897,7 +2898,7 @@ void bond_activebackup_arp_mon(struct work_struct *work)
if ((slave != bond->curr_active_slave) &&
(!bond->current_arp_slave) &&
- (((jiffies - slave_last_rx(bond, slave)) >= 3*delta_in_ticks) &&
+ (time_after_eq(jiffies, slave_last_rx(bond, slave) + 3*delta_in_ticks) &&
bond_has_ip(bond))) {
/* a backup slave has gone down; three times
* the delta allows the current slave to be
@@ -2943,10 +2944,10 @@ void bond_activebackup_arp_mon(struct work_struct *work)
* before being taken out. if a primary is being used, check
* if it is up and needs to take over as the curr_active_slave
*/
- if ((((jiffies - slave->dev->trans_start) >= (2*delta_in_ticks)) ||
- (((jiffies - slave_last_rx(bond, slave)) >= (2*delta_in_ticks)) &&
- bond_has_ip(bond))) &&
- ((jiffies - slave->jiffies) >= 2*delta_in_ticks)) {
+ if ((time_after_eq(jiffies, slave->dev->trans_start + 2*delta_in_ticks) ||
+ (time_after_eq(jiffies, slave_last_rx(bond, slave) + 2*delta_in_ticks) &&
+ bond_has_ip(bond))) &&
+ time_after_eq(jiffies, slave->jiffies + 2*delta_in_ticks)) {
slave->link = BOND_LINK_DOWN;
^ permalink raw reply related
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Wang Chen @ 2007-12-05 8:39 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev
In-Reply-To: <20071205021559.GA27707@gondor.apana.org.au>
Herbert Xu said the following on 2007-12-5 10:15:
> +#define UDPX_INC_STATS_BH(sk, field) \
> + do { \
> + if ((sk)->sk_family == AF_INET) \
> + UDP_INC_STATS_BH(field, 0); \
> + else \
> + UDP6_INC_STATS_BH(field, 0); \
> + } while (0);
> +
Because there are lot of address families, not only AF_INET
and AF_INET6.
How about?
---
switch (sk->sk_family) {
case AF_INET:
...
case AF_INET6:
...
default:
}
--
WCN
^ permalink raw reply
* Re: [PATCH] don't allow netfilter --setmss to increase mss
From: Patrick McHardy @ 2007-12-05 8:37 UTC (permalink / raw)
To: Benjamin LaHaise; +Cc: David Miller, netdev, Netfilter Development Mailinglist
In-Reply-To: <47565F64.2000907@trash.net>
Patrick McHardy wrote:
> Benjamin LaHaise wrote:
>> Yes, I am aware of --clamp-mss-to-pmtu, but it doesn't work for
>> outgoing connections from clients (ie web traffic), as it only looks
>> at the PMTU on the destination route, not the source of the packet
>> (the DSL interfaces in question have a 1442 byte MTU while the
>> destination ethernet interface is 1500 -- there are problematic hosts
>> which use a 1300 byte MTU). Reworking that is probably a good idea at
>> some point, but it's more work than this is.
>
> Yes, this has always annoyed me too as it doesn't really work for me
> in a similar setup where traffic first goes through an IPsec tunnel,
> then through a MSS mangling gateway and then over a DSL line. I'll
> add a patch on top of yours to take the MSS in reverse direction
> into account.
This gets a bit too ugly because of rp-filters and actually might
not properly work in setups with routing by fwmark, so I'll leave
it as it is, its probably better to manually set the MSS in these
cases.
^ permalink raw reply
* Re: [PATCH] don't allow netfilter --setmss to increase mss
From: Patrick McHardy @ 2007-12-05 8:20 UTC (permalink / raw)
To: Benjamin LaHaise; +Cc: David Miller, netdev, Netfilter Development Mailinglist
In-Reply-To: <20071204224535.GY27007@kvack.org>
Please send netfilter-related patches to netfilter-devel.
Benjamin LaHaise wrote:
> When terminating DSL connections for an assortment of random customers, I've
> found it necessary to use iptables to clamp the MSS used for connections to
> work around the various ICMP blackholes in the greater net. Unfortunately,
> the current behaviour in Linux is imperfect and actually make things worse,
> so I'm proposing the following: increasing the MSS in a packet can never be
> a good thing, so make --set-mss only lower the MSS in a packet.
>
> Yes, I am aware of --clamp-mss-to-pmtu, but it doesn't work for outgoing
> connections from clients (ie web traffic), as it only looks at the PMTU on
> the destination route, not the source of the packet (the DSL interfaces in
> question have a 1442 byte MTU while the destination ethernet interface is
> 1500 -- there are problematic hosts which use a 1300 byte MTU). Reworking
> that is probably a good idea at some point, but it's more work than this is.
Yes, this has always annoyed me too as it doesn't really work for me
in a similar setup where traffic first goes through an IPsec tunnel,
then through a MSS mangling gateway and then over a DSL line. I'll
add a patch on top of yours to take the MSS in reverse direction
into account.
> Thoughts? Would it be better to add a new flag?
No, this is obviously a good idea, I actually thought we'd already
prevent increasing it in all cases. I've applied your patch, thanks.
^ permalink raw reply
* Re: [LARTC] Adding qdiscs crashes kernel??
From: Patrick McHardy @ 2007-12-05 8:07 UTC (permalink / raw)
To: Leigh Sharpe; +Cc: lartc, Linux Netdev List
In-Reply-To: <96CF49BD8B56384395D698BA99007FA32DB9@exchange.pacwire.local>
Please always report bugs to netdev@vger.kernel.org.
Leigh Sharpe wrote:
> Oh,
> kernel version 2.6.23, since I forgot to mention it.
>
> Leigh.
>
> ________________________________
>
> From: lartc-bounces@mailman.ds9a.nl
> [mailto:lartc-bounces@mailman.ds9a.nl] On Behalf Of Leigh Sharpe
> Sent: Wednesday, 5 December 2007 3:37 PM
> To: lartc@mailman.ds9a.nl
> Subject: [LARTC] Adding qdiscs crashes kernel??
>
>
> Hi all,
> I'm having some problems setting up qdiscs on a bridge.The config looks
> a little like this:
>
>
> ifconfig ifb0 up # Bring up the IFB for this bridge.
> tc qdisc add dev eth2 ingress
> tc qdisc add dev eth3 ingress
> tc qdisc add dev ifb0 root handle 1:0 cbq bandwidth 100Mbit avpkt 1000
> cell 8
> # Raw qdiscs on each bridge port
> tc qdisc add dev eth2 root handle 1:0 cbq bandwidth 100Mbit avpkt 1000
> cell 8
> tc qdisc add dev eth3 root handle 1:0 cbq bandwidth 100Mbit avpkt 1000
> cell 8
>
> tc filter add dev eth2 parent 1: protocol 0x8100 prio 5 u32 match u16
> 3000 0x0fff at 0 flowid 1:1 action ipt -j MARK --or-mark 0x01000000 #
> mark packets for VLAN 3000.
> tc filter add dev eth3 parent 1: protocol 0x8100 prio 5 u32 match u16
> 3000 0x0fff at 0 flowid 1:1 action ipt -j MARK --or-mark 0x01000000 #
> mark packets for VLAN 3000.
>
> tc class add dev eth2 parent 1:0 classid 1:1 cbq bandwidth 100Mbit rate
> 2000Kbit weight 200Kbit prio 1 allot 1514 cell 8 maxburst 20 avpkt 1000
> bounded isolated # 2000 Kbit rate limit on entry point.
> tc class add dev eth3 parent 1:0 classid 1:1 cbq bandwidth 100Mbit rate
> 2000Kbit weight 200Kbit prio 1 allot 1514 cell 8 maxburst 20 avpkt 1000
> bounded isolated # 2000 Kbit rate limit on entry point.
>
> tc qdisc add dev eth2 parent 1:1 handle 2: cbq bandwidth 100Mbit avpkt
> 1000 cell 8
> tc qdisc add dev eth3 parent 1:1 handle 2: cbq bandwidth 100Mbit avpkt
> 1000 cell 8
> tc class add dev eth2 parent 2:0 classid 2:1 cbq bandwidth 100Mbit rate
> 2000Kbit weight 200Kbit prio 2 allot 1514 cell 8 maxburst 20 avpkt 1000
> sharing
> tc filter add dev eth2 parent 2:0 protocol 0x8100 prio 2 u32 match u16
> 3000 0x0fff at 0 flowid 2:1 action ipt -j MARK --or-mark 0x00100000
> tc qdisc add dev eth2 parent 2:1 handle 3: cbq bandwidth 100Mbit avpkt
> 1000 cell 8
> tc filter add dev eth2 parent 3:0 protocol 0x8100 prio 4 u32 match u32 0
> 0 flowid 3:3 # Traffic
> class 3 - catchall. Don't MARK further.
>
> (There's lot's more, mostly a repeat of the above with different
> criteria.)
> When I first boot the box, and apply the traffic shaping before any
> traffic flows, all is fine. However, if I apply this same config whilst
> the bridge is passing lots of traffic, it completely crashes the box.
> Everything freezes, I don't even get a kernel panic message on the
> console. Nothing responds and the only way to recover is by a
> power-cycle.
>
> If I take the link down on the ethernet port (with ip link set ethx
> down), apply the configs, and then bring it back up again, all is OK.
> Obviously, though, this isn't really acceptable.
>
> It always crashes immediately after a 'tc qdisc add...' line, but not
> always in the same place. Are there any known issues with adding qdiscs
> whilst traffic is being queued on it?
> I've also tried it using HTB instead of CBQ, and I get the same results.
>
> Anybody got any other ideas as to what might be going on?
Which qdisc add crashes it? Please post the full oops.
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: Herbert Xu @ 2007-12-05 7:39 UTC (permalink / raw)
To: David Miller; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071204.233432.136250076.davem@davemloft.net>
On Tue, Dec 04, 2007 at 11:34:32PM -0800, David Miller wrote:
>
> TCP has some built-in assumptions about characteristics of
> interent links and what constitutes a timeout which is "too long"
> and should thus result in a full connection failure.
>
> IPSEC changes this because of IPSEC route resolution via
> ISAKMP.
>
> With this in mind I can definitely see people preferring
> the "block until IPSEC resolves" behavior, especially for
> something like, say, periodic remote backups and stuff like
> that where you really want the thing to just sit and wait
> for the connect() to succeed instead of failing.
Hmm, but connect(2) should succeed in that case thanks to the
blackhole route, no? The subsequent SYNs will then be dropped
until the IPsec SAs are in place.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: David Miller @ 2007-12-05 7:34 UTC (permalink / raw)
To: herbert; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071205071607.GA11852@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 5 Dec 2007 18:16:07 +1100
> Right. This is definitely bad for protocols without a retransmission
> mechanism.
>
> However, is the 0 setting ever useful for TCP and in particular, TCP's
> connect(2) call? Perhaps we can just make that one always drop.
TCP has some built-in assumptions about characteristics of
interent links and what constitutes a timeout which is "too long"
and should thus result in a full connection failure.
IPSEC changes this because of IPSEC route resolution via
ISAKMP.
With this in mind I can definitely see people preferring
the "block until IPSEC resolves" behavior, especially for
something like, say, periodic remote backups and stuff like
that where you really want the thing to just sit and wait
for the connect() to succeed instead of failing.
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: Herbert Xu @ 2007-12-05 7:16 UTC (permalink / raw)
To: David Miller; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071204.231200.117152338.davem@davemloft.net>
On Tue, Dec 04, 2007 at 11:12:00PM -0800, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 5 Dec 2007 17:51:32 +1100
>
> > Does anybody actually need the 0 setting? What would we break if
> > the default became 1?
>
> I bet there are UDP apps out there that would break if we
> didn't do this.
Right. This is definitely bad for protocols without a retransmission
mechanism.
However, is the 0 setting ever useful for TCP and in particular, TCP's
connect(2) call? Perhaps we can just make that one always drop.
Well, until someone implements queueing to fix all of this properly
that is :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: David Miller @ 2007-12-05 7:12 UTC (permalink / raw)
To: herbert; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071205065132.GA11476@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 5 Dec 2007 17:51:32 +1100
> Does anybody actually need the 0 setting? What would we break if
> the default became 1?
I bet there are UDP apps out there that would break if we
didn't do this.
Actually, consider even a case like DNS. Let's say the timeout
is set to 2 seconds or something and you have 3 DNS servers
listed, on different IPSEC destinations, in your resolv.conf
Each IPSEC route that isn't currently resolved will cause packet loss
of the DNS lookup request with xfrm_larval_drop set to '1'.
If all 3 need to be resolved, the DNS lookup will fully fail
which defeats the purpose of listing 3 servers for redundancy
don't you think? :-)
As much as I even personally prefer the xfrm_larval_drop=1
behavior, it cases like above that keep me from jumping at
making it the default.
Arguably, potentially blocking forever (which is what can easily
happen with xfrm_larval_drop=0 if your IPSEC daemon cannot resolve the
IPSEC path for whatever reason) is worse than the above, but the
other cases are still something to consider as well.
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: Herbert Xu @ 2007-12-05 6:51 UTC (permalink / raw)
To: David Miller; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071204.223023.262159049.davem@davemloft.net>
On Tue, Dec 04, 2007 at 10:30:23PM -0800, David Miller wrote:
>
> We made an explicit decision not to do things this way.
Thanks for pointing this out.
> Non-blocking has a meaning dependant upon the xfrm_larval_drop sysctl
> setting, and this is across the board. If xfrm_larval_drop is zero,
> non-blocking semantics do not extend to IPSEC route resolution,
> otherwise it does.
>
> If he sets this sysctl to "1" as I detailed in my reply, he'll
> get the behavior he wants.
Does anybody actually need the 0 setting? What would we break if
the default became 1?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 1/4] netns: Tag the network flow with the network namespace it is in (v2)
From: Denis V. Lunev @ 2007-12-05 6:49 UTC (permalink / raw)
To: Herbert Xu; +Cc: shemminger, den, davem, devel, netdev, containers
In-Reply-To: <E1IzgRN-0006Hq-00@gondolin.me.apana.org.au>
Herbert Xu wrote:
> Denis V. Lunev <den@sw.ru> wrote:
>> could you exactly point me out the flow cache your are talking about.
>> Is this dst entry cache or struct flow_cache described in the
>> net/core/flow.c
>
> The flow object is embedded in struct rtable so does its size change?
>
> Cheers,
SLAB allocator, x86_64 host
Before the patch:
ip6_dst_cache 384 10
xfrm_dst_cache 384 10
ip_dst_cache 384 10
After the patch:
ip6_dst_cache 384 10
xfrm_dst_cache 384 10
ip_dst_cache 384 10
Regards,
Den
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: David Miller @ 2007-12-05 6:30 UTC (permalink / raw)
To: herbert; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071205001230.GA11391@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 5 Dec 2007 11:12:32 +1100
> [INET]: Export non-blocking flags to proto connect call
>
> Previously we made connect(2) block on IPsec SA resolution. This is
> good in general but not desirable for non-blocking sockets.
>
> To fix this properly we'd need to implement the larval IPsec dst stuff
> that we talked about. For now let's just revert to the old behaviour
> on non-blocking sockets.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
We made an explicit decision not to do things this way.
Non-blocking has a meaning dependant upon the xfrm_larval_drop sysctl
setting, and this is across the board. If xfrm_larval_drop is zero,
non-blocking semantics do not extend to IPSEC route resolution,
otherwise it does.
If he sets this sysctl to "1" as I detailed in my reply, he'll
get the behavior he wants.
^ permalink raw reply
* Re: NET: ASSERT_RTNL in __dev_set_promiscuity makes debug warning
From: Jarek Poplawski @ 2007-12-05 6:25 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: Joonwoo Park, netdev, 'Linux Kernel Mailing List'
In-Reply-To: <4755D42E.7050602@gmail.com>
On 04-12-2007 23:26, Jarek Poplawski wrote:
...
> But, IMHO, blowing ASSERT_RTNL up in a few places shouldn't be much
> worse. After all, how long such a debugging code should be kept. It
> seems, at least sometimes we should be a bit more confident of how
> it's called.
I see this won't be done this way, but, if it were, then there is no
reason to remove the second: documenting feature of ASSERT_RTNL, so
some comment about locking should be added.
Jarek P.
^ permalink raw reply
* Re: namespace support requires network modules to say "GPL"
From: David Miller @ 2007-12-05 6:14 UTC (permalink / raw)
To: ebiederm
Cc: greearb, daniel.lezcano, netdev, linux-kernel, containers, lkml,
shemminger
In-Reply-To: <m1odd6w9p6.fsf@ebiederm.dsl.xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 04 Dec 2007 12:17:57 -0700
> Ben Greear <greearb@candelatech.com> writes:
>
> > If I *do* need to add some sort of namespace
> > awareness to just achieve today's functionality, I don't mind making the
> > changes,
> > so long as I don't need to change to GPL licensing. Perhaps at the least you
> > can export enough symbols w/out GPL tag to achieve backwards compat with .23
> > and previous kernels, or rework dev_get_by_* etc to not need GPL'd namespace
> > symbols and just return the device in the default namespace?
>
> IANAL but to me your code sounds like a derivative work of the linux
> kernel. Which implies that if you are distributing your module you
> need to change to GPL licensing. The _GPL tag on EXPORT_SYMBOL does
> not change those rules.
Eric, YANAL and you are also full of hot air. You are really
testing my patience on this issue.
You fail to ever describe on what factual basis you are making
these claims. And the reason is that you have ZERO factual basis
for your claims.
Here are the facts:
1) Never, ever, have the function for looking up network devices been
classified as GPL-only symbols.
They provide a device based upon a lookup key.
2) You in no way have changed what those functions do in any way
whatsoever. They still provide a reference to a network device
based upon a given lookup key.
The functions are still doing the same thing they always have.
Therefore, you have decided to uniliaterally change the licensing of
these functions based solely upon your opinion, and not because of
some real change you've made to the code in question.
You have no right to do this.
This is unreasonable, and you must fix this immediately.
And I do mean now, not after you've written several more excessively
long diatribes about how you feel in this matter.
Thank you.
^ permalink raw reply
* Re: [PATCH] Updates to nfsroot documentation (take 2)
From: Simon Horman @ 2007-12-05 6:13 UTC (permalink / raw)
To: Amos Waterland; +Cc: akpm, linux-kernel, Netdev
In-Reply-To: <20071205045932.GA7160@us.ibm.com>
[ CCed netdev ]
On Tue, Dec 04, 2007 at 11:59:32PM -0500, Amos Waterland wrote:
> The difference between ip=off and ip=::::::off has been a cause of much
> confusion. Document how each behaves, and do not contradict ourselves
> by saying that "off" is the default when in fact "any" is the default
> and is descibed as being so lower in the file.
This seems fine to me in so far as it is correct. And I do like the
removal of the duplicat documentation from ipconfig.c. However, as I
mentioned yesterday, it really seems to me that the code in ipconfig.c
should be updated so that ip=off and ip=::::::off do the same thing.
>
> Signed-off-by: Amos Waterland <apw@us.ibm.com>
>
> Documentation/nfsroot.txt | 9 ++++++---
> net/ipv4/ipconfig.c | 20 +-------------------
> 2 files changed, 7 insertions(+), 22 deletions(-)
>
> diff --git a/Documentation/nfsroot.txt b/Documentation/nfsroot.txt
> index 16a7cae..ac04a1d 100644
> --- a/Documentation/nfsroot.txt
> +++ b/Documentation/nfsroot.txt
> @@ -92,8 +92,11 @@ ip=<client-ip>:<server-ip>:<gw-ip>:<netmask>:<hostname>:<device>:<autoconf>
> autoconfiguration.
>
> The <autoconf> parameter can appear alone as the value to the `ip'
> - parameter (without all the ':' characters before) in which case auto-
> - configuration is used.
> + parameter (without all the ':' characters before). If the value is
> + "ip=off" or "ip=none", no autoconfiguration will take place, otherwise
> + autoconfiguration will take place. Note that "ip=off" is not the same
> + thing as "ip=::::::off", because in the latter autoconfiguration will take
> + place if any of DHCP, BOOTP or RARP are compiled in the kernel.
>
> <client-ip> IP address of the client.
>
> @@ -142,7 +145,7 @@ ip=<client-ip>:<server-ip>:<gw-ip>:<netmask>:<hostname>:<device>:<autoconf>
> into the kernel will be used, regardless of the value of
> this option.
>
> - off or none: don't use autoconfiguration (default)
> + off or none: don't use autoconfiguration
> on or any: use any protocol available in the kernel
> dhcp: use DHCP
> bootp: use BOOTP
> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index c5c107a..96400b0 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -1396,25 +1396,7 @@ late_initcall(ip_auto_config);
>
> /*
> * Decode any IP configuration options in the "ip=" or "nfsaddrs=" kernel
> - * command line parameter. It consists of option fields separated by colons in
> - * the following order:
> - *
> - * <client-ip>:<server-ip>:<gw-ip>:<netmask>:<host name>:<device>:<PROTO>
> - *
> - * Any of the fields can be empty which means to use a default value:
> - * <client-ip> - address given by BOOTP or RARP
> - * <server-ip> - address of host returning BOOTP or RARP packet
> - * <gw-ip> - none, or the address returned by BOOTP
> - * <netmask> - automatically determined from <client-ip>, or the
> - * one returned by BOOTP
> - * <host name> - <client-ip> in ASCII notation, or the name returned
> - * by BOOTP
> - * <device> - use all available devices
> - * <PROTO>:
> - * off|none - don't do autoconfig at all (DEFAULT)
> - * on|any - use any configured protocol
> - * dhcp|bootp|rarp - use only the specified protocol
> - * both - use both BOOTP and RARP (not DHCP)
> + * command line parameter. See Documentation/nfsroot.txt.
> */
> static int __init ic_proto_name(char *name)
> {
--
Horms
^ permalink raw reply
* Re: [PATCH 0/2] netem: trace enhancement
From: Ariane Keller @ 2007-12-05 6:12 UTC (permalink / raw)
To: Ben Greear
Cc: Ariane Keller, Patrick McHardy, Stephen Hemminger, netdev,
herbert, Rainer Baumann
In-Reply-To: <4755D2EB.4000807@candelatech.com>
> If you actually run out of the trace buffers, do you just continue to
> run with the last settings? If so, that would keep up throughput
> even if you are out of trace buffers...
Upon configuring the qdisc you can specify a default value, which is
taken when the buffers are empty. It is either drop the packet or just
forward it with no delay.
> What rates do you see, btw? (pps, bps).
My machine was an AMD Athlon 2083MHz, with a default installation of
Debian with Kernel 2.6.16 and HZ set to 1000.
Up to 80'000 pps (with "small" udp packets) everything (without netem,
with netem and with netem trace) worked fine (tested with up to 10ms delay).
For 90'000 pps the kernel dropped some packets even with no netem
running, some more with netem and allmost all with netem trace.
As soon as I have changed the mechanism for the data transfer to
rtnetlink I'll do some new tests, trying to reach a higher packet rate.
Then I'll see whether it is necessary to add more buffers, or whether
the system collapses before.
Thanks again!
Ariane
^ permalink raw reply
* Re: namespace support requires network modules to say "GPL"
From: David Miller @ 2007-12-05 6:07 UTC (permalink / raw)
To: greearb; +Cc: ebiederm, kaber, shemminger, lkml, linux-kernel, netdev,
containers
In-Reply-To: <4755A2FD.9020608@candelatech.com>
From: Ben Greear <greearb@candelatech.com>
Date: Tue, 04 Dec 2007 10:57:01 -0800
> echo add_my_bridge my_br1 eth0 namespaceX eth1 namespaceY >
> /proc/net/foo/config
Each process executes in a namespace, so specifying the namespace
is redundant, just fetch the current process's namespace to pass
into the dev_get_by_*() routines.
Anyone interested in using a different namespace's devices
can change the namespace the process is executing in before
the procfs echo.
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: David Miller @ 2007-12-05 6:06 UTC (permalink / raw)
To: simon; +Cc: linux-kernel, netdev
In-Reply-To: <4755A21F.2020407@simon.arlott.org.uk>
From: Simon Arlott <simon@fire.lp0.eu>
Date: Tue, 04 Dec 2007 18:53:19 +0000
> If I have a IPsec rule like:
> spdadd 192.168.7.8 1.2.3.4 any -P out ipsec esp/transport//require;
> (i.e. a remote host 1.2.3.4 which will not respond)
>
> Then any attempt to communicate with 1.2.3.4 will block, even when using non-blocking sockets:
If you don't like this behavior:
echo "1" >/proc/sys/net/core/xfrm_larval_drop
but those initial connection setup packets will be dropped while
waiting for the IPSEC route to be resolved, and in your 8 hour case
the TCP connect will fail.
Anyways, the choice for different behavior is there, select it
to suit your tastes.
^ permalink raw reply
* Re: namespace support requires network modules to say "GPL"
From: David Miller @ 2007-12-05 6:01 UTC (permalink / raw)
To: ebiederm
Cc: daniel.lezcano, greearb, netdev, linux-kernel, containers, lkml,
shemminger
In-Reply-To: <m163zexrqi.fsf@ebiederm.dsl.xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 04 Dec 2007 11:03:01 -0700
> I am confused. I don't see a path forward that feels right.
Eric, instead of writing a book about how you feel, look
at the simple facts and resolve this quickly.
You added a new key, the namespace, to the looking up of
network objects.
Big deal.
That does not imply a change in licensing for the interfaces
where you added that new aspect of the key to the argument
list.
This symbol licensing decision was not your's to make, so you must
revert the new licensing change you are enforcing upon everyone.
I want a de-GPL patch in my mailbox from you within the next 24 hours
or I will code one up myself. This is getting beyond rediculious.
My patience is completely gone on this matter, resolve this now.
Thanks.
^ 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