Message ID | 1359535089-18348-1-git-send-email-amwang@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, Jan 30, 2013 at 12:38 AM, Cong Wang <amwang@redhat.com> wrote: > From: Cong Wang <amwang@redhat.com> > > skb_gso_segment() is almost always called in tx path, > except for openvswitch. It calls this function when > it receives the packet and tries to queue it to user-space. > In this special case, the ->ip_summed check inside > skb_gso_segment() is no longer true, as ->ip_summed value > has different meanings on rx path. I don't think that this is really specific to Open vSwitch - it's possible that skb_gso_segment() could be called in the transmit path after bridging. I also don't really think that it is true any more that the meaning of skb->ip_summed is different on receive vs. transmit paths (this was definitely not the case in the past). However, it's certainly possible to see different types of packets. > This patch adjusts skb_gso_segment() so that we can at least > avoid such warnings on checksum. When do you see GSO packets with CHECKSUM_NONE on receive? > diff --git a/net/core/dev.c b/net/core/dev.c > index a87bc74..f6e7b3f 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2347,7 +2356,7 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, > rcu_read_lock(); > list_for_each_entry_rcu(ptype, &offload_base, list) { > if (ptype->type == type && ptype->callbacks.gso_segment) { > - if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { > + if (unlikely(skb_needs_check(skb))) { > err = ptype->callbacks.gso_send_check(skb); > segs = ERR_PTR(err); > if (err || skb_gso_ok(skb, features)) Even if we don't warn we likely still need to fix the checksum. > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > index d8c13a9..0b75964 100644 > --- a/net/openvswitch/datapath.c > +++ b/net/openvswitch/datapath.c > @@ -302,7 +302,7 @@ static int queue_gso_packets(struct net *net, int dp_ifindex, > int err; > > segs = skb_gso_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM); > - if (IS_ERR(segs)) > + if (IS_ERR_OR_NULL(segs)) > return PTR_ERR(segs); In what case would we expect that NULL is returned here? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-01-30 at 13:54 -0800, Jesse Gross wrote: > On Wed, Jan 30, 2013 at 12:38 AM, Cong Wang <amwang@redhat.com> wrote: > > From: Cong Wang <amwang@redhat.com> > > > > skb_gso_segment() is almost always called in tx path, > > except for openvswitch. It calls this function when > > it receives the packet and tries to queue it to user-space. > > In this special case, the ->ip_summed check inside > > skb_gso_segment() is no longer true, as ->ip_summed value > > has different meanings on rx path. > > I don't think that this is really specific to Open vSwitch - it's > possible that skb_gso_segment() could be called in the transmit path > after bridging. I also don't really think that it is true any more > that the meaning of skb->ip_summed is different on receive vs. > transmit paths (this was definitely not the case in the past). > However, it's certainly possible to see different types of packets. Take a look at the fat comment in include/linux/skbuff.h, unless it is out-of-date. > > > This patch adjusts skb_gso_segment() so that we can at least > > avoid such warnings on checksum. > > When do you see GSO packets with CHECKSUM_NONE on receive? I see CHECKSUM_UNCESSARY set by ixgbe driver or CHECKSUM_PARTIAL set by gro. According to comments in include/linux/skbuff.h, CHECKSUM_NONE is set when device is not able to do checksum, it is not my case. The full backtrace below is the one I got on RHEL6 kernel: WARNING: at net/core/dev.c:1760 skb_gso_segment+0x1df/0x2b0() (Not tainted) Hardware name: ProLiant DL580 G7 802.1Q VLAN Support: caps=(0x190833, 0x0) len=1500 data_len=1448 ip_summed=1 Modules linked in: bonding 8021q garp stp llc openvswitch sunrpc cpufreq_ondemand freq_table pcc_cpufreq ipv6 power_meter qlcnic cxgb4 be2net ixgbe dca ptp pps_core mdio netxen_nic mlx4_core microcode serio_raw iTCO_wdt iTCO_vendor_support hpilo hpwdt sg i7core_edac edac_core shpchp ext4 mbcache jbd2 sr_mod cdrom sd_mod crc_t10dif pata_acpi ata_generic ata_piix hpsa radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core dm_mirror dm_region_hash dm_log dm_mod [last unloaded: llc] Pid: 0, comm: swapper Not tainted 2.6.32-356.el6.x86_64 #1 Call Trace: <IRQ> [<ffffffff8106e2e7>] ? warn_slowpath_common+0x87/0xc0 [<ffffffff8106e3d6>] ? warn_slowpath_fmt+0x46/0x50 [<ffffffff81439084>] ? sock_def_readable+0x44/0x80 [<ffffffff8144871f>] ? skb_gso_segment+0x1df/0x2b0 [<ffffffffa043f92e>] ? queue_gso_packets+0x4e/0x1d0 [openvswitch] [<ffffffffa0443558>] ? ovs_flow_extract+0x6a8/0xa80 [openvswitch] [<ffffffffa043fb0d>] ? ovs_dp_upcall+0x5d/0xb0 [openvswitch] [<ffffffffa043fc8e>] ? ovs_dp_process_received_packet+0x12e/0x140 [openvswitch] [<ffffffffa0443a30>] ? ovs_vport_receive+0x30/0x40 [openvswitch] [<ffffffffa0444893>] ? ovs_netdev_frame_hook+0x83/0xac [openvswitch] [<ffffffff814482aa>] ? __netif_receive_skb+0x60a/0x750 [<ffffffff8144a528>] ? netif_receive_skb+0x58/0x60 [<ffffffff8144a630>] ? napi_skb_finish+0x50/0x70 [<ffffffff814e7024>] ? vlan_gro_receive+0x84/0xa0 [<ffffffffa030c08e>] ? ixgbe_poll+0x6ae/0x1280 [ixgbe] [<ffffffff810e3d48>] ? handle_edge_irq+0x98/0x180 [<ffffffff8144ccf3>] ? net_rx_action+0x103/0x2f0 [<ffffffff81076fb1>] ? __do_softirq+0xc1/0x1e0 [<ffffffff810e1640>] ? handle_IRQ_event+0x60/0x170 [<ffffffff8100c1cc>] ? call_softirq+0x1c/0x30 [<ffffffff8100de05>] ? do_softirq+0x65/0xa0 [<ffffffff81076d95>] ? irq_exit+0x85/0x90 [<ffffffff81516c45>] ? do_IRQ+0x75/0xf0 [<ffffffff8100b9d3>] ? ret_from_intr+0x0/0x11 <EOI> [<ffffffff810a871d>] ? tick_nohz_restart_sched_tick+0x16d/0x190 [<ffffffff810a870f>] ? tick_nohz_restart_sched_tick+0x15f/0x190 [<ffffffff81009f90>] ? cpu_idle+0x80/0x110 [<ffffffff81009ff9>] ? cpu_idle+0xe9/0x110 [<ffffffff814f2eda>] ? rest_init+0x7a/0x80 [<ffffffff81c27f7b>] ? start_kernel+0x424/0x430 [<ffffffff81c2733a>] ? x86_64_start_reservations+0x125/0x129 [<ffffffff81c27438>] ? x86_64_start_kernel+0xfa/0x109 ---[ end trace 84ef9bd9ae5d9360 ]--- > > > diff --git a/net/core/dev.c b/net/core/dev.c > > index a87bc74..f6e7b3f 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -2347,7 +2356,7 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, > > rcu_read_lock(); > > list_for_each_entry_rcu(ptype, &offload_base, list) { > > if (ptype->type == type && ptype->callbacks.gso_segment) { > > - if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { > > + if (unlikely(skb_needs_check(skb))) { > > err = ptype->callbacks.gso_send_check(skb); > > segs = ERR_PTR(err); > > if (err || skb_gso_ok(skb, features)) > > Even if we don't warn we likely still need to fix the checksum. By calling skb_checksum_complete()? My initial version patch had it, but it didn't work. > > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > > index d8c13a9..0b75964 100644 > > --- a/net/openvswitch/datapath.c > > +++ b/net/openvswitch/datapath.c > > @@ -302,7 +302,7 @@ static int queue_gso_packets(struct net *net, int dp_ifindex, > > int err; > > > > segs = skb_gso_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM); > > - if (IS_ERR(segs)) > > + if (IS_ERR_OR_NULL(segs)) > > return PTR_ERR(segs); > > In what case would we expect that NULL is returned here? BUG: unable to handle kernel NULL pointer dereference at 00000000000000b9 IP: [<ffffffffa043f581>] queue_userspace_packet+0x21/0x380 [openvswitch] PGD 0 Oops: 0000 [#1] SMP last sysfs file: /sys/devices/pci0000:00/0000:00:1e.0/0000:01:03.0/irq CPU 0 Modules linked in: bonding 8021q garp stp llc openvswitch sunrpc cpufreq_ondemand freq_table pcc_cpufreq ipv6 power_meter qlcnic cxgb4 be2net ixgbe dca ptp pps_core mdio netxen_nic mlx4_core microcode serio_raw iTCO_wdt iTCO_vendor_support hpilo hpwdt sg i7core_edac edac_core shpchp ext4 mbcache jbd2 sr_mod cdrom sd_mod crc_t10dif pata_acpi ata_generic ata_piix hpsa radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core dm_mirror dm_region_hash dm_log dm_mod [last unloaded: llc] Pid: 0, comm: swapper Tainted: G W --------------- 2.6.32-356.el6.x86_64 #1 HP ProLiant DL580 G7 RIP: 0010:[<ffffffffa043f581>] [<ffffffffa043f581>] queue_userspace_packet+0x21/0x380 [openvswitch] RSP: 0018:ffff88002f6039d0 EFLAGS: 00010282 RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff880228c58422 RDX: ffff88002f603b70 RSI: 0000000000000000 RDI: 0000000000000060 RBP: ffff88002f603a40 R08: ffffffff81c07728 R09: 0000000000000040 R10: 000000000000000f R11: 0000000000000008 R12: 0000000000000000 R13: ffff88002f603b70 R14: 0000000000000060 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff88002f600000(0000) knlGS:0000000000000000 CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b CR2: 00000000000000b9 CR3: 0000000001a85000 CR4: 00000000000007f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process swapper (pid: 0, threadinfo ffffffff81a00000, task ffffffff81a8d020) Stack: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 <d> 0000000000000000 0000000000000000 0000000200000000 0241c7121a501498 <d> ffff880000031dd8 0000000000000000 0000000000000000 ffff88002f603b70 Call Trace: <IRQ> [<ffffffffa043f96a>] queue_gso_packets+0x8a/0x1d0 [openvswitch] [<ffffffffa0443558>] ? ovs_flow_extract+0x6a8/0xa80 [openvswitch] [<ffffffffa043fb0d>] ovs_dp_upcall+0x5d/0xb0 [openvswitch] [<ffffffffa043fc8e>] ovs_dp_process_received_packet+0x12e/0x140 [openvswitch] [<ffffffffa0443a30>] ovs_vport_receive+0x30/0x40 [openvswitch] [<ffffffffa0444893>] ovs_netdev_frame_hook+0x83/0xac [openvswitch] [<ffffffff814482aa>] __netif_receive_skb+0x60a/0x750 [<ffffffff8144a528>] netif_receive_skb+0x58/0x60 [<ffffffff8144a630>] napi_skb_finish+0x50/0x70 [<ffffffff814e7024>] vlan_gro_receive+0x84/0xa0 [<ffffffffa030c08e>] ixgbe_poll+0x6ae/0x1280 [ixgbe] [<ffffffff810e3d48>] ? handle_edge_irq+0x98/0x180 [<ffffffff8144ccf3>] net_rx_action+0x103/0x2f0 [<ffffffff81076fb1>] __do_softirq+0xc1/0x1e0 [<ffffffff810e1640>] ? handle_IRQ_event+0x60/0x170 [<ffffffff8100c1cc>] call_softirq+0x1c/0x30 [<ffffffff8100de05>] do_softirq+0x65/0xa0 [<ffffffff81076d95>] irq_exit+0x85/0x90 [<ffffffff81516c45>] do_IRQ+0x75/0xf0 [<ffffffff8100b9d3>] ret_from_intr+0x0/0x11 <EOI> [<ffffffff810a871d>] ? tick_nohz_restart_sched_tick+0x16d/0x190 [<ffffffff810a870f>] ? tick_nohz_restart_sched_tick+0x15f/0x190 [<ffffffff81009f90>] ? cpu_idle+0x80/0x110 [<ffffffff81009ff9>] cpu_idle+0xe9/0x110 [<ffffffff814f2eda>] rest_init+0x7a/0x80 [<ffffffff81c27f7b>] start_kernel+0x424/0x430 [<ffffffff81c2733a>] x86_64_start_reservations+0x125/0x129 [<ffffffff81c27438>] x86_64_start_kernel+0xfa/0x109 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Cong Wang <amwang@redhat.com> Date: Wed, 30 Jan 2013 16:38:09 +0800 > From: Cong Wang <amwang@redhat.com> > > skb_gso_segment() is almost always called in tx path, > except for openvswitch. It calls this function when > it receives the packet and tries to queue it to user-space. > In this special case, the ->ip_summed check inside > skb_gso_segment() is no longer true, as ->ip_summed value > has different meanings on rx path. > > This patch adjusts skb_gso_segment() so that we can at least > avoid such warnings on checksum. > > I am not very sure if this is a real fix or just a workaround, > at least this patch works fine for me, the kernel warning inside > skb_gso_segment() disappears and the traffic is normal too. > > (Note I only tested it on 2.6.32, but ->ip_summed has never > been changed since then.) > > Cc: Jesse Gross <jesse@nicira.com> > Cc: David S. Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <amwang@redhat.com> Instead of having the crazy test, make the caller tell the context, TX or RX. struct sk_buff *__skb_gso_segment(struct sk_buff *skb, netdev_features_t features, bool tx_path); static inline struct sk_buff * skb_gso_segment(struct sk_buff *skb, netdev_features_t features) { __skb_gso_segment(skb, features, true); } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-01-30 at 22:46 -0500, David Miller wrote: > > Instead of having the crazy test, make the caller > tell the context, TX or RX. > > struct sk_buff *__skb_gso_segment(struct sk_buff *skb, > netdev_features_t features, > bool tx_path); > > static inline struct sk_buff * > skb_gso_segment(struct sk_buff *skb, netdev_features_t features) > { > __skb_gso_segment(skb, features, true); > } Yeah, good idea! Will do. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 30, 2013 at 5:48 PM, Cong Wang <amwang@redhat.com> wrote: > On Wed, 2013-01-30 at 13:54 -0800, Jesse Gross wrote: >> On Wed, Jan 30, 2013 at 12:38 AM, Cong Wang <amwang@redhat.com> wrote: >> > From: Cong Wang <amwang@redhat.com> >> > >> > skb_gso_segment() is almost always called in tx path, >> > except for openvswitch. It calls this function when >> > it receives the packet and tries to queue it to user-space. >> > In this special case, the ->ip_summed check inside >> > skb_gso_segment() is no longer true, as ->ip_summed value >> > has different meanings on rx path. >> >> I don't think that this is really specific to Open vSwitch - it's >> possible that skb_gso_segment() could be called in the transmit path >> after bridging. I also don't really think that it is true any more >> that the meaning of skb->ip_summed is different on receive vs. >> transmit paths (this was definitely not the case in the past). >> However, it's certainly possible to see different types of packets. > > Take a look at the fat comment in include/linux/skbuff.h, unless it is > out-of-date. I would argue that the meanings are the same on transmit and receive but the values that you are likely to see differ depending on the path. With bridging (which includes both Open vSwitch and the bridge) it's very difficult to know whether you are on the transmit or receive path. Personally, I would just remove the warning completely. >> > diff --git a/net/core/dev.c b/net/core/dev.c >> > index a87bc74..f6e7b3f 100644 >> > --- a/net/core/dev.c >> > +++ b/net/core/dev.c >> > @@ -2347,7 +2356,7 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, >> > rcu_read_lock(); >> > list_for_each_entry_rcu(ptype, &offload_base, list) { >> > if (ptype->type == type && ptype->callbacks.gso_segment) { >> > - if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { >> > + if (unlikely(skb_needs_check(skb))) { >> > err = ptype->callbacks.gso_send_check(skb); >> > segs = ERR_PTR(err); >> > if (err || skb_gso_ok(skb, features)) >> >> Even if we don't warn we likely still need to fix the checksum. > > By calling skb_checksum_complete()? My initial version patch had it, but > it didn't work. You can just drop the change in the block above and gso_send_check() will take care of the rest. Regardless of the state of the incoming packet, GSO needs the checksum to be reset to CHECKSUM_PARTIAL in order to work. >> >> > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c >> > index d8c13a9..0b75964 100644 >> > --- a/net/openvswitch/datapath.c >> > +++ b/net/openvswitch/datapath.c >> > @@ -302,7 +302,7 @@ static int queue_gso_packets(struct net *net, int dp_ifindex, >> > int err; >> > >> > segs = skb_gso_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM); >> > - if (IS_ERR(segs)) >> > + if (IS_ERR_OR_NULL(segs)) >> > return PTR_ERR(segs); >> >> In what case would we expect that NULL is returned here? > > > BUG: unable to handle kernel NULL pointer dereference at > 00000000000000b9 > IP: [<ffffffffa043f581>] queue_userspace_packet+0x21/0x380 > [openvswitch] > PGD 0 > Oops: 0000 [#1] SMP > last sysfs file: /sys/devices/pci0000:00/0000:00:1e.0/0000:01:03.0/irq > CPU 0 > Modules linked in: bonding 8021q garp stp llc openvswitch sunrpc > cpufreq_ondemand freq_table pcc_cpufreq ipv6 power_meter qlcnic cxgb4 > be2net ixgbe dca ptp pps_core mdio netxen_nic mlx4_core microcode > serio_raw iTCO_wdt iTCO_vendor_support hpilo hpwdt sg i7core_edac > edac_core shpchp ext4 mbcache jbd2 sr_mod cdrom sd_mod crc_t10dif > pata_acpi ata_generic ata_piix hpsa radeon ttm drm_kms_helper drm > i2c_algo_bit i2c_core dm_mirror dm_region_hash dm_log dm_mod [last > unloaded: llc] > > Pid: 0, comm: swapper Tainted: G W --------------- > 2.6.32-356.el6.x86_64 #1 HP ProLiant DL580 G7 > RIP: 0010:[<ffffffffa043f581>] [<ffffffffa043f581>] > queue_userspace_packet+0x21/0x380 [openvswitch] > RSP: 0018:ffff88002f6039d0 EFLAGS: 00010282 > RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff880228c58422 > RDX: ffff88002f603b70 RSI: 0000000000000000 RDI: 0000000000000060 > RBP: ffff88002f603a40 R08: ffffffff81c07728 R09: 0000000000000040 > R10: 000000000000000f R11: 0000000000000008 R12: 0000000000000000 > R13: ffff88002f603b70 R14: 0000000000000060 R15: 0000000000000000 > FS: 0000000000000000(0000) GS:ffff88002f600000(0000) > knlGS:0000000000000000 > CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b > CR2: 00000000000000b9 CR3: 0000000001a85000 CR4: 00000000000007f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > Process swapper (pid: 0, threadinfo ffffffff81a00000, task > ffffffff81a8d020) > Stack: > 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > <d> 0000000000000000 0000000000000000 0000000200000000 0241c7121a501498 > <d> ffff880000031dd8 0000000000000000 0000000000000000 ffff88002f603b70 > Call Trace: > <IRQ> > [<ffffffffa043f96a>] queue_gso_packets+0x8a/0x1d0 [openvswitch] > [<ffffffffa0443558>] ? ovs_flow_extract+0x6a8/0xa80 [openvswitch] > [<ffffffffa043fb0d>] ovs_dp_upcall+0x5d/0xb0 [openvswitch] > [<ffffffffa043fc8e>] ovs_dp_process_received_packet+0x12e/0x140 > [openvswitch] > [<ffffffffa0443a30>] ovs_vport_receive+0x30/0x40 [openvswitch] > [<ffffffffa0444893>] ovs_netdev_frame_hook+0x83/0xac [openvswitch] > [<ffffffff814482aa>] __netif_receive_skb+0x60a/0x750 > [<ffffffff8144a528>] netif_receive_skb+0x58/0x60 > [<ffffffff8144a630>] napi_skb_finish+0x50/0x70 > [<ffffffff814e7024>] vlan_gro_receive+0x84/0xa0 > [<ffffffffa030c08e>] ixgbe_poll+0x6ae/0x1280 [ixgbe] > [<ffffffff810e3d48>] ? handle_edge_irq+0x98/0x180 > [<ffffffff8144ccf3>] net_rx_action+0x103/0x2f0 > [<ffffffff81076fb1>] __do_softirq+0xc1/0x1e0 > [<ffffffff810e1640>] ? handle_IRQ_event+0x60/0x170 > [<ffffffff8100c1cc>] call_softirq+0x1c/0x30 > [<ffffffff8100de05>] do_softirq+0x65/0xa0 > [<ffffffff81076d95>] irq_exit+0x85/0x90 > [<ffffffff81516c45>] do_IRQ+0x75/0xf0 > [<ffffffff8100b9d3>] ret_from_intr+0x0/0x11 > <EOI> > [<ffffffff810a871d>] ? tick_nohz_restart_sched_tick+0x16d/0x190 > [<ffffffff810a870f>] ? tick_nohz_restart_sched_tick+0x15f/0x190 > [<ffffffff81009f90>] ? cpu_idle+0x80/0x110 > [<ffffffff81009ff9>] cpu_idle+0xe9/0x110 > [<ffffffff814f2eda>] rest_init+0x7a/0x80 > [<ffffffff81c27f7b>] start_kernel+0x424/0x430 > [<ffffffff81c2733a>] x86_64_start_reservations+0x125/0x129 > [<ffffffff81c27438>] x86_64_start_kernel+0xfa/0x109 I think this is a different problem, unrelated to the checksum issue. Can you test this patch: http://git.kernel.org/?p=linux/kernel/git/jesse/openvswitch.git;a=commit;h=d9d59089c43fc33eb403cbb928e56c131f191dd5 I believe it should fix the problem, although there are still a few lingering reports that I'm trying to track down. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-01-31 at 17:09 -0800, Jesse Gross wrote: > > I would argue that the meanings are the same on transmit and receive > but the values that you are likely to see differ depending on the > path. With bridging (which includes both Open vSwitch and the bridge) > it's very difficult to know whether you are on the transmit or receive > path. Personally, I would just remove the warning completely. I would keep it as it is, I fixed several RHEL kernel bugs exposed by this warning, it is pretty useful for debugging. ... > > You can just drop the change in the block above and gso_send_check() > will take care of the rest. Regardless of the state of the incoming > packet, GSO needs the checksum to be reset to CHECKSUM_PARTIAL in > order to work. Right, makes sense. > > I think this is a different problem, unrelated to the checksum issue. > Can you test this patch: > http://git.kernel.org/?p=linux/kernel/git/jesse/openvswitch.git;a=commit;h=d9d59089c43fc33eb403cbb928e56c131f191dd5 > > I believe it should fix the problem, although there are still a few > lingering reports that I'm trying to track down. I will try your patch together with my updated patch. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-01-31 at 17:09 -0800, Jesse Gross wrote: > I think this is a different problem, unrelated to the checksum issue. > Can you test this patch: > http://git.kernel.org/?p=linux/kernel/git/jesse/openvswitch.git;a=commit;h=d9d59089c43fc33eb403cbb928e56c131f191dd5 > > I believe it should fix the problem, although there are still a few > lingering reports that I'm trying to track down. Our QE tested your patch and my updated patch on 2.6.32, no panic any more. I will post my updated patch now. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/dev.c b/net/core/dev.c index a87bc74..f6e7b3f 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2302,6 +2302,15 @@ out: } EXPORT_SYMBOL(skb_checksum_help); +/* openvswitch calls skb_gso_segment() on rx path, but ->ip_summed has + * different meanings on rx path with tx path. + */ +static inline bool skb_needs_check(struct sk_buff *skb) +{ + return (!skb->skb_iif && skb->ip_summed != CHECKSUM_PARTIAL) || + (skb->skb_iif && skb->ip_summed == CHECKSUM_NONE); +} + /** * skb_gso_segment - Perform segmentation on skb. * @skb: buffer to segment @@ -2336,7 +2345,7 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, skb->mac_len = skb->network_header - skb->mac_header; __skb_pull(skb, skb->mac_len); - if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { + if (unlikely(skb_needs_check(skb))) { skb_warn_bad_offload(skb); if (skb_header_cloned(skb) && @@ -2347,7 +2356,7 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, rcu_read_lock(); list_for_each_entry_rcu(ptype, &offload_base, list) { if (ptype->type == type && ptype->callbacks.gso_segment) { - if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { + if (unlikely(skb_needs_check(skb))) { err = ptype->callbacks.gso_send_check(skb); segs = ERR_PTR(err); if (err || skb_gso_ok(skb, features)) diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index d8c13a9..0b75964 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -302,7 +302,7 @@ static int queue_gso_packets(struct net *net, int dp_ifindex, int err; segs = skb_gso_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM); - if (IS_ERR(segs)) + if (IS_ERR_OR_NULL(segs)) return PTR_ERR(segs); /* Queue all of the segments. */