| Message ID | 20201211122612.869225-1-jonas@norrbonn.se |
|---|---|
| Headers | show |
| Series | gtp: IPv6 support | expand |
On Fri, Dec 11, 2020 at 4:28 AM Jonas Bonn <jonas@norrbonn.se> wrote: > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > --- > drivers/net/gtp.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > index 236ebbcb37bf..7bbeec173113 100644 > --- a/drivers/net/gtp.c > +++ b/drivers/net/gtp.c > @@ -536,7 +536,11 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev) > if (unlikely(r)) > goto err_rt; > > - skb_reset_inner_headers(skb); > + r = udp_tunnel_handle_offloads(skb, true); > + if (unlikely(r)) > + goto err_rt; > + > + skb_set_inner_protocol(skb, skb->protocol); > This should be skb_set_inner_ipproto(), since GTP is L3 protocol. > gtp_push_header(skb, pctx, &port); > > @@ -618,6 +622,8 @@ static void gtp_link_setup(struct net_device *dev) > > dev->priv_flags |= IFF_NO_QUEUE; > dev->features |= NETIF_F_LLTX; > + dev->hw_features |= NETIF_F_SG | NETIF_F_GSO_SOFTWARE | NETIF_F_HW_CSUM; > + dev->features |= NETIF_F_SG | NETIF_F_GSO_SOFTWARE | NETIF_F_HW_CSUM; > netif_keep_dst(dev); > > dev->needed_headroom = LL_MAX_HEADER + max_gtp_header_len; > -- > 2.27.0 >
On Fri, Dec 11, 2020 at 4:29 AM Jonas Bonn <jonas@norrbonn.se> wrote: > > All GTP traffic is currently sent from the same source port. This makes > everything look like one big flow which is difficult to balance across > network resources. > > From 3GPP TS 29.281: > "...the UDP Source Port or the Flow Label field... should be set dynamically > by the sending GTP-U entity to help balancing the load in the transport > network." > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > --- > drivers/net/gtp.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > index 4a3a52970856..236ebbcb37bf 100644 > --- a/drivers/net/gtp.c > +++ b/drivers/net/gtp.c > @@ -477,7 +477,7 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev) > __be32 saddr; > struct iphdr *iph; > int headroom; > - __be16 port; > + __be16 sport, port; > int r; > > /* Read the IP destination address and resolve the PDP context. > @@ -527,6 +527,10 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev) > return -EMSGSIZE; > } > > + sport = udp_flow_src_port(sock_net(pctx->sk), skb, > + 0, USHRT_MAX, > + true); > + why use_eth is true for this is L3 GTP devices, Am missing something? > /* Ensure there is sufficient headroom. */ > r = skb_cow_head(skb, headroom); > if (unlikely(r)) > @@ -545,7 +549,7 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev) > iph->tos, > ip4_dst_hoplimit(&rt->dst), > 0, > - port, port, > + sport, port, > !net_eq(sock_net(pctx->sk), > dev_net(pctx->dev)), > false); > -- > 2.27.0 >
On 12/12/2020 06:35, Pravin Shelar wrote: > On Fri, Dec 11, 2020 at 4:29 AM Jonas Bonn <jonas@norrbonn.se> wrote: >> /* Read the IP destination address and resolve the PDP context. >> @@ -527,6 +527,10 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev) >> return -EMSGSIZE; >> } >> >> + sport = udp_flow_src_port(sock_net(pctx->sk), skb, >> + 0, USHRT_MAX, >> + true); >> + > why use_eth is true for this is L3 GTP devices, Am missing something? No, you are right. Will fix. /Jonas
On 12/12/2020 06:31, Pravin Shelar wrote: > On Fri, Dec 11, 2020 at 4:28 AM Jonas Bonn <jonas@norrbonn.se> wrote: >> >> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> >> --- >> drivers/net/gtp.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c >> index 236ebbcb37bf..7bbeec173113 100644 >> --- a/drivers/net/gtp.c >> +++ b/drivers/net/gtp.c >> @@ -536,7 +536,11 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev) >> if (unlikely(r)) >> goto err_rt; >> >> - skb_reset_inner_headers(skb); >> + r = udp_tunnel_handle_offloads(skb, true); >> + if (unlikely(r)) >> + goto err_rt; >> + >> + skb_set_inner_protocol(skb, skb->protocol); >> > This should be skb_set_inner_ipproto(), since GTP is L3 protocol. I think you're right, but I barely see the point of this. It all ends up in the same place, as far as I can tell. Is this supposed to be some sort of safeguard against trying to offload tunnels-in-tunnels? /Jonas
On Fri, Dec 11, 2020 at 01:26:01PM +0100, Jonas Bonn wrote: > The GTP link is brought up with a default MTU of zero. This can lead to > some rather unexpected behaviour for users who are more accustomed to > interfaces coming online with reasonable defaults. > > This patch sets an initial MTU for the GTP link of 1500 less worst-case > tunnel overhead. Thanks, LGTM. I would probably have gone to a #define or a 'const' variable, but I guess compilers should be smart enough to figure out that this is static at compile time even the way you wrote it. Acked-by: Harald Welte <laforge@gnumonks.org> -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Hi Jonas, On Fri, Dec 11, 2020 at 01:26:02PM +0100, Jonas Bonn wrote: > Querying link info for the GTP interface doesn't reveal in which "role" the > device is set to operate. Include this information in the info query > result. > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> Acked-by: Harald Welte <laforge@gnumonks.org> -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On Fri, Dec 11, 2020 at 01:26:03PM +0100, Jonas Bonn wrote: > Blindly assuming that packet transmission crosses namespaces results in > skb marks being lost in the single namespace case. > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> Acked-by: Harald Welte <laforge@gnumonks.org> -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On Fri, Dec 11, 2020 at 01:26:04PM +0100, Jonas Bonn wrote:
> The call to skb_dst_drop() is already done as part of udp_tunnel_xmit().
I must be blind, can you please point out where exactly this happens?
I don't see any skb_dst_drop in udp_tunnel_xmit_skb, and
in iptunnel_xmit() there's only a skb_dst_set (which doesn't call skb_dst_drop internally)
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
Hi Jonas, On Fri, Dec 11, 2020 at 01:26:07PM +0100, Jonas Bonn wrote: > From 3GPP TS 29.281: > "...the UDP Source Port or the Flow Label field... should be set dynamically > by the sending GTP-U entity to help balancing the load in the transport > network." You unfortuantely didn't specifiy which 3GPP release you are referring to. At least in V15.7.0 (2020-01) Release 15 I can only find: "For the messages described below, the UDP Source Port (except as specified for the Echo Response message) may be allocated either statically or dynamically by the sending GTP-U entity. NOTE: Dynamic allocation of the UDP source port can help balancing the load in the network, depending on network deployments and network node implementations." For GTPv0, TS 29.060 states: "The UDP Source Port is a locally allocated port number at the sending GSN/RNC." unfortuantely it doesn't say if it's a locally allocated number globally for that entire GSN/RNC, or it's dynamic per flow or per packet. As I'm aware of a lot of very tight packet filtering between GSNs, I would probably not go for fully dynamic source port allocation without some kind of way how the user (GTP-control instance) being able to decide on that policy. -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On 12/12/2020 10:50, Harald Welte wrote: > On Fri, Dec 11, 2020 at 01:26:04PM +0100, Jonas Bonn wrote: >> The call to skb_dst_drop() is already done as part of udp_tunnel_xmit(). > > I must be blind, can you please point out where exactly this happens? It's in skb_scrub_packet() which is called by iptunnel_xmit(). /Jonas > > I don't see any skb_dst_drop in udp_tunnel_xmit_skb, and > in iptunnel_xmit() there's only a skb_dst_set (which doesn't call skb_dst_drop internally) >
Hi Harald, On 12/12/2020 11:07, Harald Welte wrote: > Hi Jonas, > > On Fri, Dec 11, 2020 at 01:26:07PM +0100, Jonas Bonn wrote: >> From 3GPP TS 29.281: >> "...the UDP Source Port or the Flow Label field... should be set dynamically >> by the sending GTP-U entity to help balancing the load in the transport >> network." > > You unfortuantely didn't specifiy which 3GPP release you are referring to. > Sorry about that. I'm looking at v16.1.0. > At least in V15.7.0 (2020-01) Release 15 I can only find: > > "For the messages described below, the UDP Source Port (except as > specified for the Echo Response message) may be allocated either > statically or dynamically by the sending GTP-U entity. NOTE: Dynamic > allocation of the UDP source port can help balancing the load in the > network, depending on network deployments and network node > implementations." > > For GTPv0, TS 29.060 states: > > "The UDP Source Port is a locally allocated port number at the sending > GSN/RNC." > > unfortuantely it doesn't say if it's a locally allocated number globally > for that entire GSN/RNC, or it's dynamic per flow or per packet. I could interpret that to mean that the receiving end shouldn't be too bothered about what port a packet happens to come from... That said, dynamic per flow is almost certainly what we want here as this is mostly about being able to trigger receive side scaling for network cards that can't look at inner headers. > > As I'm aware of a lot of very tight packet filtering between GSNs, > I would probably not go for fully dynamic source port allocation > without some kind of way how the user (GTP-control instance) being > able to decide on that policy. > Sure... sounds reasonable. A flag on the link setup indicating dynamic source port yes/no should be sufficient, right? /Jonas
On Fri, Dec 11, 2020 at 11:50 PM Jonas Bonn <jonas@norrbonn.se> wrote: > > > > On 12/12/2020 06:31, Pravin Shelar wrote: > > On Fri, Dec 11, 2020 at 4:28 AM Jonas Bonn <jonas@norrbonn.se> wrote: > >> > >> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > >> --- > >> drivers/net/gtp.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > >> index 236ebbcb37bf..7bbeec173113 100644 > >> --- a/drivers/net/gtp.c > >> +++ b/drivers/net/gtp.c > >> @@ -536,7 +536,11 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev) > >> if (unlikely(r)) > >> goto err_rt; > >> > >> - skb_reset_inner_headers(skb); > >> + r = udp_tunnel_handle_offloads(skb, true); > >> + if (unlikely(r)) > >> + goto err_rt; > >> + > >> + skb_set_inner_protocol(skb, skb->protocol); > >> > > This should be skb_set_inner_ipproto(), since GTP is L3 protocol. > > I think you're right, but I barely see the point of this. It all ends > up in the same place, as far as I can tell. Is this supposed to be some > sort of safeguard against trying to offload tunnels-in-tunnels? > In UDP offload this flag is used to process segments. With correct flag GTP offload handling can directly jump to L3 processing.