| Message ID | 20170919003904.5124-4-tom@quantonium.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | gtp: Additional feature support | expand |
From: Tom Herbert <tom@quantonium.net> Date: Mon, 18 Sep 2017 17:38:53 -0700 > Call ip_tunnel_get_route and dst_cache to pdp context which should > improve performance by obviating the need to perform a route lookup > on every packet. > > Signed-off-by: Tom Herbert <tom@quantonium.net> Not caused by your changes, but something to think about: > -static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4, > - const struct sock *sk, > - __be32 daddr) > -{ > - memset(fl4, 0, sizeof(*fl4)); > - fl4->flowi4_oif = sk->sk_bound_dev_if; > - fl4->daddr = daddr; > - fl4->saddr = inet_sk(sk)->inet_saddr; > - fl4->flowi4_tos = RT_CONN_FLAGS(sk); > - fl4->flowi4_proto = sk->sk_protocol; > - > - return ip_route_output_key(sock_net(sk), fl4); > -} This and the new dst caching code ignores any source address selection done by ip_route_output_key() or the new tunnel route lookup helpers. Either source address selection should be respected, or if saddr will never be modified by a route lookup for some specific reason here, that should be documented.
Hi Dave, On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote: > This and the new dst caching code ignores any source address selection > done by ip_route_output_key() or the new tunnel route lookup helpers. > > Either source address selection should be respected, or if saddr will > never be modified by a route lookup for some specific reason here, > that should be documented. The IP source address is fixed by signaling on the GTP-C control plane and nothing that the kernel can unilaterally decide to change. Such a change of address would have to be decided by and first be signaled on GTP-C to the peer by the userspace daemon, which would then update the PDP context in the kernel. So I guess you're asking us to document that rationale as form of a source code comment ?
On Mon, Sep 18, 2017 at 9:17 PM, David Miller <davem@davemloft.net> wrote: > From: Tom Herbert <tom@quantonium.net> > Date: Mon, 18 Sep 2017 17:38:53 -0700 > >> Call ip_tunnel_get_route and dst_cache to pdp context which should >> improve performance by obviating the need to perform a route lookup >> on every packet. >> >> Signed-off-by: Tom Herbert <tom@quantonium.net> > > Not caused by your changes, but something to think about: > >> -static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4, >> - const struct sock *sk, >> - __be32 daddr) >> -{ >> - memset(fl4, 0, sizeof(*fl4)); >> - fl4->flowi4_oif = sk->sk_bound_dev_if; >> - fl4->daddr = daddr; >> - fl4->saddr = inet_sk(sk)->inet_saddr; >> - fl4->flowi4_tos = RT_CONN_FLAGS(sk); >> - fl4->flowi4_proto = sk->sk_protocol; >> - >> - return ip_route_output_key(sock_net(sk), fl4); >> -} > > This and the new dst caching code ignores any source address selection > done by ip_route_output_key() or the new tunnel route lookup helpers. > > Either source address selection should be respected, or if saddr will > never be modified by a route lookup for some specific reason here, > that should be documented. Yes, I noticed that. In this case the source address is intended to be taken bound on the socket which would imply we aren't interested in source address selection. Tom
From: Harald Welte <laforge@gnumonks.org> Date: Tue, 19 Sep 2017 20:09:42 +0800 > So I guess you're asking us to document that rationale as form of a > source code comment ? Yes that would make ignoring the potential changing of the non-const 'saddr' argument at least be documented.
On 19/09/17 14:09, Harald Welte wrote: > Hi Dave, > > On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote: >> This and the new dst caching code ignores any source address selection >> done by ip_route_output_key() or the new tunnel route lookup helpers. >> >> Either source address selection should be respected, or if saddr will >> never be modified by a route lookup for some specific reason here, >> that should be documented. > > The IP source address is fixed by signaling on the GTP-C control plane > and nothing that the kernel can unilaterally decide to change. Such a > change of address would have to be decided by and first be signaled on > GTP-C to the peer by the userspace daemon, which would then update the > PDP context in the kernel. I think we had this discussion before. The sending IP and port are not part of the identity of the PDP context. So IMHO the sender is permitted to change the source IP at random. Regards Andreas > > So I guess you're asking us to document that rationale as form of a > source code comment ? >
Hi Andreas, On Wed, Sep 20, 2017 at 05:37:52PM +0200, Andreas Schultz wrote: > I think we had this discussion before. The sending IP and port are not part > of the identity of the PDP context. So IMHO the sender is permitted > to change the source IP at random. Thanks for the reminder: You are correct, at least in the uplink case (MS->GGSN) where there is mobility of the MS. In the downlink case (GGSN->MS), which is the "sending" part for the kernel GTP code used at a GGSN, I'm not sure if that theory holds true in reality. Do you agree that the current behavior of not using automatic source address selection for encapsulated GTP packets but rather using the source address of the socket is intended? Do you further agree that the dst_cache support patch by Tom retains that intended behavior and it should be merged?
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index f38e32a7ec9c..95df3bcebbb2 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -63,6 +63,8 @@ struct pdp_ctx { atomic_t tx_seq; struct rcu_head rcu_head; + + struct dst_cache dst_cache; }; /* One instance of the GTP device. */ @@ -379,20 +381,6 @@ static void gtp_dev_uninit(struct net_device *dev) free_percpu(dev->tstats); } -static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4, - const struct sock *sk, - __be32 daddr) -{ - memset(fl4, 0, sizeof(*fl4)); - fl4->flowi4_oif = sk->sk_bound_dev_if; - fl4->daddr = daddr; - fl4->saddr = inet_sk(sk)->inet_saddr; - fl4->flowi4_tos = RT_CONN_FLAGS(sk); - fl4->flowi4_proto = sk->sk_protocol; - - return ip_route_output_key(sock_net(sk), fl4); -} - static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx) { int payload_len = skb->len; @@ -479,6 +467,8 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, struct rtable *rt; struct flowi4 fl4; struct iphdr *iph; + struct sock *sk; + __be32 saddr; __be16 df; int mtu; @@ -498,19 +488,27 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, } netdev_dbg(dev, "found PDP context %p\n", pctx); - rt = ip4_route_output_gtp(&fl4, pctx->sk, pctx->peer_addr_ip4.s_addr); - if (IS_ERR(rt)) { - netdev_dbg(dev, "no route to SSGN %pI4\n", - &pctx->peer_addr_ip4.s_addr); - dev->stats.tx_carrier_errors++; - goto err; - } + sk = pctx->sk; + saddr = inet_sk(sk)->inet_saddr; - if (rt->dst.dev == dev) { - netdev_dbg(dev, "circular route to SSGN %pI4\n", - &pctx->peer_addr_ip4.s_addr); - dev->stats.collisions++; - goto err_rt; + rt = ip_tunnel_get_route(dev, skb, sk->sk_protocol, + sk->sk_bound_dev_if, RT_CONN_FLAGS(sk), + pctx->peer_addr_ip4.s_addr, &saddr, + pktinfo->gtph_port, pktinfo->gtph_port, + &pctx->dst_cache, NULL); + + if (IS_ERR(rt)) { + if (rt == ERR_PTR(-ELOOP)) { + netdev_dbg(dev, "circular route to SSGN %pI4\n", + &pctx->peer_addr_ip4.s_addr); + dev->stats.collisions++; + goto err_rt; + } else { + netdev_dbg(dev, "no route to SSGN %pI4\n", + &pctx->peer_addr_ip4.s_addr); + dev->stats.tx_carrier_errors++; + goto err; + } } skb_dst_drop(skb); @@ -543,7 +541,7 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, goto err_rt; } - gtp_set_pktinfo_ipv4(pktinfo, pctx->sk, iph, pctx, rt, &fl4, dev); + gtp_set_pktinfo_ipv4(pktinfo, sk, iph, pctx, rt, &fl4, dev); gtp_push_header(skb, pktinfo); return 0; @@ -917,6 +915,7 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk, struct pdp_ctx *pctx; bool found = false; __be32 ms_addr; + int err; ms_addr = nla_get_be32(info->attrs[GTPA_MS_ADDRESS]); hash_ms = ipv4_hashfn(ms_addr) % gtp->hash_size; @@ -951,6 +950,12 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk, if (pctx == NULL) return -ENOMEM; + err = dst_cache_init(&pctx->dst_cache, GFP_KERNEL); + if (err) { + kfree(pctx); + return err; + } + sock_hold(sk); pctx->sk = sk; pctx->dev = gtp->dev;
Call ip_tunnel_get_route and dst_cache to pdp context which should improve performance by obviating the need to perform a route lookup on every packet. Signed-off-by: Tom Herbert <tom@quantonium.net> --- drivers/net/gtp.c | 59 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 27 deletions(-)