* Re: sky2 still badly broken
From: Stephen Hemminger @ 2012-05-02 18:56 UTC (permalink / raw)
To: Niccolò Belli; +Cc: netdev
In-Reply-To: <4FA14EC6.6050207@linuxsystems.it>
On Wed, 02 May 2012 17:12:06 +0200
Niccolò Belli <darkbasic@linuxsystems.it> wrote:
> Il 30/04/2012 21:25, Stephen Hemminger ha scritto:
> > You are getting CRC and FIFO overrun errors. What laptop is this?
> > Everything works fine on my old Fuijitsu with same chip (but rev 14).
> > You could try taking out the status bit checks and see if the
> > packets are really okay and the Marvell chip is complaining about
> > bogus status.
>
> I compiled 3.4-rc5 + both sky2 patches you recently published and I did
> some more tests:
>
> Point to point: works flawlessly.
> Attached to the switch: rx errors, even downloading a very small 89 KB
> file :(
>
> This is the dump using IPv4:
> http://files.linuxsystems.it/temp/2012-05/sky2_ipv4.pcap
>
> dmesg (I did an rmmod -f sky2 before doing the test):
> [ 1147.885026] sky2 0000:06:00.0: eth0: disabling interface
> [ 1148.909548] sky2: driver version 1.30
> [ 1148.909764] sky2 0000:06:00.0: Yukon-2 EC Ultra chip revision 3
> [ 1148.914367] sky2 0000:06:00.0: irq 45 for MSI/MSI-X
> [ 1148.916310] sky2 0000:06:00.0: eth0: addr 00:13:77:b4:1b:fa
> [ 1148.942297] sky2 0000:06:00.0: eth0: enabling interface
> [ 1148.944225] ADDRCONF(NETDEV_UP): eth0: link is not ready
> [ 1151.496295] sky2 0000:06:00.0: eth0: Link is up at 1000 Mbps, full
> duplex, flow control rx
> [ 1151.497614] ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> [ 1178.479437] device eth0 entered promiscuous mode
> [ 1179.541707] *sky2 0000:06:00.0: eth0: rx error, status 0x7ffc0001
> length 1468*
> [ 1179.544110] *sky2 0000:06:00.0: eth0: rx error, status 0x7ffc0001
> length 1468*
> [ 1181.642598] device eth0 left promiscuous mode
>
>
>
>
> This is the dump using IPv6:
> http://files.linuxsystems.it/temp/2012-05/sky2_ipv6.pcap
>
> dmesg (I did an rmmod -f sky2 before doing the test):
> [ 1314.225572] sky2 0000:06:00.0: eth0: disabling interface
> [ 1315.248523] sky2: driver version 1.30
> [ 1315.248731] sky2 0000:06:00.0: Yukon-2 EC Ultra chip revision 3
> [ 1315.249333] sky2 0000:06:00.0: irq 45 for MSI/MSI-X
> [ 1315.250307] sky2 0000:06:00.0: eth0: addr 00:13:77:b4:1b:fa
> [ 1315.271364] sky2 0000:06:00.0: eth0: enabling interface
> [ 1315.273015] ADDRCONF(NETDEV_UP): eth0: link is not ready
> [ 1317.875985] sky2 0000:06:00.0: eth0: Link is up at 1000 Mbps, full
> duplex, flow control rx
> [ 1317.877311] ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> [ 1345.946062] device eth0 entered promiscuous mode
> [ 1349.119848] device eth0 left promiscuous mode
> [ 1376.231698] device eth0 entered promiscuous mode
> [ 1377.369095] *sky2 0000:06:00.0: eth0: rx error, status 0x7ffc0001
> length 1468*
> [ 1379.618257] device eth0 left promiscuous mode
>
>
>
> Switch is a Netgear GS724Tv3 firmware 5.0.3.5
>
>
> Cheers,
> Niccolò
It could be that your switch doesn't do autonegotiation or flow
control. You are getting receive fifo overflow errors.
^ permalink raw reply
* Re: [PATCH 2/2] ss: implement -M option to get all memory information
From: Stephen Hemminger @ 2012-05-02 19:00 UTC (permalink / raw)
To: Shan Wei; +Cc: xemul, NetDev
In-Reply-To: <4FA1021E.6030905@gmail.com>
On Wed, 02 May 2012 17:45:02 +0800
Shan Wei <shanwei88@gmail.com> wrote:
> Hi stephen:
>
> Stephen Hemminger said, at 2012/4/28 1:21:
>
> > Lots of options return more or different information based on kernel
> > version, probably the biggest example is how stats are processed.
>
>
> how about the following patch?
>
> ----
> [PATCH] ss: use new INET_DIAG_SKMEMINFO option to get memory information for tcp socket
>
>
> Signed-off-by: Shan Wei <davidshan@tencent.com>
> ---
> misc/ss.c | 16 ++++++++++++++--
> 1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/misc/ss.c b/misc/ss.c
> index 5f70a26..3cfc9e8 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -1336,7 +1336,17 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r)
> parse_rtattr(tb, INET_DIAG_MAX, (struct rtattr*)(r+1),
> nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
>
> - if (tb[INET_DIAG_MEMINFO]) {
> + if (tb[INET_DIAG_SKMEMINFO]) {
> + const unsigned int *skmeminfo = RTA_DATA(tb[INET_DIAG_SKMEMINFO]);
> + printf(" skmem:(r%u,rb%u,t%u,tb%u,f%u,w%u,o%u)",
> + skmeminfo[SK_MEMINFO_RMEM_ALLOC],
> + skmeminfo[SK_MEMINFO_RCVBUF],
> + skmeminfo[SK_MEMINFO_WMEM_ALLOC],
> + skmeminfo[SK_MEMINFO_SNDBUF],
> + skmeminfo[SK_MEMINFO_FWD_ALLOC],
> + skmeminfo[SK_MEMINFO_WMEM_QUEUED],
> + skmeminfo[SK_MEMINFO_OPTMEM]);
> + }else if (tb[INET_DIAG_MEMINFO]) {
> const struct inet_diag_meminfo *minfo
> = RTA_DATA(tb[INET_DIAG_MEMINFO]);
> printf(" mem:(r%u,w%u,f%u,t%u)",
> @@ -1505,8 +1515,10 @@ static int tcp_show_netlink(struct filter *f, FILE *dump_fp, int socktype)
> memset(&req.r, 0, sizeof(req.r));
> req.r.idiag_family = AF_INET;
> req.r.idiag_states = f->states;
> - if (show_mem)
> + if (show_mem) {
> req.r.idiag_ext |= (1<<(INET_DIAG_MEMINFO-1));
> + req.r.idiag_ext |= (1<<(INET_DIAG_SKMEMINFO-1));
> + }
>
> if (show_tcpinfo) {
> req.r.idiag_ext |= (1<<(INET_DIAG_INFO-1));
This looks good, is the skmeminfo a superset of the old meminfo?
But your code is broken on 64 bit. skmeminfo in kernel is an array of __u32!
^ permalink raw reply
* Re: [PATCH v3 1/3] tcp: early retransmit: tcp_enter_recovery()
From: Neal Cardwell @ 2012-05-02 19:03 UTC (permalink / raw)
To: Yuchung Cheng; +Cc: davem, ilpo.jarvinen, nanditad, netdev
In-Reply-To: <1335984391-31340-1-git-send-email-ycheng@google.com>
On Wed, May 2, 2012 at 2:46 PM, Yuchung Cheng <ycheng@google.com> wrote:
> This a prepartion patch that refactors the code to enter recovery
> into a new function tcp_enter_recovery(). It's needed to implement
> the delayed fast retransmit in ER.
>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> ---
> ChangeLog since v1:
> - swaped with part 1 and part2
> ChangeLog since v2:
> - removed RFC in commit message
Acked-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply
* Re: [PATCH v3 2/3] tcp: early retransmit
From: Neal Cardwell @ 2012-05-02 19:14 UTC (permalink / raw)
To: Yuchung Cheng; +Cc: davem, ilpo.jarvinen, nanditad, netdev
In-Reply-To: <1335984391-31340-2-git-send-email-ycheng@google.com>
On Wed, May 2, 2012 at 2:46 PM, Yuchung Cheng <ycheng@google.com> wrote:
I see this version of the patch decided against going with these
suggestions below that I made on Monday (repasted below). Do you have
a sec to share why you wanted to stick with the existing behavior?
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -365,12 +365,13 @@ struct tcp_sock {
>
> u32 frto_highmark; /* snd_nxt when RTO occurred */
> u16 advmss; /* Advertised MSS */
> - u8 frto_counter; /* Number of new acks after RTO */
> - u8 nonagle : 4,/* Disable Nagle algorithm? */
> + u16 nonagle : 4,/* Disable Nagle algorithm? */
> thin_lto : 1,/* Use linear timeouts for thin streams */
> thin_dupack : 1,/* Fast retransmit on first dupack */
> repair : 1,
> - unused : 1;
> + do_early_retrans: 1;/* Enable RFC5827 early-retransmit */
> +
> + u8 frto_counter; /* Number of new acks after RTO */
> u8 repair_queue;
To keep the change minimal and reduce the risk of mysterious
performance regressions from cache effects, I'd suggest keeping the
frto_counter and nonagle u8 bytes as u8 bytes in their current
location, and add a new u8 for the two ER bits. Same amount of space
as the scheme in the patch, just less shuffling.
> @@ -987,6 +989,7 @@ static void tcp_update_reordering(struct sock *sk, const int metric,
> tp->undo_marker ? tp->undo_retrans : 0);
> #endif
> tcp_disable_fack(tp);
> + tcp_disable_early_retrans(tp);
> }
> }
I think we should stick with the behavior where we disable early
retransmit any time tcp_update_reordering() is called with a non-zero
reordering metric. This is what we've tested and measured, and my
sense is that we could risk a significant number of spurious ER
firings if instead we relax this so that only reordering >3 causes us
to disable ER. I know the delayed ER should help avoid spurious ER
firings when there is a small degree of reordering, but my guess would
be that the max(RTT/4, 2ms) is perhaps not big enough if we're
allowing delayed ER for connections that have already witnessed small
degrees of reordering. So until we have more experimental data, I'd
recommend sticking with:
if (metric > 0)
tcp_disable_early_retrans(tp);
neal
^ permalink raw reply
* Re: WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
From: Alex Villacís Lasso @ 2012-05-02 19:26 UTC (permalink / raw)
To: netdev
In-Reply-To: <4F92EC8D.2010806@fiec.espol.edu.ec>
El 21/04/12 12:21, Alex Villacís Lasso escribió:
> I am getting the following WARNING while running 3.4-rc3 x86_64. Every time I get this, I am running my Bittorrent client (transmission-gtk).
>
> [ 1494.720011] ------------[ cut here ]------------
> [ 1494.720024] WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
> [ 1494.720027] Hardware name: OEM
> [ 1494.720031] NETDEV WATCHDOG: p17p1 (r8169): transmit queue 0 timed out
> [ 1494.720034] Modules linked in: fuse vboxpci(O) vboxnetadp(O) vboxnetflt(O) lockd vboxdrv(O) nf_conntrack_ipv4 nf_defrag_ipv4 ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables snd_hda_codec_realtek
> snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device ppdev snd_pcm uinput coretemp microcode pcspkr iTCO_wdt iTCO_vendor_support i2c_i801 parport_pc parport snd_timer snd soundcore r8169 mii snd_page_alloc sunrpc binfmt_misc floppy i915
> drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: scsi_wait_scan]
> [ 1494.720100] Pid: 0, comm: swapper/1 Tainted: G O 3.4.0-rc3 #35
> [ 1494.720103] Call Trace:
> [ 1494.720106] <IRQ> [<ffffffff81059bbf>] warn_slowpath_common+0x7f/0xc0
> [ 1494.720120] [<ffffffff81059cb6>] warn_slowpath_fmt+0x46/0x50
> [ 1494.720127] [<ffffffff8151ab97>] dev_watchdog+0x277/0x280
> [ 1494.720134] [<ffffffff81068f61>] run_timer_softirq+0x131/0x440
> [ 1494.720139] [<ffffffff8151a920>] ? qdisc_reset+0x50/0x50
> [ 1494.720145] [<ffffffff810611a8>] __do_softirq+0xb8/0x280
> [ 1494.720151] [<ffffffff8101cd62>] ? native_sched_clock+0x22/0x80
> [ 1494.720157] [<ffffffff81090895>] ? sched_clock_local+0x25/0x90
> [ 1494.720164] [<ffffffff8161619c>] call_softirq+0x1c/0x30
> [ 1494.720170] [<ffffffff810172e5>] do_softirq+0x65/0xa0
> [ 1494.720174] [<ffffffff8106169e>] irq_exit+0x9e/0xc0
> [ 1494.720180] [<ffffffff81616acb>] smp_apic_timer_interrupt+0x6b/0x98
> [ 1494.720186] [<ffffffff8161584a>] apic_timer_interrupt+0x6a/0x70
> [ 1494.720190] <EOI> [<ffffffff8101e253>] ? mwait_idle+0x93/0x320
> [ 1494.720199] [<ffffffff8101e1fa>] ? mwait_idle+0x3a/0x320
> [ 1494.720204] [<ffffffff8101ee19>] cpu_idle+0xd9/0x130
> [ 1494.720210] [<ffffffff815fb7cf>] start_secondary+0x266/0x26d
> [ 1494.720216] ---[ end trace b0a1361739c69d32 ]---
> [ 1494.721181] r8169 0000:02:00.0: p17p1: link up
>
>
Still present in 3.4-rc5 (2)
^ permalink raw reply
* Re: WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
From: Alex Villacís Lasso @ 2012-05-02 19:20 UTC (permalink / raw)
To: netdev
In-Reply-To: <4F92EC8D.2010806@fiec.espol.edu.ec>
El 21/04/12 12:21, Alex Villacís Lasso escribió:
> I am getting the following WARNING while running 3.4-rc3 x86_64. Every time I get this, I am running my Bittorrent client (transmission-gtk).
>
> [ 1494.720011] ------------[ cut here ]------------
> [ 1494.720024] WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
> [ 1494.720027] Hardware name: OEM
> [ 1494.720031] NETDEV WATCHDOG: p17p1 (r8169): transmit queue 0 timed out
> [ 1494.720034] Modules linked in: fuse vboxpci(O) vboxnetadp(O) vboxnetflt(O) lockd vboxdrv(O) nf_conntrack_ipv4 nf_defrag_ipv4 ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables snd_hda_codec_realtek
> snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device ppdev snd_pcm uinput coretemp microcode pcspkr iTCO_wdt iTCO_vendor_support i2c_i801 parport_pc parport snd_timer snd soundcore r8169 mii snd_page_alloc sunrpc binfmt_misc floppy i915
> drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: scsi_wait_scan]
> [ 1494.720100] Pid: 0, comm: swapper/1 Tainted: G O 3.4.0-rc3 #35
> [ 1494.720103] Call Trace:
> [ 1494.720106] <IRQ> [<ffffffff81059bbf>] warn_slowpath_common+0x7f/0xc0
> [ 1494.720120] [<ffffffff81059cb6>] warn_slowpath_fmt+0x46/0x50
> [ 1494.720127] [<ffffffff8151ab97>] dev_watchdog+0x277/0x280
> [ 1494.720134] [<ffffffff81068f61>] run_timer_softirq+0x131/0x440
> [ 1494.720139] [<ffffffff8151a920>] ? qdisc_reset+0x50/0x50
> [ 1494.720145] [<ffffffff810611a8>] __do_softirq+0xb8/0x280
> [ 1494.720151] [<ffffffff8101cd62>] ? native_sched_clock+0x22/0x80
> [ 1494.720157] [<ffffffff81090895>] ? sched_clock_local+0x25/0x90
> [ 1494.720164] [<ffffffff8161619c>] call_softirq+0x1c/0x30
> [ 1494.720170] [<ffffffff810172e5>] do_softirq+0x65/0xa0
> [ 1494.720174] [<ffffffff8106169e>] irq_exit+0x9e/0xc0
> [ 1494.720180] [<ffffffff81616acb>] smp_apic_timer_interrupt+0x6b/0x98
> [ 1494.720186] [<ffffffff8161584a>] apic_timer_interrupt+0x6a/0x70
> [ 1494.720190] <EOI> [<ffffffff8101e253>] ? mwait_idle+0x93/0x320
> [ 1494.720199] [<ffffffff8101e1fa>] ? mwait_idle+0x3a/0x320
> [ 1494.720204] [<ffffffff8101ee19>] cpu_idle+0xd9/0x130
> [ 1494.720210] [<ffffffff815fb7cf>] start_secondary+0x266/0x26d
> [ 1494.720216] ---[ end trace b0a1361739c69d32 ]---
> [ 1494.721181] r8169 0000:02:00.0: p17p1: link up
>
>
Still present in 3.4-rc5.
^ permalink raw reply
* Re: [PATCH v3 3/3] tcp: early retransmit: delayed fast retransmit
From: Neal Cardwell @ 2012-05-02 19:34 UTC (permalink / raw)
To: Yuchung Cheng; +Cc: davem, ilpo.jarvinen, nanditad, netdev
In-Reply-To: <1335984391-31340-3-git-send-email-ycheng@google.com>
On Wed, May 2, 2012 at 2:46 PM, Yuchung Cheng <ycheng@google.com> wrote:
> Implementing the advanced early retransmit (sysctl_tcp_early_retrans==2).
> Delays the fast retransmit by an interval of RTT/4. We borrow the
> RTO timer to implement the delay. If we receive another ACK or send
> a new packet, the timer is cancelled and restored to original RTO
> value offset by time elapsed. When the delayed-ER timer fires,
> we enter fast recovery and perform fast retransmit.
>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> ---
> ChangeLog in v2:
> - Set sysctl_tcp_early_retrans default to 2
> ChangeLog in v3:
> - use separate u8 for early retrans stats in tcp_sock
> - disable ER if detects any reordering
After reading patch 3 of the series, I see that patch 3 incorporates
most of those suggestions from Monday. I think it would be quite a bit
cleaner to just have patch 2 of the series put the
tcp_disable_early_retrans() call and tcp_sock fields in the ultimately
desired place, rather than having patch 2 put them somewhere and patch
3 move them, but maybe that's just me.
When all the patches in the series are applied, the one issue I still
see is that frto_counter is in a new place, a bit further away from
frto_highmark than it used to be before the patch series. I think it
would be good to keep frto_counter in its original location, back up
nearer to frto_highmark.
neal
^ permalink raw reply
* Re: [V2 PATCH 0/9] vhost/macvtap zeropcopy fixes
From: Eric W. Biederman @ 2012-05-02 19:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, jasowang, eric.dumazet, netdev, linux-kernel
In-Reply-To: <20120502081141.GA30549@redhat.com>
Side question. Are you aware that macvtap/vhost net is broken
in the presence of vlan accelleration and that vlan accelleration
is not optional if you are using vlan headers?
Eric
^ permalink raw reply
* Re: [PATCH] tcp: change tcp_adv_win_scale and tcp_rmem[2]
From: Neal Cardwell @ 2012-05-02 19:48 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Tom Herbert, Yuchung Cheng
In-Reply-To: <1335961721.22133.562.camel@edumazet-glaptop>
On Wed, May 2, 2012 at 8:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> tcp_adv_win_scale default value is 2, meaning we expect a good citizen
> skb to have skb->len / skb->truesize ratio of 75% (3/4)
>
> In 2.6 kernels we (mis)accounted for typical MSS=1460 frame :
> 1536 + 64 + 256 = 1856 'estimated truesize', and 1856 * 3/4 = 1392.
> So these skbs were considered as not bloated.
>
> With recent truesize fixes, a typical MSS=1460 frame truesize is now the
> more precise :
> 2048 + 256 = 2304. But 2304 * 3/4 = 1728.
> So these skb are not good citizen anymore, because 1460 < 1728
>
> (GRO can escape this problem because it build skbs with a too low
> truesize.)
>
> This also means tcp advertises a too optimistic window for a given
> allocated rcvspace : When receiving frames, sk_rmem_alloc can hit
> sk_rcvbuf limit and we call tcp_prune_queue()/tcp_collapse() too often,
> especially when application is slow to drain its receive queue or in
> case of losses (netperf is fast, scp is slow). This is a major latency
> source.
>
> We should adjust the len/truesize ratio to 50% instead of 75%
>
> This patch :
>
> 1) changes tcp_adv_win_scale default to 1 instead of 2
>
> 2) increase tcp_rmem[2] limit from 4MB to 6MB to take into account
> better truesize tracking and to allow autotuning tcp receive window to
> reach same value than before. Note that same amount of kernel memory is
> consumed compared to 2.6 kernels.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply
* [PATCH net-next] net: implement tcp coalescing in tcp_queue_rcv()
From: Eric Dumazet @ 2012-05-02 19:58 UTC (permalink / raw)
To: David Miller
Cc: Alexander Duyck, Alexander Duyck, netdev, Neal Cardwell,
Tom Herbert, Jeff Kirsher, Michael Chan, Matt Carlson, Herbert Xu,
Ben Hutchings, Ilpo Järvinen, Maciej Żenczykowski
In-Reply-To: <1335981358.22133.605.camel@edumazet-glaptop>
From: Eric Dumazet <edumazet@google.com>
Extend tcp coalescing implementing it from tcp_queue_rcv(), the main
receiver function when application is not blocked in recvmsg().
Function tcp_queue_rcv() is moved a bit to allow its call from
tcp_data_queue()
This gives good results especially if GRO could not kick, and if skb
head is a fragment.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Tom Herbert <therbert@google.com>
---
To be applied after "[PATCH v2 net-next] net: take care of cloned skbs
in tcp_try_coalesce()"
include/net/tcp.h | 3 ++-
net/ipv4/tcp.c | 10 +++++-----
net/ipv4/tcp_input.c | 40 +++++++++++++++++++++-------------------
3 files changed, 28 insertions(+), 25 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 0fb84de..a9d2fb8 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -438,7 +438,8 @@ extern int tcp_disconnect(struct sock *sk, int flags);
void tcp_connect_init(struct sock *sk);
void tcp_finish_connect(struct sock *sk, struct sk_buff *skb);
-void tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int hdrlen);
+int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb,
+ int hdrlen, bool *fragstolen);
/* From syncookies.c */
extern __u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS];
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 9670af3..bd5deff 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -980,8 +980,8 @@ static inline int select_size(const struct sock *sk, bool sg)
static int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
{
struct sk_buff *skb;
- struct tcp_skb_cb *cb;
struct tcphdr *th;
+ bool fragstolen;
skb = alloc_skb(size + sizeof(*th), sk->sk_allocation);
if (!skb)
@@ -994,14 +994,14 @@ static int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
if (memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size))
goto err_free;
- cb = TCP_SKB_CB(skb);
-
TCP_SKB_CB(skb)->seq = tcp_sk(sk)->rcv_nxt;
TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq + size;
TCP_SKB_CB(skb)->ack_seq = tcp_sk(sk)->snd_una - 1;
- tcp_queue_rcv(sk, skb, sizeof(*th));
-
+ if (tcp_queue_rcv(sk, skb, sizeof(*th), &fragstolen)) {
+ WARN_ON_ONCE(fragstolen); /* should not happen */
+ __kfree_skb(skb);
+ }
return size;
err_free:
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index f891a5e..2233468 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4662,6 +4662,22 @@ end:
skb_set_owner_r(skb, sk);
}
+int tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int hdrlen,
+ bool *fragstolen)
+{
+ int eaten;
+ struct sk_buff *tail = skb_peek_tail(&sk->sk_receive_queue);
+
+ __skb_pull(skb, hdrlen);
+ eaten = (tail &&
+ tcp_try_coalesce(sk, tail, skb, fragstolen)) ? 1 : 0;
+ tcp_sk(sk)->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
+ if (!eaten) {
+ __skb_queue_tail(&sk->sk_receive_queue, skb);
+ skb_set_owner_r(skb, sk);
+ }
+ return eaten;
+}
static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
{
@@ -4708,20 +4724,12 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
}
if (eaten <= 0) {
- struct sk_buff *tail;
queue_and_out:
if (eaten < 0 &&
tcp_try_rmem_schedule(sk, skb->truesize))
goto drop;
- tail = skb_peek_tail(&sk->sk_receive_queue);
- eaten = (tail &&
- tcp_try_coalesce(sk, tail, skb,
- &fragstolen)) ? 1 : 0;
- if (eaten <= 0) {
- skb_set_owner_r(skb, sk);
- __skb_queue_tail(&sk->sk_receive_queue, skb);
- }
+ eaten = tcp_queue_rcv(sk, skb, 0, &fragstolen);
}
tp->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
if (skb->len)
@@ -5416,14 +5424,6 @@ discard:
return 0;
}
-void tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int hdrlen)
-{
- __skb_pull(skb, hdrlen);
- __skb_queue_tail(&sk->sk_receive_queue, skb);
- skb_set_owner_r(skb, sk);
- tcp_sk(sk)->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
-}
-
/*
* TCP receive function for the ESTABLISHED state.
*
@@ -5532,6 +5532,7 @@ int tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
} else {
int eaten = 0;
int copied_early = 0;
+ bool fragstolen = false;
if (tp->copied_seq == tp->rcv_nxt &&
len - tcp_header_len <= tp->ucopy.len) {
@@ -5589,7 +5590,8 @@ int tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPHPHITS);
/* Bulk data transfer: receiver */
- tcp_queue_rcv(sk, skb, tcp_header_len);
+ eaten = tcp_queue_rcv(sk, skb, tcp_header_len,
+ &fragstolen);
}
tcp_event_data_recv(sk, skb);
@@ -5611,7 +5613,7 @@ no_ack:
else
#endif
if (eaten)
- __kfree_skb(skb);
+ kfree_skb_partial(skb, fragstolen);
else
sk->sk_data_ready(sk, 0);
return 0;
^ permalink raw reply related
* Re: [PATCH net-next] net: implement tcp coalescing in tcp_queue_rcv()
From: Joe Perches @ 2012-05-02 20:11 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Alexander Duyck, Alexander Duyck, netdev,
Neal Cardwell, Tom Herbert, Jeff Kirsher, Michael Chan,
Matt Carlson, Herbert Xu, Ben Hutchings, Ilpo Järvinen,
Maciej Żenczykowski
In-Reply-To: <1335988709.22133.632.camel@edumazet-glaptop>
On Wed, 2012-05-02 at 21:58 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Extend tcp coalescing implementing it from tcp_queue_rcv(), the main
> receiver function when application is not blocked in recvmsg().
>
> Function tcp_queue_rcv() is moved a bit to allow its call from
> tcp_data_queue()
>
> This gives good results especially if GRO could not kick, and if skb
> head is a fragment.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Tom Herbert <therbert@google.com>
> ---
> To be applied after "[PATCH v2 net-next] net: take care of cloned skbs
> in tcp_try_coalesce()"
>
> include/net/tcp.h | 3 ++-
> net/ipv4/tcp.c | 10 +++++-----
> net/ipv4/tcp_input.c | 40 +++++++++++++++++++++-------------------
> 3 files changed, 28 insertions(+), 25 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 0fb84de..a9d2fb8 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -438,7 +438,8 @@ extern int tcp_disconnect(struct sock *sk, int flags);
>
> void tcp_connect_init(struct sock *sk);
> void tcp_finish_connect(struct sock *sk, struct sk_buff *skb);
> -void tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int hdrlen);
> +int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb,
> + int hdrlen, bool *fragstolen);
>
> /* From syncookies.c */
> extern __u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS];
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 9670af3..bd5deff 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -980,8 +980,8 @@ static inline int select_size(const struct sock *sk, bool sg)
> static int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
> {
> struct sk_buff *skb;
> - struct tcp_skb_cb *cb;
> struct tcphdr *th;
> + bool fragstolen;
It might be useful to comment that this is/can be initialized to
false in tcp_try_coalesce via tcp_recv_queue.
> skb = alloc_skb(size + sizeof(*th), sk->sk_allocation);
> if (!skb)
> @@ -994,14 +994,14 @@ static int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
> if (memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size))
> goto err_free;
>
> - cb = TCP_SKB_CB(skb);
> -
> TCP_SKB_CB(skb)->seq = tcp_sk(sk)->rcv_nxt;
> TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq + size;
> TCP_SKB_CB(skb)->ack_seq = tcp_sk(sk)->snd_una - 1;
>
> - tcp_queue_rcv(sk, skb, sizeof(*th));
> -
> + if (tcp_queue_rcv(sk, skb, sizeof(*th), &fragstolen)) {
> + WARN_ON_ONCE(fragstolen); /* should not happen */
Otherwise this looks like a possibly uninitialized test
of fragstolen. Maybe there's a path where tail is null
in tcp_recv_queue and it's an uninitialized test anyway.
> + __kfree_skb(skb);
> + }
> return size;
>
> err_free:
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index f891a5e..2233468 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4662,6 +4662,22 @@ end:
> skb_set_owner_r(skb, sk);
> }
>
> +int tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int hdrlen,
> + bool *fragstolen)
> +{
> + int eaten;
> + struct sk_buff *tail = skb_peek_tail(&sk->sk_receive_queue);
> +
> + __skb_pull(skb, hdrlen);
> + eaten = (tail &&
> + tcp_try_coalesce(sk, tail, skb, fragstolen)) ? 1 : 0;
> + tcp_sk(sk)->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
> + if (!eaten) {
> + __skb_queue_tail(&sk->sk_receive_queue, skb);
> + skb_set_owner_r(skb, sk);
> + }
> + return eaten;
> +}
^ permalink raw reply
* bonding: don't increase rx_dropped after processing LACPDUs
From: Jiri Bohac @ 2012-05-02 20:23 UTC (permalink / raw)
To: Jay Vosburgh, Andy Gospodarek, netdev
Since commit 3aba891d, bonding processes LACP frames (802.3ad
mode) with bond_handle_frame(). Currently a copy of the skb is
made and the original is left to be processed by other
rx_handlers and the rest of the network stack by returning
RX_HANDLER_ANOTHER. As there is no protocol handler for
PKT_TYPE_LACPDU, the frame is dropped and dev->rx_dropped
increased.
Fix this by making bond_handle_frame() return RX_HANDLER_CONSUMED
if bonding has processed the LACP frame.
Signed-off-by: Jiri Bohac <jbohac@suse.cz>
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2173,9 +2173,10 @@ re_arm:
* received frames (loopback). Since only the payload is given to this
* function, it check for loopback.
*/
-static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
+static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
{
struct port *port;
+ int ret = RX_HANDLER_ANOTHER;
if (length >= sizeof(struct lacpdu)) {
@@ -2184,11 +2185,12 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
if (!port->slave) {
pr_warning("%s: Warning: port of slave %s is uninitialized\n",
slave->dev->name, slave->dev->master->name);
- return;
+ return ret;
}
switch (lacpdu->subtype) {
case AD_TYPE_LACPDU:
+ ret = RX_HANDLER_CONSUMED;
pr_debug("Received LACPDU on port %d\n",
port->actor_port_number);
/* Protect against concurrent state machines */
@@ -2198,6 +2200,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
break;
case AD_TYPE_MARKER:
+ ret = RX_HANDLER_CONSUMED;
// No need to convert fields to Little Endian since we don't use the marker's fields.
switch (((struct bond_marker *)lacpdu)->tlv_type) {
@@ -2219,6 +2222,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
}
}
}
+ return ret;
}
/**
@@ -2456,18 +2460,20 @@ out:
return NETDEV_TX_OK;
}
-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
struct slave *slave)
{
+ int ret = RX_HANDLER_ANOTHER;
if (skb->protocol != PKT_TYPE_LACPDU)
- return;
+ return ret;
if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
- return;
+ return ret;
read_lock(&bond->lock);
- bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
+ ret = bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
read_unlock(&bond->lock);
+ return ret;
}
/*
diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
index 235b2cc..5ee7e3c 100644
--- a/drivers/net/bonding/bond_3ad.h
+++ b/drivers/net/bonding/bond_3ad.h
@@ -274,7 +274,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
void bond_3ad_handle_link_change(struct slave *slave, char link);
int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
struct slave *slave);
int bond_3ad_set_carrier(struct bonding *bond);
void bond_3ad_update_lacp_rate(struct bonding *bond);
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 62d2409..30eace7 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1444,8 +1444,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
struct sk_buff *skb = *pskb;
struct slave *slave;
struct bonding *bond;
- void (*recv_probe)(struct sk_buff *, struct bonding *,
+ int (*recv_probe)(struct sk_buff *, struct bonding *,
struct slave *);
+ int ret = RX_HANDLER_ANOTHER;
skb = skb_share_check(skb, GFP_ATOMIC);
if (unlikely(!skb))
@@ -1464,8 +1465,10 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
if (likely(nskb)) {
- recv_probe(nskb, bond, slave);
+ ret = recv_probe(nskb, bond, slave);
dev_kfree_skb(nskb);
+ if (ret == RX_HANDLER_CONSUMED)
+ kfree_skb(skb);
}
}
@@ -1487,7 +1490,7 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
}
- return RX_HANDLER_ANOTHER;
+ return ret;
}
/* enslave device <slave> to bond device <master> */
@@ -2723,7 +2726,7 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
}
}
-static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
+static int bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
struct slave *slave)
{
struct arphdr *arp;
@@ -2731,7 +2734,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
__be32 sip, tip;
if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
- return;
+ return RX_HANDLER_ANOTHER;
read_lock(&bond->lock);
@@ -2776,6 +2779,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
out_unlock:
read_unlock(&bond->lock);
+ return RX_HANDLER_ANOTHER;
}
/*
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply related
* Re: [PATCH net-next] net: implement tcp coalescing in tcp_queue_rcv()
From: Eric Dumazet @ 2012-05-02 20:23 UTC (permalink / raw)
To: Joe Perches
Cc: David Miller, Alexander Duyck, Alexander Duyck, netdev,
Neal Cardwell, Tom Herbert, Jeff Kirsher, Michael Chan,
Matt Carlson, Herbert Xu, Ben Hutchings, Ilpo Järvinen,
Maciej Żenczykowski
In-Reply-To: <1335989484.9611.11.camel@joe2Laptop>
On Wed, 2012-05-02 at 13:11 -0700, Joe Perches wrote:
> It might be useful to comment that this is/can be initialized to
> false in tcp_try_coalesce via tcp_recv_queue.
> Otherwise this looks like a possibly uninitialized test
> of fragstolen. Maybe there's a path where tail is null
> in tcp_recv_queue and it's an uninitialized test anyway.
If tcp_queue_rcv() returns 1, fragstolen is initialized in
tcp_try_coalesce().
If tcp_queue_rcv() returns 0, fragstolen content is undefined and we
dont care.
If a compiler or static checker complains, its only their problem.
^ permalink raw reply
* Re: [PATCH net-next] net: implement tcp coalescing in tcp_queue_rcv()
From: Joe Perches @ 2012-05-02 20:34 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Alexander Duyck, Alexander Duyck, netdev,
Neal Cardwell, Tom Herbert, Jeff Kirsher, Michael Chan,
Matt Carlson, Herbert Xu, Ben Hutchings, Ilpo Järvinen,
Maciej Żenczykowski
In-Reply-To: <1335990187.22133.636.camel@edumazet-glaptop>
On Wed, 2012-05-02 at 22:23 +0200, Eric Dumazet wrote:
> On Wed, 2012-05-02 at 13:11 -0700, Joe Perches wrote:
> > It might be useful to comment that this is/can be initialized to
> > false in tcp_try_coalesce via tcp_recv_queue.
> > Otherwise this looks like a possibly uninitialized test
> > of fragstolen. Maybe there's a path where tail is null
> > in tcp_recv_queue and it's an uninitialized test anyway.
>
> If tcp_queue_rcv() returns 1, fragstolen is initialized in
> tcp_try_coalesce().
>
> If tcp_queue_rcv() returns 0, fragstolen content is undefined and we
> dont care.
>
> If a compiler or static checker complains, its only their problem.
True, but it's code that's a bit fragile and
I think as such it could be improved for any
human reader by a descriptive comment.
^ permalink raw reply
* Re: bonding: don't increase rx_dropped after processing LACPDUs
From: Eric Dumazet @ 2012-05-02 20:36 UTC (permalink / raw)
To: Jiri Bohac; +Cc: Jay Vosburgh, Andy Gospodarek, netdev
In-Reply-To: <20120502202309.GA25355@midget.suse.cz>
On Wed, 2012-05-02 at 22:23 +0200, Jiri Bohac wrote:
> Since commit 3aba891d, bonding processes LACP frames (802.3ad
> mode) with bond_handle_frame(). Currently a copy of the skb is
> made and the original is left to be processed by other
> rx_handlers and the rest of the network stack by returning
> RX_HANDLER_ANOTHER. As there is no protocol handler for
> PKT_TYPE_LACPDU, the frame is dropped and dev->rx_dropped
> increased.
>
> Fix this by making bond_handle_frame() return RX_HANDLER_CONSUMED
> if bonding has processed the LACP frame.
>
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
Nice idea but...
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1444,8 +1444,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> struct sk_buff *skb = *pskb;
> struct slave *slave;
> struct bonding *bond;
> - void (*recv_probe)(struct sk_buff *, struct bonding *,
> + int (*recv_probe)(struct sk_buff *, struct bonding *,
> struct slave *);
> + int ret = RX_HANDLER_ANOTHER;
>
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (unlikely(!skb))
> @@ -1464,8 +1465,10 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>
> if (likely(nskb)) {
> - recv_probe(nskb, bond, slave);
> + ret = recv_probe(nskb, bond, slave);
> dev_kfree_skb(nskb);
> + if (ret == RX_HANDLER_CONSUMED)
> + kfree_skb(skb);
After this point, you have use after free :
if (bond_should_deliver_exact_match(skb, slave, bond)) {
...
}
skb->dev = bond->dev;
...
> }
> }
>
> @@ -1487,7 +1490,7 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
> }
>
> - return RX_HANDLER_ANOTHER;
> + return ret;
> }
>
^ permalink raw reply
* Re: bonding: don't increase rx_dropped after processing LACPDUs
From: Jiri Bohac @ 2012-05-02 20:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jiri Bohac, Jay Vosburgh, Andy Gospodarek, netdev
In-Reply-To: <1335991009.22133.639.camel@edumazet-glaptop>
On Wed, May 02, 2012 at 10:36:49PM +0200, Eric Dumazet wrote:
> > + if (ret == RX_HANDLER_CONSUMED)
> > + kfree_skb(skb);
>
> After this point, you have use after free :
>
> if (bond_should_deliver_exact_match(skb, slave, bond)) {
> ...
> }
> skb->dev = bond->dev;
Thanks for spotting this! Let's just return immediately at that
point. Fixed version below:
Signed-off-by: Jiri Bohac <jbohac@suse.cz>
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2173,9 +2173,10 @@ re_arm:
* received frames (loopback). Since only the payload is given to this
* function, it check for loopback.
*/
-static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
+static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
{
struct port *port;
+ int ret = RX_HANDLER_ANOTHER;
if (length >= sizeof(struct lacpdu)) {
@@ -2184,11 +2185,12 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
if (!port->slave) {
pr_warning("%s: Warning: port of slave %s is uninitialized\n",
slave->dev->name, slave->dev->master->name);
- return;
+ return ret;
}
switch (lacpdu->subtype) {
case AD_TYPE_LACPDU:
+ ret = RX_HANDLER_CONSUMED;
pr_debug("Received LACPDU on port %d\n",
port->actor_port_number);
/* Protect against concurrent state machines */
@@ -2198,6 +2200,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
break;
case AD_TYPE_MARKER:
+ ret = RX_HANDLER_CONSUMED;
// No need to convert fields to Little Endian since we don't use the marker's fields.
switch (((struct bond_marker *)lacpdu)->tlv_type) {
@@ -2219,6 +2222,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
}
}
}
+ return ret;
}
/**
@@ -2456,18 +2460,20 @@ out:
return NETDEV_TX_OK;
}
-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
struct slave *slave)
{
+ int ret = RX_HANDLER_ANOTHER;
if (skb->protocol != PKT_TYPE_LACPDU)
- return;
+ return ret;
if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
- return;
+ return ret;
read_lock(&bond->lock);
- bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
+ ret = bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
read_unlock(&bond->lock);
+ return ret;
}
/*
diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
index 235b2cc..5ee7e3c 100644
--- a/drivers/net/bonding/bond_3ad.h
+++ b/drivers/net/bonding/bond_3ad.h
@@ -274,7 +274,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
void bond_3ad_handle_link_change(struct slave *slave, char link);
int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
struct slave *slave);
int bond_3ad_set_carrier(struct bonding *bond);
void bond_3ad_update_lacp_rate(struct bonding *bond);
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 62d2409..0a0f4a6 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1444,8 +1444,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
struct sk_buff *skb = *pskb;
struct slave *slave;
struct bonding *bond;
- void (*recv_probe)(struct sk_buff *, struct bonding *,
+ int (*recv_probe)(struct sk_buff *, struct bonding *,
struct slave *);
+ int ret = RX_HANDLER_ANOTHER;
skb = skb_share_check(skb, GFP_ATOMIC);
if (unlikely(!skb))
@@ -1464,8 +1465,12 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
if (likely(nskb)) {
- recv_probe(nskb, bond, slave);
+ ret = recv_probe(nskb, bond, slave);
dev_kfree_skb(nskb);
+ if (ret == RX_HANDLER_CONSUMED) {
+ kfree_skb(skb);
+ return ret;
+ }
}
}
@@ -1487,7 +1492,7 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
}
- return RX_HANDLER_ANOTHER;
+ return ret;
}
/* enslave device <slave> to bond device <master> */
@@ -2723,7 +2728,7 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
}
}
-static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
+static int bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
struct slave *slave)
{
struct arphdr *arp;
@@ -2731,7 +2736,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
__be32 sip, tip;
if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
- return;
+ return RX_HANDLER_ANOTHER;
read_lock(&bond->lock);
@@ -2776,6 +2781,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
out_unlock:
read_unlock(&bond->lock);
+ return RX_HANDLER_ANOTHER;
}
/*
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply related
* Re: [PATCH net-next] net: take care of cloned skbs in tcp_try_coalesce()
From: Alexander Duyck @ 2012-05-02 20:55 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexander Duyck, David Miller, netdev, Neal Cardwell, Tom Herbert,
Jeff Kirsher, Michael Chan, Matt Carlson, Herbert Xu,
Ben Hutchings, Ilpo Järvinen, Maciej Żenczykowski
In-Reply-To: <1335982515.22133.610.camel@edumazet-glaptop>
On 05/02/2012 11:15 AM, Eric Dumazet wrote:
> On Wed, 2012-05-02 at 11:05 -0700, Alexander Duyck wrote:
>
>> You're correct about the fragstolen case, I actually was thinking of the
>> first patch you sent, not this second one.
>>
>> However we still have a problem. What we end up with now is a case of
>> sharing in which the clone skb no longer knows that it is sharing the
>> head with another skb. The dataref will drop to 1 when we call
>> __kfree_skb. This means that any other function out there that tries to
>> see if the skb is shared would return false. This could lead to issues
>> if there is anything out there that manipulates the data in head based
>> on the false assumption that it is not cloned. What we would probably
>> need to do in this case is tweak the logic for skb_cloned. If you are
>> using a head_frag you should probably add a check that returns true if
>> cloned is true and page_count is greater than 1. We should be safe in
>> the case of skb_header_cloned since we already dropped are dataref when
>> we stole the page and freed the skb.
> I really dont understand this concern.
>
> When skb is cloned, we copy in head_frag __skb_clone()
>
> So both skbs have the bit set, and dataref = 2.
>
> first skb is freed, dataref becomes 1 and nothing special happen
>
> >From this point, skb->head is not 'shared' anymore (taken your own
> words). And we are free to do whatever we want.
>
> second skb is freed, dataref becomes 0 and we call the right destructor.
The problem is that the stack will not be able to detect sharing. As
long as page_count is greater than 2 and skb->cloned is set we should be
telling any callers to skb_cloned that the head is cloned. Otherwise we
can run into issues elsewhere with well meaning code checking and not
detecting sharing, and then mangling the header.
Also I am not sure if the big monolithic changes are really the best way
to approach this. It would be nice if we could fix this incrementally
instead of trying to do it all at once since there are multiple issues
that need to be addressed.
I will try to submit a few patches from my end later today. I still
need to look over all of the changes from the past couple of weeks that
were based on the assumption that the IP stack completely owned the skb.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH] tcp: change tcp_adv_win_scale and tcp_rmem[2]
From: Rick Jones @ 2012-05-02 21:05 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Neal Cardwell, netdev, Tom Herbert, Yuchung Cheng
In-Reply-To: <1335961721.22133.562.camel@edumazet-glaptop>
On 05/02/2012 05:28 AM, Eric Dumazet wrote:
> We should adjust the len/truesize ratio to 50% instead of 75%
As an added bonus, it would be consistent with the code behind
setsockopt(SO_[SND|RCV]BUF) doubling what the user passes-in. (modulo
the net.core.[rw]mem_max setting)
rick jones
^ permalink raw reply
* Re: [PATCH v3 3/3] tcp: early retransmit: delayed fast retransmit
From: Yuchung Cheng @ 2012-05-02 21:17 UTC (permalink / raw)
To: Neal Cardwell; +Cc: davem, ilpo.jarvinen, nanditad, netdev
In-Reply-To: <CADVnQyn6K8xH=wwi7pN0wsKS4wEhksi2pB0KbTaSDqLGWmnBaw@mail.gmail.com>
On Wed, May 2, 2012 at 12:34 PM, Neal Cardwell <ncardwell@google.com> wrote:
> On Wed, May 2, 2012 at 2:46 PM, Yuchung Cheng <ycheng@google.com> wrote:
>> Implementing the advanced early retransmit (sysctl_tcp_early_retrans==2).
>> Delays the fast retransmit by an interval of RTT/4. We borrow the
>> RTO timer to implement the delay. If we receive another ACK or send
>> a new packet, the timer is cancelled and restored to original RTO
>> value offset by time elapsed. When the delayed-ER timer fires,
>> we enter fast recovery and perform fast retransmit.
>>
>> Signed-off-by: Yuchung Cheng <ycheng@google.com>
>> ---
>> ChangeLog in v2:
>> - Set sysctl_tcp_early_retrans default to 2
>> ChangeLog in v3:
>> - use separate u8 for early retrans stats in tcp_sock
>> - disable ER if detects any reordering
>
> After reading patch 3 of the series, I see that patch 3 incorporates
> most of those suggestions from Monday. I think it would be quite a bit
> cleaner to just have patch 2 of the series put the
> tcp_disable_early_retrans() call and tcp_sock fields in the ultimately
> desired place, rather than having patch 2 put them somewhere and patch
> 3 move them, but maybe that's just me.
>
> When all the patches in the series are applied, the one issue I still
> see is that frto_counter is in a new place, a bit further away from
> frto_highmark than it used to be before the patch series. I think it
> would be good to keep frto_counter in its original location, back up
> nearer to frto_highmark.
Sorry for keep mis-placing the changes in different patch parts. Will fix asap.
>
> neal
^ permalink raw reply
* Re: [V2 PATCH 0/9] vhost/macvtap zeropcopy fixes
From: Michael S. Tsirkin @ 2012-05-02 21:31 UTC (permalink / raw)
To: Eric W. Biederman
Cc: David Miller, jasowang, eric.dumazet, netdev, linux-kernel
In-Reply-To: <87pqammkvs.fsf@xmission.com>
On Wed, May 02, 2012 at 12:40:55PM -0700, Eric W. Biederman wrote:
>
> Side question. Are you aware that macvtap/vhost net is broken
> in the presence of vlan accelleration and that vlan accelleration
> is not optional if you are using vlan headers?
>
> Eric
I didn't know. Could you explain please?
^ permalink raw reply
* Re: [net-next PATCH v4 0/8] Managing the forwarding database(FDB)
From: John Fastabend @ 2012-05-02 21:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: shemminger, bhutchings, sri, hadi, jeffrey.t.kirsher, netdev,
gregory.v.rose, krkumar2, roprabhu
In-Reply-To: <20120502150830.GA2976@redhat.com>
On 5/2/2012 8:08 AM, Michael S. Tsirkin wrote:
> On Sun, Apr 15, 2012 at 01:06:37PM -0400, David Miller wrote:
>> From: John Fastabend <john.r.fastabend@intel.com>
>> Date: Sun, 15 Apr 2012 09:43:51 -0700
>>
>>> The following series is a submission for net-next to allow
>>> embedded switches and other stacked devices other then the
>>> Linux bridge to manage a forwarding database.
>>>
>>> Previously discussed here,
>>>
>>> http://lists.openwall.net/netdev/2012/03/19/26
>>>
>>> v4: propagate return codes correctly for ndo_dflt_Fdb_dump()
>>>
>>> v3: resolve the macvlan patch 8/8 to fix a dev_set_promiscuity()
>>> error and add the flags field to change and get link routines.
>>>
>>> v2: addressed feedback from Ben Hutchings resolving a typo in the
>>> multicast add/del routines and improving the error handling
>>> when both NTF_SELF and NTF_MASTER are set.
>>>
>>> I've tested this with 'br' tool published by Stephen Hemminger
>>> soon to be renamed 'bridge' I believe and various traffic
>>> generators mostly pktgen, ping, and netperf.
>>
>> All applied, if we need any more tweaks we can just add them
>> on top of this work.
>>
>> Thanks John.
>
> John, do you plan to update kvm userspace to use this interface?
>
No immediate plans. I would really appreciate it if you or one
of the IBM developers working in this space took it on. Of course
if no one steps up I guess I can eventually get at it but it will
be sometime. For now I've been doing this manually with the bridge
tool yet to be published.
.John
^ permalink raw reply
* Re: [V2 PATCH 0/9] vhost/macvtap zeropcopy fixes
From: Eric W. Biederman @ 2012-05-02 21:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, jasowang, eric.dumazet, netdev, linux-kernel,
Basil Gor
In-Reply-To: <20120502213128.GB8266@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Wed, May 02, 2012 at 12:40:55PM -0700, Eric W. Biederman wrote:
>>
>> Side question. Are you aware that macvtap/vhost net is broken
>> in the presence of vlan accelleration and that vlan accelleration
>> is not optional if you are using vlan headers?
>>
>> Eric
>
> I didn't know. Could you explain please?
It is worth looking at the netdev history for some recent patches by
Basil Gor, as he has been hit by this issue and has been trying to come
up with a clean fix. I was burned by this issue in other parts of the
networking stack and so have been doing some basic review.
The short version is that on any normal path through the networking
stack we implement vlan header accelleration in hardware or emulation
in software. The result is that the ethernet vlan header is not
on the packet and is instead in the skb->tci field.
When coming on out the pf_packet sockets we don't include the vlan
header in the packet but instead the vlan header is put in aux data.
The result of all of this is that vhost/net.c looses the vlan
header when coming from pf_packet socket.
Furthermore macvtap_recvmsg does not act like pf_packet sockets
when there is a vlan header persent so weirdness ensues. Especially
when the packet is coming from a path where the vlan header has
been stripped and placed in skb->tci.
Eric
^ permalink raw reply
* Re: WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
From: Francois Romieu @ 2012-05-02 22:52 UTC (permalink / raw)
To: Alex Villacís Lasso; +Cc: netdev
In-Reply-To: <4FA18919.8010503@fiec.espol.edu.ec>
Alex Villacís Lasso <avillaci@fiec.espol.edu.ec> :
[...]
> Still present in 3.4-rc5.
Is it a 8168evl ?
--
Ueimor
^ permalink raw reply
* Re: bonding: don't increase rx_dropped after processing LACPDUs
From: Jay Vosburgh @ 2012-05-02 23:10 UTC (permalink / raw)
To: Jiri Bohac; +Cc: Eric Dumazet, Andy Gospodarek, netdev
In-Reply-To: <20120502205118.GB25355@midget.suse.cz>
Jiri Bohac <jbohac@suse.cz> wrote:
>On Wed, May 02, 2012 at 10:36:49PM +0200, Eric Dumazet wrote:
>> > + if (ret == RX_HANDLER_CONSUMED)
>> > + kfree_skb(skb);
>>
>> After this point, you have use after free :
>>
>> if (bond_should_deliver_exact_match(skb, slave, bond)) {
>> ...
>> }
>> skb->dev = bond->dev;
>
>Thanks for spotting this! Let's just return immediately at that
>point. Fixed version below:
Won't this make it impossible to bind a PF_PACKET socket to
sll_protocol == ETH_P_SLOW and see the LACPDUs, but only when bonding is
running 802.3ad? This because the ptype_all check in
__netif_receive_skb happens before the rx_handler, but the ptype_base
check (bound packet socket, for example) happens after. Currently,
libpcap looks to bind to ETH_P_ALL, so it won't be affected.
If so, is that something we care about?
-J
>Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -2173,9 +2173,10 @@ re_arm:
> * received frames (loopback). Since only the payload is given to this
> * function, it check for loopback.
> */
>-static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
>+static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
> {
> struct port *port;
>+ int ret = RX_HANDLER_ANOTHER;
>
> if (length >= sizeof(struct lacpdu)) {
>
>@@ -2184,11 +2185,12 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> if (!port->slave) {
> pr_warning("%s: Warning: port of slave %s is uninitialized\n",
> slave->dev->name, slave->dev->master->name);
>- return;
>+ return ret;
> }
>
> switch (lacpdu->subtype) {
> case AD_TYPE_LACPDU:
>+ ret = RX_HANDLER_CONSUMED;
> pr_debug("Received LACPDU on port %d\n",
> port->actor_port_number);
> /* Protect against concurrent state machines */
>@@ -2198,6 +2200,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> break;
>
> case AD_TYPE_MARKER:
>+ ret = RX_HANDLER_CONSUMED;
> // No need to convert fields to Little Endian since we don't use the marker's fields.
>
> switch (((struct bond_marker *)lacpdu)->tlv_type) {
>@@ -2219,6 +2222,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> }
> }
> }
>+ return ret;
> }
>
> /**
>@@ -2456,18 +2460,20 @@ out:
> return NETDEV_TX_OK;
> }
>
>-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> struct slave *slave)
> {
>+ int ret = RX_HANDLER_ANOTHER;
> if (skb->protocol != PKT_TYPE_LACPDU)
>- return;
>+ return ret;
>
> if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
>- return;
>+ return ret;
>
> read_lock(&bond->lock);
>- bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
>+ ret = bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
> read_unlock(&bond->lock);
>+ return ret;
> }
>
> /*
>diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
>index 235b2cc..5ee7e3c 100644
>--- a/drivers/net/bonding/bond_3ad.h
>+++ b/drivers/net/bonding/bond_3ad.h
>@@ -274,7 +274,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
> void bond_3ad_handle_link_change(struct slave *slave, char link);
> int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
>-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> struct slave *slave);
> int bond_3ad_set_carrier(struct bonding *bond);
> void bond_3ad_update_lacp_rate(struct bonding *bond);
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 62d2409..0a0f4a6 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1444,8 +1444,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> struct sk_buff *skb = *pskb;
> struct slave *slave;
> struct bonding *bond;
>- void (*recv_probe)(struct sk_buff *, struct bonding *,
>+ int (*recv_probe)(struct sk_buff *, struct bonding *,
> struct slave *);
>+ int ret = RX_HANDLER_ANOTHER;
>
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (unlikely(!skb))
>@@ -1464,8 +1465,12 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>
> if (likely(nskb)) {
>- recv_probe(nskb, bond, slave);
>+ ret = recv_probe(nskb, bond, slave);
> dev_kfree_skb(nskb);
>+ if (ret == RX_HANDLER_CONSUMED) {
>+ kfree_skb(skb);
>+ return ret;
>+ }
> }
> }
>
>@@ -1487,7 +1492,7 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
> }
>
>- return RX_HANDLER_ANOTHER;
>+ return ret;
> }
>
> /* enslave device <slave> to bond device <master> */
>@@ -2723,7 +2728,7 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
> }
> }
>
>-static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>+static int bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
> struct slave *slave)
> {
> struct arphdr *arp;
>@@ -2731,7 +2736,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
> __be32 sip, tip;
>
> if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
>- return;
>+ return RX_HANDLER_ANOTHER;
>
> read_lock(&bond->lock);
>
>@@ -2776,6 +2781,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>
> out_unlock:
> read_unlock(&bond->lock);
>+ return RX_HANDLER_ANOTHER;
> }
>
> /*
>
>
>--
>Jiri Bohac <jbohac@suse.cz>
>SUSE Labs, SUSE CZ
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* Re: WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
From: Alex Villacís Lasso @ 2012-05-02 23:28 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev
In-Reply-To: <20120502225227.GA12309@electric-eye.fr.zoreil.com>
El 02/05/12 17:52, Francois Romieu escribió:
> Alex Villacís Lasso<avillaci@fiec.espol.edu.ec> :
> [...]
>> Still present in 3.4-rc5.
> Is it a 8168evl ?
>
I am currently away from the target computer. How should I check for this? lspci?
^ 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