* Re: [PATCH 05/19] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Jesper Dangaard Brouer @ 2012-08-19 19:37 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netfilter-devel, netdev
In-Reply-To: <Pine.GSO.4.63.1208181423050.17600@stinky-local.trash.net>
On Sat, 2012-08-18 at 14:26 +0200, Patrick McHardy wrote:
> On Fri, 17 Aug 2012, Jesper Dangaard Brouer wrote:
>
> > On Thu, 2012-08-09 at 22:08 +0200, kaber@trash.net wrote:
> >> From: Patrick McHardy <kaber@trash.net>
> >>
> >> The IPv6 conntrack fragmentation currently has a couple of shortcomings.
> >> Fragmentes are collected in PREROUTING/OUTPUT, are defragmented, the
> >> defragmented packet is then passed to conntrack, the resulting conntrack
> >> information is attached to each original fragment and the fragments then
> >> continue their way through the stack.
> >>
> >> Helper invocation occurs in the POSTROUTING hook, at which point only
> >> the original fragments are available. The result of this is that
> >> fragmented packets are never passed to helpers.
> >>
> >> This patch improves the situation in the following way:
> >>
> >> - If a reassembled packet belongs to a connection that has a helper
> >> assigned, the reassembled packet is passed through the stack instead
> >> of the original fragments.
> >
> > I'm working on IPv6 fragment handling for IPVS, and are taking advantage
> > of the "replay" by nf_ct_frag6_output() at hook prio -399
> > (NF_IP6_PRI_CONNTRACK_DEFRAG + 1).
> > By making a hook at NF_INET_PRE_ROUTING at prio -99 (NF_IP6_PRI_NAT_DST
> > + 1).
> >
> > I can see that the code path can be changed (with this patch), if a
> > helper is assigned. Then the "replay" starts at prio -199
> > (NF_IP6_PRI_CONNTRACK + 1), I guess I'm safe as I run at -99.
> >
> > I have tested that your patchset works, with my ipvs patches, but would
> > like the trigger the changed code path, to make sure.
> >
> > Could you provide an iptables command/rule, that trigger this code path?
>
> The easiest way is a large ping with the NAT patches also applied,
> in that case we also pass the first packet of a connection through
> the stack reassembled.
So, a fragmented IPv6 ICMPv6 packet, I assume?
Don't I need to load some of the helper modules, or just the
nf_conntrack_ipv6 module, or perhaps only nf_defrag_ipv6 ?
> >> @@ -199,9 +200,13 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
> >> static unsigned int __ipv6_conntrack_in(struct net *net,
> >> unsigned int hooknum,
> >> struct sk_buff *skb,
> >> + const struct net_device *in,
> >> + const struct net_device *out,
> >> int (*okfn)(struct sk_buff *))
> >> {
> >> struct sk_buff *reasm = skb->nfct_reasm;
> >> + struct nf_conn *ct;
> >> + enum ip_conntrack_info ctinfo;
> >>
> >> /* This packet is fragmented and has reassembled packet. */
> >> if (reasm) {
> >> @@ -213,6 +218,20 @@ static unsigned int __ipv6_conntrack_in(struct net *net,
> >> if (ret != NF_ACCEPT)
> >> return ret;
> >> }
> >> +
> >> + /* Conntrack helpers need the entire reassembled packet in the
> >> + * POST_ROUTING hook.
> >> + */
> >> + ct = nf_ct_get(reasm, &ctinfo);
> >> + if (ct != NULL && test_bit(IPS_HELPER_BIT, &ct->status)) {
> >> + nf_conntrack_get_reasm(skb);
> >> + NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, reasm,
> >> + (struct net_device *)in,
> >> + (struct net_device *)out,
> >> + okfn, NF_IP6_PRI_CONNTRACK + 1);
> >
> > Hook prio change to NF_IP6_PRI_CONNTRACK + 1
>
> I didn't get this part, you want to change to PRE_CONNTRACK + 1? What
> about raw and SELinux?
No - I don't want any changes.
I was just pointing out *where* the changes occur in your patch. This is
just a "service" to other email readers, so they can spot the changes
faster, I were referring to.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: regression with poll(2)
From: Sage Weil @ 2012-08-19 18:49 UTC (permalink / raw)
To: mgorman, davem, netdev
Cc: linux-kernel, ceph-devel, neilb, a.p.zijlstra, michaelc, emunson,
eric.dumazet, sebastian, cl, akpm, torvalds
In-Reply-To: <alpine.DEB.2.00.1208151226250.15721@cobra.newdream.net>
I've bisected and identified this commit:
netvm: propagate page->pfmemalloc to skb
The skb->pfmemalloc flag gets set to true iff during the slab allocation
of data in __alloc_skb that the the PFMEMALLOC reserves were used. If the
packet is fragmented, it is possible that pages will be allocated from the
PFMEMALLOC reserve without propagating this information to the skb. This
patch propagates page->pfmemalloc from pages allocated for fragments to
the skb.
Signed-off-by: Mel Gorman <mgorman@suse.de>
Acked-by: David S. Miller <davem@davemloft.net>
Cc: Neil Brown <neilb@suse.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Eric B Munson <emunson@mgebm.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
I've retested several times and confirmed that this change leads to the
breakage, and also confirmed that reverting it on top of -rc1 also fixes
the problem.
I've also added some additional instrumentation to my code and confirmed
that the process is blocking on poll(2) while netstat is reporting
data available on the socket.
What can I do to help track this down?
Thanks!
sage
On Wed, 15 Aug 2012, Sage Weil wrote:
> I'm experiencing a stall with Ceph daemons communicating over TCP that
> occurs reliably with 3.6-rc1 (and linus/master) but not 3.5. The basic
> situation is:
>
> - the socket is two processes communicating over TCP on the same host, e.g.
>
> tcp 0 2164849 10.214.132.38:6801 10.214.132.38:51729 ESTABLISHED
>
> - one end writes a bunch of data in
> - the other end consumes data, but at some point stalls.
> - reads are nonblocking, e.g.
>
> int got = ::recv( sd, buf, len, MSG_DONTWAIT );
>
> and between those calls we wait with
>
> struct pollfd pfd;
> short evmask;
> pfd.fd = sd;
> pfd.events = POLLIN;
> #if defined(__linux__)
> pfd.events |= POLLRDHUP;
> #endif
>
> if (poll(&pfd, 1, msgr->timeout) <= 0)
> return -1;
>
> - in my case the timeout is ~15 minutes. at that point it errors out,
> and the daemons reconnect and continue for a while until hitting this
> again.
>
> - at the time of the stall, the reading process is blocked on that
> poll(2) call. There are a bunch of threads stuck on poll(2), some of them
> stuck and some not, but they all have stacks like
>
> [<ffffffff8118f6f9>] poll_schedule_timeout+0x49/0x70
> [<ffffffff81190baf>] do_sys_poll+0x35f/0x4c0
> [<ffffffff81190deb>] sys_poll+0x6b/0x100
> [<ffffffff8163d369>] system_call_fastpath+0x16/0x1b
>
> - you'll note that the netstat output shows data queued:
>
> tcp 0 1163264 10.214.132.36:6807 10.214.132.36:41738 ESTABLISHED
> tcp 0 1622016 10.214.132.36:41738 10.214.132.36:6807 ESTABLISHED
>
> etc.
>
> Is this a known regression? Or might I be misusing the API? What
> information would help track it down?
>
> Thanks!
> sage
>
>
>
^ permalink raw reply
* [PATCH] serial: add a new helper function
From: Huang Shijie @ 2012-08-19 18:27 UTC (permalink / raw)
To: gregkh; +Cc: alan, jirislaby, linux-kernel, linux-serial, netdev, Huang Shijie
In most of the time, the driver needs to check if the cts flow control
is enabled. But now, the driver checks the ASYNC_CTS_FLOW flag manually,
which is not a grace way. So add a new wraper function to make the code
tidy and clean.
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
drivers/char/pcmcia/synclink_cs.c | 2 +-
drivers/tty/amiserial.c | 2 +-
drivers/tty/cyclades.c | 2 +-
drivers/tty/isicom.c | 2 +-
drivers/tty/mxser.c | 2 +-
drivers/tty/serial/mxs-auart.c | 2 +-
drivers/tty/serial/serial_core.c | 4 ++--
drivers/tty/synclink.c | 2 +-
drivers/tty/synclink_gt.c | 2 +-
drivers/tty/synclinkmp.c | 2 +-
include/linux/tty.h | 7 +++++++
net/irda/ircomm/ircomm_tty.c | 4 ++--
net/irda/ircomm/ircomm_tty_attach.c | 2 +-
13 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 5db08c7..3f57d5de 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -1050,7 +1050,7 @@ static void cts_change(MGSLPC_INFO *info, struct tty_struct *tty)
wake_up_interruptible(&info->status_event_wait_q);
wake_up_interruptible(&info->event_wait_q);
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (info->serial_signals & SerialSignal_CTS) {
if (debug_level >= DEBUG_LEVEL_ISR)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 2b7535d..42d0a25 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -420,7 +420,7 @@ static void check_modem_status(struct serial_state *info)
tty_hangup(port->tty);
}
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (port->tty->hw_stopped) {
if (!(status & SER_CTS)) {
#if (defined(SERIAL_DEBUG_INTR) || defined(SERIAL_DEBUG_FLOW))
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index e3954da..0a6a0bc 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -727,7 +727,7 @@ static void cyy_chip_modem(struct cyclades_card *cinfo, int chip,
else
tty_hangup(tty);
}
- if ((mdm_change & CyCTS) && (info->port.flags & ASYNC_CTS_FLOW)) {
+ if ((mdm_change & CyCTS) && tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (mdm_status & CyCTS) {
/* cy_start isn't used
diff --git a/drivers/tty/isicom.c b/drivers/tty/isicom.c
index 99cf22e..d7492e1 100644
--- a/drivers/tty/isicom.c
+++ b/drivers/tty/isicom.c
@@ -600,7 +600,7 @@ static irqreturn_t isicom_interrupt(int irq, void *dev_id)
port->status &= ~ISI_DCD;
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (header & ISI_CTS) {
port->port.tty->hw_stopped = 0;
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index bb2da4c..cfda47d 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -830,7 +830,7 @@ static void mxser_check_modem_status(struct tty_struct *tty,
wake_up_interruptible(&port->port.open_wait);
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (status & UART_MSR_CTS) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 3a667ee..dafeef2 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -262,7 +262,7 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
ctrl &= ~AUART_CTRL2_RTSEN;
if (mctrl & TIOCM_RTS) {
- if (u->state->port.flags & ASYNC_CTS_FLOW)
+ if (tty_port_cts_enabled(&u->state->port))
ctrl |= AUART_CTRL2_RTSEN;
}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5b308c8..1920e5c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -176,7 +176,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
spin_lock_irq(&uport->lock);
if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
tty->hw_stopped = 1;
@@ -2493,7 +2493,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
uport->icount.cts++;
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (tty->hw_stopped) {
if (status) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index 666aa14..70e3a52 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -1359,7 +1359,7 @@ static void mgsl_isr_io_pin( struct mgsl_struct *info )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if (info->port.tty->hw_stopped) {
if (status & MISCSTATUS_CTS) {
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 45f6136..b38e954 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -2053,7 +2053,7 @@ static void cts_change(struct slgt_info *info, unsigned short status)
wake_up_interruptible(&info->event_wait_q);
info->pending_bh |= BH_STATUS;
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (info->port.tty) {
if (info->port.tty->hw_stopped) {
if (info->signals & SerialSignal_CTS) {
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index 53429c8..f17d9f3 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -2500,7 +2500,7 @@ static void isr_io_pin( SLMP_INFO *info, u16 status )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if ( info->port.tty ) {
if (info->port.tty->hw_stopped) {
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 69a787f..e0b4871 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -43,6 +43,7 @@
#include <linux/tty_driver.h>
#include <linux/tty_ldisc.h>
#include <linux/mutex.h>
+#include <linux/serial.h>
@@ -513,6 +514,12 @@ static inline struct tty_port *tty_port_get(struct tty_port *port)
return port;
}
+/* If the cts flow control is enabled, return true. */
+static inline bool tty_port_cts_enabled(struct tty_port *port)
+{
+ return port->flags & ASYNC_CTS_FLOW;
+}
+
extern struct tty_struct *tty_port_tty_get(struct tty_port *port);
extern void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty);
extern int tty_port_carrier_raised(struct tty_port *port);
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9668990..95a3a7a 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -1070,7 +1070,7 @@ void ircomm_tty_check_modem_status(struct ircomm_tty_cb *self)
goto put;
}
}
- if (tty && self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty && tty_port_cts_enabled(&self->port)) {
if (tty->hw_stopped) {
if (status & IRCOMM_CTS) {
IRDA_DEBUG(2,
@@ -1313,7 +1313,7 @@ static void ircomm_tty_line_info(struct ircomm_tty_cb *self, struct seq_file *m)
seq_puts(m, "Flags:");
sep = ' ';
- if (self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&self->port)) {
seq_printf(m, "%cASYNC_CTS_FLOW", sep);
sep = '|';
}
diff --git a/net/irda/ircomm/ircomm_tty_attach.c b/net/irda/ircomm/ircomm_tty_attach.c
index 3ab70e7..edab393 100644
--- a/net/irda/ircomm/ircomm_tty_attach.c
+++ b/net/irda/ircomm/ircomm_tty_attach.c
@@ -578,7 +578,7 @@ void ircomm_tty_link_established(struct ircomm_tty_cb *self)
* will have to wait for the peer device (DCE) to raise the CTS
* line.
*/
- if ((self->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&self->port) &&
((self->settings.dce & IRCOMM_CTS) == 0)) {
IRDA_DEBUG(0, "%s(), waiting for CTS ...\n", __func__ );
goto put;
--
1.7.4.4
^ permalink raw reply related
* Re: BUG: unable to handle kernel paging request at 00010016
From: Shaun Ruffell @ 2012-08-19 17:19 UTC (permalink / raw)
To: Artem Savkov; +Cc: Dave Haywood, Linux Kernel, netdev
In-Reply-To: <20120819082117.GA4634@thinkpad.lan>
[ Fixing netdev cc to use proper dadress... ]
On Sun, Aug 19, 2012 at 12:21:17PM +0400, Artem Savkov wrote:
> On Sat, Aug 18, 2012 at 11:25:43PM -0500, Shaun Ruffell wrote:
> > Adding linux-net to the CC list.
> >
> > On Fri, Aug 17, 2012 at 11:57:56PM +0100, Dave Haywood wrote:
> > > [1.] One line summary of the problem:
> > > BUG: unable to handle kernel paging request at 00010016
> > >
> > > System boots then crashes a 5-10 or so seconds after getting to the login prompt
> > > Booting without the network cable attached prevents the crash (no evidence beyond 10 minutes after boot)
> > >
> > > Diagnostics:
> > > Captured the boot and managed a login + dmesg before the crash
> > > Some of the log looks corrupted. Probably my crappy usb dongle serial flow control but left it in anyway
> >
> > [snip]
> >
> > Just a note that I see this as well. It happens reliably for me after trying to
> > login to the machine via ssh.
> >
> > Here is the back trace I collected on the serial port.
>
> There is a patch posted on netdev that fixes this for me:
> http://patchwork.ozlabs.org/patch/178525/
Thanks, I just applied that patch and confirmed that it does indeed
resolve the crash for me as well.
Cheers,
Shaun
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Julian Anastasov @ 2012-08-19 16:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
Hello,
On Sun, 19 Aug 2012, Eric Dumazet wrote:
> Hmm, this looks like sk_reset_timer() is called on a socket, and timer
> triggers _before_ the sock_hold()
>
> So the timer handler decrements sk_refcnt to 0 and calls sk_free()
>
> Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp:
> improve latencies of timer triggered events)
>
> I always found sk_reset_timer() a bit racy...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> if (!mod_timer(timer, expires))
> sock_hold(sk); // MIGHT BE TOO LATE
> }
>
> Following should be safer...
Above code is fine as long as caller holds reference.
Your change for tcp_release_cb looks correct.
Also, may be tcp_v4_mtu_reduced is missing a check
for TCP_CLOSE state? tcp_v6_mtu_reduced already has such
check.
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
This should not be needed:
> sock_hold(sk);
> if (mod_timer(timer, expires))
> sock_put(sk);
> }
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-19 16:17 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120819141641.GA9082@Krystal>
On 08/19/2012 04:16 PM, Mathieu Desnoyers wrote:
> * Mathieu Desnoyers (mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org) wrote:
>> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> [...]
>>> +/**
>>> + * hash_for_each_possible - iterate over all possible objects for a given key
>>> + * @name: hashtable to iterate
>>> + * @obj: the type * to use as a loop cursor for each bucket
>>> + * @bits: bit count of hashing function of the hashtable
>>> + * @node: the &struct list_head to use as a loop cursor for each bucket
>>> + * @member: the name of the hlist_node within the struct
>>> + * @key: the key of the objects to iterate over
>>> + */
>>> +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \
>>> + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member)
>>
>> Second point: "for_each_possible" does not express the iteration scope.
>> Citing WordNet: "possible adj 1: capable of happening or existing;" --
>> which has nothing to do with iteration on duplicate keys within a hash
>> table.
>>
>> I would recommend to rename "possible" to "duplicate", e.g.:
>>
>> hash_for_each_duplicate()
>>
>> which clearly says what is the scope of this iteration: duplicate keys.
>
> OK, about this part: I now see that you iterate over all objects within
> the same hash chain. I guess the description "iterate over all possible
> objects for a given key" is misleading: it's not all objects with a
> given key, but rather all objects hashing to the same bucket.
>
> I understand that you don't want to build knowledge of the key
> comparison function in the iterator (which makes sense for a simple hash
> table).
>
> By the way, the comment "@obj: the type * to use as a loop cursor for
> each bucket" is also misleading: it's a loop cursor for each entry,
> since you iterate on all nodes within single bucket. Same for "@node:
> the &struct list_head to use as a loop cursor for each bucket".
>
> So with these documentation changes applied, hash_for_each_possible
> starts to make more sense, because it refers to entries that can
> _possibly_ be a match (or not). Other options would be
> hash_chain_for_each() or hash_bucket_for_each().
I'd rather avoid starting to use chain/bucket since they're not used anywhere
else (I've tried keeping internal hashing/bucketing opaque to the user).
Otherwise makes sense, I'll improve the documentation as suggested. Thanks!
> Thanks,
>
> Mathieu
>
^ permalink raw reply
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-19 16:08 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120819131637.GA8272@Krystal>
On 08/19/2012 03:16 PM, Mathieu Desnoyers wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
>> This hashtable implementation is using hlist buckets to provide a simple
>> hashtable to prevent it from getting reimplemented all over the kernel.
>>
>> Signed-off-by: Sasha Levin <levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> include/linux/hashtable.h | 284 +++++++++++++++++++++++++++++++++++++++++++++
> [...]
>
> Hi Sasha,
>
> There are still a few API naming nits that I'd like to discuss:
>
>> +
>> +/**
>> + * hash_for_each_size - iterate over a hashtable
>> + * @name: hashtable to iterate
>> + * @bits: bit count of hashing function of the hashtable
>> + * @bkt: integer to use as bucket loop cursor
>> + * @node: the &struct list_head to use as a loop cursor for each bucket
>> + * @obj: the type * to use as a loop cursor for each bucket
>> + * @member: the name of the hlist_node within the struct
>> + */
>> +#define hash_for_each_size(name, bits, bkt, node, obj, member) \
>
> What is the meaning of "for each size" ?
>
> By looking at the implementation, I see that it takes an extra "bits"
> argument to specify the key width.
>
> But in the other patches of this patchset, I cannot find a single user
> of the "*_size" API. If you do not typically expect users to specify
> this parameter by hand (thanks to use of HASH_BITS(name) in for_each
> functions that do not take the bits parameter), I would recommend to
> only expose hash_for_each() and similar defines, but n I'd ot the *_size
> variants.
>
> So I recommend merging hash_for_each_size into hash_for_each (and
> doing similarly for other *_size variants). On the plus side, it will
> cut down the number of for_each macros from 12 down to 6, whiy och is more
> reasonable.
This is actually how the hashtable API was looking in the first place - without
the _size functions.
The story here is that when I've introduced the original API which used macro
magic to work out the size, one of the first comments was that there are several
dynamically allocated hashtables around the kernels, and all this macro magic
wouldn't work.
I've grepped around the code and indeed saw several places which k*alloc/vmalloc
their hashtable, so I agreed with that and happily went ahead to extend the API
to have _size functions.
When I started converting more kernel code to use this new API, I also converted
two modules which kmalloced the hashtable, but instead of using the _size API I
ended up removing the allocation completely because it was unnecessary and
wasteful. And this is why you don't see _size being used anywhere in any of the
patches.
Looking at the kernel code again, I see several places where removal of dynamic
allocation won't work (see 'new_tl_hash' in drivers/block/drbd/drbd_nl.c for
example).
So I'd rather leave it in at least until we finish converting, as I see several
places which will definitely need it.
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 15:55 UTC (permalink / raw)
To: Lin Ming; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <CAF1ivSZK8Y2XpjUXVpNg=m=bs7bGCYqF0ZQU2u_YpEUrmOrR-g@mail.gmail.com>
On Sun, 2012-08-19 at 23:05 +0800, Lin Ming wrote:
> On Sun, Aug 19, 2012 at 10:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
> >
> >> Will it still has problem if code goes here without sock_hold(sk)?
> >
> > Not sure of what you mean.
>
> See my comments in the function.
> Is that a potential problem?
>
No problem.
It always been like that. Thats the whole point having a refcount at the
first place.
The last sock_put(sk) should free the socket.
^ permalink raw reply
* Re: [PATCH] serial: add a new helper function
From: Alan Cox @ 2012-08-19 15:46 UTC (permalink / raw)
To: Greg KH; +Cc: Huang Shijie, alan, jirislaby, linux-kernel, linux-serial, netdev
In-Reply-To: <20120819064429.GA3252@kroah.com>
On Sat, 18 Aug 2012 23:44:29 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sun, Aug 19, 2012 at 02:27:12PM -0400, Huang Shijie wrote:
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -43,6 +43,7 @@
> > #include <linux/tty_driver.h>
> > #include <linux/tty_ldisc.h>
> > #include <linux/mutex.h>
> > +#include <linux/serial.h>
> >
> >
> >
> > @@ -513,6 +514,12 @@ static inline struct tty_port *tty_port_get(struct tty_port *port)
> > return port;
> > }
> >
> > +/* If the cts flow control is enabled, return true. */
> > +static inline bool tty_port_cts_enabled(struct tty_port *port)
> > +{
> > + return port->flags & ASYNC_CTS_FLOW;
> > +}
> > +
>
> The fact that you have to add serial.h to this file kind of implies that
> this function shouldn't be here, right?
>
> How about serial.h instead? Not all tty drivers are serial drivers :)
tty_port is tty generic so possibly if there is a generic helper the
flags and helper should likewise be this way.
As it stands at the moment ASYNC_CTS_FLOW is a convention a few drivers
use. So calling it tty_port_xxx is going to misleading.
Alan
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Lin Ming @ 2012-08-19 15:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345387532.5158.222.camel@edumazet-glaptop>
On Sun, Aug 19, 2012 at 10:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
>
>> Will it still has problem if code goes here without sock_hold(sk)?
>
> Not sure of what you mean.
See my comments in the function.
Is that a potential problem?
static void tcp_write_timer(unsigned long data)
{
struct sock *sk = (struct sock *)data;
bh_lock_sock(sk);
if (!sock_owned_by_user(sk)) {
tcp_write_timer_handler(sk);
//if we arrive here with sk_refcnt=1
//then below sock_put(sk) will also free the sock.
} else {
/* deleguate our work to tcp_release_cb() */
set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
}
bh_unlock_sock(sk);
sock_put(sk);
}
>
> At the time tcp_write_timer() runs, we own one reference on the socket.
> (this reference was taken in sk_reset_timer())
>
> On old kernels, if we found the socket locked by the user, we used to
> rearm the timer for a 50ms delay (and thus did sock_hold() again)
>
> Another way to avoid the bug would to make sure sk_reset_timer()
> increases refcount _before_ setting the timer, but its adding one atomic
> in fast path...
I think this is safer than your previous fix.
Thanks.
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8f67ced..d1745b7 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2114,8 +2114,9 @@ EXPORT_SYMBOL(sk_send_sigurg);
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> - if (!mod_timer(timer, expires))
> - sock_hold(sk);
> + sock_hold(sk);
> + if (mod_timer(timer, expires))
> + __sock_put(sk);
> }
> EXPORT_SYMBOL(sk_reset_timer);
>
>
>
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 14:45 UTC (permalink / raw)
To: Lin Ming; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <CAF1ivSaU_ur9BkKG3eAsNw7d+ND5xgYbPKutemVqNec3X9m4SA@mail.gmail.com>
On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
> Will it still has problem if code goes here without sock_hold(sk)?
Not sure of what you mean.
At the time tcp_write_timer() runs, we own one reference on the socket.
(this reference was taken in sk_reset_timer())
On old kernels, if we found the socket locked by the user, we used to
rearm the timer for a 50ms delay (and thus did sock_hold() again)
Another way to avoid the bug would to make sure sk_reset_timer()
increases refcount _before_ setting the timer, but its adding one atomic
in fast path...
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..d1745b7 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2114,8 +2114,9 @@ EXPORT_SYMBOL(sk_send_sigurg);
void sk_reset_timer(struct sock *sk, struct timer_list* timer,
unsigned long expires)
{
- if (!mod_timer(timer, expires))
- sock_hold(sk);
+ sock_hold(sk);
+ if (mod_timer(timer, expires))
+ __sock_put(sk);
}
EXPORT_SYMBOL(sk_reset_timer);
^ permalink raw reply related
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-08-19 14:16 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120819131637.GA8272@Krystal>
* Mathieu Desnoyers (mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org) wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
[...]
> > +/**
> > + * hash_for_each_possible - iterate over all possible objects for a given key
> > + * @name: hashtable to iterate
> > + * @obj: the type * to use as a loop cursor for each bucket
> > + * @bits: bit count of hashing function of the hashtable
> > + * @node: the &struct list_head to use as a loop cursor for each bucket
> > + * @member: the name of the hlist_node within the struct
> > + * @key: the key of the objects to iterate over
> > + */
> > +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \
> > + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member)
>
> Second point: "for_each_possible" does not express the iteration scope.
> Citing WordNet: "possible adj 1: capable of happening or existing;" --
> which has nothing to do with iteration on duplicate keys within a hash
> table.
>
> I would recommend to rename "possible" to "duplicate", e.g.:
>
> hash_for_each_duplicate()
>
> which clearly says what is the scope of this iteration: duplicate keys.
OK, about this part: I now see that you iterate over all objects within
the same hash chain. I guess the description "iterate over all possible
objects for a given key" is misleading: it's not all objects with a
given key, but rather all objects hashing to the same bucket.
I understand that you don't want to build knowledge of the key
comparison function in the iterator (which makes sense for a simple hash
table).
By the way, the comment "@obj: the type * to use as a loop cursor for
each bucket" is also misleading: it's a loop cursor for each entry,
since you iterate on all nodes within single bucket. Same for "@node:
the &struct list_head to use as a loop cursor for each bucket".
So with these documentation changes applied, hash_for_each_possible
starts to make more sense, because it refers to entries that can
_possibly_ be a match (or not). Other options would be
hash_chain_for_each() or hash_bucket_for_each().
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Lin Ming @ 2012-08-19 14:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
On Sun, Aug 19, 2012 at 8:51 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
<snip...>
> Hi Fengguang, thanks for this report.
>
> Hmm, this looks like sk_reset_timer() is called on a socket, and timer
> triggers _before_ the sock_hold()
>
> So the timer handler decrements sk_refcnt to 0 and calls sk_free()
>
> Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp:
> improve latencies of timer triggered events)
>
> I always found sk_reset_timer() a bit racy...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> if (!mod_timer(timer, expires))
> sock_hold(sk); // MIGHT BE TOO LATE
> }
>
> Following should be safer...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> sock_hold(sk);
> if (mod_timer(timer, expires))
> sock_put(sk);
> }
>
> Could you test following patch ?
>
> By the way, there is a typo in tcp_delack_timer()
> (should use TCP_DELACK_TIMER_DEFERRED instead of
> TCP_WRITE_TIMER_DEFERRED)
>
>
> Thanks !
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 7678237..6278a11 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -417,10 +417,12 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
>
> if (code == ICMP_FRAG_NEEDED) { /* PMTU discovery (RFC1191) */
> tp->mtu_info = info;
> - if (!sock_owned_by_user(sk))
> + if (!sock_owned_by_user(sk)) {
> tcp_v4_mtu_reduced(sk);
> - else
> - set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags);
> + } else {
> + if (!test_and_set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags))
> + sock_hold(sk);
> + }
> goto out;
> }
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 20dfd89..d046326 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -910,14 +910,18 @@ void tcp_release_cb(struct sock *sk)
> if (flags & (1UL << TCP_TSQ_DEFERRED))
> tcp_tsq_handler(sk);
>
> - if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED))
> + if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) {
> tcp_write_timer_handler(sk);
> -
> - if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED))
> + __sock_put(sk);
> + }
> + if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) {
> tcp_delack_timer_handler(sk);
> -
> - if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED))
> + __sock_put(sk);
> + }
> + if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) {
> sk->sk_prot->mtu_reduced(sk);
> + __sock_put(sk);
> + }
> }
> EXPORT_SYMBOL(tcp_release_cb);
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 6df36ad..b774a03 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -252,7 +252,8 @@ static void tcp_delack_timer(unsigned long data)
> inet_csk(sk)->icsk_ack.blocked = 1;
> NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOCKED);
> /* deleguate our work to tcp_release_cb() */
> - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
> + if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
> + sock_hold(sk);
> }
> bh_unlock_sock(sk);
> sock_put(sk);
> @@ -481,7 +482,8 @@ static void tcp_write_timer(unsigned long data)
> tcp_write_timer_handler(sk);
Will it still has problem if code goes here without sock_hold(sk)?
> } else {
> /* deleguate our work to tcp_release_cb() */
> - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
> + if (!test_and_set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
> + sock_hold(sk);
> }
> bh_unlock_sock(sk);
> sock_put(sk);
>
^ permalink raw reply
* Re: [RFC] Interface for TCP Metrics
From: Eric Dumazet @ 2012-08-19 13:50 UTC (permalink / raw)
To: Julian Anastasov; +Cc: netdev
In-Reply-To: <alpine.LFD.2.00.1208191402560.1893@ja.ssi.bg>
On Sun, 2012-08-19 at 14:42 +0300, Julian Anastasov wrote:
> Hello,
>
> Once DaveM mentioned that TCP Metrics need their
> own interface. I'm planning to implement such interface
> but lets first decide how it should look. After little
> research, here is my plan:
>
> - will use genl with TCP_METRICS_GENL_NAME "tcp_metrics",
> TCP_METRICS_GENL_VERSION 0x01
>
> - provide dumpit method and one cmd to read metrics by exact addr,
> will use TCP_METRICS_CMD_{GET,...} and TCP_METRICS_ATTR_xxx in
> new file include/linux/tcp_metrics.h
>
> - Is command to delete cached entry needed? Delete will need
> new rcu_head. Useful to flush the cache or to delete entries
> with filter.
>
> - without support for delete cmd, may be we can add command to
> reset entry with default values from dst?
>
> - Where to put the new netlink code?
> tcp_metrics_netlink.c
> tcp_metrics_nl.c
> or just in current tcp_metrics.c ?
>
> - will provide support for ip tool:
>
> ip tcpm[etrics] <cmd> ...
> ip -6 tcpm[etrics] get <addr> => TCP_METRICS_CMD_GET for 1 entry
> ip tcpm[etrics] list => dumpit
>
> - any wishes how to look the output? How to select specific
> values/metrics in output? For example, list only specified
> named values: ip tcpm list [-o] rtt
>
> - command to modify specific metric for addr, by name? Only
> for tcpm_vals? If not locked?
>
> Do we need modify/delete/reset support or just read
> support is enough? Comments?
>
> Regards
One way to delete one or all entries would be good, for being able to
reproduce some tests from a known state.
^ permalink raw reply
* [PATCH net-next] ipv6: gre: fix ip6gre_err()
From: Eric Dumazet @ 2012-08-19 13:47 UTC (permalink / raw)
To: Dmitry Kozlov; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
ip6gre_err() miscomputes grehlen (sizeof(ipv6h) is 4 or 8,
not 40 as expected), and should take into account 'offset' parameter.
Also uses pskb_may_pull() to cope with some fragged skbs
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Dmitry Kozlov <xeb@mail.ru>
---
net/ipv6/ip6_gre.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index a84ad5d..424d11a 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -415,8 +415,8 @@ static void ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
u8 type, u8 code, int offset, __be32 info)
{
const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)skb->data;
- __be16 *p = (__be16 *)(ipv6h + 1);
- int grehlen = sizeof(ipv6h) + 4;
+ __be16 *p = (__be16 *)(skb->data + offset);
+ int grehlen = offset + 4;
struct ip6_tnl *t;
__be16 flags;
@@ -432,8 +432,10 @@ static void ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
}
/* If only 8 bytes returned, keyed message will be dropped here */
- if (skb_headlen(skb) < grehlen)
+ if (!pskb_may_pull(skb, grehlen))
return;
+ ipv6h = (const struct ipv6hdr *)skb->data;
+ p = (__be16 *)(skb->data + offset);
rcu_read_lock();
^ permalink raw reply related
* Re: [PATCH net v2] net: tcp: move sk_rx_dst_set call after tcp_create_openreq_child()
From: Eric Dumazet @ 2012-08-19 13:41 UTC (permalink / raw)
To: Neal Cardwell; +Cc: David Miller, netdev, Eric Dumazet
In-Reply-To: <1345383038-8330-1-git-send-email-ncardwell@google.com>
On Sun, 2012-08-19 at 09:30 -0400, Neal Cardwell wrote:
> This commit removes the sk_rx_dst_set calls from
> tcp_create_openreq_child(), because at that point the icsk_af_ops
> field of ipv6_mapped TCP sockets has not been set to its proper final
> value.
>
> Instead, to make sure we get the right sk_rx_dst_set variant
> appropriate for the address family of the new connection, we have
> tcp_v{4,6}_syn_recv_sock() directly call the appropriate function
> shortly after the call to tcp_create_openreq_child() returns.
>
> This also moves inet6_sk_rx_dst_set() to avoid a forward declaration
> with the new approach.
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Reported-by: Artem Savkov <artem.savkov@gmail.com>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
> net/ipv4/tcp_ipv4.c | 1 +
> net/ipv4/tcp_minisocks.c | 2 --
> net/ipv6/tcp_ipv6.c | 25 +++++++++++++------------
> 3 files changed, 14 insertions(+), 14 deletions(-)
Thanks Neal !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [PATCH net v2] net: tcp: move sk_rx_dst_set call after tcp_create_openreq_child()
From: Neal Cardwell @ 2012-08-19 13:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Eric Dumazet, Neal Cardwell
This commit removes the sk_rx_dst_set calls from
tcp_create_openreq_child(), because at that point the icsk_af_ops
field of ipv6_mapped TCP sockets has not been set to its proper final
value.
Instead, to make sure we get the right sk_rx_dst_set variant
appropriate for the address family of the new connection, we have
tcp_v{4,6}_syn_recv_sock() directly call the appropriate function
shortly after the call to tcp_create_openreq_child() returns.
This also moves inet6_sk_rx_dst_set() to avoid a forward declaration
with the new approach.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reported-by: Artem Savkov <artem.savkov@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_ipv4.c | 1 +
net/ipv4/tcp_minisocks.c | 2 --
net/ipv6/tcp_ipv6.c | 25 +++++++++++++------------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7678237..5bf2040 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1462,6 +1462,7 @@ struct sock *tcp_v4_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
goto exit_nonewsk;
newsk->sk_gso_type = SKB_GSO_TCPV4;
+ inet_sk_rx_dst_set(newsk, skb);
newtp = tcp_sk(newsk);
newinet = inet_sk(newsk);
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index d9c9dce..6ff7f10 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -387,8 +387,6 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
struct tcp_sock *oldtp = tcp_sk(sk);
struct tcp_cookie_values *oldcvp = oldtp->cookie_values;
- newicsk->icsk_af_ops->sk_rx_dst_set(newsk, skb);
-
/* TCP Cookie Transactions require space for the cookie pair,
* as it differs for each connection. There is no need to
* copy any s_data_payload stored at the original socket.
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index bb9ce2b..a3e60cc 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -94,6 +94,18 @@ static struct tcp_md5sig_key *tcp_v6_md5_do_lookup(struct sock *sk,
}
#endif
+static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
+{
+ struct dst_entry *dst = skb_dst(skb);
+ const struct rt6_info *rt = (const struct rt6_info *)dst;
+
+ dst_hold(dst);
+ sk->sk_rx_dst = dst;
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
+ if (rt->rt6i_node)
+ inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
+}
+
static void tcp_v6_hash(struct sock *sk)
{
if (sk->sk_state != TCP_CLOSE) {
@@ -1270,6 +1282,7 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
newsk->sk_gso_type = SKB_GSO_TCPV6;
__ip6_dst_store(newsk, dst, NULL, NULL);
+ inet6_sk_rx_dst_set(newsk, skb);
newtcp6sk = (struct tcp6_sock *)newsk;
inet_sk(newsk)->pinet6 = &newtcp6sk->inet6;
@@ -1729,18 +1742,6 @@ static struct timewait_sock_ops tcp6_timewait_sock_ops = {
.twsk_destructor= tcp_twsk_destructor,
};
-static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
-{
- struct dst_entry *dst = skb_dst(skb);
- const struct rt6_info *rt = (const struct rt6_info *)dst;
-
- dst_hold(dst);
- sk->sk_rx_dst = dst;
- inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
- if (rt->rt6i_node)
- inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
-}
-
static const struct inet_connection_sock_af_ops ipv6_specific = {
.queue_xmit = inet6_csk_xmit,
.send_check = tcp_v6_send_check,
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-08-19 13:16 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <1345337550-24304-2-git-send-email-levinsasha928@gmail.com>
* Sasha Levin (levinsasha928@gmail.com) wrote:
> This hashtable implementation is using hlist buckets to provide a simple
> hashtable to prevent it from getting reimplemented all over the kernel.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
> include/linux/hashtable.h | 284 +++++++++++++++++++++++++++++++++++++++++++++
[...]
Hi Sasha,
There are still a few API naming nits that I'd like to discuss:
> +
> +/**
> + * hash_for_each_size - iterate over a hashtable
> + * @name: hashtable to iterate
> + * @bits: bit count of hashing function of the hashtable
> + * @bkt: integer to use as bucket loop cursor
> + * @node: the &struct list_head to use as a loop cursor for each bucket
> + * @obj: the type * to use as a loop cursor for each bucket
> + * @member: the name of the hlist_node within the struct
> + */
> +#define hash_for_each_size(name, bits, bkt, node, obj, member) \
What is the meaning of "for each size" ?
By looking at the implementation, I see that it takes an extra "bits"
argument to specify the key width.
But in the other patches of this patchset, I cannot find a single user
of the "*_size" API. If you do not typically expect users to specify
this parameter by hand (thanks to use of HASH_BITS(name) in for_each
functions that do not take the bits parameter), I would recommend to
only expose hash_for_each() and similar defines, but not the *_size
variants.
So I recommend merging hash_for_each_size into hash_for_each (and
doing similarly for other *_size variants). On the plus side, it will
cut down the number of for_each macros from 12 down to 6, which is more
reasonable.
> + for (bkt = 0; bkt < HASH_SIZE(bits); bkt++) \
> + hlist_for_each_entry(obj, node, &name[bkt], member)
> +
> +/**
> + * hash_for_each - iterate over a hashtable
> + * @name: hashtable to iterate
> + * @bkt: integer to use as bucket loop cursor
> + * @node: the &struct list_head to use as a loop cursor for each bucket
> + * @obj: the type * to use as a loop cursor for each bucket
> + * @member: the name of the hlist_node within the struct
> + */
> +#define hash_for_each(name, bkt, node, obj, member) \
> + hash_for_each_size(name, HASH_BITS(name), bkt, node, obj, member)
> +
[...]
> +/**
> + * hash_for_each_possible - iterate over all possible objects for a given key
> + * @name: hashtable to iterate
> + * @obj: the type * to use as a loop cursor for each bucket
> + * @bits: bit count of hashing function of the hashtable
> + * @node: the &struct list_head to use as a loop cursor for each bucket
> + * @member: the name of the hlist_node within the struct
> + * @key: the key of the objects to iterate over
> + */
> +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \
> + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member)
Second point: "for_each_possible" does not express the iteration scope.
Citing WordNet: "possible adj 1: capable of happening or existing;" --
which has nothing to do with iteration on duplicate keys within a hash
table.
I would recommend to rename "possible" to "duplicate", e.g.:
hash_for_each_duplicate()
which clearly says what is the scope of this iteration: duplicate keys.
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Fengguang Wu @ 2012-08-19 13:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
> Could you test following patch ?
Yes, sure. Thanks for the quick fix! I should have reported it earlier
(so as to get it fixed earlier) once find the bisect will be time consuming..
Please be patient -- it will take 1-2 days to confirm the bug fix.
Thanks,
Fengguang
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 12:51 UTC (permalink / raw)
To: Fengguang Wu; +Cc: David Miller, networking, LKML
In-Reply-To: <20120818021918.GA6499@localhost>
On Sat, 2012-08-18 at 10:19 +0800, Fengguang Wu wrote:
> Hi David,
>
> The bug should be introduced somewhere between 3.5 and 3.6-rc1.
>
> [ 2866.131281] IPv4: Attempt to release TCP socket in state 1 ffff880019ec0000
> [ 2866.131726]
> [ 2866.132188] =========================
> [ 2866.132281] [ BUG: held lock freed! ]
> [ 2866.132281] 3.6.0-rc1+ #622 Not tainted
> [ 2866.132281] -------------------------
> [ 2866.132281] kworker/0:1/652 is freeing memory ffff880019ec0000-ffff880019ec0a1f, with a lock still held there!
> [ 2866.132281] (sk_lock-AF_INET-RPC){+.+...}, at: [<ffffffff81903619>] tcp_sendmsg+0x29/0xcc6
> [ 2866.132281] 4 locks held by kworker/0:1/652:
> [ 2866.132281] #0: (rpciod){.+.+.+}, at: [<ffffffff81083567>] process_one_work+0x1de/0x47f
> [ 2866.132281] #1: ((&task->u.tk_work)){+.+.+.}, at: [<ffffffff81083567>] process_one_work+0x1de/0x47f
> [ 2866.132281] #2: (sk_lock-AF_INET-RPC){+.+...}, at: [<ffffffff81903619>] tcp_sendmsg+0x29/0xcc6
> [ 2866.132281] #3: (&icsk->icsk_retransmit_timer){+.-...}, at: [<ffffffff81078017>] run_timer_softirq+0x1ad/0x35f
> [ 2866.132281]
> [ 2866.132281] stack backtrace:
> [ 2866.132281] Pid: 652, comm: kworker/0:1 Not tainted 3.6.0-rc1+ #622
> [ 2866.132281] Call Trace:
> [ 2866.132281] <IRQ> [<ffffffff810bc527>] debug_check_no_locks_freed+0x112/0x159
> [ 2866.132281] [<ffffffff818a0839>] ? __sk_free+0xfd/0x114
> [ 2866.132281] [<ffffffff811549fa>] kmem_cache_free+0x6b/0x13a
> [ 2866.132281] [<ffffffff818a0839>] __sk_free+0xfd/0x114
> [ 2866.132281] [<ffffffff818a08c0>] sk_free+0x1c/0x1e
> [ 2866.132281] [<ffffffff81911e1c>] tcp_write_timer+0x51/0x56
> [ 2866.132281] [<ffffffff81078082>] run_timer_softirq+0x218/0x35f
> [ 2866.132281] [<ffffffff81078017>] ? run_timer_softirq+0x1ad/0x35f
> [ 2866.132281] [<ffffffff810f5831>] ? rb_commit+0x58/0x85
> [ 2866.132281] [<ffffffff81911dcb>] ? tcp_write_timer_handler+0x148/0x148
> [ 2866.132281] [<ffffffff81070bd6>] __do_softirq+0xcb/0x1f9
> [ 2866.132281] [<ffffffff81a0a00c>] ? _raw_spin_unlock+0x29/0x2e
> [ 2866.132281] [<ffffffff81a1227c>] call_softirq+0x1c/0x30
> [ 2866.132281] [<ffffffff81039f38>] do_softirq+0x4a/0xa6
> [ 2866.132281] [<ffffffff81070f2b>] irq_exit+0x51/0xad
> [ 2866.132281] [<ffffffff81a129cd>] do_IRQ+0x9d/0xb4
> [ 2866.132281] [<ffffffff81a0a3ef>] common_interrupt+0x6f/0x6f
> [ 2866.132281] <EOI> [<ffffffff8109d006>] ? sched_clock_cpu+0x58/0xd1
> [ 2866.132281] [<ffffffff81a0a172>] ? _raw_spin_unlock_irqrestore+0x4c/0x56
> [ 2866.132281] [<ffffffff81078692>] mod_timer+0x178/0x1a9
> [ 2866.132281] [<ffffffff818a00aa>] sk_reset_timer+0x19/0x26
> [ 2866.132281] [<ffffffff8190b2cc>] tcp_rearm_rto+0x99/0xa4
> [ 2866.132281] [<ffffffff8190dfba>] tcp_event_new_data_sent+0x6e/0x70
> [ 2866.132281] [<ffffffff8190f7ea>] tcp_write_xmit+0x7de/0x8e4
> [ 2866.132281] [<ffffffff818a565d>] ? __alloc_skb+0xa0/0x1a1
> [ 2866.132281] [<ffffffff8190f952>] __tcp_push_pending_frames+0x2e/0x8a
> [ 2866.132281] [<ffffffff81904122>] tcp_sendmsg+0xb32/0xcc6
> [ 2866.132281] [<ffffffff819229c2>] inet_sendmsg+0xaa/0xd5
> [ 2866.132281] [<ffffffff81922918>] ? inet_autobind+0x5f/0x5f
> [ 2866.132281] [<ffffffff810ee7f1>] ? trace_clock_local+0x9/0xb
> [ 2866.132281] [<ffffffff8189adab>] sock_sendmsg+0xa3/0xc4
> [ 2866.132281] [<ffffffff810f5de6>] ? rb_reserve_next_event+0x26f/0x2d5
> [ 2866.132281] [<ffffffff8103e6a9>] ? native_sched_clock+0x29/0x6f
> [ 2866.132281] [<ffffffff8103e6f8>] ? sched_clock+0x9/0xd
> [ 2866.132281] [<ffffffff810ee7f1>] ? trace_clock_local+0x9/0xb
> [ 2866.132281] [<ffffffff8189ae03>] kernel_sendmsg+0x37/0x43
> [ 2866.132281] [<ffffffff8199ce49>] xs_send_kvec+0x77/0x80
> [ 2866.132281] [<ffffffff8199cec1>] xs_sendpages+0x6f/0x1a0
> [ 2866.132281] [<ffffffff8107826d>] ? try_to_del_timer_sync+0x55/0x61
> [ 2866.132281] [<ffffffff8199d0d2>] xs_tcp_send_request+0x55/0xf1
> [ 2866.132281] [<ffffffff8199bb90>] xprt_transmit+0x89/0x1db
> [ 2866.132281] [<ffffffff81999bcd>] ? call_connect+0x3c/0x3c
> [ 2866.132281] [<ffffffff81999d92>] call_transmit+0x1c5/0x20e
> [ 2866.132281] [<ffffffff819a0d55>] __rpc_execute+0x6f/0x225
> [ 2866.132281] [<ffffffff81999bcd>] ? call_connect+0x3c/0x3c
> [ 2866.132281] [<ffffffff819a0f33>] rpc_async_schedule+0x28/0x34
> [ 2866.132281] [<ffffffff810835d6>] process_one_work+0x24d/0x47f
> [ 2866.132281] [<ffffffff81083567>] ? process_one_work+0x1de/0x47f
> [ 2866.132281] [<ffffffff819a0f0b>] ? __rpc_execute+0x225/0x225
> [ 2866.132281] [<ffffffff81083a6d>] worker_thread+0x236/0x317
> [ 2866.132281] [<ffffffff81083837>] ? process_scheduled_works+0x2f/0x2f
> [ 2866.132281] [<ffffffff8108b7b8>] kthread+0x9a/0xa2
> [ 2866.132281] [<ffffffff81a12184>] kernel_thread_helper+0x4/0x10
> [ 2866.132281] [<ffffffff81a0a4b0>] ? retint_restore_args+0x13/0x13
> [ 2866.132281] [<ffffffff8108b71e>] ? __init_kthread_worker+0x5a/0x5a
> [ 2866.132281] [<ffffffff81a12180>] ? gs_change+0x13/0x13
> [ 2866.308506] IPv4: Attempt to release TCP socket in state 1 ffff880019ec0000
> [ 2866.309689] =============================================================================
> [ 2866.310254] BUG TCP (Not tainted): Object already free
> [ 2866.310254] -----------------------------------------------------------------------------
> [ 2866.310254]
>
> This happens only in one of my test boxes and is rather hard to
> reproduce. I find that it could only happen once out of 100 boots,
> each boot test takes 1.5 hour. It's basically an NFSROOT system that
> runs a bunch of IO and MM tests.
>
> I've been running auto+manual bisects for one week and it has failed
> several times. Here is the log for last try. It reports 0bb4087 as
> the first bad commit, however the bug still shows up when reverting
> 0bb4087 on top of 3.6-rc2. So the 'good' commits in the below log may
> not really be good. The 'bad' commits, however, are bad for sure.
>
> wfg@bee ~/linux% git bisect log
> # bad: [0d7614f09c1ebdbaa1599a5aba7593f147bf96ee] Linux 3.6-rc1
> # good: [28a33cbc24e4256c143dce96c7d93bf423229f92] Linux 3.5
> git bisect start 'v3.6-rc1' 'v3.5' '--'
> # bad: [614a6d4341b3760ca98a1c2c09141b71db5d1e90] Merge branch 'for-3.6' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup
> git bisect bad 614a6d4341b3760ca98a1c2c09141b71db5d1e90
> # bad: [320f5ea0cedc08ef65d67e056bcb9d181386ef2c] genetlink: define lockdep_genl_is_held() when CONFIG_LOCKDEP
> git bisect bad 320f5ea0cedc08ef65d67e056bcb9d181386ef2c
> # skip: [dbfa600148a25903976910863c75dae185f8d187] cxgb3: set maximal number of default RSS queues
> git bisect skip dbfa600148a25903976910863c75dae185f8d187
> # bad: [b6dfd939fdc249fcf8cd7b8006f76239b33eb581] ixgbe: add support for new 82599 device
> git bisect bad b6dfd939fdc249fcf8cd7b8006f76239b33eb581
> # bad: [39cb681b3bb4da17e74d48e553d1bb9a1b759aa5] ixgbe: Fix handling of FDIR_HASH flag
> git bisect bad 39cb681b3bb4da17e74d48e553d1bb9a1b759aa5
> # bad: [9297127b9cdd8d30c829ef5fd28b7cc0323a7bcd] ixgbe: Change how we check for pre-existing and assigned VFs
> git bisect bad 9297127b9cdd8d30c829ef5fd28b7cc0323a7bcd
> # bad: [99d744875d01d57d832b8dbfc36d9a1990d503b8] ixgbe: Drop probe_vf and merge functionality into ixgbe_enable_sriov
> git bisect bad 99d744875d01d57d832b8dbfc36d9a1990d503b8
> # bad: [186e868786f97c8026f0a81400b451ace306b3a4] forcedeth: spin_unlock_irq in interrupt handler fix
> git bisect bad 186e868786f97c8026f0a81400b451ace306b3a4
> # good: [3e4b9459fb0e149c6b74c9e89399a8fc39a92b44] Merge tag 'md-3.5-fixes' of git://neil.brown.name/md
> git bisect good 3e4b9459fb0e149c6b74c9e89399a8fc39a92b44
> # good: [e8c7b335faca2cbce715da3b0e1663d75d422f5b] wlcore: increase command completion timeout
> git bisect good e8c7b335faca2cbce715da3b0e1663d75d422f5b
> # good: [1d248b1cf4e09dbec8cef5f7fbeda5874248bd09] net: Pass neighbours and dest address into NETEVENT_REDIRECT events.
> git bisect good 1d248b1cf4e09dbec8cef5f7fbeda5874248bd09
> # good: [73dea3983a9d2e413e1c065ed4e132a7d4127929] be2net: Add description about various RSS hash types
> git bisect good 73dea3983a9d2e413e1c065ed4e132a7d4127929
> # good: [8b0d2f9ed3d8e92feada7c5d70fa85be46e6f948] net: e100: ucode is optional in some cases
> git bisect good 8b0d2f9ed3d8e92feada7c5d70fa85be46e6f948
> # good: [36eb22e97a2b621fb707eead58ef915ab0f46e9e] libertas: firmware.c: remove duplicated include
> git bisect good 36eb22e97a2b621fb707eead58ef915ab0f46e9e
> # good: [5815d5e7aae3cc9c5e85af83094d4d6498c3f4fc] tcp: use hash_32() in tcp_metrics
> git bisect good 5815d5e7aae3cc9c5e85af83094d4d6498c3f4fc
> # good: [e4bce0f288bafd8505ba5ce9c5284a4478f1b725] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-next
> git bisect good e4bce0f288bafd8505ba5ce9c5284a4478f1b725
> # good: [a1b5d0dd28e9cb4fe42ad2df4ebbe5cce96866d7] openvswitch: Check gso_type for correct sk_buff in queue_gso_packets().
> git bisect good a1b5d0dd28e9cb4fe42ad2df4ebbe5cce96866d7
> # bad: [0bb4087] ipv4: Fix neigh lookup keying over loopback/point-to-point devices.
> git bisect bad 0bb4087
> # good: [b09e786bd1dd66418b69348cb110f3a64764626a] tun: fix a crash bug and a memory leak
> git bisect good b09e786bd1dd66418b69348cb110f3a64764626a
> # good: [fa0afcd10951afad2022dda09777d2bf70cdab3d] atl1c: fix issue of io access mode for AR8152 v2.1
> git bisect good fa0afcd10951afad2022dda09777d2bf70cdab3d
>
> Thanks,
> Fengguang
Hi Fengguang, thanks for this report.
Hmm, this looks like sk_reset_timer() is called on a socket, and timer
triggers _before_ the sock_hold()
So the timer handler decrements sk_refcnt to 0 and calls sk_free()
Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp:
improve latencies of timer triggered events)
I always found sk_reset_timer() a bit racy...
void sk_reset_timer(struct sock *sk, struct timer_list* timer,
unsigned long expires)
{
if (!mod_timer(timer, expires))
sock_hold(sk); // MIGHT BE TOO LATE
}
Following should be safer...
void sk_reset_timer(struct sock *sk, struct timer_list* timer,
unsigned long expires)
{
sock_hold(sk);
if (mod_timer(timer, expires))
sock_put(sk);
}
Could you test following patch ?
By the way, there is a typo in tcp_delack_timer()
(should use TCP_DELACK_TIMER_DEFERRED instead of
TCP_WRITE_TIMER_DEFERRED)
Thanks !
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7678237..6278a11 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -417,10 +417,12 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
if (code == ICMP_FRAG_NEEDED) { /* PMTU discovery (RFC1191) */
tp->mtu_info = info;
- if (!sock_owned_by_user(sk))
+ if (!sock_owned_by_user(sk)) {
tcp_v4_mtu_reduced(sk);
- else
- set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags);
+ } else {
+ if (!test_and_set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags))
+ sock_hold(sk);
+ }
goto out;
}
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 20dfd89..d046326 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -910,14 +910,18 @@ void tcp_release_cb(struct sock *sk)
if (flags & (1UL << TCP_TSQ_DEFERRED))
tcp_tsq_handler(sk);
- if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED))
+ if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) {
tcp_write_timer_handler(sk);
-
- if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED))
+ __sock_put(sk);
+ }
+ if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) {
tcp_delack_timer_handler(sk);
-
- if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED))
+ __sock_put(sk);
+ }
+ if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) {
sk->sk_prot->mtu_reduced(sk);
+ __sock_put(sk);
+ }
}
EXPORT_SYMBOL(tcp_release_cb);
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 6df36ad..b774a03 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -252,7 +252,8 @@ static void tcp_delack_timer(unsigned long data)
inet_csk(sk)->icsk_ack.blocked = 1;
NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOCKED);
/* deleguate our work to tcp_release_cb() */
- set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
+ if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
+ sock_hold(sk);
}
bh_unlock_sock(sk);
sock_put(sk);
@@ -481,7 +482,8 @@ static void tcp_write_timer(unsigned long data)
tcp_write_timer_handler(sk);
} else {
/* deleguate our work to tcp_release_cb() */
- set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
+ if (!test_and_set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
+ sock_hold(sk);
}
bh_unlock_sock(sk);
sock_put(sk);
^ permalink raw reply related
* $650,500.00 USD
From: United Nations Trust Funds @ 2012-08-19 11:47 UTC (permalink / raw)
To: Recipients
Reply to claim.
^ permalink raw reply
* $650,500.00 USD
From: United Nations Trust Funds @ 2012-08-19 11:42 UTC (permalink / raw)
To: Recipients
Reply to claim.
^ permalink raw reply
* [RFC] Interface for TCP Metrics
From: Julian Anastasov @ 2012-08-19 11:42 UTC (permalink / raw)
To: netdev
Hello,
Once DaveM mentioned that TCP Metrics need their
own interface. I'm planning to implement such interface
but lets first decide how it should look. After little
research, here is my plan:
- will use genl with TCP_METRICS_GENL_NAME "tcp_metrics",
TCP_METRICS_GENL_VERSION 0x01
- provide dumpit method and one cmd to read metrics by exact addr,
will use TCP_METRICS_CMD_{GET,...} and TCP_METRICS_ATTR_xxx in
new file include/linux/tcp_metrics.h
- Is command to delete cached entry needed? Delete will need
new rcu_head. Useful to flush the cache or to delete entries
with filter.
- without support for delete cmd, may be we can add command to
reset entry with default values from dst?
- Where to put the new netlink code?
tcp_metrics_netlink.c
tcp_metrics_nl.c
or just in current tcp_metrics.c ?
- will provide support for ip tool:
ip tcpm[etrics] <cmd> ...
ip -6 tcpm[etrics] get <addr> => TCP_METRICS_CMD_GET for 1 entry
ip tcpm[etrics] list => dumpit
- any wishes how to look the output? How to select specific
values/metrics in output? For example, list only specified
named values: ip tcpm list [-o] rtt
- command to modify specific metric for addr, by name? Only
for tcpm_vals? If not locked?
Do we need modify/delete/reset support or just read
support is enough? Comments?
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* $650,500.00 USD
From: United Nations Trust Funds @ 2012-08-19 11:15 UTC (permalink / raw)
To: Recipients
Reply to claim.
^ permalink raw reply
* Re: network packet corruption in v3.6.0-rc1 (and also in v3.5)
From: Eric Dumazet @ 2012-08-19 11:02 UTC (permalink / raw)
To: Andrew Worsley; +Cc: David S. Miller, Alexey Kuznetsov, James Morris, netdev
In-Reply-To: <CA+Y=x3=jSrEaYdnaAF75p0utQq02MhoSh4SkshRVSc7JjjVPOw@mail.gmail.com>
On Sat, 2012-08-18 at 16:40 +1000, Andrew Worsley wrote:
> Some times I get a burst of errors and dropped tcp connections with
> this warning. I had a similar issue with v3.5 (in fact v3.5 was
> unusable for https connections which would give bad MAC code messages
> at the application level)..
>
> The issue seems better under v3.6-rc1 where as it was very bad under
> v3.5. Let me know if there is something I can do to avoid this
> problem. I am using the USB r8712u wireless NIC if that is relevant as
> I haven't been able to get the native wireless working under the MAC
> book yet (requires proprietary firmware extraction which I am not
> familiar with)
>
> Much obliged if there is anything I can do to avoid it as it is rather
> annoying. Doesn't happen at all with my Debian wheezy 3.2.0-2-amd64
> kernel.
OK, this seems to be related to r8712u, still in staging tree.
At first glance, its using a buggy skb_clone(), or kind of a 'reuse skb'
trick I have no time to investigate.
I suspect that some memory area are overwritten by this driver, and this
was not noticed with older kernels. With new kernels, its triggering a
WARN_ON() in tcp stack.
Any chance you can try to reproduce the bug with another adapter ?
^ 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