* [PATCH] ipv6: send unsolicited neighbour advertisements to all-nodes
From: Hannes Frederic Sowa @ 2012-11-07 2:18 UTC (permalink / raw)
To: netdev
As documented in RFC4861 (Neighbor Discovery for IP version 6) 7.2.6.,
unsolicited neighbour advertisements should be sent to the all-nodes
multicast address.
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
net/ipv6/ndisc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 61b3354..8271db6 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -535,7 +535,7 @@ static void ndisc_send_unsol_na(struct net_device *dev)
{
struct inet6_dev *idev;
struct inet6_ifaddr *ifa;
- struct in6_addr mcaddr;
+ struct in6_addr mcaddr = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
idev = in6_dev_get(dev);
if (!idev)
@@ -543,7 +543,6 @@ static void ndisc_send_unsol_na(struct net_device *dev)
read_lock_bh(&idev->lock);
list_for_each_entry(ifa, &idev->addr_list, if_list) {
- addrconf_addr_solict_mult(&ifa->addr, &mcaddr);
ndisc_send_na(dev, NULL, &mcaddr, &ifa->addr,
/*router=*/ !!idev->cnf.forwarding,
/*solicited=*/ false, /*override=*/ true,
^ permalink raw reply related
* Re: [PATCH] tcp: Replace infinite loop on recvmsg bug with proper crash
From: Eric Dumazet @ 2012-11-07 1:51 UTC (permalink / raw)
To: Dave Jones
Cc: Julius Werner, linux-kernel, netdev, Patrick McHardy,
Hideaki YOSHIFUJI, James Morris, Alexey Kuznetsov,
David S. Miller, Sameer Nanda, Mandeep Singh Baines, Eric Dumazet
In-Reply-To: <20121107013907.GA31185@redhat.com>
On Tue, 2012-11-06 at 20:39 -0500, Dave Jones wrote:
> On Tue, Nov 06, 2012 at 04:15:35PM -0800, Julius Werner wrote:
> > tcp_recvmsg contains a sanity check that WARNs when there is a gap
> > between the socket's copied_seq and the first buffer in the
> > sk_receive_queue. In theory, the TCP stack makes sure that This Should
> > Never Happen (TM)... however, practice shows that there are still a few
> > bug reports from it out there (and one in my inbox).
> >
> > Unfortunately, when it does happen for whatever reason, the situation
> > is not handled very well: the kernel logs a warning and breaks out of
> > the loop that walks the receive queue. It proceeds to find nothing else
> > to do on the socket and hits sk_wait_data, which cannot block because
> > the receive queue is not empty. As no data was read, the outer while
> > loop repeats (logging the same warning again) ad infinitum until the
> > system's syslog exhausts all available hard drive capacity.
> >
> > This patch improves that behavior by going straight to a proper kernel
> > crash. The cause of the error can be identified right away and the
> > system's hard drive is not unnecessarily strained.
> >
> > Signed-off-by: Julius Werner <jwerner@chromium.org>
> > ---
> > net/ipv4/tcp.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 197c000..fcb0927 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1628,7 +1628,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
> > "recvmsg bug: copied %X seq %X rcvnxt %X fl %X\n",
> > *seq, TCP_SKB_CB(skb)->seq, tp->rcv_nxt,
> > flags))
> > - break;
> > + BUG();
> >
> > offset = *seq - TCP_SKB_CB(skb)->seq;
> > if (tcp_hdr(skb)->syn)
>
> We've had reports of this WARN against the Fedora kernel for a while.
> Had this been immediately followed by a BUG(), we'd have never seen those traces at all,
> and just got "my machine just locked up" reports instead.
>
> The proper fix here is to find out why we're getting into this state.
Yes, but there is no need to fill syslog over and over.
In fact, some drivers are buggy and can overwrite skbs.
Thats also a security issue, as payload can be changed without notice
(unless SSL or application checksums are done, see commit
abf02cfc179bb4bd for an example)
Quite frankly BUG_ON() here is the only way we can fix bugs instead of
being lazy.
^ permalink raw reply
* Re: [PATCH] tcp: Replace infinite loop on recvmsg bug with proper crash
From: Julius Werner @ 2012-11-07 1:51 UTC (permalink / raw)
To: Dave Jones, Julius Werner, linux-kernel, netdev, Patrick McHardy,
Hideaki YOSHIFUJI, James Morris, Alexey Kuznetsov,
David S. Miller, Sameer Nanda, Mandeep Singh Baines, Eric Dumazet
In-Reply-To: <20121107013907.GA31185@redhat.com>
> We've had reports of this WARN against the Fedora kernel for a while.
> Had this been immediately followed by a BUG(), we'd have never seen those traces at all,
> and just got "my machine just locked up" reports instead.
>
> The proper fix here is to find out why we're getting into this state.
Are you sure you don't mean the WARN below that ("recvmsg bug 2")
instead? I don't think this one can happen without eventually running
into the syslog overflow issue I described.
I agree that the underlying cause must be fixed too, but as we will
always have bugs in the kernel I think proper handling when it does
happen is also important (and filling the hard disk with junk is
obviously not the best approach). If you think a full panic is too
extreme, I have an alternative version of this patch that logs the
WARN once, closes the socket, and returns EBADFD from the syscall...
would you think that is more appropriate?
^ permalink raw reply
* Re: [PATCH] tcp: Replace infinite loop on recvmsg bug with proper crash
From: Dave Jones @ 2012-11-07 1:39 UTC (permalink / raw)
To: Julius Werner
Cc: linux-kernel, netdev, Patrick McHardy, Hideaki YOSHIFUJI,
James Morris, Alexey Kuznetsov, David S. Miller, Sameer Nanda,
Mandeep Singh Baines, Eric Dumazet
In-Reply-To: <1352247335-10396-1-git-send-email-jwerner@chromium.org>
On Tue, Nov 06, 2012 at 04:15:35PM -0800, Julius Werner wrote:
> tcp_recvmsg contains a sanity check that WARNs when there is a gap
> between the socket's copied_seq and the first buffer in the
> sk_receive_queue. In theory, the TCP stack makes sure that This Should
> Never Happen (TM)... however, practice shows that there are still a few
> bug reports from it out there (and one in my inbox).
>
> Unfortunately, when it does happen for whatever reason, the situation
> is not handled very well: the kernel logs a warning and breaks out of
> the loop that walks the receive queue. It proceeds to find nothing else
> to do on the socket and hits sk_wait_data, which cannot block because
> the receive queue is not empty. As no data was read, the outer while
> loop repeats (logging the same warning again) ad infinitum until the
> system's syslog exhausts all available hard drive capacity.
>
> This patch improves that behavior by going straight to a proper kernel
> crash. The cause of the error can be identified right away and the
> system's hard drive is not unnecessarily strained.
>
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
> net/ipv4/tcp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 197c000..fcb0927 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1628,7 +1628,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
> "recvmsg bug: copied %X seq %X rcvnxt %X fl %X\n",
> *seq, TCP_SKB_CB(skb)->seq, tp->rcv_nxt,
> flags))
> - break;
> + BUG();
>
> offset = *seq - TCP_SKB_CB(skb)->seq;
> if (tcp_hdr(skb)->syn)
We've had reports of this WARN against the Fedora kernel for a while.
Had this been immediately followed by a BUG(), we'd have never seen those traces at all,
and just got "my machine just locked up" reports instead.
The proper fix here is to find out why we're getting into this state.
Dave
^ permalink raw reply
* Re: [PATCH v3 5/6] net: calxedaxgmac: rework transmit ring handling
From: David Miller @ 2012-11-07 1:10 UTC (permalink / raw)
To: rob.herring; +Cc: netdev, eric.dumazet
In-Reply-To: <5099AB57.7000805@calxeda.com>
From: Rob Herring <rob.herring@calxeda.com>
Date: Tue, 06 Nov 2012 18:29:11 -0600
> David,
>
> On 11/06/2012 05:57 PM, David Miller wrote:
>> From: Rob Herring <robherring2@gmail.com>
>> Date: Mon, 5 Nov 2012 10:22:23 -0600
>>
>>> Only generate tx interrupts on every ring size / 4 descriptors.
>>
>> I thought we told you that you cannot do this.
>>
>> With this change if we get a few packets, then stop generating any
>> traffic, there will be SKBs that just sit dead in your TX queue.
>
> And as I previously mentioned, we do get a tx complete interrupt in
> addition. The h/w will interrupt when all packets are transmitted and
> there is not another descriptor ready.
Ok, in that case it's fine. I'll keep reviewing this series then.
^ permalink raw reply
* [PATCH] sunrpc: fix clnt.c kernel-doc warning
From: Randy Dunlap @ 2012-11-07 1:08 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Trond Myklebust, J. Bruce Fields,
linux-nfs-u79uwXL29TY76Z2rM5mHXA
From: Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Fix new kernel-doc warnings in clnt.c:
Warning(net/sunrpc/clnt.c:561): No description found for parameter 'flavor'
Warning(net/sunrpc/clnt.c:561): Excess function parameter 'auth' description in 'rpc_clone_client_set_auth'
Signed-off-by: Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
Cc: "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
net/sunrpc/clnt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- lnx-37-rc4.orig/net/sunrpc/clnt.c
+++ lnx-37-rc4/net/sunrpc/clnt.c
@@ -552,7 +552,7 @@ EXPORT_SYMBOL_GPL(rpc_clone_client);
* rpc_clone_client_set_auth - Clone an RPC client structure and set its auth
*
* @clnt: RPC client whose parameters are copied
- * @auth: security flavor for new client
+ * @flavor: security flavor for new client
*
* Returns a fresh RPC client or an ERR_PTR.
*/
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 5/6] net: calxedaxgmac: rework transmit ring handling
From: Rob Herring @ 2012-11-07 0:29 UTC (permalink / raw)
To: David Miller; +Cc: netdev, eric.dumazet
In-Reply-To: <20121106.185704.1212469675670236955.davem@davemloft.net>
David,
On 11/06/2012 05:57 PM, David Miller wrote:
> From: Rob Herring <robherring2@gmail.com>
> Date: Mon, 5 Nov 2012 10:22:23 -0600
>
>> Only generate tx interrupts on every ring size / 4 descriptors.
>
> I thought we told you that you cannot do this.
>
> With this change if we get a few packets, then stop generating any
> traffic, there will be SKBs that just sit dead in your TX queue.
And as I previously mentioned, we do get a tx complete interrupt in
addition. The h/w will interrupt when all packets are transmitted and
there is not another descriptor ready. That is the only tx interrupt we
get without this patch. With this patch, we will get interrupts for
every N descriptors in addition to a tx complete/idle interrupt. This
patch is to avoid the transmitter from going idle and only refilling the
tx ring after finishing sending all frames. I can repost this patch and
make the commit message more clear, but I don't think there is any
functional change needed. This one is not so important compared to the
rest of the series, so you can just drop it if you still don't agree.
Rob
>
> This cannot ever happen. All TX SKBs must be freed up in a short,
> finite, amount of time. Under all conditions, and in every situation.
>
> Otherwise memory accounted to sockets is not liberated, and such
> sockets cannot be destroyed or closed.
>
> SKBs also hold onto other kinds of resources, for which it is critical
> to liberate in a finite amount of time.
>
> I'm not applying this series, it still needs more work.
>
^ permalink raw reply
* [PATCH] tcp: Replace infinite loop on recvmsg bug with proper crash
From: Julius Werner @ 2012-11-07 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, Patrick McHardy, Hideaki YOSHIFUJI, James Morris,
Alexey Kuznetsov, David S. Miller, Sameer Nanda,
Mandeep Singh Baines, Eric Dumazet, Julius Werner
tcp_recvmsg contains a sanity check that WARNs when there is a gap
between the socket's copied_seq and the first buffer in the
sk_receive_queue. In theory, the TCP stack makes sure that This Should
Never Happen (TM)... however, practice shows that there are still a few
bug reports from it out there (and one in my inbox).
Unfortunately, when it does happen for whatever reason, the situation
is not handled very well: the kernel logs a warning and breaks out of
the loop that walks the receive queue. It proceeds to find nothing else
to do on the socket and hits sk_wait_data, which cannot block because
the receive queue is not empty. As no data was read, the outer while
loop repeats (logging the same warning again) ad infinitum until the
system's syslog exhausts all available hard drive capacity.
This patch improves that behavior by going straight to a proper kernel
crash. The cause of the error can be identified right away and the
system's hard drive is not unnecessarily strained.
Signed-off-by: Julius Werner <jwerner@chromium.org>
---
net/ipv4/tcp.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 197c000..fcb0927 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1628,7 +1628,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
"recvmsg bug: copied %X seq %X rcvnxt %X fl %X\n",
*seq, TCP_SKB_CB(skb)->seq, tp->rcv_nxt,
flags))
- break;
+ BUG();
offset = *seq - TCP_SKB_CB(skb)->seq;
if (tcp_hdr(skb)->syn)
--
1.7.8.6
^ permalink raw reply related
* Re: [PATCH v2 net-next] htb: fix two bugs
From: David Miller @ 2012-11-07 0:06 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, j.vimal
In-Reply-To: <1352169649.3140.59.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 05 Nov 2012 18:40:49 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> Commit 56b765b79e9 (htb: improved accuracy at high rates)
> introduced two bugs :
>
> 1) one bstats_update() was inadvertently removed from
> htb_dequeue_tree(), breaking statistics/rate estimation.
>
> 2) Missing qdisc_put_rtab() calls in htb_change_class(),
> leaking kernel memory, now struct htb_class no longer
> retains pointers to qdisc_rate_table structs.
>
> Since only rate is used, dont use qdisc_get_rtab() calls
> copying data we ignore anyway.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH net-next 2/2] tg3: Call tg3_netif_stop() from tg3_stop()
From: David Miller @ 2012-11-07 0:00 UTC (permalink / raw)
To: mchan; +Cc: netdev
In-Reply-To: <1352161590-1586-2-git-send-email-mchan@broadcom.com>
From: "Michael Chan" <mchan@broadcom.com>
Date: Mon, 5 Nov 2012 16:26:30 -0800
> From: Nithin Nayak Sujir <nsujir@broadcom.com>
>
> instead of making separate tg3_napi_disable() and netif_tx_disable() calls.
>
> Update version to 3.126.
>
> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 1/2] tg3: Support 5717 C0
From: David Miller @ 2012-11-07 0:00 UTC (permalink / raw)
To: mchan; +Cc: netdev
In-Reply-To: <1352161590-1586-1-git-send-email-mchan@broadcom.com>
From: "Michael Chan" <mchan@broadcom.com>
Date: Mon, 5 Nov 2012 16:26:29 -0800
> Add support for 5717C0 which is a 5720A0 with special bonds-out option.
>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
Applied.
^ permalink raw reply
* Re: [PATCH 8/9] isdn: Make CONFIG_ISDN depend on CONFIG_NETDEVICES
From: David Miller @ 2012-11-06 23:57 UTC (permalink / raw)
To: lee.jones; +Cc: pebolle, linux-kernel, isdn, netdev
In-Reply-To: <20121105103126.GB5220@gmail.com>
From: Lee Jones <lee.jones@linaro.org>
Date: Mon, 5 Nov 2012 11:31:26 +0100
> Does something like look like a better solution?
>
> Author: Lee Jones <lee.jones@linaro.org>
> Date: Sat Nov 3 22:06:02 2012 +0100
>
> isdn: Make CONFIG_ISDN depend on CONFIG_NETDEVICES
Yes, it looks good, please resubmit it formally.
^ permalink raw reply
* Re: [PATCH v3 5/6] net: calxedaxgmac: rework transmit ring handling
From: David Miller @ 2012-11-06 23:57 UTC (permalink / raw)
To: robherring2; +Cc: netdev, eric.dumazet, rob.herring
In-Reply-To: <1352132544-15809-6-git-send-email-robherring2@gmail.com>
From: Rob Herring <robherring2@gmail.com>
Date: Mon, 5 Nov 2012 10:22:23 -0600
> Only generate tx interrupts on every ring size / 4 descriptors.
I thought we told you that you cannot do this.
With this change if we get a few packets, then stop generating any
traffic, there will be SKBs that just sit dead in your TX queue.
This cannot ever happen. All TX SKBs must be freed up in a short,
finite, amount of time. Under all conditions, and in every situation.
Otherwise memory accounted to sockets is not liberated, and such
sockets cannot be destroyed or closed.
SKBs also hold onto other kinds of resources, for which it is critical
to liberate in a finite amount of time.
I'm not applying this series, it still needs more work.
^ permalink raw reply
* Re: [PATCH V6 0/22] qlcnic: patches for new adapter - Qlogic 83XX CNA
From: David Miller @ 2012-11-06 23:51 UTC (permalink / raw)
To: sony.chacko; +Cc: netdev, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1352153535-5006-1-git-send-email-sony.chacko@qlogic.com>
From: Sony Chacko <sony.chacko@qlogic.com>
Date: Mon, 5 Nov 2012 17:11:54 -0500
> I apologize for the above mistake.
> We have fixed the above warning and have verified the rest of the patches.
> Please apply the updated 22 patch series to net-next.
Your changes are still broken:
drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c: In function ‘qlcnic_fw_create_ctx’:
drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c:565:1: warning: the frame size of 8192 bytes is larger than 2048 bytes [-Wframe-larger-than=]
A multi-kilobyte on-stack variable? Are you kidding me?
This submission is terrible, in every way, shape, and form. Nobody
wants to review these changes in detail (have you noticed that besides
me, there is zero feedback coming in from anyone). You have to
recognize that it's demoralizing to review patches when they don't
even build cleanly like your's do.
Stop submitting garbage patches, now.
I want you to not resubmit this for at least a week. You must
eliminate all of these kinds of problems, you must validate the build,
you must double check everything.
And you cannot be trusted to do that in just one or two days.
Again, don't resubmit this until you've worked over and double checked
everything in it for at least a week.
^ permalink raw reply
* Re: [PATCH 0/2 v2] net: at91_ether add dt and pinctrl support
From: David Miller @ 2012-11-06 23:33 UTC (permalink / raw)
To: plagnioj
Cc: linux-arm-kernel, netdev, devicetree-discuss, nicolas.ferre,
manabian
In-Reply-To: <20121105073015.GM19021@game.jcrosoft.org>
From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Date: Mon, 5 Nov 2012 08:30:15 +0100
> Hi,
>
> v2: fix typo in doc + miising empty line
>
> This patch serie add dt and pinctrl support to the at91 ether
>
> this is need to use the network on at91rm9200 as we now have dt
> support on it
>
> Jean-Christophe PLAGNIOL-VILLARD (2):
> net: at91_ether: add dt support
> net: at91_ether: add pinctrl support
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] drivers: ethernet: qlogic: netxen_nic_ethtool.c: Fixed a coding style issue
From: David Miller @ 2012-11-06 23:33 UTC (permalink / raw)
To: gmate.amit
Cc: sony.chacko, rajesh.borundia, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <1352094368-5037-1-git-send-email-gmate.amit@gmail.com>
From: Kumar Amit Mehta <gmate.amit@gmail.com>
Date: Sun, 4 Nov 2012 21:46:08 -0800
> Fixed some coding style issues.
>
> Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] drivers: ethernet: qlogic: qlge_dbg.c: Fixed a coding style issue
From: David Miller @ 2012-11-06 23:33 UTC (permalink / raw)
To: gmate.amit
Cc: jitendra.kalsaria, ron.mercer, linux-driver, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <1352092292-4413-1-git-send-email-gmate.amit@gmail.com>
From: Kumar Amit Mehta <gmate.amit@gmail.com>
Date: Sun, 4 Nov 2012 21:11:32 -0800
> checkpatch.pl throws error message for the current code. This patch fixes
> this coding style issue.
>
> Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] sparc: bpf_jit_comp: add VLAN instructions for BPF JIT
From: David Miller @ 2012-11-06 23:33 UTC (permalink / raw)
To: dxchgb; +Cc: netdev
In-Reply-To: <20121105025930.GA5197@thinkbox>
From: Daniel Borkmann <dxchgb@gmail.com>
Date: Mon, 5 Nov 2012 03:59:30 +0100
> This patch is a follow-up for patch "net: filter: add vlan tag access"
> to support the new VLAN_TAG/VLAN_TAG_PRESENT accessors in BPF JIT.
>
> Signed-off-by: Daniel Borkmann <daniel.borkmann@tik.ee.ethz.ch>
> ---
> Disclaimer: uncompiled and untested, since I don't have a SPARC machine,
> but it should (hopefully) integrate cleanly. ;)
Looks good, applied, thanks Dan.
^ permalink raw reply
* Re: [PATCH v4 3/6] block/genhd.c: apply pm_runtime_set_memalloc_noio on block devices
From: Andrew Morton @ 2012-11-06 23:24 UTC (permalink / raw)
To: Ming Lei
Cc: linux-kernel, Alan Stern, Oliver Neukum, Minchan Kim,
Greg Kroah-Hartman, Rafael J. Wysocki, Jens Axboe,
David S. Miller, netdev, linux-usb, linux-pm, linux-mm
In-Reply-To: <1351931714-11689-4-git-send-email-ming.lei@canonical.com>
On Sat, 3 Nov 2012 16:35:11 +0800
Ming Lei <ming.lei@canonical.com> wrote:
> This patch applyes the introduced pm_runtime_set_memalloc_noio on
> block device so that PM core will teach mm to not allocate memory with
> GFP_IOFS when calling the runtime_resume and runtime_suspend callback
> for block devices and its ancestors.
>
> ...
>
> @@ -532,6 +533,13 @@ static void register_disk(struct gendisk *disk)
> return;
> }
> }
> +
> + /* avoid probable deadlock caused by allocating memory with
Again, please fix the comment style. Take a look at the rest of this file!
> + * GFP_KERNEL in runtime_resume callback of its all ancestor
> + * deivces
typo
> + */
--
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: [PATCH v4 2/6] PM / Runtime: introduce pm_runtime_set_memalloc_noio()
From: Andrew Morton @ 2012-11-06 23:24 UTC (permalink / raw)
To: Ming Lei
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Alan Stern, Oliver Neukum,
Minchan Kim, Greg Kroah-Hartman, Rafael J. Wysocki, Jens Axboe,
David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg
In-Reply-To: <1351931714-11689-3-git-send-email-ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
On Sat, 3 Nov 2012 16:35:10 +0800
Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> The patch introduces the flag of memalloc_noio in 'struct dev_pm_info'
> to help PM core to teach mm not allocating memory with GFP_KERNEL
> flag for avoiding probable deadlock.
>
> As explained in the comment, any GFP_KERNEL allocation inside
> runtime_resume() or runtime_suspend() on any one of device in
> the path from one block or network device to the root device
> in the device tree may cause deadlock, the introduced
> pm_runtime_set_memalloc_noio() sets or clears the flag on
> device in the path recursively.
>
checkpatch finds a number of problems with this patch, all of which
should be fixed. Please always use checkpatch.
> index 3148b10..d477924 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -124,6 +124,63 @@ unsigned long pm_runtime_autosuspend_expiration(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
>
> +static int dev_memalloc_noio(struct device *dev, void *data)
> +{
> + return dev->power.memalloc_noio;
> +}
> +
> +/*
> + * pm_runtime_set_memalloc_noio - Set a device's memalloc_noio flag.
> + * @dev: Device to handle.
> + * @enable: True for setting the flag and False for clearing the flag.
> + *
> + * Set the flag for all devices in the path from the device to the
> + * root device in the device tree if @enable is true, otherwise clear
> + * the flag for devices in the path whose siblings don't set the flag.
> + *
> + * The function should only be called by block device, or network
> + * device driver for solving the deadlock problem during runtime
> + * resume/suspend:
> + * if memory allocation with GFP_KERNEL is called inside runtime
> + * resume/suspend callback of any one of its ancestors(or the
> + * block device itself), the deadlock may be triggered inside the
> + * memory allocation since it might not complete until the block
> + * device becomes active and the involed page I/O finishes. The
> + * situation is pointed out first by Alan Stern. Network device
> + * are involved in iSCSI kind of situation.
> + *
> + * The lock of dev_hotplug_mutex is held in the function for handling
> + * hotplug race because pm_runtime_set_memalloc_noio() may be called
> + * in async probe().
> + *
> + * The function should be called between device_add() and device_del()
> + * on the affected device(block/network device).
> + */
> +void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
> +{
> + static DEFINE_MUTEX(dev_hotplug_mutex);
> +
> + mutex_lock(&dev_hotplug_mutex);
> + for(;;) {
> + /* hold power lock since bitfield is not SMP-safe. */
> + spin_lock_irq(&dev->power.lock);
> + dev->power.memalloc_noio = enable;
> + spin_unlock_irq(&dev->power.lock);
> +
> + dev = dev->parent;
> +
> + /* only clear the flag for one device if all
> + * children of the device don't set the flag.
> + */
Such a comment is usually laid out as
/*
* Only ...
More significantly, the comment describes what the code is doing but
not why the code is doing it. The former is (usually) obvious from
reading the C, and the latter is what good code comments address.
And it's needed in this case. Why does the code do this?
Also, can a device have more than one child? If so, the code doesn't
do what the comment says it does.
> + if (!dev || (!enable &&
> + device_for_each_child(dev, NULL,
> + dev_memalloc_noio)))
> + break;
> + }
> + mutex_unlock(&dev_hotplug_mutex);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_set_memalloc_noio);
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Hi there! I'm now looking out for man!
From: Harper Saris @ 2012-11-06 23:23 UTC (permalink / raw)
To: netdev
Keeping Fit, although too much time in America recently messed that up Having Fun at weekends, getting out as much as i can!! Travel (Lots of hot holidays), Cars, Bikes. My career is something i take kinda seriously, and i have a bit of a passion for engineering and progressing my career. Love nights out and try to plan a holiday as much as i possibly can as i have a sun addiction and cant get enough of it! Live in T-wells area.I like art and music and seek perfection where possible.
^ permalink raw reply
* Re: [PATCH v4 1/6] mm: teach mm by current context info to not do I/O during memory allocation
From: Andrew Morton @ 2012-11-06 23:23 UTC (permalink / raw)
To: Ming Lei
Cc: linux-kernel, Alan Stern, Oliver Neukum, Minchan Kim,
Greg Kroah-Hartman, Rafael J. Wysocki, Jens Axboe,
David S. Miller, netdev, linux-usb, linux-pm, linux-mm,
Jiri Kosina, Mel Gorman, KAMEZAWA Hiroyuki, Michal Hocko,
Ingo Molnar, Peter Zijlstra
In-Reply-To: <1351931714-11689-2-git-send-email-ming.lei@canonical.com>
On Sat, 3 Nov 2012 16:35:09 +0800
Ming Lei <ming.lei@canonical.com> wrote:
> This patch introduces PF_MEMALLOC_NOIO on process flag('flags' field of
> 'struct task_struct'), so that the flag can be set by one task
> to avoid doing I/O inside memory allocation in the task's context.
>
> The patch trys to solve one deadlock problem caused by block device,
> and the problem may happen at least in the below situations:
>
> - during block device runtime resume, if memory allocation with
> GFP_KERNEL is called inside runtime resume callback of any one
> of its ancestors(or the block device itself), the deadlock may be
> triggered inside the memory allocation since it might not complete
> until the block device becomes active and the involed page I/O finishes.
> The situation is pointed out first by Alan Stern. It is not a good
> approach to convert all GFP_KERNEL[1] in the path into GFP_NOIO because
> several subsystems may be involved(for example, PCI, USB and SCSI may
> be involved for usb mass stoarage device, network devices involved too
> in the iSCSI case)
>
> - during block device runtime suspend, because runtime resume need
> to wait for completion of concurrent runtime suspend.
>
> - during error handling of usb mass storage deivce, USB bus reset
> will be put on the device, so there shouldn't have any
> memory allocation with GFP_KERNEL during USB bus reset, otherwise
> the deadlock similar with above may be triggered. Unfortunately, any
> usb device may include one mass storage interface in theory, so it
> requires all usb interface drivers to handle the situation. In fact,
> most usb drivers don't know how to handle bus reset on the device
> and don't provide .pre_set() and .post_reset() callback at all, so
> USB core has to unbind and bind driver for these devices. So it
> is still not practical to resort to GFP_NOIO for solving the problem.
>
> Also the introduced solution can be used by block subsystem or block
> drivers too, for example, set the PF_MEMALLOC_NOIO flag before doing
> actual I/O transfer.
>
> It is not a good idea to convert all these GFP_KERNEL in the
> affected path into GFP_NOIO because these functions doing that may be
> implemented as library and will be called in many other contexts.
>
> In fact, memalloc_noio() can convert some of current static GFP_NOIO
> allocation into GFP_KERNEL back in other non-affected contexts, at least
> almost all GFP_NOIO in USB subsystem can be converted into GFP_KERNEL
> after applying the approach and make allocation with GFP_IO
> only happen in runtime resume/bus reset/block I/O transfer contexts
> generally.
It's unclear from the description why we're also clearing __GFP_FS in
this situation.
If we can avoid doing this then there will be a very small gain: there
are some situations in which a filesystem can clean pagecache without
performing I/O.
It doesn't appear that the patch will add overhead to the alloc/free
hotpaths, which is good.
>
> ...
>
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1805,6 +1805,7 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
> #define PF_FROZEN 0x00010000 /* frozen for system suspend */
> #define PF_FSTRANS 0x00020000 /* inside a filesystem transaction */
> #define PF_KSWAPD 0x00040000 /* I am kswapd */
> +#define PF_MEMALLOC_NOIO 0x00080000 /* Allocating memory without IO involved */
> #define PF_LESS_THROTTLE 0x00100000 /* Throttle me less: I clean memory */
> #define PF_KTHREAD 0x00200000 /* I am a kernel thread */
> #define PF_RANDOMIZE 0x00400000 /* randomize virtual address space */
> @@ -1842,6 +1843,15 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
> #define tsk_used_math(p) ((p)->flags & PF_USED_MATH)
> #define used_math() tsk_used_math(current)
>
> +#define memalloc_noio() (current->flags & PF_MEMALLOC_NOIO)
> +#define memalloc_noio_save(flag) do { \
> + (flag) = current->flags & PF_MEMALLOC_NOIO; \
> + current->flags |= PF_MEMALLOC_NOIO; \
> +} while (0)
> +#define memalloc_noio_restore(flag) do { \
> + current->flags = (current->flags & ~PF_MEMALLOC_NOIO) | flag; \
> +} while (0)
> +
Again with the ghastly macros. Please, do this properly in regular old
C, as previously discussed. It really doesn't matter what daft things
local_irq_save() did 20 years ago. Just do it right!
Also, you can probably put the unlikely() inside memalloc_noio() and
avoid repeating it at all the callsites.
And it might be neater to do:
/*
* Nice comment goes here
*/
static inline gfp_t memalloc_noio_flags(gfp_t flags)
{
if (unlikely(current->flags & PF_MEMALLOC_NOIO))
flags &= ~GFP_IOFS;
return flags;
}
> * task->jobctl flags
> */
>
> ...
>
> @@ -2304,6 +2304,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
> .gfp_mask = sc.gfp_mask,
> };
>
> + if (unlikely(memalloc_noio())) {
> + gfp_mask &= ~GFP_IOFS;
> + sc.gfp_mask = gfp_mask;
> + shrink.gfp_mask = sc.gfp_mask;
> + }
We can avoid writing to shrink.gfp_mask twice. And maybe sc.gfp_mask
as well. Unclear, I didn't think about it too hard ;)
> throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
>
> /*
>
> ...
>
--
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: [PATCH v4 0/6] solve deadlock caused by memory allocation with I/O
From: Andrew Morton @ 2012-11-06 23:23 UTC (permalink / raw)
To: Ming Lei
Cc: linux-kernel, Alan Stern, Oliver Neukum, Minchan Kim,
Greg Kroah-Hartman, Rafael J. Wysocki, Jens Axboe,
David S. Miller, netdev, linux-usb, linux-pm, linux-mm,
Rafael J. Wysocki
In-Reply-To: <1351931714-11689-1-git-send-email-ming.lei@canonical.com>
On Sat, 3 Nov 2012 16:35:08 +0800
Ming Lei <ming.lei@canonical.com> wrote:
> This patchset try to solve one deadlock problem which might be caused
> by memory allocation with block I/O during runtime PM and block device
> error handling path. Traditionly, the problem is addressed by passing
> GFP_NOIO statically to mm, but that is not a effective solution, see
> detailed description in patch 1's commit log.
It generally looks OK to me. I have a few comments and I expect to grab
v5.
Rafael, your thoughts?
--
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: [PATCH v3 5/7] pppoatm: take ATM socket lock in pppoatm_send()
From: Woodhouse, David @ 2012-11-06 22:57 UTC (permalink / raw)
To: Krzysztof Mazur
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Chas Williams - CONTRACTOR, davem@davemloft.net
In-Reply-To: <1352240222-363-6-git-send-email-krzysiek@podlesie.net>
On Tue, 2012-11-06 at 23:17 +0100, Krzysztof Mazur wrote:
> + if (sock_owned_by_user(sk_atm(vcc)))
> + goto nospace;
I still think this one can lead to an infinite stall of the PPP channel,
because we return 0 from pppoatm_send() but never make a later call to
ppp_output_wakeup() to unblock it.
In the existing cases where we could return 0 (because
pppoatm_may_send() said no), we were careful to set the BLOCKED flag and
we *knew* that a packet was in flight so we'd get to wake it up again on
a subsequent pppoatm_pop().
None of that works for this new code path that returns zero.
--
Sent with MeeGo's ActiveSync support.
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* [PATCH v3 6/7] pppoatm: don't send frames on not-ready vcc
From: Krzysztof Mazur @ 2012-11-06 22:17 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, Chas Williams - CONTRACTOR, David Woodhouse, davem,
Krzysztof Mazur
In-Reply-To: <1352240222-363-1-git-send-email-krzysiek@podlesie.net>
Patches "atm: detach protocol before closing vcc"
and "pppoatm: allow assign only on a connected socket" fixed
common cases where the pppoatm_send() crashes while sending
frame to not-ready vcc. However there are still some other cases
where we can send frames to vcc, which is flagged as ATM_VF_CLOSE
(for instance after vcc_release_async()) or it's opened but not
ready yet.
Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready.
Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Cc: David Woodhouse <David.Woodhouse@intel.com>
---
net/atm/pppoatm.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index c4a57bc..bf5d6c9 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -284,6 +284,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
bh_lock_sock(sk_atm(vcc));
if (sock_owned_by_user(sk_atm(vcc)))
goto nospace;
+ if (test_bit(ATM_VF_RELEASED, &vcc->flags)
+ || test_bit(ATM_VF_CLOSE, &vcc->flags)
+ || !test_bit(ATM_VF_READY, &vcc->flags))
+ goto nospace;
switch (pvcc->encaps) { /* LLC encapsulation needed */
case e_llc:
--
1.8.0.233.g54991f2
^ permalink raw reply related
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