* Re: TCP SACK issue, hung connection, tcpdump included
From: Darryl L. Miles @ 2007-07-31 5:03 UTC (permalink / raw)
To: linux-kernel; +Cc: Netdev
In-Reply-To: <20070729160721.GA31276@1wt.eu>
I've been able to capture a tcpdump from both ends during the problem
and its my belief there is a bug in 2.6.20.1 (at the client side) in
that it issues a SACK option for an old sequence which the current
window being advertised is beyond it. This is the most concerning issue
as the integrity of the sequence numbers doesn't seem right (to my
limited understanding anyhow).
There is another concern of why the SERVER performed a retransmission in
the first place, when the tcpdump shows the ack covering it has been seen.
I have made available the full dumps at:
http://darrylmiles.org/snippets/lkml/20070731/
There are some changes in 2.6.22 that appear to affect TCP SACK handling
does this fix a known issue ?
This sequence is interesting from the client side:
03:58:56.419034 IP SERVER.ssh > CLIENT.43726: . 26016:27464(1448) ack
4239 win 2728 <nop,nop,timestamp 16345815 819458859> # S1
03:58:56.419100 IP CLIENT.43726 > SERVER.ssh: . ack 27464 win 501
<nop,nop,timestamp 819458884 16345815> # C1
03:58:56.422019 IP SERVER.ssh > CLIENT.43726: P 27464:28176(712) ack
4239 win 2728 <nop,nop,timestamp 16345815 819458859> # S2
03:58:56.422078 IP CLIENT.43726 > SERVER.ssh: . ack 28176 win 501
<nop,nop,timestamp 819458884 16345815> # C2
The above 4 packets look as expect to me. Then we suddenly see a
retransmission of 26016:27464.
03:58:56.731597 IP SERVER.ssh > CLIENT.43726: . 26016:27464(1448) ack
4239 win 2728 <nop,nop,timestamp 16346128 819458864> # S3
So the client instead of discarding the retransmission of duplicate
segment, issues a SACK.
03:58:56.731637 IP CLIENT.43726 > SERVER.ssh: . ack 28176 win 501
<nop,nop,timestamp 819458962 16345815,nop,nop,sack sack 1 {26016:27464}
> # C3
In response to this the server is confused ??? It responds to
sack{26016:27464} but the client is also saying "wnd 28176". Wouldn't
the server expect "wnd < 26016" to there is a segment to retransmit ?
03:58:57.322800 IP SERVER.ssh > CLIENT.43726: . 26016:27464(1448) ack
4239 win 2728 <nop,nop,timestamp 16346718 819458864> # S4
Now viewed from the server side:
03:58:56.365655 IP SERVER.ssh > CLIENT.43726: . 26016:27464(1448) ack
4239 win 2728 <nop,nop,timestamp 16345815 819458859> # S1
03:58:56.365662 IP SERVER.ssh > CLIENT.43726: P 27464:28176(712) ack
4239 win 2728 <nop,nop,timestamp 16345815 819458859> # S2
03:58:56.374633 IP CLIENT.43726 > SERVER.ssh: . ack 24144 win 488
<nop,nop,timestamp 819458861 16345731> # propagation delay
03:58:56.381630 IP CLIENT.43726 > SERVER.ssh: . ack 25592 win 501
<nop,nop,timestamp 819458863 16345734> # propagation delay
03:58:56.384503 IP CLIENT.43726 > SERVER.ssh: . ack 26016 win 501
<nop,nop,timestamp 819458864 16345734> # propagation delay
03:58:56.462583 IP CLIENT.43726 > SERVER.ssh: . ack 27464 win 501
<nop,nop,timestamp 819458884 16345815> # C1
03:58:56.465707 IP CLIENT.43726 > SERVER.ssh: . ack 28176 win 501
<nop,nop,timestamp 819458884 16345815> # C2
The above packets just as expected.
03:58:56.678546 IP SERVER.ssh > CLIENT.43726: . 26016:27464(1448) ack
4239 win 2728 <nop,nop,timestamp 16346128 819458864> # S3
I guess the above packet is indeed a retransmission of "# S1" but why
was it retransmitted, when we can clearly see "# C1" above acks this
segment ? It is not even as if the retransmission escaped before the
kernel had time to process the ack, as 200ms elapsed. CONCERN NUMBER TWO
03:58:56.774778 IP CLIENT.43726 > SERVER.ssh: . ack 28176 win 501
<nop,nop,timestamp 819458962 16345815,nop,nop,sack sack 1 {26016:27464}
> # C3
CONCERN NUMBER ONE, why in response to that escaped retransmission was a
SACK the appropriate response ? When at the time the client sent the
SACK it had received all data upto 28176, a fact it continues to
advertise in the "# C3" packet above.
There is nothing wrong is the CLIENT expecting to see a retransmission
of that segment at this point in time that is an expected circumstance.
03:58:57.269529 IP SERVER.ssh > CLIENT.43726: . 26016:27464(1448) ack
4239 win 2728 <nop,nop,timestamp 16346718 819458864> # S4
Darryl
^ permalink raw reply
* Re: RFC: on [ab]use of skb->cb by VLAN code
From: David Miller @ 2007-07-31 5:18 UTC (permalink / raw)
To: greearb; +Cc: hadi, kaber, netdev, mcarlson
In-Reply-To: <46AEBADB.7030007@candelatech.com>
From: Ben Greear <greearb@candelatech.com>
Date: Mon, 30 Jul 2007 21:30:19 -0700
> So, shall we add a new field to the skb in order to get the info out of cb?
>
> Looks like a single 32-bit field would be sufficient.
I'm trying to brainstorm how to avoid consuming new space in
sk_buff and I'd like others to do so as well.
This isn't super urgent at all so I'll just think about it
on the backburner, things like the NAPI changes and dealing
with all of today's networking bug fixes is my top priority
at the moment.
^ permalink raw reply
* Re: [PATCH net-2.6 1/2] [TCP]: Fix ratehalving with bidirectional flows
From: David Miller @ 2007-07-31 5:21 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: netdev
In-Reply-To: <Pine.LNX.4.64.0707310751260.15079@kivilampi-30.cs.helsinki.fi>
From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Tue, 31 Jul 2007 07:59:10 +0300 (EEST)
> I think it's probably good to add tp->snd_una != prior_snd_una
> check there too... It's not going to make a large difference,
> mostly just to be conservative when skb collapse stuff got done
> (and maybe to annoy cheaters too though I couldn't at this point
> figure out how they could abuse it)...
>
> ...I think I can come up with that on Wednesday, so please hold
> stable push until that.
It'll definitely need to wait at least a day as I cannot even make TCP
or UDP connections out from my machine with the current net-2.6 tree,
and this is what I'm debugging at the moment.
I'm hoping that it's just something stupid like the make not
rebuilding everything necessary after I changed the __u16's into
__u32's in skb_frag_t.
^ permalink raw reply
* Re: [PATCH net-2.6 2/2] [TCP]: Bidir flow must not disregard SACK blocks for lost marking
From: Ilpo Järvinen @ 2007-07-31 5:33 UTC (permalink / raw)
To: David Miller; +Cc: Netdev
In-Reply-To: <20070730.195346.124870288.davem@davemloft.net>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1569 bytes --]
On Mon, 30 Jul 2007, David Miller wrote:
> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Mon, 30 Jul 2007 20:19:40 +0300 (EEST)
>
> > I'm not completely pleased with this solution because readability
> > of the code is somewhat questionable as 'is_dupack' in SACK case
> > is no longer about dupacks only but would mean something like
> > 'lost_marker_work_todo' too... But because of Eifel stuff done
> > in CA_Recovery, the FLAG_DATA_SACKED check cannot be placed to
> > the if statement which seems attractive solution. Nevertheless,
> > I didn't like adding another variable just for that either... :-)
>
> I don't mind the complex conditionals so much in loss
> handling, they are almost inevitable. However I believe
> they could be simplified as a lot of pieces of code ask
> similar if not identical questions.
>
> We could ask several of these things up-front, regardless
> of path we will take (reno, DSACK, reorder, FRTO, etc.)
> and pass the answers along in a bitmask. We do that to
> some extent already with how we analyze the retransmit
> queue at the beginning of ACK processing.
That's true, ...and my thought too as I was thinking of
adding FLAG_SND_UNA_ADVANCED (!= FLAG_DATA_ACKED) (or it's
opposite as I haven't yet checked which way it is more
useful). That could help in many places and probably reduces
compare pressure here and there as the bitmask operation
can be done in one compare where it now has to do two
compares, one for snd_una and other for the flag.
...I'll keep an eye on other possible bits too... :-)
--
i.
^ permalink raw reply
* Re: RFC: on [ab]use of skb->cb by VLAN code
From: Ben Greear @ 2007-07-31 5:33 UTC (permalink / raw)
To: David Miller; +Cc: hadi, kaber, netdev, mcarlson
In-Reply-To: <20070730.221847.74747100.davem@davemloft.net>
David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Mon, 30 Jul 2007 21:30:19 -0700
>
>
>> So, shall we add a new field to the skb in order to get the info out of cb?
>>
>> Looks like a single 32-bit field would be sufficient.
>>
>
> I'm trying to brainstorm how to avoid consuming new space in
> sk_buff and I'd like others to do so as well.
>
> This isn't super urgent at all so I'll just think about it
> on the backburner, things like the NAPI changes and dealing
> with all of today's networking bug fixes is my top priority
> at the moment.
>
Ok. From a quick look through skbuff.h, here is an idea:
Do we really need an 'unsigned int' for mac_len? Maybe we could use
a 16-bit counter here, and then use the other 16 bits for the VLAN bits?
(I now think that only 16 bits are needed for VLAN, because it looks
like the
NICs must know how to grab the vlan encapsulated protocol out of the
skb?? Or,
maybe I just got confused when reading the vlan hwaccel logic...)
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH] possible deadlock in tulip driver
From: Denis V. Lunev @ 2007-07-31 6:52 UTC (permalink / raw)
To: Valerie Henson; +Cc: Denis V. Lunev, netdev, davem, tulip-users, devel
In-Reply-To: <20070730191232.GC16182@rainbow>
Manual code check. The similar fixes are present in almost all drivers,
f.e. tg3 one. I have an unrelated deadlock with rtnl.
Regards,
Den
Valerie Henson wrote:
> (No longer maintainer, btw.)
>
> What situation have you tested this under? Thanks,
>
> -VAL
>
> On Tue, Jul 24, 2007 at 11:49:08AM +0400, Denis V. Lunev wrote:
>> Calling flush_scheduled_work() may deadlock if called under rtnl_lock
>> (from dev->stop) as linkwatch_event() may be on the workqueue and it will try
>> to get the rtnl_lock
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>>
>> tulip_core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> --- ./drivers/net/tulip/tulip_core.c.tulip 2007-07-16 12:54:29.000000000 +0400
>> +++ ./drivers/net/tulip/tulip_core.c 2007-07-23 19:06:24.000000000 +0400
>> @@ -726,8 +726,6 @@ static void tulip_down (struct net_devic
>> void __iomem *ioaddr = tp->base_addr;
>> unsigned long flags;
>>
>> - flush_scheduled_work();
>> -
>> del_timer_sync (&tp->timer);
>> #ifdef CONFIG_TULIP_NAPI
>> del_timer_sync (&tp->oom_timer);
>> @@ -1788,6 +1786,8 @@ static void __devexit tulip_remove_one (
>> if (!dev)
>> return;
>>
>> + flush_scheduled_work();
>> +
>> tp = netdev_priv(dev);
>> unregister_netdev(dev);
>> pci_free_consistent (pdev,
>> -
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 2/7] Preparatory refactoring part 2.
From: Corey Hickey @ 2007-07-31 7:43 UTC (permalink / raw)
To: Patrick McHardy, Linux Netdev List
In-Reply-To: <46ADEEC5.9030600@trash.net>
Patrick McHardy wrote:
> Corey Hickey wrote:
>> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
>> index 8ae077f..0c46938 100644
>> --- a/net/sched/sch_sfq.c
>> +++ b/net/sched/sch_sfq.c
>> @@ -380,71 +380,71 @@ static void sfq_perturbation(unsigned long arg)
>> }
>> }
>>
>> -static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
>> +static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>> {
>> - struct sfq_sched_data *q = qdisc_priv(sch);
>> struct tc_sfq_qopt *ctl = RTA_DATA(opt);
>> - unsigned int qlen;
>> + int i;
>>
>> - if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
>> + if (opt && opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
>
>
> opt is dereferenced above (RTA_DATA), so if it is NULL we've already
> crashed.
I think that test made ESFQ not crash when I did a "qdisc change"
without giving any parameters, but that was a while ago and I might be
mistaken. I'll need to rewrite much of this function anyway, and I'll
pay attention to what happens when I get there.
>> return -EINVAL;
>>
>> - sch_tree_lock(sch);
>> - q->quantum = ctl->quantum ? : psched_mtu(sch->dev);
>> - q->perturb_period = ctl->perturb_period*HZ;
>> - if (ctl->limit)
>> - q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
>> + q->perturbation = 0;
>> + q->max_depth = 0;
>> + q->tail = q->limit = SFQ_DEPTH;
>> + if (opt == NULL) {
>> + q->perturb_period = 0;
>> + } else {
>> + struct tc_sfq_qopt *ctl = RTA_DATA(opt);
>> + if (ctl->quantum)
>> + q->quantum = ctl->quantum;
>> + q->perturb_period = ctl->perturb_period*HZ;
>>
>> - qlen = sch->q.qlen;
>> - while (sch->q.qlen >= q->limit-1)
>> - sfq_drop(sch);
>> - qdisc_tree_decrease_qlen(sch, qlen - sch->q.qlen);
>
>
> I hope that patch that makes changing possible brings this back ..
> <checking> .. it doesn't. Please either keep this or fix up 6/7
> to bring it back.
It got lost in translation; I will add it to 6/7.
Thanks,
Corey
^ permalink raw reply
* Re: [PATCH 6/7] Make qdisc changeable.
From: Corey Hickey @ 2007-07-31 7:43 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev
In-Reply-To: <46ADF18A.4060008@trash.net>
Patrick McHardy wrote:
> Corey Hickey wrote:
>> Re-implement sfq_change() and enable Qdisc_opts.change so "tc qdisc
>> change" will work.
>>
>> Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
>> ---
>> net/sched/sch_sfq.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 50 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
>> index e6a6a21..e042cd0 100644
>> --- a/net/sched/sch_sfq.c
>> +++ b/net/sched/sch_sfq.c
>> @@ -485,6 +485,55 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
>> return 0;
>> }
>>
>> +static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
>> +{
>> + struct sfq_sched_data *q = qdisc_priv(sch);
>> + struct sfq_sched_data tmp;
>> + struct sk_buff *skb;
>> + int err;
>> +
>> + /* set up tmp queue */
>> + memset(&tmp, 0, sizeof(struct sfq_sched_data));
>> + tmp.quantum = psched_mtu(sch->dev); /* default */
>
>
> If no value is given it should use the old value instead of
> reinitializing to the default.
>
>> + if ((err = sfq_q_init(&tmp, opt)))
>> + return err;
>
>
> This will also use defaults for all unspecified values. It would
> be more consistent with other qdiscs to only change those values
> that are actually specified, so something like "tc qdisc change ...
> perturb 10" will *only* change the perturbation parameter.
I somehow had it in my head that a qdisc change was supposed to work
that way. I'll change my patch.
> I'm not sure reusing the initialization function and copying the
> parameters is the best way to do this.
I'm not sure either, but I do like that it's conceptually simple and it
keeps the parameter handling in one place. I've thought and stared for a
while, and I don't see a better way, but that could of course be due to
my limited understanding and general newbiehood in that regard.
Thanks again for the review. I'll try to get a new batch of patches sent
off tomorrow.
-Corey
^ permalink raw reply
* Re: NETPOLL=y , NETDEVICES=n compile error ( Re: 2.6.23-rc1-mm1 )
From: Jarek Poplawski @ 2007-07-31 8:32 UTC (permalink / raw)
To: Gabriel C; +Cc: Andrew Morton, linux-kernel, netdev, jason.wessel, amitkale
In-Reply-To: <46AB8DF9.8060903@googlemail.com>
On 28-07-2007 20:42, Gabriel C wrote:
> Andrew Morton wrote:
>> On Sat, 28 Jul 2007 17:44:45 +0200 Gabriel C <nix.or.die@googlemail.com> wrote:
>>
>>> Hi,
>>>
>>> I got this compile error with a randconfig ( http://194.231.229.228/MM/randconfig-auto-82.broken.netpoll.c ).
>>>
>>> ...
>>>
>>> net/core/netpoll.c: In function 'netpoll_poll':
>>> net/core/netpoll.c:155: error: 'struct net_device' has no member named 'poll_controller'
>>> net/core/netpoll.c:159: error: 'struct net_device' has no member named 'poll_controller'
>>> net/core/netpoll.c: In function 'netpoll_setup':
>>> net/core/netpoll.c:670: error: 'struct net_device' has no member named 'poll_controller'
>>> make[2]: *** [net/core/netpoll.o] Error 1
>>> make[1]: *** [net/core] Error 2
>>> make: *** [net] Error 2
>>> make: *** Waiting for unfinished jobs....
>>>
>>> ...
>>>
>>>
>>> I think is because KGDBOE selects just NETPOLL.
>>>
>> Looks like it.
>>
>> Select went and selected NETPOLL and NETPOLL_TRAP but things like
>> CONFIG_NETDEVICES and CONFIG_NET_POLL_CONTROLLER remain unset. `select'
>> remains evil.
...
> I think there may be a logical issue ( again if I got it right ).
> We need some ethernet card to work with kgdboe right ? but we don't have any if !NETDEVICES && !NET_ETHERNET.
>
> So maybe some ' depends on ... && NETDEVICES!=n && NET_ETHERNET!=n ' is needed too ?
IMHO, the only logical issue here is netpoll.c mustn't use
CONFIG_NET_POLL_CONTROLLER code without #ifdef if it doesn't
add this dependency itself.
Cheers,
Jarek P.
^ permalink raw reply
* Re: [RESEND][PATCH 1/3] PPPoE: improved hashing routine
From: David Miller @ 2007-07-31 8:26 UTC (permalink / raw)
To: florz; +Cc: mostrows, netdev
In-Reply-To: <20070731080719.GA12071@florz.florz.dyndns.org>
From: Florian Zumbiehl <florz@florz.de>
Date: Tue, 31 Jul 2007 10:07:19 +0200
> Erm, I'd say this not only produces different results than the old
> version, but it also produces "wrong" results, in that it ignores quite
> a bit of the data that's supposed to be hashed. If I didn't overlook
> something, it only considers addr&0x0f0f0f0f0f00 and sid&0x0f0f, given
> the right endianness of addr and that PPPOE_HASH_SIZE stays 16.
You're right, I need to fix the shifts up. How does this version
look?
hash ^= (hash >> 4) ^ (hash >> 12) ^ (hash >> 20);
return (head ^ (hash >> 8) ^ (hash >> 24)) &
(PPPOE_HASH_SIZE - 1);
Actually it might be simpler and more efficient to just make
PPPOE_HASH_SHIFT be 8.
^ permalink raw reply
* Re: zd1211rw and mac80211: multicast/v6 doesn't work in 2.6.21.5
From: Pekka Savola @ 2007-07-31 8:27 UTC (permalink / raw)
To: netdev
In-Reply-To: <Pine.LNX.4.64.0706280814090.21541@netcore.fi>
FWIW,
multicast/v6 is still broken on zd1211rw on 2.6.22.1 based Fedora 7
kernel (2.6.22.1-33.fc7).
On Thu, 28 Jun 2007, Pekka Savola wrote:
> On Fedora 7 (kernel 2.6.21-1.3228.fc7, based on 2.6.21.5), my
> zd1211rw_mac80211 WLAN USB stick and multicast/v6 no longer works. On Fedora
> 6 (kernel 2.6.20, no mac80211) it was OK.
>
> I get "wlan0: duplicate address detected!" on dmesg when the kernel is trying
> to autoconfigure a global address.
>
> I suppose this is caused by the IPv6 stack seeing my own ICMPv6 neighbor
> solicitation and thinking it was originated by someone else:
>
> 08:46:21.762807 00:02:72:5b:dc:28 > 33:33:ff:5b:dc:28, ethertype IPv6
> (0x86dd), length 78: (hlim 255, next-header: ICMPv6 (58), length: 24) :: >
> ff02::1:ff5b:dc28: [icmp6 sum ok] ICMP6, neighbor solicitation, length 24,
> who has 2001:708:10:90:202:72ff:fe5b:dc28
>
> Strangely, the same error isn't printed with the link-local address which
> uses the same EUI-64.
>
> z1211rw Wiki seems to imply that this might be an issue in generic mac80211
> code:
>
> http://www.linuxwireless.org/en/users/Drivers/zd1211rw/mac80211Issues
>
> It'd be nice to get this fixed. Or has it already been?
>
>
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
^ permalink raw reply
* [IPSEC]: Ensure that state inner family is set
From: Herbert Xu @ 2007-07-31 8:53 UTC (permalink / raw)
To: David S. Miller, netdev
Hi Dave:
[IPSEC]: Ensure that state inner family is set
Similar to the issue we had with template families which
specified the inner families of policies, we need to set
the inner families of states as the main xfrm user Openswan
leaves it as zero.
af_key is unaffected because the inner family is set by it
and not the KM.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
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
--
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index c06883b..61339e1 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -322,6 +322,13 @@ static void copy_from_user_state(struct xfrm_state *x, struct xfrm_usersa_info *
x->props.family = p->family;
memcpy(&x->props.saddr, &p->saddr, sizeof(x->props.saddr));
x->props.flags = p->flags;
+
+ /*
+ * Set inner address family if the KM left it as zero.
+ * See comment in validate_tmpl.
+ */
+ if (!x->sel.family)
+ x->sel.family = p->family;
}
/*
^ permalink raw reply related
* Re: [RESEND][PATCH 1/3] PPPoE: improved hashing routine
From: Florian Zumbiehl @ 2007-07-31 8:07 UTC (permalink / raw)
To: David Miller; +Cc: mostrows, netdev
In-Reply-To: <20070730.174721.17862949.davem@davemloft.net>
Hi,
> > -static int hash_item(unsigned long sid, unsigned char *addr)
> > +#if 8%PPPOE_HASH_BITS
> > +#error 8 must be a multiple of PPPOE_HASH_BITS
> > +#endif
>
> Since PPPOE_HASH_BITS is "4" I would think this check will break the
> build. :-)
Erm, I thought that 8 was 4*2, but maybe I didn't quite understand that
natural numbers business ;-)
> Anyways, I looked at this myself and half of the problem comes from
> the fact that "sid" is passed around as an "unsigned long" throughout
> this entire file yet the thing is just a "__u16".
>
> So the first thing to fix is to use __u16 consistently for sid values.
> Then the sid hash looks obviously wrong and is easy to fix.
>
> Then you end up with a hash_item() that simply looks like:
>
> static unsigned int hash_item(__u16 sid, unsigned char *addr)
> {
> unsigned int hash;
>
> hash = (((unsigned int)addr[0] << 24) |
> ((unsigned int)addr[1] << 16) |
> ((unsigned int)addr[2] << 8) |
> ((unsigned int)addr[3] << 0));
>
> hash ^= (((unsigned int)addr[4] << 8) |
> ((unsigned int)addr[5] << 0));
>
> hash ^= sid;
>
> return ((hash ^ (hash >> 8) ^ (hash >> 16)) &
> (PPPOE_HASH_SIZE - 1));
> }
>
> which is what I've checked into my tree.
Erm, I'd say this not only produces different results than the old
version, but it also produces "wrong" results, in that it ignores quite
a bit of the data that's supposed to be hashed. If I didn't overlook
something, it only considers addr&0x0f0f0f0f0f00 and sid&0x0f0f, given
the right endianness of addr and that PPPOE_HASH_SIZE stays 16.
Florian
^ permalink raw reply
* Re: [RESEND][PATCH 1/3] PPPoE: improved hashing routine
From: Florian Zumbiehl @ 2007-07-31 9:01 UTC (permalink / raw)
To: David Miller; +Cc: mostrows, netdev
In-Reply-To: <20070731.012609.41193630.davem@davemloft.net>
Hi,
> > Erm, I'd say this not only produces different results than the old
> > version, but it also produces "wrong" results, in that it ignores quite
> > a bit of the data that's supposed to be hashed. If I didn't overlook
> > something, it only considers addr&0x0f0f0f0f0f00 and sid&0x0f0f, given
> > the right endianness of addr and that PPPOE_HASH_SIZE stays 16.
>
> You're right, I need to fix the shifts up. How does this version
> look?
>
> hash ^= (hash >> 4) ^ (hash >> 12) ^ (hash >> 20);
> return (head ^ (hash >> 8) ^ (hash >> 24)) &
> (PPPOE_HASH_SIZE - 1);
Assuming that it was supposed to read s/head/hash/: Same disclaimers
apply, but I'd say this considers only addr&0xff0fff0f000f and
sid&0x0fff, so, well, yes, it's better, but still not quite what I
think it should be ;-)
> Actually it might be simpler and more efficient to just make
> PPPOE_HASH_SHIFT be 8.
SHIFT? SIZE? BITS?
Florian
^ permalink raw reply
* Re: [IPSEC]: Ensure that state inner family is set
From: David Miller @ 2007-07-31 9:04 UTC (permalink / raw)
To: herbert; +Cc: netdev
In-Reply-To: <20070731085356.GA22900@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 31 Jul 2007 16:53:56 +0800
> Hi Dave:
>
> [IPSEC]: Ensure that state inner family is set
>
> Similar to the issue we had with template families which
> specified the inner families of policies, we need to set
> the inner families of states as the main xfrm user Openswan
> leaves it as zero.
>
> af_key is unaffected because the inner family is set by it
> and not the KM.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied, thanks Herbert.
^ permalink raw reply
* Re: [RESEND][PATCH 1/3] PPPoE: improved hashing routine
From: David Miller @ 2007-07-31 9:16 UTC (permalink / raw)
To: florz; +Cc: mostrows, netdev
In-Reply-To: <20070731090159.GB12071@florz.florz.dyndns.org>
From: Florian Zumbiehl <florz@florz.de>
Date: Tue, 31 Jul 2007 11:01:59 +0200
> Assuming that it was supposed to read s/head/hash/: Same disclaimers
> apply, but I'd say this considers only addr&0xff0fff0f000f and
> sid&0x0fff, so, well, yes, it's better, but still not quite what I
> think it should be ;-)
Indeed it's still bogus.
> > Actually it might be simpler and more efficient to just make
> > PPPOE_HASH_SHIFT be 8.
>
> SHIFT? SIZE? BITS?
You know what I meant :-)
PPPOE_HASH_BITS.
I guess otherwise we degenerate back to your original patch :)
^ permalink raw reply
* Re: [PATCH net-2.6 1/2] [TCP]: Fix ratehalving with bidirectional flows
From: Stephen Hemminger @ 2007-07-31 9:51 UTC (permalink / raw)
To: David Miller; +Cc: ilpo.jarvinen, netdev
In-Reply-To: <20070730.222112.41634340.davem@davemloft.net>
On Mon, 30 Jul 2007 22:21:12 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Tue, 31 Jul 2007 07:59:10 +0300 (EEST)
>
> > I think it's probably good to add tp->snd_una != prior_snd_una
> > check there too... It's not going to make a large difference,
> > mostly just to be conservative when skb collapse stuff got done
> > (and maybe to annoy cheaters too though I couldn't at this point
> > figure out how they could abuse it)...
> >
> > ...I think I can come up with that on Wednesday, so please hold
> > stable push until that.
>
> It'll definitely need to wait at least a day as I cannot even make TCP
> or UDP connections out from my machine with the current net-2.6 tree,
> and this is what I'm debugging at the moment.
>
> I'm hoping that it's just something stupid like the make not
> rebuilding everything necessary after I changed the __u16's into
> __u32's in skb_frag_t.
I running tests over emulator this morning.
Hopefully, this will fix the window collapse that occurs on startup
of large queue sizes.
^ permalink raw reply
* Re: [PATCH net-2.6 1/2] [TCP]: Fix ratehalving with bidirectional flows
From: David Miller @ 2007-07-31 9:58 UTC (permalink / raw)
To: shemminger; +Cc: ilpo.jarvinen, netdev
In-Reply-To: <20070731105113.4730d9bb@oldman.hamilton.local>
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Tue, 31 Jul 2007 10:51:13 +0100
> On Mon, 30 Jul 2007 22:21:12 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
>
> > From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> > Date: Tue, 31 Jul 2007 07:59:10 +0300 (EEST)
> >
> > > I think it's probably good to add tp->snd_una != prior_snd_una
> > > check there too... It's not going to make a large difference,
> > > mostly just to be conservative when skb collapse stuff got done
> > > (and maybe to annoy cheaters too though I couldn't at this point
> > > figure out how they could abuse it)...
> > >
> > > ...I think I can come up with that on Wednesday, so please hold
> > > stable push until that.
> >
> > It'll definitely need to wait at least a day as I cannot even make TCP
> > or UDP connections out from my machine with the current net-2.6 tree,
> > and this is what I'm debugging at the moment.
> >
> > I'm hoping that it's just something stupid like the make not
> > rebuilding everything necessary after I changed the __u16's into
> > __u32's in skb_frag_t.
>
> I running tests over emulator this morning.
> Hopefully, this will fix the window collapse that occurs on startup
> of large queue sizes.
Great.
For the record the bug I was chasing was an IPSEC issue which
Herbert fixed a few moments ago.
^ permalink raw reply
* Scatter gather and TCP MD5
From: Siddharth Taneja @ 2007-07-31 10:17 UTC (permalink / raw)
To: netdev
Hello,
I am using a vanilla 2.6.22.1 kernel and I see the same kind of
problem as had been mentioned some time back on this list
http://lkml.org/lkml/2007/5/22/45
The issue is essentially that with the MD5 option enabled for the
specific TCP connection, the SYN and SYN-ACKS are passed ok and the
connection establishes fine, but the other end (a cisco router)
complains about incorrect MD5 signatures on any other message that is
sent after this.
Setting the scatter-gather offloading option to off on the NIC seems to
correct this problem. Recently I had seen a checkin (as a response to
the problem mentioned in the above link) where the TSO option was
turned off to make MD5 work (my kernel has that fix). Is a similar
solution needed here too?
This is the information about my system:
> uname -a
Linux stdalone 2.6.22.1 #1 SMP Mon Jul 23 20:15:21 PDT 2007 i686 i686
i386 GNU/Linux
> ethtool -i eth0
driver: e1000
version: 7.3.20-k2
firmware-version: N/A
bus-info: 0000:01:0a.0
> ethtool -k eth0
Offload parameters for eth0:
rx-checksumming: on
tx-checksumming: on
scatter-gather: off
tcp segmentation offload: off
Thanks for your help.
Siddharth
PS: I would like to be CC'ed on the reply to this email. Thanks.
^ permalink raw reply
* Re: NETPOLL=y , NETDEVICES=n compile error ( Re: 2.6.23-rc1-mm1 )
From: Gabriel C @ 2007-07-31 10:14 UTC (permalink / raw)
To: Jarek Poplawski
Cc: Andrew Morton, linux-kernel, netdev, jason.wessel, amitkale
In-Reply-To: <20070731083210.GA1797@ff.dom.local>
Jarek Poplawski wrote:
> On 28-07-2007 20:42, Gabriel C wrote:
>> Andrew Morton wrote:
>>> On Sat, 28 Jul 2007 17:44:45 +0200 Gabriel C <nix.or.die@googlemail.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> I got this compile error with a randconfig ( http://194.231.229.228/MM/randconfig-auto-82.broken.netpoll.c ).
>>>>
>>>> ...
>>>>
>>>> net/core/netpoll.c: In function 'netpoll_poll':
>>>> net/core/netpoll.c:155: error: 'struct net_device' has no member named 'poll_controller'
>>>> net/core/netpoll.c:159: error: 'struct net_device' has no member named 'poll_controller'
>>>> net/core/netpoll.c: In function 'netpoll_setup':
>>>> net/core/netpoll.c:670: error: 'struct net_device' has no member named 'poll_controller'
>>>> make[2]: *** [net/core/netpoll.o] Error 1
>>>> make[1]: *** [net/core] Error 2
>>>> make: *** [net] Error 2
>>>> make: *** Waiting for unfinished jobs....
>>>>
>>>> ...
>>>>
>>>>
>>>> I think is because KGDBOE selects just NETPOLL.
>>>>
>>> Looks like it.
>>>
>>> Select went and selected NETPOLL and NETPOLL_TRAP but things like
>>> CONFIG_NETDEVICES and CONFIG_NET_POLL_CONTROLLER remain unset. `select'
>>> remains evil.
> ...
>> I think there may be a logical issue ( again if I got it right ).
>> We need some ethernet card to work with kgdboe right ? but we don't have any if !NETDEVICES && !NET_ETHERNET.
>>
>> So maybe some ' depends on ... && NETDEVICES!=n && NET_ETHERNET!=n ' is needed too ?
>
> IMHO, the only logical issue here is netpoll.c mustn't use
> CONFIG_NET_POLL_CONTROLLER code without #ifdef if it doesn't
> add this dependency itself.
>
Well it does if NETDEVICES && if NET_ETHERNET which booth are N when !NETDEVICES is why KGDBOE uses select and not depends on.
Now KGDBOE just selects NETPOLL and NETPOLL_TRAP.
Adding 'select CONFIG_NET_POLL_CONTROLLER' let kgdboe compiles but the question is does it work without any ethernet card ?
> Cheers,
> Jarek P.
>
Gabriel
^ permalink raw reply
* Re: [PATCH]: Fix sk_buff page offsets and lengths.
From: Evgeniy Polyakov @ 2007-07-31 10:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev, sfr, shemminger
In-Reply-To: <20070730.185028.26532012.davem@davemloft.net>
On Mon, Jul 30, 2007 at 06:50:28PM -0700, David Miller (davem@davemloft.net) wrote:
>
> Stephen Rothwell pointed out to me that the skb_frag_struct
> is broken on platforms using 64K or larger page sizes, it
> even generates warnings when (for example) the myri10ge driver
> tries to assign PAGE_SIZE into frag->size.
>
> I've thus increased page offset and size to __u32 in the patch below.
Maybe wrap it into
#if PAGE_OFFSET > 12
#endif
or something like that?
I'm not sure actually why drivers would want to have list of 64k pages,
instead driver could call give_me_pages(size) instead of alloc_pages
and per-arch allocator would return one page or set of pages. This is a
handwaving for now...
--
Evgeniy Polyakov
^ permalink raw reply
* Re: [PATCH net-2.6.22-rc7] xfrm beet interfamily support
From: Joakim Koskela @ 2007-07-31 10:39 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev, David Miller
In-Reply-To: <469F7952.207@trash.net>
On Thursday 19 July 2007 17:46:42 Patrick McHardy wrote:
> Joakim Koskela wrote:
> > + skb_push(skb, hdrlen);
> > + iphv6 = ipv6_hdr(skb);
> > +
> > + skb_reset_network_header(skb);
> > + top_iphv6 = ipv6_hdr(skb);
> > +
> > + protocol = iphv6->nexthdr;
> > + skb_pull(skb, delta);
> > + skb_reset_network_header(skb);
> > + top_iphv4 = ip_hdr(skb);
> > + skb_set_transport_header(skb, hdrlen);
> > + top_iphv4->ihl = (sizeof(struct iphdr) >> 2);
> > + top_iphv4->version = 4;
> > + top_iphv4->id = 0;
> > + top_iphv4->frag_off = htons(IP_DF);
> > + top_iphv4->ttl = dst_metric(dst->child, RTAX_HOPLIMIT);
> > + top_iphv4->saddr = x->props.saddr.a4;
> > + top_iphv4->daddr = x->id.daddr.a4;
> > + skb->transport_header += top_iphv4->ihl*4;
> > + top_iphv4->protocol = protocol;
> > +
> > + skb->protocol = htons(ETH_P_IP);
> > +#endif
>
> The output function in the IPv6/IPv4 case is called from
> xfrm6_output_one, which loops until after a tunnel mode
> encapsulation is done and then returns to the outer loop
> in xfrm6_output_finish2, which passes the packet through
> the netfilter hooks and continues with the next transform.
>
I'm not sure I really got this. IPv6/IPv4 means IPv6 inner, IPv4 outer, right?
Isn't that called from xfrm4_output_one and subsequently passed through the
right filters as well (as it has a ipv4 header by then)?
> There are multiple problems resulting from the inter-family
> encapsulation. First of all, the inner loop continues after
> beet mode encapsulation, skipping the netfilter hooks in
> case there are more transforms. It should (as with decaps = 1
> on input) at least call netfilter hooks after an inter-family
> transform. If the beet transform is the last one, the IPv4
> skb will be passed through the IPv6 netfilter hooks, which is
> clearly wrong. To fix these problems some restructuring in
> xfrm[46]_output.c seems to be necessary.
Couldn't this be solved just by ending the inner loop in case of beet mode (as
it is done for tunnel)?
br, j
^ permalink raw reply
* Re: [PATCH 1/7] Preparatory refactoring part 1.
From: Patrick McHardy @ 2007-07-31 10:46 UTC (permalink / raw)
To: Corey Hickey; +Cc: netdev
In-Reply-To: <46AE8FD7.3010201@fatooh.org>
Corey Hickey wrote:
> Patrick McHardy wrote:
>
>>> -static int
>>> -sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>>> +static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data
>>> *q, unsigned int end)
>>
>>
>>
>> Please make sure to break at 80 chars and to keep the style
>> in this file consistent (newline before function name).
>
>
> Ok. For what it's worth, though, most of the original functions in the
> file don't have a newline before the function name. Omitting the newline
> would thus make the new/changed functions more consistent with the rest
> of the file. I don't have a preference either way, so unless you change
> your mind I'll put the newline back in..
You're right, just keep it consistent please and break at 80 chars.
>>> - sch->qstats.backlog += skb->len;
>>
>>
>> Why not keep this instead of having both callers do it?
>
>
> My idea was to have all the sfq_q_* functions operate on "struct
> sfq_sched_data" and have no knowledge of the "struct Qdisc". I did this
> in order to be able to use the new functions in sfq_change() when the
> temporary sfq_sched_data doesn't have a parent Qdisc.
>
> There's probably a better way, and I am of course open to suggestions,
> but what I did made sense to me.
Also sounds fine.
^ permalink raw reply
* Re: [PATCH net-2.6.22-rc7] xfrm beet interfamily support
From: Patrick McHardy @ 2007-07-31 10:51 UTC (permalink / raw)
To: Joakim Koskela; +Cc: netdev, David Miller
In-Reply-To: <200707311339.39913.joakim.koskela@hiit.fi>
Joakim Koskela wrote:
> On Thursday 19 July 2007 17:46:42 Patrick McHardy wrote:
>
>>Joakim Koskela wrote:
>>
>>>+ skb_push(skb, hdrlen);
>>>+ iphv6 = ipv6_hdr(skb);
>>>+
>>>+ skb_reset_network_header(skb);
>>>+ top_iphv6 = ipv6_hdr(skb);
>>>+
>>>+ protocol = iphv6->nexthdr;
>>>+ skb_pull(skb, delta);
>>>+ skb_reset_network_header(skb);
>>>+ top_iphv4 = ip_hdr(skb);
>>>+ skb_set_transport_header(skb, hdrlen);
>>>+ top_iphv4->ihl = (sizeof(struct iphdr) >> 2);
>>>+ top_iphv4->version = 4;
>>>+ top_iphv4->id = 0;
>>>+ top_iphv4->frag_off = htons(IP_DF);
>>>+ top_iphv4->ttl = dst_metric(dst->child, RTAX_HOPLIMIT);
>>>+ top_iphv4->saddr = x->props.saddr.a4;
>>>+ top_iphv4->daddr = x->id.daddr.a4;
>>>+ skb->transport_header += top_iphv4->ihl*4;
>>>+ top_iphv4->protocol = protocol;
>>>+
>>>+ skb->protocol = htons(ETH_P_IP);
>>>+#endif
>>
>>The output function in the IPv6/IPv4 case is called from
>>xfrm6_output_one, which loops until after a tunnel mode
>>encapsulation is done and then returns to the outer loop
>>in xfrm6_output_finish2, which passes the packet through
>>the netfilter hooks and continues with the next transform.
>>
>
>
> I'm not sure I really got this. IPv6/IPv4 means IPv6 inner, IPv4 outer, right?
> Isn't that called from xfrm4_output_one and subsequently passed through the
> right filters as well (as it has a ipv4 header by then)?
I think you're right, it uses xfrm4_output. But there's a mismatch
in either case, in both cases (IPv4 and IPv6) we first call the
POSTROUTING hook for this family, than do the transform (changing
the family), then call the OUTPUT hook for the same family. So
either the POSTROUTING or the OUTPUT hook is called for the wrong
family.
>>There are multiple problems resulting from the inter-family
>>encapsulation. First of all, the inner loop continues after
>>beet mode encapsulation, skipping the netfilter hooks in
>>case there are more transforms. It should (as with decaps = 1
>>on input) at least call netfilter hooks after an inter-family
>>transform. If the beet transform is the last one, the IPv4
>>skb will be passed through the IPv6 netfilter hooks, which is
>>clearly wrong. To fix these problems some restructuring in
>>xfrm[46]_output.c seems to be necessary.
>
>
> Couldn't this be solved just by ending the inner loop in case of beet mode (as
> it is done for tunnel)?
If the assumption that xfrm4_output is used for IPv6 in IPv4
encapsulation is correct and the problem mentioned above is
fixed, that should work fine.
^ permalink raw reply
* Re: [PATCH 0/4][RFC] lro: Generic Large Receive Offload for TCP traffic
From: Jan-Bernd Themann @ 2007-07-31 10:33 UTC (permalink / raw)
To: Andrew Gallatin
Cc: netdev, Christoph Raisch, Jan-Bernd Themann, linux-kernel,
linux-ppc, Marcus Eder, Thomas Klein, Stefan Roscher,
David Miller, Jeff Garzik
In-Reply-To: <46AE4AC9.6060109@myri.com>
Hi,
Thanks for finding these bugs! I'll post an updated version soon (2 patches
with no separate Kconfig patches, one LRO and one eHEA patch). See comments below.
Thanks,
Jan-Bernd
On Monday 30 July 2007 22:32, Andrew Gallatin wrote:
> I was working on testing the myri10ge patch, and I ran into a few
> problems. I've attached a patch to inet_lro.c to fix some of them,
> and a patch to myri10ge.c to show how to use the page based
> interface. Both patches are signed off by Andrew Gallatin
> <gallatin@myri.com>
>
> First, the LRO_MAX_PG_HLEN is still a problem. Minimally sized 60
> byte frames still cause problems in lro_gen_skb due to skb->len
> going negative. Fixed in the attached patch. It may be simpler
> to just drop LRO_MAX_PG_HLEN to ETH_ZLEN, but I'm not sure if
> that is enough. Are there "smart" NICs which might chop off padding
> themselves?
I'd tend to stick to an explicit check as implemented in your patch
for now
>
> Second, you still need to set skb->ip_summed = CHECKSUM_UNNECESSARY
> when modified packets are flushed, else the stack will see bad
> checksums for packets from CHECKSUM_COMPLETE drivers using the
> skb interface. Fixed in the attached patch.
I thought about it... As we do update the TCP checksum for aggregated
packets we could add a second ip_summed field in the net_lro_mgr struct
used for aggregated packets to support HW that does not have any checksum helper
functionality. These drivers could set this ip_summed field to CHECKSUM_NONE,
and thus leave the checksum check to the stack. I'm not sure if these old devices benefit
a lot from LRO. So what do you think?
>
> Fourth, I did some traffic sniffing to try to figure out what's going
> on above, and saw tcpdump complain about bad checksums. Have you tried
> running tcpdump -s 65535 -vvv? Have you also seen bad checksums?
> I seem to see this for both page- and skb-based versions of the driver.
>
Hmmm, can't confirm that. For our skb-based version I see
correct checksums for aggregated packets and for the page-based version as well.
I used: (tcpdump -i ethX -s 0 -w dump.bin) in combination with ethereal.
Don't see problems as well with your tcpdump command.
^ 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