| Message ID | 20170109150404.30215.44512.stgit@firesoul |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Jan 9, 2017 at 7:04 AM, Jesper Dangaard Brouer <brouer@redhat.com> wrote: > This reverts commit 9a99d4a50cb8 ("icmp: avoid allocating large struct > on stack"), because struct icmp_bxm no really a large struct, and > allocating and free of this small 112 bytes hurts performance. The original commit fixes a warning for large stack usage, icmp_send() is deep in the call stack. Your optimization for a slow path makes no sense to me.
On Mon, 2017-01-09 at 16:04 +0100, Jesper Dangaard Brouer wrote: > This reverts commit 9a99d4a50cb8 ("icmp: avoid allocating large struct > on stack"), because struct icmp_bxm no really a large struct, and > allocating and free of this small 112 bytes hurts performance. > > Fixes: 9a99d4a50cb8 ("icmp: avoid allocating large struct on stack") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- Acked-by: Eric Dumazet <edumazet@google.com>
On Mon, 2017-01-09 at 09:42 -0800, Cong Wang wrote: > On Mon, Jan 9, 2017 at 7:04 AM, Jesper Dangaard Brouer > <brouer@redhat.com> wrote: > > This reverts commit 9a99d4a50cb8 ("icmp: avoid allocating large struct > > on stack"), because struct icmp_bxm no really a large struct, and > > allocating and free of this small 112 bytes hurts performance. > > The original commit fixes a warning for large stack usage, icmp_send() > is deep in the call stack. > > Your optimization for a slow path makes no sense to me. Do you have the stack trace of this event ? Even Linus allowed vmalloc() kernel stacks, while it certainly was an heresy 10 years ago. I doubt it makes a difference trying to save 104 bytes of kernel stack.
On Mon, Jan 9, 2017 at 9:50 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Mon, 2017-01-09 at 09:42 -0800, Cong Wang wrote: >> On Mon, Jan 9, 2017 at 7:04 AM, Jesper Dangaard Brouer >> <brouer@redhat.com> wrote: >> > This reverts commit 9a99d4a50cb8 ("icmp: avoid allocating large struct >> > on stack"), because struct icmp_bxm no really a large struct, and >> > allocating and free of this small 112 bytes hurts performance. >> >> The original commit fixes a warning for large stack usage, icmp_send() >> is deep in the call stack. >> >> Your optimization for a slow path makes no sense to me. > > Do you have the stack trace of this event ? > > Even Linus allowed vmalloc() kernel stacks, while it certainly was an > heresy 10 years ago. > > I doubt it makes a difference trying to save 104 bytes of kernel stack. I think you should have known this, quote from Eric Dumazet (hopefully the same one): On Fri, 2013-05-31 at 22:22 -0700, Eric Dumazet wrote: Strange, I posted a patch like that some days ago. which is from: https://patchwork.ozlabs.org/patch/248051/ Facepalm...
On Mon, 2017-01-09 at 09:59 -0800, Cong Wang wrote: > On Mon, Jan 9, 2017 at 9:50 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > On Mon, 2017-01-09 at 09:42 -0800, Cong Wang wrote: > >> On Mon, Jan 9, 2017 at 7:04 AM, Jesper Dangaard Brouer > >> <brouer@redhat.com> wrote: > >> > This reverts commit 9a99d4a50cb8 ("icmp: avoid allocating large struct > >> > on stack"), because struct icmp_bxm no really a large struct, and > >> > allocating and free of this small 112 bytes hurts performance. > >> > >> The original commit fixes a warning for large stack usage, icmp_send() > >> is deep in the call stack. > >> > >> Your optimization for a slow path makes no sense to me. > > > > Do you have the stack trace of this event ? > > > > Even Linus allowed vmalloc() kernel stacks, while it certainly was an > > heresy 10 years ago. > > > > I doubt it makes a difference trying to save 104 bytes of kernel stack. > > I think you should have known this, quote from Eric Dumazet > (hopefully the same one): > > On Fri, 2013-05-31 at 22:22 -0700, Eric Dumazet wrote: > > Strange, I posted a patch like that some days ago. > > which is from: https://patchwork.ozlabs.org/patch/248051/ > > Facepalm... We are in 2017. Whatever was said in 2013 is irrelevant. You really should come to netdev conferences so that you understand goals and efforts, instead of living in your cave. Then you can slap me in the face, since this is obviously your desire. Then, we will drink a beer and relax.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 9 Jan 2017 09:59:34 -0800 > Facepalm... This is completely unnecessary. Discuss facts and real issues, rather than make emotional retorts. Thank you.
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 09 Jan 2017 10:07:04 -0800 > You really should come to netdev conferences so that you understand > goals and efforts, instead of living in your cave. I completely agree with Eric. Cong we have a very serious problem with you exactly because you make quite vicious emotional statements targetted at other developers merely when they say something you disagree with. This is completely unacceptable behavior, and you must stop doing this, now. And I am absolutely positive that if you had met us all in person at netdev you would not be treating us like this. So please do us a _HUGE_ favor, and leave your cave, and come to an upcoming netdev. Thank you.
On Mon, 2017-01-09 at 10:07 -0800, Eric Dumazet wrote: > On Mon, 2017-01-09 at 09:59 -0800, Cong Wang wrote: [] > > Facepalm... [] > We are in 2017. Whatever was said in 2013 is irrelevant. Unless you morph into the PEOTUS, as a general statement, this is untrue.
On Mon, 09 Jan 2017 13:52:59 -0500 (EST) David Miller <davem@davemloft.net> wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 09 Jan 2017 10:07:04 -0800 > > > You really should come to netdev conferences so that you understand > > goals and efforts, instead of living in your cave. > > I completely agree with Eric. > > Cong we have a very serious problem with you exactly because you make > quite vicious emotional statements targetted at other developers > merely when they say something you disagree with. > > This is completely unacceptable behavior, and you must stop doing > this, now. I agree, and it is even documented in: Documentation/process/code-of-conflict.rst Quote: "As a reviewer of code, please strive to keep things civil and focused on the technical issues involved. [...]" https://www.kernel.org/doc/html/latest/process/code-of-conflict.html
On Mon, Jan 9, 2017 at 10:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > We are in 2017. Whatever was said in 2013 is irrelevant. > > You really should come to netdev conferences so that you understand > goals and efforts, instead of living in your cave. > > Then you can slap me in the face, since this is obviously your desire. > > Then, we will drink a beer and relax. LOL... Eric, you really need to focus on the technical part, which is you asked for a question you already knew the answer 4 years ago, and now you are going to be against yourself. /me don't contribute anything expect helping you to remember what you said.
On Mon, Jan 9, 2017 at 10:52 AM, David Miller <davem@davemloft.net> wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 09 Jan 2017 10:07:04 -0800 > >> You really should come to netdev conferences so that you understand >> goals and efforts, instead of living in your cave. > > I completely agree with Eric. > > Cong we have a very serious problem with you exactly because you make > quite vicious emotional statements targetted at other developers > merely when they say something you disagree with. What emotional? Pointing out Eris's words from 4 years ago is NOT emotional, it is just a help. > > This is completely unacceptable behavior, and you must stop doing > this, now. > > And I am absolutely positive that if you had met us all in person at > netdev you would not be treating us like this. This is not an invitation from any point of view, David. > > So please do us a _HUGE_ favor, and leave your cave, and come to > an upcoming netdev. > Not everyone is as free to travel to Canada without a visa as you, unfortunately.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Tue, 10 Jan 2017 10:06:01 -0800 > On Mon, Jan 9, 2017 at 10:52 AM, David Miller <davem@davemloft.net> wrote: >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Mon, 09 Jan 2017 10:07:04 -0800 >> >>> You really should come to netdev conferences so that you understand >>> goals and efforts, instead of living in your cave. >> >> I completely agree with Eric. >> >> Cong we have a very serious problem with you exactly because you make >> quite vicious emotional statements targetted at other developers >> merely when they say something you disagree with. > > What emotional? Pointing out Eris's words from 4 years ago is NOT > emotional, it is just a help. Saying "Facepalm" is emotional and has nothing to do with the technical issues. You can keep showing us how expertly you can deflect the real issue we are discussion here, but that won't improve the situation at all I am afraid. > Not everyone is as free to travel to Canada without a visa as you, > unfortunately. We hold netdev in other countries all over the world, stop making excuses.
On Tue, Jan 10, 2017 at 10:12 AM, David Miller <davem@davemloft.net> wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Tue, 10 Jan 2017 10:06:01 -0800 > >> On Mon, Jan 9, 2017 at 10:52 AM, David Miller <davem@davemloft.net> wrote: >>> From: Eric Dumazet <eric.dumazet@gmail.com> >>> Date: Mon, 09 Jan 2017 10:07:04 -0800 >>> >>>> You really should come to netdev conferences so that you understand >>>> goals and efforts, instead of living in your cave. >>> >>> I completely agree with Eric. >>> >>> Cong we have a very serious problem with you exactly because you make >>> quite vicious emotional statements targetted at other developers >>> merely when they say something you disagree with. >> >> What emotional? Pointing out Eris's words from 4 years ago is NOT >> emotional, it is just a help. > > Saying "Facepalm" is emotional and has nothing to do with the > technical issues. Facepalm is heavily used on Internet: https://en.wikipedia.org/wiki/Facepalm#Internet_usage it is how people including me react to disappointing. It is those who only see facepalm make it irrelevant to the technical discussion in this thread. > > You can keep showing us how expertly you can deflect the real > issue we are discussion here, but that won't improve the situation > at all I am afraid. Of course, there are just too many people too lazy to do a google search: https://lists.debian.org/debian-kernel/2013/05/msg00500.html > >> Not everyone is as free to travel to Canada without a visa as you, >> unfortunately. > > We hold netdev in other countries all over the world, stop making > excuses. The only countries you hold netdev are Canada, Japan and Spain (to my knowledge). If you check: https://en.wikipedia.org/wiki/Visa_requirements_for_Chinese_citizens#Visa_requirements It is very easy to find out if it is an excuse or a fact.
On Tue, Jan 10, 2017 at 10:44 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > Of course, there are just too many people too lazy to do a google search: > > https://lists.debian.org/debian-kernel/2013/05/msg00500.html > One more: https://communities.intel.com/thread/28935 (<-- search icmp_send)
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Tue, 10 Jan 2017 10:44:59 -0800 > On Tue, Jan 10, 2017 at 10:12 AM, David Miller <davem@davemloft.net> wrote: > Facepalm is heavily used on Internet: > https://en.wikipedia.org/wiki/Facepalm#Internet_usage > > it is how people including me react to disappointing. Everyone here, except you, believe that this is rude behavior. Keep coming up with your own reasons why you are special and can behave this way. > The only countries you hold netdev are Canada, Japan and Spain > (to my knowledge). If you check: > > https://en.wikipedia.org/wiki/Visa_requirements_for_Chinese_citizens#Visa_requirements > > It is very easy to find out if it is an excuse or a fact. The conference explciitly offers VISA help for anyone who needs it. Many people come to the conference on a VISA we helped them obtain. It is not hard to do. You have put in exactly zero effort in trying to solve this problem. If you had simply contacted the conference organizers asking for help, you would have gotten all the help you needed. In fact we explicitly asked you to attend, and you turned us down. So don't say that every effort hasn't been made to get you into the conference.
On Tue, 10 Jan 2017 10:44:59 -0800 Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Tue, Jan 10, 2017 at 10:12 AM, David Miller <davem@davemloft.net> wrote: [...] > > You can keep showing us how expertly you can deflect the real > > issue we are discussion here, but that won't improve the situation > > at all I am afraid. > > Of course, there are just too many people too lazy to do a google search: > > https://lists.debian.org/debian-kernel/2013/05/msg00500.html My analysis of the problem shown in above link is not related to using all the stack space, but instead that skb->cb was not cleared. This can cause the ip_options_echo() call in icmp_send() to access garbage as this is: __ip_options_echo(dopt, skb, &IPCB(skb)->opt). Fixed by commit a622260254ee ("ip_tunnel: fix kernel panic with icmp_dest_unreach") https://git.kernel.org/torvalds/c/a622260254ee Thus, it is (likely) the __ip_options_echo() call that violates stack access, as it is passed in a pointer to the stack, and advance this based on garbage "optlen".
On Tue, 2017-01-10 at 13:12 -0500, David Miller wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Tue, 10 Jan 2017 10:06:01 -0800 > > > On Mon, Jan 9, 2017 at 10:52 AM, David Miller <davem@davemloft.net> wrote: > >> From: Eric Dumazet <eric.dumazet@gmail.com> > >> Date: Mon, 09 Jan 2017 10:07:04 -0800 > >> > >>> You really should come to netdev conferences so that you understand > >>> goals and efforts, instead of living in your cave. > >> > >> I completely agree with Eric. > >> > >> Cong we have a very serious problem with you exactly because you make > >> quite vicious emotional statements targetted at other developers "quite vicious" is overstatement here. > >> merely when they say something you disagree with. > > > > What emotional? Pointing out Eris's words from 4 years ago is NOT > > emotional, it is just a help. Not really. Your "facepalm" didn't seem to be written as humor but more as an expression of your exasperation. > Saying "Facepalm" is emotional and has nothing to do with the > technical issues. Many words are emotional or conflictual. Saying "stupid" is emotional and also has no technical content. Stupid is used here all the time. Happily, it's generally used as a reference to self and not about others. However it is still occasionally and unfortunately used when referencing proposed code or other person's behaviors. Thankfully, these types of words are being used less and less in discussions here. I agree with David and Eric that it's generally better to avoid these word choices.
On Tue, 2017-01-10 at 21:08 +0100, Jesper Dangaard Brouer wrote: > On Tue, 10 Jan 2017 10:44:59 -0800 Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > On Tue, Jan 10, 2017 at 10:12 AM, David Miller <davem@davemloft.net> wrote: > [...] > > > You can keep showing us how expertly you can deflect the real > > > issue we are discussion here, but that won't improve the situation > > > at all I am afraid. > > > > Of course, there are just too many people too lazy to do a google search: > > > > https://lists.debian.org/debian-kernel/2013/05/msg00500.html > > My analysis of the problem shown in above link is not related to using > all the stack space, but instead that skb->cb was not cleared. This > can cause the ip_options_echo() call in icmp_send() to access garbage > as this is: __ip_options_echo(dopt, skb, &IPCB(skb)->opt). > > Fixed by commit a622260254ee ("ip_tunnel: fix kernel panic with icmp_dest_unreach") > https://git.kernel.org/torvalds/c/a622260254ee > > Thus, it is (likely) the __ip_options_echo() call that violates stack > access, as it is passed in a pointer to the stack, and advance this > based on garbage "optlen". > I totally agree. This can not be stack being too small in current kernels. > #0 [ffff88003fd03798] machine_kexec at ffffffff81027430 > #1 [ffff88003fd037e8] crash_kexec at ffffffff8107da80 > #2 [ffff88003fd038b8] panic at ffffffff81540026 > #3 [ffff88003fd03938] __stack_chk_fail at ffffffff81037f77 > #4 [ffff88003fd03948] icmp_send at ffffffff814d5fec > #5 [ffff88003fd03b78] dev_hard_start_xmit at ffffffff8146e032 > #6 [ffff88003fd03bc8] sch_direct_xmit at ffffffff81487d66 > #7 [ffff88003fd03c08] __qdisc_run at ffffffff81487efd > #8 [ffff88003fd03c48] dev_queue_xmit at ffffffff8146e5a7 > #9 [ffff88003fd03c88] ip_finish_output at ffffffff814ab596 > #10 [ffff88003fd03ce8] __netif_receive_skb at ffffffff8146ed13 > #11 [ffff88003fd03d88] napi_gro_receive at ffffffff8146fc50 > #12 [ffff88003fd03da8] e1000_clean_rx_irq at ffffffff813bc67b > #13 [ffff88003fd03e48] e1000e_poll at ffffffff813c3a20 > #14 [ffff88003fd03e98] net_rx_action at ffffffff8146f796 > #15 [ffff88003fd03ee8] __do_softirq at ffffffff8103ebb9 > #16 [ffff88003fd03f38] call_softirq at ffffffff8154444c > #17 [ffff88003fd03f50] do_softirq at ffffffff810047dd > #18 [ffff88003fd03f80] do_IRQ at ffffffff81003f6c Total stack used is about 3FFF - 3938, which is less than 2KB. x86_64 is supposed to have at least 16 KB irq stacks.
On Tue, Jan 10, 2017 at 1:48 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2017-01-10 at 21:08 +0100, Jesper Dangaard Brouer wrote: >> On Tue, 10 Jan 2017 10:44:59 -0800 Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> > On Tue, Jan 10, 2017 at 10:12 AM, David Miller <davem@davemloft.net> wrote: >> [...] >> > > You can keep showing us how expertly you can deflect the real >> > > issue we are discussion here, but that won't improve the situation >> > > at all I am afraid. >> > >> > Of course, there are just too many people too lazy to do a google search: >> > >> > https://lists.debian.org/debian-kernel/2013/05/msg00500.html >> >> My analysis of the problem shown in above link is not related to using >> all the stack space, but instead that skb->cb was not cleared. This >> can cause the ip_options_echo() call in icmp_send() to access garbage >> as this is: __ip_options_echo(dopt, skb, &IPCB(skb)->opt). >> >> Fixed by commit a622260254ee ("ip_tunnel: fix kernel panic with icmp_dest_unreach") >> https://git.kernel.org/torvalds/c/a622260254ee >> >> Thus, it is (likely) the __ip_options_echo() call that violates stack >> access, as it is passed in a pointer to the stack, and advance this >> based on garbage "optlen". >> > > I totally agree. I can't agree, iptunnel or ipgre symbols are not in the above stack trace at all. Although I do agree that the above stack usage is not aggressive, especially when compared with the other I sent. My vague memory told me the original problem I fixed is related to vxlan but after trying to search all netdev archives in 2013 May/Jun, I still can't find it, perhaps it was reported to LKML or somewhere else rather than netdev. It was certainly a real problem. Even though the irq stack is 16K, but it is too easy to stack netdevices and stack qdisc's too, so for TX path I am not surprised at all if 16K could be exhausted eventually. Yeah, it is hard to blame one of them in the call chain, but 112 bytes _alone_ are aggressive for such a function deeply in the call stack. That's my whole point.
On Tue, Jan 10, 2017 at 10:54 AM, David Miller <davem@davemloft.net> wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Tue, 10 Jan 2017 10:44:59 -0800 >> The only countries you hold netdev are Canada, Japan and Spain >> (to my knowledge). If you check: >> >> https://en.wikipedia.org/wiki/Visa_requirements_for_Chinese_citizens#Visa_requirements >> >> It is very easy to find out if it is an excuse or a fact. > > The conference explciitly offers VISA help for anyone who needs it. > Many people come to the conference on a VISA we helped them obtain. I never complain about visa sponsorship or money, this is the last problem for me to consider. > > It is not hard to do. You have put in exactly zero effort in trying > to solve this problem. If you had simply contacted the conference > organizers asking for help, you would have gotten all the help you > needed. If obtaining a visa were as easy as obtaining a sponsorship, I would go as many conferences as I know. But the fact is there are already too many hassles to obtain even _one_ visa, not to mention I need to obtain _two_ visas to go to Canada (or Japan, Spain) and return to US. I'd be very happy to attend it if it were held in US, but this never happens.
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 0777ea949223..b4b9807329a7 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -571,7 +571,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) { struct iphdr *iph; int room; - struct icmp_bxm *icmp_param; + struct icmp_bxm icmp_param; struct rtable *rt = skb_rtable(skb_in); struct ipcm_cookie ipc; struct flowi4 fl4; @@ -648,13 +648,9 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) } } - icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC); - if (!icmp_param) - return; - sk = icmp_xmit_lock(net); if (!sk) - goto out_free; + return; /* * Construct source address and options. @@ -681,7 +677,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) iph->tos; mark = IP4_REPLY_MARK(net, skb_in->mark); - if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in)) + if (ip_options_echo(&icmp_param.replyopts.opt.opt, skb_in)) goto out_unlock; @@ -689,22 +685,22 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) * Prepare data for ICMP header. */ - icmp_param->data.icmph.type = type; - icmp_param->data.icmph.code = code; - icmp_param->data.icmph.un.gateway = info; - icmp_param->data.icmph.checksum = 0; - icmp_param->skb = skb_in; - icmp_param->offset = skb_network_offset(skb_in); + icmp_param.data.icmph.type = type; + icmp_param.data.icmph.code = code; + icmp_param.data.icmph.un.gateway = info; + icmp_param.data.icmph.checksum = 0; + icmp_param.skb = skb_in; + icmp_param.offset = skb_network_offset(skb_in); inet_sk(sk)->tos = tos; sk->sk_mark = mark; ipc.addr = iph->saddr; - ipc.opt = &icmp_param->replyopts.opt; + ipc.opt = &icmp_param.replyopts.opt; ipc.tx_flags = 0; ipc.ttl = 0; ipc.tos = -1; rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr, tos, mark, - type, code, icmp_param); + type, code, &icmp_param); if (IS_ERR(rt)) goto out_unlock; @@ -716,21 +712,19 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) room = dst_mtu(&rt->dst); if (room > 576) room = 576; - room -= sizeof(struct iphdr) + icmp_param->replyopts.opt.opt.optlen; + room -= sizeof(struct iphdr) + icmp_param.replyopts.opt.opt.optlen; room -= sizeof(struct icmphdr); - icmp_param->data_len = skb_in->len - icmp_param->offset; - if (icmp_param->data_len > room) - icmp_param->data_len = room; - icmp_param->head_len = sizeof(struct icmphdr); + icmp_param.data_len = skb_in->len - icmp_param.offset; + if (icmp_param.data_len > room) + icmp_param.data_len = room; + icmp_param.head_len = sizeof(struct icmphdr); - icmp_push_reply(icmp_param, &fl4, &ipc, &rt); + icmp_push_reply(&icmp_param, &fl4, &ipc, &rt); ende: ip_rt_put(rt); out_unlock: icmp_xmit_unlock(sk); -out_free: - kfree(icmp_param); out:; } EXPORT_SYMBOL(icmp_send);
This reverts commit 9a99d4a50cb8 ("icmp: avoid allocating large struct on stack"), because struct icmp_bxm no really a large struct, and allocating and free of this small 112 bytes hurts performance. Fixes: 9a99d4a50cb8 ("icmp: avoid allocating large struct on stack") Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- net/ipv4/icmp.c | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-)