* [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. @ 2018-06-27 13:34 Flavio Leitner 2018-06-27 13:34 ` [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct Flavio Leitner ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Flavio Leitner @ 2018-06-27 13:34 UTC (permalink / raw) To: netdev Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal, netfilter-devel, Flavio Leitner The sock reference is lost when scrubbing the packet and that breaks TSQ (TCP Small Queues) and XPS (Transmit Packet Steering) causing performance impacts of about 50% in a single TCP stream when crossing network namespaces. XPS breaks because the queue mapping stored in the socket is not available, so another random queue might be selected when the stack needs to transmit something like a TCP ACK, or TCP Retransmissions. That causes packet re-ordering and/or performance issues. TSQ breaks because it orphans the packet while it is still in the host, so packets are queued contributing to the buffer bloat problem. Preserving the sock reference fixes both issues. The socket is orphaned anyways in the receiving path before any relevant action, but the transmit side needs some extra checking included in the first patch. The first patch will update netfilter to check if the socket netns is local before use it. The second patch removes the skb_orphan() from the skb_scrub_packet() and improve the documentation. ChangeLog: - split into two (Eric) - addressed Paolo's offline feedback to swap the checks in xt_socket.c to preserve original behavior. - improved ip-sysctl.txt (reported by Cong) Flavio Leitner (2): netfilter: check if the socket netns is correct. skbuff: preserve sock reference when scrubbing the skb. Documentation/networking/ip-sysctl.txt | 10 +++++----- include/net/netfilter/nf_log.h | 3 ++- net/core/skbuff.c | 1 - net/ipv4/netfilter/nf_log_ipv4.c | 8 ++++---- net/ipv6/netfilter/nf_log_ipv6.c | 8 ++++---- net/netfilter/nf_conntrack_broadcast.c | 2 +- net/netfilter/nf_log_common.c | 5 +++-- net/netfilter/nf_nat_core.c | 6 +++++- net/netfilter/nft_meta.c | 9 ++++++--- net/netfilter/nft_socket.c | 5 ++++- net/netfilter/xt_cgroup.c | 6 ++++-- net/netfilter/xt_owner.c | 2 +- net/netfilter/xt_recent.c | 3 ++- net/netfilter/xt_socket.c | 8 ++++++++ 14 files changed, 49 insertions(+), 27 deletions(-) -- 2.14.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct. 2018-06-27 13:34 [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb Flavio Leitner @ 2018-06-27 13:34 ` Flavio Leitner 2018-06-27 14:22 ` Florian Westphal 2018-06-27 13:34 ` [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb Flavio Leitner ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Flavio Leitner @ 2018-06-27 13:34 UTC (permalink / raw) To: netdev Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal, netfilter-devel, Flavio Leitner Netfilter assumes that if the socket is present in the skb, then it can be used because that reference is cleaned up while the skb is crossing netns. We want to change that to preserve the socket reference in a future patch, so this is a preparation updating netfilter to check if the socket netns matches before use it. Signed-off-by: Flavio Leitner <fbl@redhat.com> --- include/net/netfilter/nf_log.h | 3 ++- net/ipv4/netfilter/nf_log_ipv4.c | 8 ++++---- net/ipv6/netfilter/nf_log_ipv6.c | 8 ++++---- net/netfilter/nf_conntrack_broadcast.c | 2 +- net/netfilter/nf_log_common.c | 5 +++-- net/netfilter/nf_nat_core.c | 6 +++++- net/netfilter/nft_meta.c | 9 ++++++--- net/netfilter/nft_socket.c | 5 ++++- net/netfilter/xt_cgroup.c | 6 ++++-- net/netfilter/xt_owner.c | 2 +- net/netfilter/xt_recent.c | 3 ++- net/netfilter/xt_socket.c | 8 ++++++++ 12 files changed, 44 insertions(+), 21 deletions(-) diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h index e811ac07ea94..0d3920896d50 100644 --- a/include/net/netfilter/nf_log.h +++ b/include/net/netfilter/nf_log.h @@ -106,7 +106,8 @@ int nf_log_dump_udp_header(struct nf_log_buf *m, const struct sk_buff *skb, int nf_log_dump_tcp_header(struct nf_log_buf *m, const struct sk_buff *skb, u8 proto, int fragment, unsigned int offset, unsigned int logflags); -void nf_log_dump_sk_uid_gid(struct nf_log_buf *m, struct sock *sk); +void nf_log_dump_sk_uid_gid(struct net *net, struct nf_log_buf *m, + struct sock *sk); void nf_log_dump_packet_common(struct nf_log_buf *m, u_int8_t pf, unsigned int hooknum, const struct sk_buff *skb, const struct net_device *in, diff --git a/net/ipv4/netfilter/nf_log_ipv4.c b/net/ipv4/netfilter/nf_log_ipv4.c index 4388de0e5380..1e6f28c97d3a 100644 --- a/net/ipv4/netfilter/nf_log_ipv4.c +++ b/net/ipv4/netfilter/nf_log_ipv4.c @@ -35,7 +35,7 @@ static const struct nf_loginfo default_loginfo = { }; /* One level of recursion won't kill us */ -static void dump_ipv4_packet(struct nf_log_buf *m, +static void dump_ipv4_packet(struct net *net, struct nf_log_buf *m, const struct nf_loginfo *info, const struct sk_buff *skb, unsigned int iphoff) { @@ -183,7 +183,7 @@ static void dump_ipv4_packet(struct nf_log_buf *m, /* Max length: 3+maxlen */ if (!iphoff) { /* Only recurse once. */ nf_log_buf_add(m, "["); - dump_ipv4_packet(m, info, skb, + dump_ipv4_packet(net, m, info, skb, iphoff + ih->ihl*4+sizeof(_icmph)); nf_log_buf_add(m, "] "); } @@ -251,7 +251,7 @@ static void dump_ipv4_packet(struct nf_log_buf *m, /* Max length: 15 "UID=4294967295 " */ if ((logflags & NF_LOG_UID) && !iphoff) - nf_log_dump_sk_uid_gid(m, skb->sk); + nf_log_dump_sk_uid_gid(net, m, skb->sk); /* Max length: 16 "MARK=0xFFFFFFFF " */ if (!iphoff && skb->mark) @@ -333,7 +333,7 @@ static void nf_log_ip_packet(struct net *net, u_int8_t pf, if (in != NULL) dump_ipv4_mac_header(m, loginfo, skb); - dump_ipv4_packet(m, loginfo, skb, 0); + dump_ipv4_packet(net, m, loginfo, skb, 0); nf_log_buf_close(m); } diff --git a/net/ipv6/netfilter/nf_log_ipv6.c b/net/ipv6/netfilter/nf_log_ipv6.c index b397a8fe88b9..c6bf580d0f33 100644 --- a/net/ipv6/netfilter/nf_log_ipv6.c +++ b/net/ipv6/netfilter/nf_log_ipv6.c @@ -36,7 +36,7 @@ static const struct nf_loginfo default_loginfo = { }; /* One level of recursion won't kill us */ -static void dump_ipv6_packet(struct nf_log_buf *m, +static void dump_ipv6_packet(struct net *net, struct nf_log_buf *m, const struct nf_loginfo *info, const struct sk_buff *skb, unsigned int ip6hoff, int recurse) @@ -258,7 +258,7 @@ static void dump_ipv6_packet(struct nf_log_buf *m, /* Max length: 3+maxlen */ if (recurse) { nf_log_buf_add(m, "["); - dump_ipv6_packet(m, info, skb, + dump_ipv6_packet(net, m, info, skb, ptr + sizeof(_icmp6h), 0); nf_log_buf_add(m, "] "); } @@ -278,7 +278,7 @@ static void dump_ipv6_packet(struct nf_log_buf *m, /* Max length: 15 "UID=4294967295 " */ if ((logflags & NF_LOG_UID) && recurse) - nf_log_dump_sk_uid_gid(m, skb->sk); + nf_log_dump_sk_uid_gid(net, m, skb->sk); /* Max length: 16 "MARK=0xFFFFFFFF " */ if (recurse && skb->mark) @@ -365,7 +365,7 @@ static void nf_log_ip6_packet(struct net *net, u_int8_t pf, if (in != NULL) dump_ipv6_mac_header(m, loginfo, skb); - dump_ipv6_packet(m, loginfo, skb, skb_network_offset(skb), 1); + dump_ipv6_packet(net, m, loginfo, skb, skb_network_offset(skb), 1); nf_log_buf_close(m); } diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c index a1086bdec242..5423b197d98a 100644 --- a/net/netfilter/nf_conntrack_broadcast.c +++ b/net/netfilter/nf_conntrack_broadcast.c @@ -32,7 +32,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, __be32 mask = 0; /* we're only interested in locally generated packets */ - if (skb->sk == NULL) + if (skb->sk == NULL || !net_eq(nf_ct_net(ct), sock_net(skb->sk))) goto out; if (rt == NULL || !(rt->rt_flags & RTCF_BROADCAST)) goto out; diff --git a/net/netfilter/nf_log_common.c b/net/netfilter/nf_log_common.c index dc61399e30be..a8c5c846aec1 100644 --- a/net/netfilter/nf_log_common.c +++ b/net/netfilter/nf_log_common.c @@ -132,9 +132,10 @@ int nf_log_dump_tcp_header(struct nf_log_buf *m, const struct sk_buff *skb, } EXPORT_SYMBOL_GPL(nf_log_dump_tcp_header); -void nf_log_dump_sk_uid_gid(struct nf_log_buf *m, struct sock *sk) +void nf_log_dump_sk_uid_gid(struct net *net, struct nf_log_buf *m, + struct sock *sk) { - if (!sk || !sk_fullsock(sk)) + if (!sk || !sk_fullsock(sk) || !net_eq(net, sock_net(sk))) return; read_lock_bh(&sk->sk_callback_lock); diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c index 46f9df99d276..86df2a1666fd 100644 --- a/net/netfilter/nf_nat_core.c +++ b/net/netfilter/nf_nat_core.c @@ -108,6 +108,7 @@ int nf_xfrm_me_harder(struct net *net, struct sk_buff *skb, unsigned int family) struct flowi fl; unsigned int hh_len; struct dst_entry *dst; + struct sock *sk = skb->sk; int err; err = xfrm_decode_session(skb, &fl, family); @@ -119,7 +120,10 @@ int nf_xfrm_me_harder(struct net *net, struct sk_buff *skb, unsigned int family) dst = ((struct xfrm_dst *)dst)->route; dst_hold(dst); - dst = xfrm_lookup(net, dst, &fl, skb->sk, 0); + if (sk && !net_eq(net, sock_net(sk))) + sk = NULL; + + dst = xfrm_lookup(net, dst, &fl, sk, 0); if (IS_ERR(dst)) return PTR_ERR(dst); diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c index 1105a23bda5e..2b94dcc43456 100644 --- a/net/netfilter/nft_meta.c +++ b/net/netfilter/nft_meta.c @@ -107,7 +107,8 @@ static void nft_meta_get_eval(const struct nft_expr *expr, break; case NFT_META_SKUID: sk = skb_to_full_sk(skb); - if (!sk || !sk_fullsock(sk)) + if (!sk || !sk_fullsock(sk) || + !net_eq(nft_net(pkt), sock_net(sk))) goto err; read_lock_bh(&sk->sk_callback_lock); @@ -123,7 +124,8 @@ static void nft_meta_get_eval(const struct nft_expr *expr, break; case NFT_META_SKGID: sk = skb_to_full_sk(skb); - if (!sk || !sk_fullsock(sk)) + if (!sk || !sk_fullsock(sk) || + !net_eq(nft_net(pkt), sock_net(sk))) goto err; read_lock_bh(&sk->sk_callback_lock); @@ -214,7 +216,8 @@ static void nft_meta_get_eval(const struct nft_expr *expr, #ifdef CONFIG_CGROUP_NET_CLASSID case NFT_META_CGROUP: sk = skb_to_full_sk(skb); - if (!sk || !sk_fullsock(sk)) + if (!sk || !sk_fullsock(sk) || + !net_eq(nft_net(pkt), sock_net(sk))) goto err; *dest = sock_cgroup_classid(&sk->sk_cgrp_data); break; diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c index 74e1b3bd6954..998c2b546f6d 100644 --- a/net/netfilter/nft_socket.c +++ b/net/netfilter/nft_socket.c @@ -23,6 +23,9 @@ static void nft_socket_eval(const struct nft_expr *expr, struct sock *sk = skb->sk; u32 *dest = ®s->data[priv->dreg]; + if (sk && !net_eq(nft_net(pkt), sock_net(sk))) + sk = NULL; + if (!sk) switch(nft_pf(pkt)) { case NFPROTO_IPV4: @@ -39,7 +42,7 @@ static void nft_socket_eval(const struct nft_expr *expr, return; } - if(!sk) { + if (!sk) { nft_reg_store8(dest, 0); return; } diff --git a/net/netfilter/xt_cgroup.c b/net/netfilter/xt_cgroup.c index 7df2dece57d3..5d92e1781980 100644 --- a/net/netfilter/xt_cgroup.c +++ b/net/netfilter/xt_cgroup.c @@ -72,8 +72,9 @@ static bool cgroup_mt_v0(const struct sk_buff *skb, struct xt_action_param *par) { const struct xt_cgroup_info_v0 *info = par->matchinfo; + struct sock *sk = skb->sk; - if (skb->sk == NULL || !sk_fullsock(skb->sk)) + if (!sk || !sk_fullsock(sk) || !net_eq(xt_net(par), sock_net(sk))) return false; return (info->id == sock_cgroup_classid(&skb->sk->sk_cgrp_data)) ^ @@ -85,8 +86,9 @@ static bool cgroup_mt_v1(const struct sk_buff *skb, struct xt_action_param *par) const struct xt_cgroup_info_v1 *info = par->matchinfo; struct sock_cgroup_data *skcd = &skb->sk->sk_cgrp_data; struct cgroup *ancestor = info->priv; + struct sock *sk = skb->sk; - if (!skb->sk || !sk_fullsock(skb->sk)) + if (!sk || !sk_fullsock(sk) || !net_eq(xt_net(par), sock_net(sk))) return false; if (ancestor) diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c index 3d705c688a27..46686fb73784 100644 --- a/net/netfilter/xt_owner.c +++ b/net/netfilter/xt_owner.c @@ -67,7 +67,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) struct sock *sk = skb_to_full_sk(skb); struct net *net = xt_net(par); - if (sk == NULL || sk->sk_socket == NULL) + if (!sk || !sk->sk_socket || !net_eq(net, sock_net(sk))) return (info->match ^ info->invert) == 0; else if (info->match & info->invert & XT_OWNER_SOCKET) /* diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c index 07085c22b19c..f44de4bc2100 100644 --- a/net/netfilter/xt_recent.c +++ b/net/netfilter/xt_recent.c @@ -265,7 +265,8 @@ recent_mt(const struct sk_buff *skb, struct xt_action_param *par) } /* use TTL as seen before forwarding */ - if (xt_out(par) != NULL && skb->sk == NULL) + if (xt_out(par) != NULL && + (!skb->sk || !net_eq(net, sock_net(skb->sk)))) ttl++; spin_lock_bh(&recent_lock); diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c index 5c0779c4fa3c..0472f3472842 100644 --- a/net/netfilter/xt_socket.c +++ b/net/netfilter/xt_socket.c @@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par, struct sk_buff *pskb = (struct sk_buff *)skb; struct sock *sk = skb->sk; + if (!net_eq(xt_net(par), sock_net(sk))) + sk = NULL; + if (!sk) sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par)); + if (sk) { bool wildcard; bool transparent = true; @@ -113,8 +117,12 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par) struct sk_buff *pskb = (struct sk_buff *)skb; struct sock *sk = skb->sk; + if (!net_eq(xt_net(par), sock_net(sk))) + sk = NULL; + if (!sk) sk = nf_sk_lookup_slow_v6(xt_net(par), skb, xt_in(par)); + if (sk) { bool wildcard; bool transparent = true; -- 2.14.3 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct. 2018-06-27 13:34 ` [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct Flavio Leitner @ 2018-06-27 14:22 ` Florian Westphal 0 siblings, 0 replies; 13+ messages in thread From: Florian Westphal @ 2018-06-27 14:22 UTC (permalink / raw) To: Flavio Leitner Cc: netdev, Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal, netfilter-devel Flavio Leitner <fbl@redhat.com> wrote: > Netfilter assumes that if the socket is present in the skb, then > it can be used because that reference is cleaned up while the skb > is crossing netns. > > We want to change that to preserve the socket reference in a future > patch, so this is a preparation updating netfilter to check if the > socket netns matches before use it. Acked-by: Florian Westphal <fw@strlen.de> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb. 2018-06-27 13:34 [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb Flavio Leitner 2018-06-27 13:34 ` [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct Flavio Leitner @ 2018-06-27 13:34 ` Flavio Leitner 2018-06-27 13:56 ` Eric Dumazet 2018-06-27 19:39 ` [PATCH v2 net-next 0/2] net: " Cong Wang 2018-06-28 13:21 ` David Miller 3 siblings, 1 reply; 13+ messages in thread From: Flavio Leitner @ 2018-06-27 13:34 UTC (permalink / raw) To: netdev Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal, netfilter-devel, Flavio Leitner The sock reference is lost when scrubbing the packet and that breaks TSQ (TCP Small Queues) and XPS (Transmit Packet Steering) causing performance impacts of about 50% in a single TCP stream when crossing network namespaces. XPS breaks because the queue mapping stored in the socket is not available, so another random queue might be selected when the stack needs to transmit something like a TCP ACK, or TCP Retransmissions. That causes packet re-ordering and/or performance issues. TSQ breaks because it orphans the packet while it is still in the host, so packets are queued contributing to the buffer bloat problem. Preserving the sock reference fixes both issues. The socket is orphaned anyways in the receiving path before any relevant action and on TX side the netfilter checks if the reference is local before use it. Signed-off-by: Flavio Leitner <fbl@redhat.com> --- Documentation/networking/ip-sysctl.txt | 10 +++++----- net/core/skbuff.c | 1 - 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt index ce8fbf5aa63c..f4c042be0216 100644 --- a/Documentation/networking/ip-sysctl.txt +++ b/Documentation/networking/ip-sysctl.txt @@ -733,11 +733,11 @@ tcp_limit_output_bytes - INTEGER Controls TCP Small Queue limit per tcp socket. TCP bulk sender tends to increase packets in flight until it gets losses notifications. With SNDBUF autotuning, this can - result in a large amount of packets queued in qdisc/device - on the local machine, hurting latency of other flows, for - typical pfifo_fast qdiscs. - tcp_limit_output_bytes limits the number of bytes on qdisc - or device to reduce artificial RTT/cwnd and reduce bufferbloat. + result in a large amount of packets queued on the local machine + (e.g.: qdiscs, CPU backlog, or device) hurting latency of other + flows, for typical pfifo_fast qdiscs. tcp_limit_output_bytes + limits the number of bytes on qdisc or device to reduce artificial + RTT/cwnd and reduce bufferbloat. Default: 262144 tcp_challenge_ack_limit - INTEGER diff --git a/net/core/skbuff.c b/net/core/skbuff.c index b1f274f22d85..f59e98ca72c5 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -4911,7 +4911,6 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) return; ipvs_reset(skb); - skb_orphan(skb); skb->mark = 0; } EXPORT_SYMBOL_GPL(skb_scrub_packet); -- 2.14.3 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb. 2018-06-27 13:34 ` [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb Flavio Leitner @ 2018-06-27 13:56 ` Eric Dumazet 0 siblings, 0 replies; 13+ messages in thread From: Eric Dumazet @ 2018-06-27 13:56 UTC (permalink / raw) To: Flavio Leitner, netdev Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal, netfilter-devel On 06/27/2018 06:34 AM, Flavio Leitner wrote: > The sock reference is lost when scrubbing the packet and that breaks > TSQ (TCP Small Queues) and XPS (Transmit Packet Steering) causing > performance impacts of about 50% in a single TCP stream when crossing > network namespaces. > > XPS breaks because the queue mapping stored in the socket is not > available, so another random queue might be selected when the stack > needs to transmit something like a TCP ACK, or TCP Retransmissions. > That causes packet re-ordering and/or performance issues. Note we do not care if another random queue is selected when TCP retransmit after timeout happens. The problem is really when sending a normal train of packets (being retransmission or not). We want all of them going through one queue to avoid reorders. After an idle period (no packets are in any qdisc/NIC queue), we absolutely are okay to select another "random queue". This choice is driven by skb->ooo_okay Most TCP ACK packets are sent while no prior packet is in qdisc, so should have ooo_okay set to 1. > > TSQ breaks because it orphans the packet while it is still in the > host, so packets are queued contributing to the buffer bloat problem. > > Preserving the sock reference fixes both issues. The socket is > orphaned anyways in the receiving path before any relevant action > and on TX side the netfilter checks if the reference is local before > use it. > > Signed-off-by: Flavio Leitner <fbl@redhat.com> > --- > Documentation/networking/ip-sysctl.txt | 10 +++++----- > net/core/skbuff.c | 1 - > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt > index ce8fbf5aa63c..f4c042be0216 100644 > --- a/Documentation/networking/ip-sysctl.txt > +++ b/Documentation/networking/ip-sysctl.txt > @@ -733,11 +733,11 @@ tcp_limit_output_bytes - INTEGER > Controls TCP Small Queue limit per tcp socket. > TCP bulk sender tends to increase packets in flight until it > gets losses notifications. With SNDBUF autotuning, this can > - result in a large amount of packets queued in qdisc/device > - on the local machine, hurting latency of other flows, for > - typical pfifo_fast qdiscs. > - tcp_limit_output_bytes limits the number of bytes on qdisc > - or device to reduce artificial RTT/cwnd and reduce bufferbloat. > + result in a large amount of packets queued on the local machine > + (e.g.: qdiscs, CPU backlog, or device) hurting latency of other > + flows, for typical pfifo_fast qdiscs. tcp_limit_output_bytes > + limits the number of bytes on qdisc or device to reduce artificial > + RTT/cwnd and reduce bufferbloat. > Default: 262144 > > tcp_challenge_ack_limit - INTEGER > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index b1f274f22d85..f59e98ca72c5 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -4911,7 +4911,6 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) > return; > > ipvs_reset(skb); > - skb_orphan(skb); > skb->mark = 0; > } > EXPORT_SYMBOL_GPL(skb_scrub_packet); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-27 13:34 [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb Flavio Leitner 2018-06-27 13:34 ` [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct Flavio Leitner 2018-06-27 13:34 ` [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb Flavio Leitner @ 2018-06-27 19:39 ` Cong Wang 2018-06-28 13:20 ` David Miller 2018-06-28 21:53 ` Cong Wang 2018-06-28 13:21 ` David Miller 3 siblings, 2 replies; 13+ messages in thread From: Cong Wang @ 2018-06-27 19:39 UTC (permalink / raw) To: Flavio Leitner Cc: Linux Kernel Network Developers, Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal, NetFilter Let me rephrase why I don't like this patchset: 1. Let's forget about TSQ for a moment, skb_orphan() before leaving the stack is not just reasonable but also aligning to network isolation design. You can't claim skb_orphan() is broken from beginning, it is designed in this way and it is intentional. 2. Now, let's consider the current TSQ behavior (without any patch): 2a. For packets leaving the host or just leaving the stack to another netns, there is no difference, and this should be expected from user's point of view, because I don't need to think about its destination to decide how I should configure tcp_limit_output_bytes. 2b. The hidden pipeline behind TSQ is well defined, that is, any queues in between L4 and L2, most importantly qdisc. I can easily predict the number of queues my packets will go through with a given configuration. This also aligns with 2a. 2c. Isolation is respected as it should. TCP sockets in this netns won't be influenced by any factor in another netns. Now with your patchset: 2a. There is an apparent difference for packets leaving the host and for packets just leaving this stack. 2b. You extend the pipeline to another netns's L3, which means the number of queues is now unpredictable. 2c. Isolation is now slightly broken, the other netns could influence the source netns. I don't see you have any good argument on any of these 3 points. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-27 19:39 ` [PATCH v2 net-next 0/2] net: " Cong Wang @ 2018-06-28 13:20 ` David Miller 2018-06-28 21:41 ` Cong Wang 2018-06-28 21:53 ` Cong Wang 1 sibling, 1 reply; 13+ messages in thread From: David Miller @ 2018-06-28 13:20 UTC (permalink / raw) To: xiyou.wangcong; +Cc: fbl, netdev, eric.dumazet, pabeni, fw, netfilter-devel From: Cong Wang <xiyou.wangcong@gmail.com> Date: Wed, 27 Jun 2018 12:39:01 -0700 > Let me rephrase why I don't like this patchset: Cong, I don't think you are seeing the situation clearly and I am certainly going to apply this patch series even in the face of your objections. Suggesting that solving the lack of back pressure on a UDP socket caused by this problem by using cgroups or cpu usage controllers is just complete and utter madness. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-28 13:20 ` David Miller @ 2018-06-28 21:41 ` Cong Wang 2018-06-29 2:20 ` David Miller 0 siblings, 1 reply; 13+ messages in thread From: Cong Wang @ 2018-06-28 21:41 UTC (permalink / raw) To: David Miller Cc: Flavio Leitner, Linux Kernel Network Developers, Eric Dumazet, Paolo Abeni, Florian Westphal, NetFilter On Thu, Jun 28, 2018 at 6:20 AM David Miller <davem@davemloft.net> wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Wed, 27 Jun 2018 12:39:01 -0700 > > > Let me rephrase why I don't like this patchset: > > Cong, I don't think you are seeing the situation clearly and > I am certainly going to apply this patch series even in the > face of your objections. > > Suggesting that solving the lack of back pressure on a UDP > socket caused by this problem by using cgroups or cpu > usage controllers is just complete and utter madness. Pretty sure you didn't even read the rest of my reply, I can't help you if you just stop at where you quoted. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-28 21:41 ` Cong Wang @ 2018-06-29 2:20 ` David Miller 0 siblings, 0 replies; 13+ messages in thread From: David Miller @ 2018-06-29 2:20 UTC (permalink / raw) To: xiyou.wangcong; +Cc: fbl, netdev, eric.dumazet, pabeni, fw, netfilter-devel From: Cong Wang <xiyou.wangcong@gmail.com> Date: Thu, 28 Jun 2018 14:41:16 -0700 > Pretty sure you didn't even read the rest of my reply, > I can't help you if you just stop at where you quoted. I read your entire email, please do not accuse me of things you are not sure of. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-27 19:39 ` [PATCH v2 net-next 0/2] net: " Cong Wang 2018-06-28 13:20 ` David Miller @ 2018-06-28 21:53 ` Cong Wang 2018-06-29 2:22 ` David Miller 1 sibling, 1 reply; 13+ messages in thread From: Cong Wang @ 2018-06-28 21:53 UTC (permalink / raw) To: Flavio Leitner Cc: Linux Kernel Network Developers, Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal, NetFilter On Wed, Jun 27, 2018 at 12:39 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > Let me rephrase why I don't like this patchset: > > 1. Let's forget about TSQ for a moment, skb_orphan() before leaving > the stack is not just reasonable but also aligning to network isolation > design. You can't claim skb_orphan() is broken from beginning, it is > designed in this way and it is intentional. > > 2. Now, let's consider the current TSQ behavior (without any patch): > > 2a. For packets leaving the host or just leaving the stack to another > netns, there is no difference, and this should be expected from user's > point of view, because I don't need to think about its destination to > decide how I should configure tcp_limit_output_bytes. > > 2b. The hidden pipeline behind TSQ is well defined, that is, any > queues in between L4 and L2, most importantly qdisc. I can easily > predict the number of queues my packets will go through with a > given configuration. This also aligns with 2a. > > 2c. Isolation is respected as it should. TCP sockets in this netns > won't be influenced by any factor in another netns. > > Now with your patchset: > > 2a. There is an apparent difference for packets leaving the host > and for packets just leaving this stack. > > 2b. You extend the pipeline to another netns's L3, which means > the number of queues is now unpredictable. > > 2c. Isolation is now slightly broken, the other netns could influence > the source netns. > > I don't see you have any good argument on any of these 3 points. No one finishes reading this. I will send a revert with quote of the above. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-28 21:53 ` Cong Wang @ 2018-06-29 2:22 ` David Miller 2018-06-30 0:15 ` Cong Wang 0 siblings, 1 reply; 13+ messages in thread From: David Miller @ 2018-06-29 2:22 UTC (permalink / raw) To: xiyou.wangcong; +Cc: fbl, netdev, eric.dumazet, pabeni, fw, netfilter-devel From: Cong Wang <xiyou.wangcong@gmail.com> Date: Thu, 28 Jun 2018 14:53:09 -0700 > I will send a revert with quote of the above. And it will go to /dev/null as far as I am concerned. I read it the first time, so posting it again will not change my opinion of what you have to say. Cong, you really need to calm down and understand that people perhaps simply fundamentally disagree with you. And I am one of those people. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-29 2:22 ` David Miller @ 2018-06-30 0:15 ` Cong Wang 0 siblings, 0 replies; 13+ messages in thread From: Cong Wang @ 2018-06-30 0:15 UTC (permalink / raw) To: David Miller Cc: Flavio Leitner, Linux Kernel Network Developers, Eric Dumazet, Paolo Abeni, Florian Westphal, NetFilter On Thu, Jun 28, 2018 at 7:22 PM David Miller <davem@davemloft.net> wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Thu, 28 Jun 2018 14:53:09 -0700 > > > I will send a revert with quote of the above. > > And it will go to /dev/null as far as I am concerned. I read it the > first time, so posting it again will not change my opinion of what you > have to say. David, you claim you read it, now tell me, where is "cgroups" or "cpu" from? This is the link of my reply you quoted: https://marc.info/?l=linux-netdev&m=153013948711582&w=2 I did mention cgroups to Eric because of isolation, the softnet_data is per-CPU, and CPU is not isolated by netns apparently, therefore sd->input_pkt_queue can't be totally isolated for netns without cpuset. But this is never the reason why I dislike it, this is why I never even mentioned it in the link above. > > Cong, you really need to calm down and understand that people perhaps > simply fundamentally disagree with you. 1. Eric's "forwarding to eth0" is missing, never brought up until in his private reply. Without this information, XPS makes no sense at all in this patchset. For the record, I provide a different solution for Eric. 2. No one responses to: https://marc.info/?l=linux-netdev&m=153013948711582&w=2 I never expect you agree with me on all of them, but no one gives me any response to my concerns. 3. I will write a blog post to draw you some pictures, since it is so hard to understand the isolation... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb. 2018-06-27 13:34 [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb Flavio Leitner ` (2 preceding siblings ...) 2018-06-27 19:39 ` [PATCH v2 net-next 0/2] net: " Cong Wang @ 2018-06-28 13:21 ` David Miller 3 siblings, 0 replies; 13+ messages in thread From: David Miller @ 2018-06-28 13:21 UTC (permalink / raw) To: fbl; +Cc: netdev, eric.dumazet, pabeni, fw, netfilter-devel From: Flavio Leitner <fbl@redhat.com> Date: Wed, 27 Jun 2018 10:34:24 -0300 > The sock reference is lost when scrubbing the packet and that breaks > TSQ (TCP Small Queues) and XPS (Transmit Packet Steering) causing > performance impacts of about 50% in a single TCP stream when crossing > network namespaces. > > XPS breaks because the queue mapping stored in the socket is not > available, so another random queue might be selected when the stack > needs to transmit something like a TCP ACK, or TCP Retransmissions. > That causes packet re-ordering and/or performance issues. > > TSQ breaks because it orphans the packet while it is still in the > host, so packets are queued contributing to the buffer bloat problem. > > Preserving the sock reference fixes both issues. The socket is > orphaned anyways in the receiving path before any relevant action, > but the transmit side needs some extra checking included in the > first patch. > > The first patch will update netfilter to check if the socket > netns is local before use it. > > The second patch removes the skb_orphan() from the skb_scrub_packet() > and improve the documentation. > > ChangeLog: > - split into two (Eric) > - addressed Paolo's offline feedback to swap the checks in xt_socket.c > to preserve original behavior. > - improved ip-sysctl.txt (reported by Cong) Series applied, thanks Flavio. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-06-30 0:16 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-27 13:34 [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb Flavio Leitner 2018-06-27 13:34 ` [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct Flavio Leitner 2018-06-27 14:22 ` Florian Westphal 2018-06-27 13:34 ` [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb Flavio Leitner 2018-06-27 13:56 ` Eric Dumazet 2018-06-27 19:39 ` [PATCH v2 net-next 0/2] net: " Cong Wang 2018-06-28 13:20 ` David Miller 2018-06-28 21:41 ` Cong Wang 2018-06-29 2:20 ` David Miller 2018-06-28 21:53 ` Cong Wang 2018-06-29 2:22 ` David Miller 2018-06-30 0:15 ` Cong Wang 2018-06-28 13:21 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.