[tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
We fix a very real starvation problem that may occur when the link level runs into send buffer congestion. At the same time we make the interaction between the socket and link layer simpler and more consistent. v2: - Simplified link congestion check to only check against own importance limit. This reduces the risk of higher levels starving out lower levels. Jon Maloy (3): tipc: unify tipc_wait_for_sndpkt() and tipc_wait_for_sndmsg() functions tipc: modify struct tipc_plist to be more versatile tipc: reduce risk of user starvation during link congestion net/tipc/bcast.c | 2 +- net/tipc/link.c | 81 - net/tipc/msg.h| 8 +- net/tipc/name_table.c | 100 +++ net/tipc/name_table.h | 21 +-- net/tipc/node.c | 2 +- net/tipc/socket.c | 450 ++ 7 files changed, 327 insertions(+), 337 deletions(-) -- 2.7.4 -- ___ tipc-discussion mailing list tipc-discussion@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tipc-discussion
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
Ying, Partha, It would be very nice I could get "acked" or "reviewed" on this so I can send it to David before net-next closes. ///jon > -Original Message- > From: Jon Maloy [mailto:jon.ma...@ericsson.com] > Sent: Tuesday, 29 November, 2016 12:04 > To: tipc-discussion@lists.sourceforge.net; Parthasarathy Bhuvaragan > ; Ying Xue > ; Jon Maloy > Cc: ma...@donjonn.com; thompa@gmail.com > Subject: [PATCH net-next v2 0/3] tipc: improve interaction socket-link > > We fix a very real starvation problem that may occur when the link > level runs into send buffer congestion. At the same time we make the > interaction between the socket and link layer simpler and more > consistent. > > v2: - Simplified link congestion check to only check against own > importance limit. This reduces the risk of higher levels > starving out lower levels. > > Jon Maloy (3): > tipc: unify tipc_wait_for_sndpkt() and tipc_wait_for_sndmsg() > functions > tipc: modify struct tipc_plist to be more versatile > tipc: reduce risk of user starvation during link congestion > > net/tipc/bcast.c | 2 +- > net/tipc/link.c | 81 - > net/tipc/msg.h| 8 +- > net/tipc/name_table.c | 100 +++ > net/tipc/name_table.h | 21 +-- > net/tipc/node.c | 2 +- > net/tipc/socket.c | 450 > ++ > 7 files changed, 327 insertions(+), 337 deletions(-) > > -- > 2.7.4 -- ___ tipc-discussion mailing list tipc-discussion@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tipc-discussion
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
Hi Jon, With your patches, I get the following crash when loading the tipc module. Leaving home now, so couldnt investigate further. [ 58.201114] tipc: Started in single node mode [ 58.212991] Started in network mode [ 58.213796] Own node address <1.1.1>, network identity 4711 [ 58.238416] 8021q: adding VLAN 0 to HW filter on device data0 [ 58.252217] 8021q: adding VLAN 0 to HW filter on device data1 [ 58.270822] Enabled bearer , discovery domain <1.1.0>, priority 10 [ 58.571114] general protection fault: [#1] SMP [ 58.572031] Modules linked in: tipc ip6_udp_tunnel udp_tunnel 9pnet_virtio 9p 9pnet virtio_net virtio_pci virtio_ring virtio [ 58.572031] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc6+ #15 [ 58.572031] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 58.572031] task: 81c0d540 task.stack: 81c0 [ 58.572031] RIP: 0010:[] [] skb_release_head_state+0x4d/0xa0 [ 58.572031] RSP: 0018:880037c03ba0 EFLAGS: 00010246 [ 58.572031] RAX: 0001 RBX: 880033fffa00 RCX: 00ff [ 58.572031] RDX: RSI: 880037c03bca RDI: 880033fffa00 [ 58.572031] RBP: 880037c03ba8 R08: a005f2c0 R09: [ 58.572031] R10: 880035b0f0a0 R11: ea00 R12: 880033fffa00 [ 58.572031] R13: a0048fd4 R14: 81cfbec0 R15: 880033718000 [ 58.572031] FS: () GS:880037c0() knlGS: [ 58.572031] CS: 0010 DS: ES: CR0: 80050033 [ 58.572031] CR2: 00851bf0 CR3: 35b0 CR4: 06f0 [ 58.572031] Stack: [ 58.572031] 880033fffa00 880037c03bc0 8162f2b2 880033fffa00 [ 58.572031] 880037c03be8 8162f327 880033fffa00 [ 58.572031] 880035b32540 880037c03c68 a0048fd4 0082 [ 58.572031] Call Trace: [ 58.572031] [ 58.572031] [] skb_release_all+0x12/0x30 [ 58.572031] [] kfree_skb+0x37/0xa0 [ 58.572031] [] tipc_disc_rcv+0x84/0x1d0 [tipc] [ 58.572031] [] tipc_rcv+0x3ac/0x3c0 [tipc] [ 58.572031] [] ? find_busiest_group+0x117/0x940 [ 58.572031] [] tipc_l2_rcv_msg+0x48/0x60 [tipc] [ 58.572031] [] __netif_receive_skb_core+0x2e5/0xa60 [ 58.572031] [] ? __build_skb+0x2a/0xe0 [ 58.572031] [] ? __build_skb+0x2a/0xe0 [ 58.572031] [] __netif_receive_skb+0x1b/0x70 [ 58.572031] [] netif_receive_skb_internal+0x2d/0x90 [ 58.572031] [] napi_gro_receive+0x94/0x130 [ 58.572031] [] virtnet_receive+0x1a5/0x8a0 [virtio_net] [ 58.572031] [] virtnet_poll+0x1d/0x80 [virtio_net] [ 58.572031] [] net_rx_action+0x20e/0x390 [ 58.572031] [] __do_softirq+0x9b/0x2a2 [ 58.572031] [] irq_exit+0x60/0x70 [ 58.572031] [] do_IRQ+0x54/0xd0 [ 58.572031] [] common_interrupt+0x7f/0x7f [ 58.572031] [ 58.572031] [] ? default_idle+0x20/0xe0 [ 58.572031] [] ? next_zone+0x29/0x30 [ 58.572031] [] arch_cpu_idle+0xf/0x20 [ 58.572031] [] default_idle_call+0x2c/0x30 [ 58.572031] [] cpu_startup_entry+0x177/0x1e0 [ 58.572031] [] rest_init+0x77/0x80 [ 58.572031] [] start_kernel+0x40e/0x41b [ 58.572031] [] x86_64_start_reservations+0x2a/0x2c [ 58.572031] [] x86_64_start_kernel+0xea/0xed [ 58.572031] Code: 00 00 48 8b 7b 68 48 85 ff 74 05 f0 ff 0f 74 36 48 8b 43 60 48 85 c0 74 14 65 8b 15 96 d3 9d 7e 81 e2 00 00 0f 00 75 30 48 89 df d0 48 8b 7b 70 48 85 ff 74 05 f0 ff 0f 74 03 5b 5d c3 e8 bb [ 58.572031] RIP [] skb_release_head_state+0x4d/0xa0 [ 58.572031] RSP [ 58.662814] ---[ end trace fa57695d3ce8757f ]--- [ 58.663875] Kernel panic - not syncing: Fatal exception in interrupt [ 58.664872] Kernel Offset: disabled [ 58.664872] ---[ end Kernel panic - not syncing: Fatal exception in interrupt regards Partha On 11/29/2016 06:07 PM, Jon Maloy wrote: > Ying, Partha, > It would be very nice I could get "acked" or "reviewed" on this so I can send > it to David before net-next closes. > > ///jon > > >> -Original Message- >> From: Jon Maloy [mailto:jon.ma...@ericsson.com] >> Sent: Tuesday, 29 November, 2016 12:04 >> To: tipc-discussion@lists.sourceforge.net; Parthasarathy Bhuvaragan >> ; Ying Xue >> ; Jon Maloy >> Cc: ma...@donjonn.com; thompa@gmail.com >> Subject: [PATCH net-next v2 0/3] tipc: improve interaction socket-link >> >> We fix a very real starvation problem that may occur when the link >> level runs into send buffer congestion. At the same time we make the >> interaction between the socket and link layer simpler and more >> consistent. >> >> v2: - Simplified link congestion check to only check against own >> importance limit. This reduces the risk of higher levels >> starving out lower levels. >> >> Jon Maloy (3): >> tipc: unify tipc_wait_for_sndpkt() and tipc_wait_for_sndmsg() >> functions >> tipc: modify struct tipc_plist to be more versatile >> tipc: reduce risk of user starvation du
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
Weird. Looks like a corrupted incoming buffer directly at startup, before any of my new code is active. Is this repeatable? ///jon On 11/30/2016 08:52 AM, Parthasarathy Bhuvaragan wrote: > Hi Jon, > > With your patches, I get the following crash when loading the tipc > module. Leaving home now, so couldnt investigate further. > > [ 58.201114] tipc: Started in single node mode > [ 58.212991] Started in network mode > [ 58.213796] Own node address <1.1.1>, network identity 4711 > [ 58.238416] 8021q: adding VLAN 0 to HW filter on device data0 > [ 58.252217] 8021q: adding VLAN 0 to HW filter on device data1 > [ 58.270822] Enabled bearer , discovery domain <1.1.0>, > priority 10 > [ 58.571114] general protection fault: [#1] SMP > [ 58.572031] Modules linked in: tipc ip6_udp_tunnel udp_tunnel > 9pnet_virtio 9p 9pnet virtio_net virtio_pci virtio_ring virtio > [ 58.572031] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc6+ #15 > [ 58.572031] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 > [ 58.572031] task: 81c0d540 task.stack: 81c0 > [ 58.572031] RIP: 0010:[] [] > skb_release_head_state+0x4d/0xa0 > [ 58.572031] RSP: 0018:880037c03ba0 EFLAGS: 00010246 > [ 58.572031] RAX: 0001 RBX: 880033fffa00 RCX: > 00ff > [ 58.572031] RDX: RSI: 880037c03bca RDI: > 880033fffa00 > [ 58.572031] RBP: 880037c03ba8 R08: a005f2c0 R09: > > [ 58.572031] R10: 880035b0f0a0 R11: ea00 R12: > 880033fffa00 > [ 58.572031] R13: a0048fd4 R14: 81cfbec0 R15: > 880033718000 > [ 58.572031] FS: () GS:880037c0() > knlGS: > [ 58.572031] CS: 0010 DS: ES: CR0: 80050033 > [ 58.572031] CR2: 00851bf0 CR3: 35b0 CR4: > 06f0 > [ 58.572031] Stack: > [ 58.572031] 880033fffa00 880037c03bc0 8162f2b2 > 880033fffa00 > [ 58.572031] 880037c03be8 8162f327 880033fffa00 > > [ 58.572031] 880035b32540 880037c03c68 a0048fd4 > 0082 > [ 58.572031] Call Trace: > [ 58.572031] [ 58.572031] [] > skb_release_all+0x12/0x30 > [ 58.572031] [] kfree_skb+0x37/0xa0 > [ 58.572031] [] tipc_disc_rcv+0x84/0x1d0 [tipc] > [ 58.572031] [] tipc_rcv+0x3ac/0x3c0 [tipc] > [ 58.572031] [] ? find_busiest_group+0x117/0x940 > [ 58.572031] [] tipc_l2_rcv_msg+0x48/0x60 [tipc] > [ 58.572031] [] __netif_receive_skb_core+0x2e5/0xa60 > [ 58.572031] [] ? __build_skb+0x2a/0xe0 > [ 58.572031] [] ? __build_skb+0x2a/0xe0 > [ 58.572031] [] __netif_receive_skb+0x1b/0x70 > [ 58.572031] [] netif_receive_skb_internal+0x2d/0x90 > [ 58.572031] [] napi_gro_receive+0x94/0x130 > [ 58.572031] [] virtnet_receive+0x1a5/0x8a0 > [virtio_net] > [ 58.572031] [] virtnet_poll+0x1d/0x80 [virtio_net] > [ 58.572031] [] net_rx_action+0x20e/0x390 > [ 58.572031] [] __do_softirq+0x9b/0x2a2 > [ 58.572031] [] irq_exit+0x60/0x70 > [ 58.572031] [] do_IRQ+0x54/0xd0 > [ 58.572031] [] common_interrupt+0x7f/0x7f > [ 58.572031] [ 58.572031] [] ? > default_idle+0x20/0xe0 > [ 58.572031] [] ? next_zone+0x29/0x30 > [ 58.572031] [] arch_cpu_idle+0xf/0x20 > [ 58.572031] [] default_idle_call+0x2c/0x30 > [ 58.572031] [] cpu_startup_entry+0x177/0x1e0 > [ 58.572031] [] rest_init+0x77/0x80 > [ 58.572031] [] start_kernel+0x40e/0x41b > [ 58.572031] [] x86_64_start_reservations+0x2a/0x2c > [ 58.572031] [] x86_64_start_kernel+0xea/0xed > [ 58.572031] Code: 00 00 48 8b 7b 68 48 85 ff 74 05 f0 ff 0f 74 36 > 48 8b 43 60 48 85 c0 74 14 65 8b 15 96 d3 9d 7e 81 e2 00 00 0f 00 75 > 30 48 89 df d0 48 8b 7b 70 48 85 ff 74 05 f0 ff 0f 74 03 5b 5d c3 > e8 bb > [ 58.572031] RIP [] skb_release_head_state+0x4d/0xa0 > [ 58.572031] RSP > [ 58.662814] ---[ end trace fa57695d3ce8757f ]--- > [ 58.663875] Kernel panic - not syncing: Fatal exception in interrupt > [ 58.664872] Kernel Offset: disabled > [ 58.664872] ---[ end Kernel panic - not syncing: Fatal exception in > interrupt > > regards > Partha > > On 11/29/2016 06:07 PM, Jon Maloy wrote: >> Ying, Partha, >> It would be very nice I could get "acked" or "reviewed" on this so I >> can send it to David before net-next closes. >> >> ///jon >> >> >>> -Original Message- >>> From: Jon Maloy [mailto:jon.ma...@ericsson.com] >>> Sent: Tuesday, 29 November, 2016 12:04 >>> To: tipc-discussion@lists.sourceforge.net; Parthasarathy Bhuvaragan >>> ; Ying Xue >>> ; Jon Maloy >>> Cc: ma...@donjonn.com; thompa@gmail.com >>> Subject: [PATCH net-next v2 0/3] tipc: improve interaction socket-link >>> >>> We fix a very real starvation problem that may occur when the link >>> level runs into send buffer congestion. At the same time we make the >>> interaction between the socket and link layer simpler and more >>> cons
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
Hi Jon, Sorry for the delay, could not work due to sick child. The crash occurs due to the last commit: "tipc: reduce risk of user starvation during link congestion" I examined the crash today, the crash due to array out of bounds for skb->cb[48]. The max size allowed for the callback area is 48bytes, whereas the new struct tipc_skb_cb is 64 bytes. This overrides the skb->destructor callback lying below the 'skb->cb'. The sizeof struct sk_buff_head itself is 48bytes. crash> p *(struct sk_buff*)0x88003f007600 : dev = 0x88003f985000, cb = "\000\00\000", _skb_refdst = 0, destructor = 0x1, << insane function pointer >> I think the simpler way to place these packets 'pkts' into the backlogq and allow temporary over-committing and keep the wakeup mechanism as it is. This way, we transmit the packet in tipc_link_advance_backlog() instead of doing it in link_prepare_wakeup(). Its misleading that link_prepare_wakeup() transmits packets. /Partha On 11/30/2016 07:48 PM, Jon Maloy wrote: > Weird. Looks like a corrupted incoming buffer directly at startup, > before any of my new code is active. Is this repeatable? > > ///jon > > > On 11/30/2016 08:52 AM, Parthasarathy Bhuvaragan wrote: >> Hi Jon, >> >> With your patches, I get the following crash when loading the tipc >> module. Leaving home now, so couldnt investigate further. >> >> [ 58.201114] tipc: Started in single node mode >> [ 58.212991] Started in network mode >> [ 58.213796] Own node address <1.1.1>, network identity 4711 >> [ 58.238416] 8021q: adding VLAN 0 to HW filter on device data0 >> [ 58.252217] 8021q: adding VLAN 0 to HW filter on device data1 >> [ 58.270822] Enabled bearer , discovery domain <1.1.0>, >> priority 10 >> [ 58.571114] general protection fault: [#1] SMP >> [ 58.572031] Modules linked in: tipc ip6_udp_tunnel udp_tunnel >> 9pnet_virtio 9p 9pnet virtio_net virtio_pci virtio_ring virtio >> [ 58.572031] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc6+ #15 >> [ 58.572031] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 >> [ 58.572031] task: 81c0d540 task.stack: 81c0 >> [ 58.572031] RIP: 0010:[] [] >> skb_release_head_state+0x4d/0xa0 >> [ 58.572031] RSP: 0018:880037c03ba0 EFLAGS: 00010246 >> [ 58.572031] RAX: 0001 RBX: 880033fffa00 RCX: >> 00ff >> [ 58.572031] RDX: RSI: 880037c03bca RDI: >> 880033fffa00 >> [ 58.572031] RBP: 880037c03ba8 R08: a005f2c0 R09: >> >> [ 58.572031] R10: 880035b0f0a0 R11: ea00 R12: >> 880033fffa00 >> [ 58.572031] R13: a0048fd4 R14: 81cfbec0 R15: >> 880033718000 >> [ 58.572031] FS: () GS:880037c0() >> knlGS: >> [ 58.572031] CS: 0010 DS: ES: CR0: 80050033 >> [ 58.572031] CR2: 00851bf0 CR3: 35b0 CR4: >> 06f0 >> [ 58.572031] Stack: >> [ 58.572031] 880033fffa00 880037c03bc0 8162f2b2 >> 880033fffa00 >> [ 58.572031] 880037c03be8 8162f327 880033fffa00 >> >> [ 58.572031] 880035b32540 880037c03c68 a0048fd4 >> 0082 >> [ 58.572031] Call Trace: >> [ 58.572031] [ 58.572031] [] >> skb_release_all+0x12/0x30 >> [ 58.572031] [] kfree_skb+0x37/0xa0 >> [ 58.572031] [] tipc_disc_rcv+0x84/0x1d0 [tipc] >> [ 58.572031] [] tipc_rcv+0x3ac/0x3c0 [tipc] >> [ 58.572031] [] ? find_busiest_group+0x117/0x940 >> [ 58.572031] [] tipc_l2_rcv_msg+0x48/0x60 [tipc] >> [ 58.572031] [] __netif_receive_skb_core+0x2e5/0xa60 >> [ 58.572031] [] ? __build_skb+0x2a/0xe0 >> [ 58.572031] [] ? __build_skb+0x2a/0xe0 >> [ 58.572031] [] __netif_receive_skb+0x1b/0x70 >> [ 58.572031] [] netif_receive_skb_internal+0x2d/0x90 >> [ 58.572031] [] napi_gro_receive+0x94/0x130 >> [ 58.572031] [] virtnet_receive+0x1a5/0x8a0 >> [virtio_net] >> [ 58.572031] [] virtnet_poll+0x1d/0x80 [virtio_net] >> [ 58.572031] [] net_rx_action+0x20e/0x390 >> [ 58.572031] [] __do_softirq+0x9b/0x2a2 >> [ 58.572031] [] irq_exit+0x60/0x70 >> [ 58.572031] [] do_IRQ+0x54/0xd0 >> [ 58.572031] [] common_interrupt+0x7f/0x7f >> [ 58.572031] [ 58.572031] [] ? >> default_idle+0x20/0xe0 >> [ 58.572031] [] ? next_zone+0x29/0x30 >> [ 58.572031] [] arch_cpu_idle+0xf/0x20 >> [ 58.572031] [] default_idle_call+0x2c/0x30 >> [ 58.572031] [] cpu_startup_entry+0x177/0x1e0 >> [ 58.572031] [] rest_init+0x77/0x80 >> [ 58.572031] [] start_kernel+0x40e/0x41b >> [ 58.572031] [] x86_64_start_reservations+0x2a/0x2c >> [ 58.572031] [] x86_64_start_kernel+0xea/0xed >> [ 58.572031] Code: 00 00 48 8b 7b 68 48 85 ff 74 05 f0 ff 0f 74 36 >> 48 8b 43 60 48 85 c0 74 14 65 8b 15 96 d3 9d 7e 81 e2 00 00 0f 00 75 >> 30 48 89 df d0 48 8b 7b 70 48 85 ff 74 05 f0 ff 0f 74 03 5b 5d c3 >> e8 bb >> [ 58.572031] RIP
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
> -Original Message- > From: Parthasarathy Bhuvaragan > Sent: Monday, 05 December, 2016 15:11 > To: Jon Maloy ; Jon Maloy ; > tipc-discussion@lists.sourceforge.net; Ying Xue > Cc: thompa@gmail.com > Subject: Re: [PATCH net-next v2 0/3] tipc: improve interaction socket-link > > Hi Jon, > > Sorry for the delay, could not work due to sick child. > > The crash occurs due to the last commit: > "tipc: reduce risk of user starvation during link congestion" > > I examined the crash today, the crash due to array out of bounds for > skb->cb[48]. > The max size allowed for the callback area is 48bytes, whereas the new struct > tipc_skb_cb is 64 bytes. Weird. I did of course test this, and on my system sizeof(tipc_skb_cb) yields 40, and everything works flawlessly. > This overrides the skb->destructor callback lying below the 'skb->cb'. > The sizeof struct sk_buff_head itself is 48bytes. > > crash> p *(struct sk_buff*)0x88003f007600 >: >dev = 0x88003f985000, >cb = "\000\00\000", >_skb_refdst = 0, >destructor = 0x1, << insane function pointer >> > > I think the simpler way to place these packets 'pkts' into the backlogq and > allow > temporary over-committing and keep the wakeup mechanism as it is. You are right. The end result will be the same. I'll change it and recommit. ///jon > > This way, we transmit the packet in tipc_link_advance_backlog() instead of > doing > it in > link_prepare_wakeup(). Its misleading that link_prepare_wakeup() transmits > packets. > > /Partha > > > On 11/30/2016 07:48 PM, Jon Maloy wrote: > > Weird. Looks like a corrupted incoming buffer directly at startup, > > before any of my new code is active. Is this repeatable? > > > > ///jon > > > > > > On 11/30/2016 08:52 AM, Parthasarathy Bhuvaragan wrote: > >> Hi Jon, > >> > >> With your patches, I get the following crash when loading the tipc > >> module. Leaving home now, so couldnt investigate further. > >> > >> [ 58.201114] tipc: Started in single node mode > >> [ 58.212991] Started in network mode > >> [ 58.213796] Own node address <1.1.1>, network identity 4711 > >> [ 58.238416] 8021q: adding VLAN 0 to HW filter on device data0 > >> [ 58.252217] 8021q: adding VLAN 0 to HW filter on device data1 > >> [ 58.270822] Enabled bearer , discovery domain <1.1.0>, > >> priority 10 > >> [ 58.571114] general protection fault: [#1] SMP > >> [ 58.572031] Modules linked in: tipc ip6_udp_tunnel udp_tunnel > >> 9pnet_virtio 9p 9pnet virtio_net virtio_pci virtio_ring virtio > >> [ 58.572031] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc6+ #15 > >> [ 58.572031] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 > >> [ 58.572031] task: 81c0d540 task.stack: 81c0 > >> [ 58.572031] RIP: 0010:[] [] > >> skb_release_head_state+0x4d/0xa0 > >> [ 58.572031] RSP: 0018:880037c03ba0 EFLAGS: 00010246 > >> [ 58.572031] RAX: 0001 RBX: 880033fffa00 RCX: > >> 00ff > >> [ 58.572031] RDX: RSI: 880037c03bca RDI: > >> 880033fffa00 > >> [ 58.572031] RBP: 880037c03ba8 R08: a005f2c0 R09: > >> > >> [ 58.572031] R10: 880035b0f0a0 R11: ea00 R12: > >> 880033fffa00 > >> [ 58.572031] R13: a0048fd4 R14: 81cfbec0 R15: > >> 880033718000 > >> [ 58.572031] FS: () GS:880037c0() > >> knlGS: > >> [ 58.572031] CS: 0010 DS: ES: CR0: 80050033 > >> [ 58.572031] CR2: 00851bf0 CR3: 35b0 CR4: > >> 06f0 > >> [ 58.572031] Stack: > >> [ 58.572031] 880033fffa00 880037c03bc0 8162f2b2 > >> 880033fffa00 > >> [ 58.572031] 880037c03be8 8162f327 880033fffa00 > >> > >> [ 58.572031] 880035b32540 880037c03c68 a0048fd4 > >> 0082 > >> [ 58.572031] Call Trace: > >> [ 58.572031] [ 58.572031] [] > >> skb_release_all+0x12/0x30 > >> [ 58.572031] [] kfree_skb+0x37/0xa0 > >> [ 58.572031] [] tipc_disc_rcv+0x84/0x1d0 [tipc] > >> [ 58.572031] [] tipc_rcv+0x3ac/0x3c0 [tipc] > >> [ 58.572031] [] ? find_busiest_group+0x117/0x940 > >> [ 58.572031] [] tipc_l2_rcv_msg+0x48/0x60 [tipc] > >> [ 58.572031] [] __netif_receive_skb_core+0x2e5/0xa60 > >> [ 58.572031] [] ? __build_skb+0x2a/0xe0 > >> [ 58.572031] [] ? __build_skb+0x2a/0xe0 > >> [ 58.572031] [] __netif_receive_skb+0x1b/0x70 > >> [ 58.572031] [] netif_receive_skb_internal+0x2d/0x90 > >> [ 58.572031] [] napi_gro_receive+0x94/0x130 > >> [ 58.572031] [] virtnet_receive+0x1a5/0x8a0 > >> [virtio_net] > >> [ 58.572031] [] virtnet_poll+0x1d/0x80 [virtio_net] > >> [ 58.572031] [] net_rx_action+0x20e/0x390 > >> [ 58.572031] [] __do_softirq+0x9b/0x2a2 > >> [ 58.572031] [] irq_exit+0x60/0x70 > >> [ 58.572031] [] do_IRQ+0x54/0xd0 > >> [ 58.572031] [] common_interrupt+0x7f
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
On 12/05/2016 09:59 PM, Jon Maloy wrote: > > >> -Original Message- >> From: Parthasarathy Bhuvaragan >> Sent: Monday, 05 December, 2016 15:11 >> To: Jon Maloy ; Jon Maloy ; >> tipc-discussion@lists.sourceforge.net; Ying Xue >> Cc: thompa@gmail.com >> Subject: Re: [PATCH net-next v2 0/3] tipc: improve interaction socket-link >> >> Hi Jon, >> >> Sorry for the delay, could not work due to sick child. >> >> The crash occurs due to the last commit: >> "tipc: reduce risk of user starvation during link congestion" >> >> I examined the crash today, the crash due to array out of bounds for >> skb->cb[48]. >> The max size allowed for the callback area is 48bytes, whereas the new struct >> tipc_skb_cb is 64 bytes. > > Weird. I did of course test this, and on my system sizeof(tipc_skb_cb) yields > 40, and everything works flawlessly. Jon, you are on 32-bit and iam on 64-bit, thats why the size differs. > >> This overrides the skb->destructor callback lying below the 'skb->cb'. >> The sizeof struct sk_buff_head itself is 48bytes. >> >> crash> p *(struct sk_buff*)0x88003f007600 >>: >>dev = 0x88003f985000, >>cb = "\000\00\000", >>_skb_refdst = 0, >>destructor = 0x1, << insane function pointer >> >> >> I think the simpler way to place these packets 'pkts' into the backlogq and >> allow >> temporary over-committing and keep the wakeup mechanism as it is. > > You are right. The end result will be the same. I'll change it and recommit. As i discussed with you earlier, my implementation on 3.12 track to reduce latency followed the following principles: - We schedule a user-socket based on the limit settings in the backlog queue. Thus the importance setting of a user socket influences the scheduling decision of TIPC. - Every socket will be permitted to transmit 1 message (the first message or after a poll/epoll write event) to the link layer after which the above scheduling principles are applied. I still think that this ratio of 1:2:3:4 has wide span and penalizes the low importance sockets. This is an inverse ratio of user-kernel context switches. I feel that this should be user configurable and the default ratio should be 1:1.25:1.5:2. /partha > > ///jon > >> >> This way, we transmit the packet in tipc_link_advance_backlog() instead of >> doing >> it in >> link_prepare_wakeup(). Its misleading that link_prepare_wakeup() transmits >> packets. >> >> /Partha >> >> >> On 11/30/2016 07:48 PM, Jon Maloy wrote: >>> Weird. Looks like a corrupted incoming buffer directly at startup, >>> before any of my new code is active. Is this repeatable? >>> >>> ///jon >>> >>> >>> On 11/30/2016 08:52 AM, Parthasarathy Bhuvaragan wrote: Hi Jon, With your patches, I get the following crash when loading the tipc module. Leaving home now, so couldnt investigate further. [ 58.201114] tipc: Started in single node mode [ 58.212991] Started in network mode [ 58.213796] Own node address <1.1.1>, network identity 4711 [ 58.238416] 8021q: adding VLAN 0 to HW filter on device data0 [ 58.252217] 8021q: adding VLAN 0 to HW filter on device data1 [ 58.270822] Enabled bearer , discovery domain <1.1.0>, priority 10 [ 58.571114] general protection fault: [#1] SMP [ 58.572031] Modules linked in: tipc ip6_udp_tunnel udp_tunnel 9pnet_virtio 9p 9pnet virtio_net virtio_pci virtio_ring virtio [ 58.572031] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc6+ #15 [ 58.572031] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 58.572031] task: 81c0d540 task.stack: 81c0 [ 58.572031] RIP: 0010:[] [] skb_release_head_state+0x4d/0xa0 [ 58.572031] RSP: 0018:880037c03ba0 EFLAGS: 00010246 [ 58.572031] RAX: 0001 RBX: 880033fffa00 RCX: 00ff [ 58.572031] RDX: RSI: 880037c03bca RDI: 880033fffa00 [ 58.572031] RBP: 880037c03ba8 R08: a005f2c0 R09: [ 58.572031] R10: 880035b0f0a0 R11: ea00 R12: 880033fffa00 [ 58.572031] R13: a0048fd4 R14: 81cfbec0 R15: 880033718000 [ 58.572031] FS: () GS:880037c0() knlGS: [ 58.572031] CS: 0010 DS: ES: CR0: 80050033 [ 58.572031] CR2: 00851bf0 CR3: 35b0 CR4: 06f0 [ 58.572031] Stack: [ 58.572031] 880033fffa00 880037c03bc0 8162f2b2 880033fffa00 [ 58.572031] 880037c03be8 8162f327 880033fffa00 [ 58.572031] 880035b32540 880037c03c68 a0048fd4 0082 [ 58.572031] Call Trace: [ 58.572031] [ 58.572031] [] skb_release_all+0x12/0x30 [ 58.572031] [] kfree_skb+0x37/0xa0 [ 58
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
> -Original Message- > From: Parthasarathy Bhuvaragan > Sent: Tuesday, 06 December, 2016 03:46 > To: Jon Maloy ; Jon Maloy ; > tipc-discussion@lists.sourceforge.net; Ying Xue > Cc: thompa@gmail.com > Subject: Re: [PATCH net-next v2 0/3] tipc: improve interaction socket-link > > On 12/05/2016 09:59 PM, Jon Maloy wrote: > > > > > >> -Original Message- > >> From: Parthasarathy Bhuvaragan > >> Sent: Monday, 05 December, 2016 15:11 > >> To: Jon Maloy ; Jon Maloy > ; > >> tipc-discussion@lists.sourceforge.net; Ying Xue > >> Cc: thompa@gmail.com > >> Subject: Re: [PATCH net-next v2 0/3] tipc: improve interaction socket-link > >> > >> Hi Jon, > >> > >> Sorry for the delay, could not work due to sick child. > >> > >> The crash occurs due to the last commit: > >> "tipc: reduce risk of user starvation during link congestion" > >> > >> I examined the crash today, the crash due to array out of bounds for skb- > >cb[48]. > >> The max size allowed for the callback area is 48bytes, whereas the new > >> struct > >> tipc_skb_cb is 64 bytes. > > > > Weird. I did of course test this, and on my system sizeof(tipc_skb_cb) > > yields 40, > and everything works flawlessly. > Jon, you are on 32-bit and iam on 64-bit, thats why the size differs. No, my machine is x64 as is yours. I think it is more likely you are compiling with some debug options that causes struct sk_buff_head to be bigger. > > > >> This overrides the skb->destructor callback lying below the 'skb->cb'. > >> The sizeof struct sk_buff_head itself is 48bytes. > >> > >> crash> p *(struct sk_buff*)0x88003f007600 > >>: > >>dev = 0x88003f985000, > >>cb = "\000\00\000", > >>_skb_refdst = 0, > >>destructor = 0x1, << insane function pointer >> > >> > >> I think the simpler way to place these packets 'pkts' into the backlogq and > allow > >> temporary over-committing and keep the wakeup mechanism as it is. > > > > You are right. The end result will be the same. I'll change it and recommit. > As i discussed with you earlier, my implementation on 3.12 track to > reduce latency followed the following principles: > - We schedule a user-socket based on the limit settings in the backlog > queue. Thus the importance setting of a user socket influences the > scheduling decision of TIPC. What is different from what I do here? > - Every socket will be permitted to transmit 1 message (the first > message or after a poll/epoll write event) to the link layer after which > the above scheduling principles are applied. Not sure what you mean. Each socket must be permitted to send as long as there is space in the queue at its level. Thereafter the above principles are applied. We don't want the socket to always stop after one message, just in case there *might* be somebody else wanting to send. But maybe I misunderstand you. > > I still think that this ratio of 1:2:3:4 has wide span and penalizes the > low importance sockets. This is an inverse ratio of user-kernel context > switches. I feel that this should be user configurable and the default > ratio should be 1:1.25:1.5:2. If your measurements show that this is better I am ok with changing default ratio. But I think we should leave that for a later patch. ///jon > > /partha > > > > ///jon > > > >> > >> This way, we transmit the packet in tipc_link_advance_backlog() instead of > doing > >> it in > >> link_prepare_wakeup(). Its misleading that link_prepare_wakeup() transmits > >> packets. > >> > >> /Partha > >> > >> > >> On 11/30/2016 07:48 PM, Jon Maloy wrote: > >>> Weird. Looks like a corrupted incoming buffer directly at startup, > >>> before any of my new code is active. Is this repeatable? > >>> > >>> ///jon > >>> > >>> > >>> On 11/30/2016 08:52 AM, Parthasarathy Bhuvaragan wrote: > Hi Jon, > > With your patches, I get the following crash when loading the tipc > module. Leaving home now, so couldnt investigate further. > > [ 58.201114] tipc: Started in single node mode > [ 58.212991] Started in network mode > [ 58.213796] Own node address <1.1.1>, network identity 4711 > [ 58.238416] 8021q: adding VLAN 0 to HW filter on device data0 > [ 58.252217] 8021q: adding VLAN 0 to HW filter on device data1 > [ 58.270822] Enabled bearer , discovery domain <1.1.0>, > priority 10 > [ 58.571114] general protection fault: [#1] SMP > [ 58.572031] Modules linked in: tipc ip6_udp_tunnel udp_tunnel > 9pnet_virtio 9p 9pnet virtio_net virtio_pci virtio_ring virtio > [ 58.572031] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc6+ #15 > [ 58.572031] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 > [ 58.572031] task: 81c0d540 task.stack: 81c0 > [ 58.572031] RIP: 0010:[] [] > skb_release_head_state+0x4d/0xa0 > [ 58.572031] RSP: 0018:880037c03ba0 EFLAGS: 00010246 > [ 58.572031] RAX: 0001 RBX:
Re: [tipc-discussion] [PATCH net-next v2 0/3] tipc: improve interaction socket-link
On 12/06/2016 12:41 PM, Jon Maloy wrote: > > >> -Original Message- >> From: Parthasarathy Bhuvaragan >> Sent: Tuesday, 06 December, 2016 03:46 >> To: Jon Maloy ; Jon Maloy ; >> tipc-discussion@lists.sourceforge.net; Ying Xue >> Cc: thompa@gmail.com >> Subject: Re: [PATCH net-next v2 0/3] tipc: improve interaction socket-link >> >> On 12/05/2016 09:59 PM, Jon Maloy wrote: >>> >>> -Original Message- From: Parthasarathy Bhuvaragan Sent: Monday, 05 December, 2016 15:11 To: Jon Maloy ; Jon Maloy >> ; tipc-discussion@lists.sourceforge.net; Ying Xue Cc: thompa@gmail.com Subject: Re: [PATCH net-next v2 0/3] tipc: improve interaction socket-link Hi Jon, Sorry for the delay, could not work due to sick child. The crash occurs due to the last commit: "tipc: reduce risk of user starvation during link congestion" I examined the crash today, the crash due to array out of bounds for skb- >>> cb[48]. The max size allowed for the callback area is 48bytes, whereas the new struct tipc_skb_cb is 64 bytes. >>> >>> Weird. I did of course test this, and on my system sizeof(tipc_skb_cb) >>> yields 40, >> and everything works flawlessly. >> Jon, you are on 32-bit and iam on 64-bit, thats why the size differs. > > No, my machine is x64 as is yours. I think it is more likely you are > compiling with some debug options that causes struct sk_buff_head to be > bigger. Yes, I compile with CONFIG_DEBUG_SPINLOCK which increases the size of spinlock_t by 16 bytes on 64-bit, and is a memeber of sk_buff_head. This will no longer be an issue as the final solution not exceed that size of skb->cb. > >>> This overrides the skb->destructor callback lying below the 'skb->cb'. The sizeof struct sk_buff_head itself is 48bytes. crash> p *(struct sk_buff*)0x88003f007600 : dev = 0x88003f985000, cb = "\000\00\000", _skb_refdst = 0, destructor = 0x1, << insane function pointer >> I think the simpler way to place these packets 'pkts' into the backlogq and >> allow temporary over-committing and keep the wakeup mechanism as it is. >>> >>> You are right. The end result will be the same. I'll change it and recommit. >> As i discussed with you earlier, my implementation on 3.12 track to >> reduce latency followed the following principles: >> - We schedule a user-socket based on the limit settings in the backlog >> queue. Thus the importance setting of a user socket influences the >> scheduling decision of TIPC. > > What is different from what I do here? > >> - Every socket will be permitted to transmit 1 message (the first >> message or after a poll/epoll write event) to the link layer after which >> the above scheduling principles are applied. > > Not sure what you mean. Each socket must be permitted to send as long as > there is space in the queue at its level. Thereafter the above principles are > applied. > We don't want the socket to always stop after one message, just in case there > *might* be somebody else wanting to send. But maybe I misunderstand you. No, I was describing that even if the link is congested, a socket will still be permitted to send a message. The implementation after this patch series will ensure that. No more comments from me and thanks for the patience. /Partha > >> >> I still think that this ratio of 1:2:3:4 has wide span and penalizes the >> low importance sockets. This is an inverse ratio of user-kernel context >> switches. I feel that this should be user configurable and the default >> ratio should be 1:1.25:1.5:2. > > If your measurements show that this is better I am ok with changing default > ratio. But I think we should leave that for a later patch. Ok, you can skip this in your series and we add it to backlog. > > ///jon > >> >> /partha >>> >>> ///jon >>> This way, we transmit the packet in tipc_link_advance_backlog() instead of >> doing it in link_prepare_wakeup(). Its misleading that link_prepare_wakeup() transmits packets. /Partha On 11/30/2016 07:48 PM, Jon Maloy wrote: > Weird. Looks like a corrupted incoming buffer directly at startup, > before any of my new code is active. Is this repeatable? > > ///jon > > > On 11/30/2016 08:52 AM, Parthasarathy Bhuvaragan wrote: >> Hi Jon, >> >> With your patches, I get the following crash when loading the tipc >> module. Leaving home now, so couldnt investigate further. >> >> [ 58.201114] tipc: Started in single node mode >> [ 58.212991] Started in network mode >> [ 58.213796] Own node address <1.1.1>, network identity 4711 >> [ 58.238416] 8021q: adding VLAN 0 to HW filter on device data0 >> [ 58.252217] 8021q: adding VLAN 0 to HW filter on device data1 >> [ 58.270822] Enabled bearer , discovery domain <1.1.0>, >> pri
