I'm getting what seems to be a kernel memory leak while doing a TCP
throughput test between two VMs on identical systems, in order to test a
broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version
2.4.90. The host system of the receiving (server) VM leaks memory during the
throughput test. The memory leaks fast enough to make the system completely
unusable within five minutes. Once I stop the throughput test, the
memory stops
leaking. A couple of times, the kernel on the host system has actually
killed
the qemu process for me, but this doesn't happen reliably. The leaked memory
doesn't become available again even after the VM is killed.
To investigate this, I compiled a 4.4.0-rc8 kernel with kmemleak. I can
scan the
leaking system during and after killing the throughput test and get the
following stack trace over and over again:
unreferenced object 0xffff880464f11488 (size 256):
comm "softirq", pid 0, jiffies 4312675043 (age 379.184s)
hex dump (first 32 bytes):
6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff81550e28>] kmemleak_alloc+0x28/0x50
[<ffffffff811a8bac>] __kmalloc+0x11c/0x2a0
[<ffffffff8146972e>] metadata_dst_alloc+0x1e/0x40
[<ffffffff814ed476>] udp_tun_rx_dst+0x126/0x1c0
[<ffffffff8140be68>] vxlan_udp_encap_recv+0x148/0xb10
[<ffffffff814cabe9>] udp_queue_rcv_skb+0x1e9/0x480
[<ffffffff814cb77c>] __udp4_lib_rcv+0x45c/0x700
[<ffffffff814cbf0a>] udp_rcv+0x1a/0x20
[<ffffffff8149ae54>] ip_local_deliver_finish+0x94/0x1e0
[<ffffffff8149b150>] ip_local_deliver+0x60/0xd0
[<ffffffff8149ab39>] ip_rcv_finish+0x99/0x320
[<ffffffff8149b41e>] ip_rcv+0x25e/0x380
[<ffffffff814602db>] __netif_receive_skb_core+0x2cb/0xa00
[<ffffffff81460a26>] __netif_receive_skb+0x16/0x70
[<ffffffff81460aa3>] netif_receive_skb_internal+0x23/0x80
[<ffffffff814615f5>] napi_gro_receive+0xa5/0xd0
I pulled down the kernel tree from
http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git, and
did a git
bisect and got this:
58ce31cca1ffe057f4744c3f671e3e84606d3d4a is the first bad commit
commit 58ce31cca1ffe057f4744c3f671e3e84606d3d4a
Author: Tom Herbert [off-list ref]
Date: Wed Aug 19 17:07:33 2015 -0700
vxlan: GRO support at tunnel layer
Add calls to gro_cells infrastructure to do GRO when receiving on a
tunnel.
Testing:
Ran 200 netperf TCP_STREAM instance
- With fix (GRO enabled on VXLAN interface)
Verify GRO is happening.
9084 MBps tput
3.44% CPU utilization
- Without fix (GRO disabled on VXLAN interface)
Verified no GRO is happening.
9084 MBps tput
5.54% CPU utilization
Signed-off-by: Tom Herbert [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 a7d49cb2e24ebddf620c01e27515cc756b32e46f
c3951c16da75ff3e0db1322b8ccb3e61975b1242 M drivers
:040000 040000 f36442958138eafdd472c58d06ea35be66990aa1
0e29d513e575dd11f459c59df71e05db074363de M include
For the test I'm using two HP Proliant dl360gen9's. I put two matching
broadcom
PCIe cards in each machine and ran throughput tests between two VMs on
either
machine, using the throughput testing program iperf3. On each host we
had a qemu
VM attached to an OVS bridge; these bridges are connected over a VxLAN
tunnel as
detailed here: https://community.mellanox.com/docs/DOC-1446.
The test went well with an Intel Niantic NIC, and I saw high (8.04 Gb/s)
throughput over an eighteen hour throughput test. There was no memory leak.
However, when I put in a Broadcom NIC on both systems I get the above memory
leak, if they have a VM on the receving end of the test. lspci -v output
for the
Broadcom NIC below:
04:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM57810
10 Gigabit Ethernet (rev 10)
Subsystem: Hewlett-Packard Company HP FlexFabric 10Gb 2-port
534FLR-SFP+ Adapter
Flags: bus master, fast devsel, latency 0
Memory at 97000000 (64-bit, prefetchable) [size=8M]
Memory at 96800000 (64-bit, prefetchable) [size=8M]
Memory at 98800000 (64-bit, prefetchable) [size=64K]
[virtual] Expansion ROM at 98880000 [disabled] [size=512K]
Capabilities: [48] Power Management version 3
Capabilities: [50] Vital Product Data
Capabilities: [a0] MSI-X: Enable+ Count=32 Masked-
Capabilities: [ac] Express Endpoint, MSI 00
Capabilities: [100] Advanced Error Reporting
Capabilities: [13c] Device Serial Number 14-58-d0-ff-fe-52-5b-d8
Capabilities: [150] Power Budgeting <?>
Capabilities: [160] Virtual Channel
Capabilities: [1b8] Alternative Routing-ID Interpretation (ARI)
Capabilities: [1c0] Single Root I/O Virtualization (SR-IOV)
Capabilities: [220] #15
Capabilities: [300] #19
Kernel driver in use: bnx2x
ethtool -i info:
driver: bnx2x
version: 1.712.30-0
firmware-version: bc 7.8.24
bus-info: 0000:04:00.0
supports-statistics: yes
supports-test: yes
supports-eeprom-access: yes
supports-register-dump: yes
supports-priv-flags: yes
dmesg bnx2x output:
[ 1.506071] bnx2x: QLogic 5771x/578xx 10/20-Gigabit Ethernet Driver
bnx2x 1.712.30-0 (2014/02/10)
[ 1.506205] bnx2x 0000:04:00.0: msix capability found
[ 1.506297] bnx2x 0000:04:00.0: part number
394D4342-31383735-31543030-47303030
[ 1.555970] bnx2x 0000:04:00.1: msix capability found
[ 1.556061] bnx2x 0000:04:00.1: part number
394D4342-31383735-31543030-47303030
[ 10.360477] bnx2x 0000:04:00.1 eth9: renamed from eth3
[ 10.584371] bnx2x 0000:04:00.0 rename3: renamed from eth1
[ 588.956002] bnx2x 0000:04:00.0 rename3: using MSI-X IRQs: sp 70
fp[0] 74 ... fp[7] 81
[ 589.208675] bnx2x 0000:04:00.0 rename3: Added vxlan dest port 4789
[ 640.159842] bnx2x 0000:04:00.1 eth10: renamed from eth9
[ 642.432216] bnx2x 0000:04:00.1 eth10: using MSI-X IRQs: sp 82 fp[0]
84 ... fp[7] 91
[ 642.700576] bnx2x 0000:04:00.1 eth10: Added vxlan dest port 4789
[ 1098.368845] bnx2x 0000:04:00.1 eth10: using MSI-X IRQs: sp 82 fp[0]
84 ... fp[7] 91
[ 1109.277182] bnx2x 0000:04:00.1 eth10_nolink: renamed from eth10
[ 1115.368873] bnx2x 0000:04:00.0 eth10: renamed from rename3
[ 1117.928156] bnx2x 0000:04:00.0 eth10: using MSI-X IRQs: sp 70 fp[0]
74 ... fp[7] 81
[ 1118.214861] bnx2x 0000:04:00.0 eth10: NIC Link is Up, 10000 Mbps full
duplex, Flow control: ON - receive & transmit
I've tried disabling all offloads (gro included) but the leak still
happens.
On Thu, Jan 14, 2016 at 9:17 AM, John [off-list ref] wrote:
I'm getting what seems to be a kernel memory leak while doing a TCP
throughput test between two VMs on identical systems, in order to test a
broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version
2.4.90. The host system of the receiving (server) VM leaks memory during the
throughput test. The memory leaks fast enough to make the system completely
unusable within five minutes. Once I stop the throughput test, the memory
stops
leaking. A couple of times, the kernel on the host system has actually
killed
the qemu process for me, but this doesn't happen reliably. The leaked memory
doesn't become available again even after the VM is killed.
It looks like the problem is in napi_skb_finish(). If we when do
GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free ==
NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself
but not any of the associated elements. Historically, this would have
been OK but these days we will have allocated a dst entry already for
tunnel metadata, which will get leaked.
If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a
__kfree_skb(), which will release the dst entry. That would explain
why some drivers have the problem but not others since the memory is
laid out differently.
From: Eric Dumazet <hidden> Date: 2016-01-19 22:47:36
On Tue, 2016-01-19 at 13:07 -0800, Jesse Gross wrote:
On Thu, Jan 14, 2016 at 9:17 AM, John [off-list ref] wrote:
quoted
I'm getting what seems to be a kernel memory leak while doing a TCP
throughput test between two VMs on identical systems, in order to test a
broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version
2.4.90. The host system of the receiving (server) VM leaks memory during the
throughput test. The memory leaks fast enough to make the system completely
unusable within five minutes. Once I stop the throughput test, the memory
stops
leaking. A couple of times, the kernel on the host system has actually
killed
the qemu process for me, but this doesn't happen reliably. The leaked memory
doesn't become available again even after the VM is killed.
It looks like the problem is in napi_skb_finish(). If we when do
GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free ==
NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself
but not any of the associated elements. Historically, this would have
been OK but these days we will have allocated a dst entry already for
tunnel metadata, which will get leaked.
If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a
__kfree_skb(), which will release the dst entry. That would explain
why some drivers have the problem but not others since the memory is
laid out differently.
Wow.... What is the purpose of using skb_dst_set() on skb before calling
gro_cells_receive() exactly ?
Commit 2e15ea390e6f4466655066d97e22ec66870a042c changelog is not
helpful :
Following patch create new tunnel flag which enable
tunnel metadata collection on given device.
This is rather strange since later the dst is thrown away with the
skb_valid_dst() test.
Note also that IP early demux is broken as well, since it does not use
skb_valid_dst() but a simple :
if (sysctl_ip_early_demux && !skb_dst(skb) && !skb->sk) {
...
}
On Tue, Jan 19, 2016 at 2:47 PM, Eric Dumazet [off-list ref] wrote:
On Tue, 2016-01-19 at 13:07 -0800, Jesse Gross wrote:
quoted
On Thu, Jan 14, 2016 at 9:17 AM, John [off-list ref] wrote:
quoted
I'm getting what seems to be a kernel memory leak while doing a TCP
throughput test between two VMs on identical systems, in order to test a
broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version
2.4.90. The host system of the receiving (server) VM leaks memory during the
throughput test. The memory leaks fast enough to make the system completely
unusable within five minutes. Once I stop the throughput test, the memory
stops
leaking. A couple of times, the kernel on the host system has actually
killed
the qemu process for me, but this doesn't happen reliably. The leaked memory
doesn't become available again even after the VM is killed.
It looks like the problem is in napi_skb_finish(). If we when do
GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free ==
NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself
but not any of the associated elements. Historically, this would have
been OK but these days we will have allocated a dst entry already for
tunnel metadata, which will get leaked.
If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a
__kfree_skb(), which will release the dst entry. That would explain
why some drivers have the problem but not others since the memory is
laid out differently.
Wow.... What is the purpose of using skb_dst_set() on skb before calling
gro_cells_receive() exactly ?
Commit 2e15ea390e6f4466655066d97e22ec66870a042c changelog is not
helpful :
Following patch create new tunnel flag which enable
tunnel metadata collection on given device.
Note that this isn't really the problem commit. The general issue is
lightweight tunnels - the above commit is just adding support for GRE
in a way that I think follows the existing model.
This is rather strange since later the dst is thrown away with the
skb_valid_dst() test.
This isn't really an IP routing dst - it's the tunnel encapsulation
information - so there isn't much to with it in ip_rcv_finish().
However, the information can be consumed in other places, such as
through eBPF.
Note also that IP early demux is broken as well, since it does not use
skb_valid_dst() but a simple :
if (sysctl_ip_early_demux && !skb_dst(skb) && !skb->sk) {
...
}
From: Eric Dumazet <hidden> Date: 2016-01-19 23:51:07
On Tue, 2016-01-19 at 15:34 -0800, Jesse Gross wrote:
On Tue, Jan 19, 2016 at 2:47 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Tue, 2016-01-19 at 13:07 -0800, Jesse Gross wrote:
quoted
On Thu, Jan 14, 2016 at 9:17 AM, John [off-list ref] wrote:
quoted
I'm getting what seems to be a kernel memory leak while doing a TCP
throughput test between two VMs on identical systems, in order to test a
broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version
2.4.90. The host system of the receiving (server) VM leaks memory during the
throughput test. The memory leaks fast enough to make the system completely
unusable within five minutes. Once I stop the throughput test, the memory
stops
leaking. A couple of times, the kernel on the host system has actually
killed
the qemu process for me, but this doesn't happen reliably. The leaked memory
doesn't become available again even after the VM is killed.
It looks like the problem is in napi_skb_finish(). If we when do
GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free ==
NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself
but not any of the associated elements. Historically, this would have
been OK but these days we will have allocated a dst entry already for
tunnel metadata, which will get leaked.
If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a
__kfree_skb(), which will release the dst entry. That would explain
why some drivers have the problem but not others since the memory is
laid out differently.
Wow.... What is the purpose of using skb_dst_set() on skb before calling
gro_cells_receive() exactly ?
Commit 2e15ea390e6f4466655066d97e22ec66870a042c changelog is not
helpful :
Following patch create new tunnel flag which enable
tunnel metadata collection on given device.
Note that this isn't really the problem commit. The general issue is
lightweight tunnels - the above commit is just adding support for GRE
in a way that I think follows the existing model.
I believe this commit added the first skb_dst_set() before
gro_cells_receive(), in ip_tunnel_rcv().
This is already buggy.
Then Tom in 58ce31cca1ffe057f4744c3f671e3e84606d3d4a added the
gro_cells_receive() in vxlan_rcv(), which added another bug, because of
existing skb_dst_set() call.
On Tue, Jan 19, 2016 at 3:51 PM, Eric Dumazet [off-list ref] wrote:
On Tue, 2016-01-19 at 15:34 -0800, Jesse Gross wrote:
quoted
On Tue, Jan 19, 2016 at 2:47 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Tue, 2016-01-19 at 13:07 -0800, Jesse Gross wrote:
quoted
On Thu, Jan 14, 2016 at 9:17 AM, John [off-list ref] wrote:
quoted
I'm getting what seems to be a kernel memory leak while doing a TCP
throughput test between two VMs on identical systems, in order to test a
broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version
2.4.90. The host system of the receiving (server) VM leaks memory during the
throughput test. The memory leaks fast enough to make the system completely
unusable within five minutes. Once I stop the throughput test, the memory
stops
leaking. A couple of times, the kernel on the host system has actually
killed
the qemu process for me, but this doesn't happen reliably. The leaked memory
doesn't become available again even after the VM is killed.
It looks like the problem is in napi_skb_finish(). If we when do
GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free ==
NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself
but not any of the associated elements. Historically, this would have
been OK but these days we will have allocated a dst entry already for
tunnel metadata, which will get leaked.
If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a
__kfree_skb(), which will release the dst entry. That would explain
why some drivers have the problem but not others since the memory is
laid out differently.
Wow.... What is the purpose of using skb_dst_set() on skb before calling
gro_cells_receive() exactly ?
Commit 2e15ea390e6f4466655066d97e22ec66870a042c changelog is not
helpful :
Following patch create new tunnel flag which enable
tunnel metadata collection on given device.
Note that this isn't really the problem commit. The general issue is
lightweight tunnels - the above commit is just adding support for GRE
in a way that I think follows the existing model.
I believe this commit added the first skb_dst_set() before
gro_cells_receive(), in ip_tunnel_rcv().
This is already buggy.
Then Tom in 58ce31cca1ffe057f4744c3f671e3e84606d3d4a added the
gro_cells_receive() in vxlan_rcv(), which added another bug, because of
existing skb_dst_set() call.
Oh, I wasn't disagreeing that there is a problem. I was just trying to
point out the conceptual root of the problem rather than the
individual instances in different tunnels implementations.
In any case, I think we can solve this by calling
skb_release_head_state() in napi_skb_finish() and switching early
demux to use skb_valid_dst().
From: Eric Dumazet <hidden> Date: 2016-01-20 00:17:49
On Tue, 2016-01-19 at 16:00 -0800, Jesse Gross wrote:
Oh, I wasn't disagreeing that there is a problem. I was just trying to
point out the conceptual root of the problem rather than the
individual instances in different tunnels implementations.
In any case, I think we can solve this by calling
skb_release_head_state() in napi_skb_finish() and switching early
demux to use skb_valid_dst().
Yes, but it will also have to call skb_dst_drop() or risk a leak of
prior one.
So what is the purpose of having a dst if we need to drop it ?
Adding code in GRO would be fine if someone explains me the purpose of
doing apparently useless work.
(refcounting on dst is not exactly free)
On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet [off-list ref] wrote:
On Tue, 2016-01-19 at 16:00 -0800, Jesse Gross wrote:
quoted
Oh, I wasn't disagreeing that there is a problem. I was just trying to
point out the conceptual root of the problem rather than the
individual instances in different tunnels implementations.
In any case, I think we can solve this by calling
skb_release_head_state() in napi_skb_finish() and switching early
demux to use skb_valid_dst().
Yes, but it will also have to call skb_dst_drop() or risk a leak of
prior one.
skb_dst_drop() is the key part but I mentioned
skb_release_head_state() (which calls skb_dst_drop()) as a slightly
more generic solution. But either one is fine.
So what is the purpose of having a dst if we need to drop it ?
Adding code in GRO would be fine if someone explains me the purpose of
doing apparently useless work.
(refcounting on dst is not exactly free)
In the GRO case, the dst is only dropped on the packets which have
been merged and therefore need to be freed (the GRO_MERGED_FREE case).
It's not being thrown away for the overall frame, just metadata that
has been duplicated on each individual frame, similar to the metadata
in struct sk_buff itself. And while it is not used by the IP stack
there are other consumers (eBPF/OVS/etc.). This entire process is
controlled by the COLLECT_METADATA flag on tunnels, so there is no
cost in situations where it is not actually used.
From: Thomas Graf <tgraf@suug.ch> Date: 2016-01-20 01:31:32
On 01/19/16 at 04:51pm, Jesse Gross wrote:
On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet [off-list ref] wrote:
quoted
So what is the purpose of having a dst if we need to drop it ?
Adding code in GRO would be fine if someone explains me the purpose of
doing apparently useless work.
(refcounting on dst is not exactly free)
In the GRO case, the dst is only dropped on the packets which have
been merged and therefore need to be freed (the GRO_MERGED_FREE case).
It's not being thrown away for the overall frame, just metadata that
has been duplicated on each individual frame, similar to the metadata
in struct sk_buff itself. And while it is not used by the IP stack
there are other consumers (eBPF/OVS/etc.). This entire process is
controlled by the COLLECT_METADATA flag on tunnels, so there is no
cost in situations where it is not actually used.
Right. There were thoughts around leveraging a per CPU scratch
buffer without a refcount and turn it into a full reference when
the packet gets enqueued somewhere but the need hasn't really come
up yet.
Jesse, is this what you have in mind:
On Tue, Jan 19, 2016 at 5:31 PM, Thomas Graf [off-list ref] wrote:
On 01/19/16 at 04:51pm, Jesse Gross wrote:
quoted
On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet [off-list ref] wrote:
quoted
So what is the purpose of having a dst if we need to drop it ?
Adding code in GRO would be fine if someone explains me the purpose of
doing apparently useless work.
(refcounting on dst is not exactly free)
In the GRO case, the dst is only dropped on the packets which have
been merged and therefore need to be freed (the GRO_MERGED_FREE case).
It's not being thrown away for the overall frame, just metadata that
has been duplicated on each individual frame, similar to the metadata
in struct sk_buff itself. And while it is not used by the IP stack
there are other consumers (eBPF/OVS/etc.). This entire process is
controlled by the COLLECT_METADATA flag on tunnels, so there is no
cost in situations where it is not actually used.
Right. There were thoughts around leveraging a per CPU scratch
buffer without a refcount and turn it into a full reference when
the packet gets enqueued somewhere but the need hasn't really come
up yet.
Jesse, is this what you have in mind:
Yes, that's what I was thinking. I also realized that we should
probably have a check in gro_list_prepare() to ensure that the dst of
the packets that we are about to merge are equal. And we should fix IP
early demux as Eric pointed out.
On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet [off-list ref] wrote:
quoted
So what is the purpose of having a dst if we need to drop it ?
Adding code in GRO would be fine if someone explains me the purpose of
doing apparently useless work.
(refcounting on dst is not exactly free)
In the GRO case, the dst is only dropped on the packets which have
been merged and therefore need to be freed (the GRO_MERGED_FREE case).
It's not being thrown away for the overall frame, just metadata that
has been duplicated on each individual frame, similar to the metadata
in struct sk_buff itself. And while it is not used by the IP stack
there are other consumers (eBPF/OVS/etc.). This entire process is
controlled by the COLLECT_METADATA flag on tunnels, so there is no
cost in situations where it is not actually used.
Right. There were thoughts around leveraging a per CPU scratch
buffer without a refcount and turn it into a full reference when
the packet gets enqueued somewhere but the need hasn't really come
up yet.
Jesse, is this what you have in mind:
So I've tested the below patch (same as one above with minor
modifications made to make it compile) and it worked - no memory leak.
Should I submit this or...?
From: Eric Dumazet <hidden> Date: 2016-01-21 00:07:08
On Wed, 2016-01-20 at 16:43 -0700, John wrote:
On 01/19/2016 06:31 PM, Thomas Graf wrote:
quoted
On 01/19/16 at 04:51pm, Jesse Gross wrote:
quoted
On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet [off-list ref] wrote:
quoted
So what is the purpose of having a dst if we need to drop it ?
Adding code in GRO would be fine if someone explains me the purpose of
doing apparently useless work.
(refcounting on dst is not exactly free)
In the GRO case, the dst is only dropped on the packets which have
been merged and therefore need to be freed (the GRO_MERGED_FREE case).
It's not being thrown away for the overall frame, just metadata that
has been duplicated on each individual frame, similar to the metadata
in struct sk_buff itself. And while it is not used by the IP stack
there are other consumers (eBPF/OVS/etc.). This entire process is
controlled by the COLLECT_METADATA flag on tunnels, so there is no
cost in situations where it is not actually used.
Right. There were thoughts around leveraging a per CPU scratch
buffer without a refcount and turn it into a full reference when
the packet gets enqueued somewhere but the need hasn't really come
up yet.
Jesse, is this what you have in mind:
So I've tested the below patch (same as one above with minor
modifications made to make it compile) and it worked - no memory leak.
Should I submit this or...?
Unfortunately fix is not complete.
As someone mentioned, GRO should not aggregate packets having different
dst.
This part is hard to achieve, as a pointer comparison wont be enough :
Each skb has its own meta dst allocation.
Quite frankly, I would rather disable GRO for packets with a dst,
instead of making GRO dog slow.
On Wed, Jan 20, 2016 at 4:07 PM, Eric Dumazet [off-list ref] wrote:
On Wed, 2016-01-20 at 16:43 -0700, John wrote:
quoted
On 01/19/2016 06:31 PM, Thomas Graf wrote:
quoted
On 01/19/16 at 04:51pm, Jesse Gross wrote:
quoted
On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet [off-list ref] wrote:
quoted
So what is the purpose of having a dst if we need to drop it ?
Adding code in GRO would be fine if someone explains me the purpose of
doing apparently useless work.
(refcounting on dst is not exactly free)
In the GRO case, the dst is only dropped on the packets which have
been merged and therefore need to be freed (the GRO_MERGED_FREE case).
It's not being thrown away for the overall frame, just metadata that
has been duplicated on each individual frame, similar to the metadata
in struct sk_buff itself. And while it is not used by the IP stack
there are other consumers (eBPF/OVS/etc.). This entire process is
controlled by the COLLECT_METADATA flag on tunnels, so there is no
cost in situations where it is not actually used.
Right. There were thoughts around leveraging a per CPU scratch
buffer without a refcount and turn it into a full reference when
the packet gets enqueued somewhere but the need hasn't really come
up yet.
Jesse, is this what you have in mind:
So I've tested the below patch (same as one above with minor
modifications made to make it compile) and it worked - no memory leak.
Should I submit this or...?
Unfortunately fix is not complete.
As someone mentioned, GRO should not aggregate packets having different
dst.
This part is hard to achieve, as a pointer comparison wont be enough :
Each skb has its own meta dst allocation.
Quite frankly, I would rather disable GRO for packets with a dst,
instead of making GRO dog slow.
I have a patch that implements the comparison between dsts. For
packets without a dst, there isn't really a cost and if we do have a
dst then GRO is still a benefit. So it seems like it is worth doing,
even if it is more expensive than is ideal.
From: Eric Dumazet <hidden> Date: 2016-01-21 00:34:57
On Wed, 2016-01-20 at 16:19 -0800, Jesse Gross wrote:
I have a patch that implements the comparison between dsts. For
packets without a dst, there isn't really a cost and if we do have a
dst then GRO is still a benefit. So it seems like it is worth doing,
even if it is more expensive than is ideal.
You guys really want to kill GRO performance.
Really the aggregation should happen at the first layer (ethernet
device), instead of doing it after tunnel decap.
This is already done for GRE, IPIP, SIT, ...
GRO having to access metadata looks wrong, if you think about trying to
do the same function in hardware (offload)
From: Thomas Graf <tgraf@suug.ch> Date: 2016-01-21 02:27:38
On 01/20/16 at 04:34pm, Eric Dumazet wrote:
On Wed, 2016-01-20 at 16:19 -0800, Jesse Gross wrote:
quoted
I have a patch that implements the comparison between dsts. For
packets without a dst, there isn't really a cost and if we do have a
dst then GRO is still a benefit. So it seems like it is worth doing,
even if it is more expensive than is ideal.
You guys really want to kill GRO performance.
Really the aggregation should happen at the first layer (ethernet
device), instead of doing it after tunnel decap.
This is already done for GRE, IPIP, SIT, ...
GRO having to access metadata looks wrong, if you think about trying to
do the same function in hardware (offload)
If I understand Jesse correctly then the added cost is only for
metadata enabled packets. Though I agree, what's the benefit of
doing GRO after decap?
It seems like it's way too late and we've already paid the cost
by going through the stack for each outer header packet.