| Message ID | 9b22fd3a35416b3145f1245466167b001925ce1a.1532357173.git.pabeni@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ip: hash fragments consistently | expand |
On 07/23/2018 07:50 AM, Paolo Abeni wrote: > The skb hash for locally generated ip[v6] fragments belonging > to the same datagram can vary in several circumstances: > * for connected UDP[v6] sockets, the first fragment get its hash > via set_owner_w()/skb_set_hash_from_sk() > * for unconnected IPv6 UDPv6 sockets, the first fragment can get > its hash via ip6_make_flowlabel()/skb_get_hash_flowi6(), if > auto_flowlabel is enabled > > For the following frags the hash is usually computed via > skb_get_hash(). > The above can cause OoO for unconnected IPv6 UDPv6 socket: in that > scenario the egress tx queue can be selected on a per packet basis > via the skb hash. > It may also fool flow-oriented schedulers to place fragments belonging > to the same datagram in different flows. > It also fools bond_xmit_hash(), packets of the same datagram can be sent on two bonding slaves instead of one, meaning adding pressure on the defrag unit in receiver. Reviewed-by: Eric Dumazet <edumazet@google.com>
From: Paolo Abeni <pabeni@redhat.com> Date: Mon, 23 Jul 2018 16:50:48 +0200 > The skb hash for locally generated ip[v6] fragments belonging > to the same datagram can vary in several circumstances: > * for connected UDP[v6] sockets, the first fragment get its hash > via set_owner_w()/skb_set_hash_from_sk() > * for unconnected IPv6 UDPv6 sockets, the first fragment can get > its hash via ip6_make_flowlabel()/skb_get_hash_flowi6(), if > auto_flowlabel is enabled > > For the following frags the hash is usually computed via > skb_get_hash(). > The above can cause OoO for unconnected IPv6 UDPv6 socket: in that > scenario the egress tx queue can be selected on a per packet basis > via the skb hash. > It may also fool flow-oriented schedulers to place fragments belonging > to the same datagram in different flows. > > Fix the issue by copying the skb hash from the head frag into > the others at fragmentation time. > > Before this commit: > perf probe -a "dev_queue_xmit skb skb->hash skb->l4_hash:b1@0/8 skb->sw_hash:b1@1/8" > netperf -H $IPV4 -t UDP_STREAM -l 5 -- -m 2000 -n & > perf record -e probe:dev_queue_xmit -e probe:skb_set_owner_w -a sleep 0.1 > perf script > probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=3713014309 l4_hash=1 sw_hash=0 > probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=0 l4_hash=0 sw_hash=0 > > After this commit: > probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=2171763177 l4_hash=1 sw_hash=0 > probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=2171763177 l4_hash=1 sw_hash=0 > > Fixes: b73c3d0e4f0e ("net: Save TX flow hash in sock and set in skbuf on xmit") > Fixes: 67800f9b1f4e ("ipv6: Call skb_get_hash_flowi6 to get skb->hash in ip6_make_flowlabel") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> Good catch! Applied and queued up for -stable, thanks!
On 07/23/2018 09:26 AM, Eric Dumazet wrote: > > > On 07/23/2018 07:50 AM, Paolo Abeni wrote: >> The skb hash for locally generated ip[v6] fragments belonging >> to the same datagram can vary in several circumstances: >> * for connected UDP[v6] sockets, the first fragment get its hash >> via set_owner_w()/skb_set_hash_from_sk() >> * for unconnected IPv6 UDPv6 sockets, the first fragment can get >> its hash via ip6_make_flowlabel()/skb_get_hash_flowi6(), if >> auto_flowlabel is enabled >> >> For the following frags the hash is usually computed via >> skb_get_hash(). >> The above can cause OoO for unconnected IPv6 UDPv6 socket: in that >> scenario the egress tx queue can be selected on a per packet basis >> via the skb hash. >> It may also fool flow-oriented schedulers to place fragments belonging >> to the same datagram in different flows. >> > > It also fools bond_xmit_hash(), packets of the same datagram can be sent on > two bonding slaves instead of one, meaning adding pressure on the defrag unit > in receiver. > > Reviewed-by: Eric Dumazet <edumazet@google.com> > Also we might note that flow dissector itself is buggy as found by Soukjin Bae ( https://patchwork.ozlabs.org/patch/994601/ ) I will send a v2 of his patch with a different changelog. Defrag is fixed [1] but the bug in flow dissector is adding extra work and hash inconsistencies. [1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=0d5b9311baf27bb545f187f12ecfd558220c607d
> Also we might note that flow dissector itself is buggy as > found by Soukjin Bae ( https://patchwork.ozlabs.org/patch/994601/ ) > > I will send a v2 of his patch with a different changelog. > > Defrag is fixed [1] but the bug in flow dissector is adding > extra work and hash inconsistencies. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=0d5b9311baf27bb545f187f12ecfd558220c607d Dear all, it's good news to mee too. thanks for accept my work :)
On 11/11/2018 04:40 PM, 배석진 wrote: >> Also we might note that flow dissector itself is buggy as >> found by Soukjin Bae ( https://patchwork.ozlabs.org/patch/994601/ ) >> >> I will send a v2 of his patch with a different changelog. >> >> Defrag is fixed [1] but the bug in flow dissector is adding >> extra work and hash inconsistencies. >> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=0d5b9311baf27bb545f187f12ecfd558220c607d > > > Dear all, > > it's good news to mee too. > thanks for accept my work :) Sure thing ;)
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index b3308e9d9762..0e3edd25f881 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -523,6 +523,8 @@ static void ip_copy_metadata(struct sk_buff *to, struct sk_buff *from) to->dev = from->dev; to->mark = from->mark; + skb_copy_hash(to, from); + /* Copy the flags to each fragment. */ IPCB(to)->flags = IPCB(from)->flags; diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index a14fb4fcdf18..3168847c30d1 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -570,6 +570,8 @@ static void ip6_copy_metadata(struct sk_buff *to, struct sk_buff *from) to->dev = from->dev; to->mark = from->mark; + skb_copy_hash(to, from); + #ifdef CONFIG_NET_SCHED to->tc_index = from->tc_index; #endif
The skb hash for locally generated ip[v6] fragments belonging to the same datagram can vary in several circumstances: * for connected UDP[v6] sockets, the first fragment get its hash via set_owner_w()/skb_set_hash_from_sk() * for unconnected IPv6 UDPv6 sockets, the first fragment can get its hash via ip6_make_flowlabel()/skb_get_hash_flowi6(), if auto_flowlabel is enabled For the following frags the hash is usually computed via skb_get_hash(). The above can cause OoO for unconnected IPv6 UDPv6 socket: in that scenario the egress tx queue can be selected on a per packet basis via the skb hash. It may also fool flow-oriented schedulers to place fragments belonging to the same datagram in different flows. Fix the issue by copying the skb hash from the head frag into the others at fragmentation time. Before this commit: perf probe -a "dev_queue_xmit skb skb->hash skb->l4_hash:b1@0/8 skb->sw_hash:b1@1/8" netperf -H $IPV4 -t UDP_STREAM -l 5 -- -m 2000 -n & perf record -e probe:dev_queue_xmit -e probe:skb_set_owner_w -a sleep 0.1 perf script probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=3713014309 l4_hash=1 sw_hash=0 probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=0 l4_hash=0 sw_hash=0 After this commit: probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=2171763177 l4_hash=1 sw_hash=0 probe:dev_queue_xmit: (ffffffff8c6b1b20) hash=2171763177 l4_hash=1 sw_hash=0 Fixes: b73c3d0e4f0e ("net: Save TX flow hash in sock and set in skbuf on xmit") Fixes: 67800f9b1f4e ("ipv6: Call skb_get_hash_flowi6 to get skb->hash in ip6_make_flowlabel") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/ipv4/ip_output.c | 2 ++ net/ipv6/ip6_output.c | 2 ++ 2 files changed, 4 insertions(+)