[PATCH net-next v2 7/9] geneve: Consolidate Geneve functionality in single module.
geneve_core module handles send and receive functionality.
This way OVS could use the Geneve API. Now with use of
tunnel meatadata mode OVS can directly use Geneve netdevice.
So there is no need for separate module for Geneve. Following
patch consolidates Geneve protocol processing in single module.
Signed-off-by: Pravin B Shelar
---
drivers/net/Kconfig| 2 +-
drivers/net/geneve.c | 458 +
include/net/geneve.h | 34
net/ipv4/Kconfig | 14 --
net/ipv4/Makefile | 1 -
net/ipv4/geneve_core.c | 447 ---
6 files changed, 383 insertions(+), 573 deletions(-)
delete mode 100644 net/ipv4/geneve_core.c
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index e58468b..18ff83b 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -181,7 +181,7 @@ config VXLAN
config GENEVE
tristate "Generic Network Virtualization Encapsulation netdev"
- depends on INET && GENEVE_CORE
+ depends on INET
select NET_IP_TUNNEL
---help---
This allows one to create geneve virtual interfaces that provide
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 5b43382..eb298ff 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#define GENEVE_NETDEV_VER "0.6"
@@ -33,13 +34,18 @@ static bool log_ecn_error = true;
module_param(log_ecn_error, bool, 0644);
MODULE_PARM_DESC(log_ecn_error, "Log packets received with corrupted ECN");
+#define GENEVE_VER 0
+#define GENEVE_BASE_HLEN (sizeof(struct udphdr) + sizeof(struct genevehdr))
+
/* per-network namespace private data for this module */
struct geneve_net {
- struct list_head geneve_list;
- struct hlist_head vni_list[VNI_HASH_SIZE];
- struct geneve_dev __rcu *collect_md_tun;
+ struct list_headgeneve_list;
+ struct hlist_head vni_list[VNI_HASH_SIZE];
+ struct list_headsock_list;
};
+static int geneve_net_id;
+
/* Pseudo network device */
struct geneve_dev {
struct hlist_node hlist; /* vni hash table */
@@ -55,7 +61,15 @@ struct geneve_dev {
bool collect_md;
};
-static int geneve_net_id;
+struct geneve_sock {
+ boolcollect_md;
+ struct geneve_net *gn;
+ struct list_headlist;
+ struct socket *sock;
+ struct rcu_head rcu;
+ int refcnt;
+ struct udp_offload udp_offloads;
+};
static inline __u32 geneve_net_vni_hash(u8 vni[3])
{
@@ -76,51 +90,63 @@ static __be64 vni_to_tunnel_id(const __u8 *vni)
#endif
}
-static struct geneve_dev *geneve_lookup(struct geneve_net *gn,
- struct geneve_sock *gs,
- struct iphdr *iph,
- struct genevehdr *gnvh)
+static struct geneve_dev *geneve_lookup(struct geneve_net *gn, __be16 port,
+ __be32 addr, u8 vni[])
{
- struct inet_sock *sk = inet_sk(gs->sock->sk);
struct hlist_head *vni_list_head;
struct geneve_dev *geneve;
__u32 hash;
- geneve = rcu_dereference(gn->collect_md_tun);
- if (geneve)
- return geneve;
-
/* Find the device for this VNI */
- hash = geneve_net_vni_hash(gnvh->vni);
+ hash = geneve_net_vni_hash(vni);
vni_list_head = &gn->vni_list[hash];
hlist_for_each_entry_rcu(geneve, vni_list_head, hlist) {
- if (!memcmp(gnvh->vni, geneve->vni, sizeof(geneve->vni)) &&
- iph->saddr == geneve->remote.sin_addr.s_addr &&
- sk->inet_sport == geneve->dst_port) {
+ if (!memcmp(vni, geneve->vni, sizeof(geneve->vni)) &&
+ addr == geneve->remote.sin_addr.s_addr &&
+ port == geneve->dst_port) {
return geneve;
}
}
return NULL;
}
+static inline struct genevehdr *geneve_hdr(const struct sk_buff *skb)
+{
+ return (struct genevehdr *)(udp_hdr(skb) + 1);
+}
+
/* geneve receive/decap routine */
static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
{
+ struct inet_sock *sk = inet_sk(gs->sock->sk);
struct genevehdr *gnvh = geneve_hdr(skb);
+ struct geneve_net *gn = gs->gn;
struct metadata_dst *tun_dst = NULL;
struct geneve_dev *geneve = NULL;
struct pcpu_sw_netstats *stats;
- struct geneve_net *gn;
struct iphdr *iph;
+ u8 *vni;
+ __be32 addr;
+ bool xnet;
int err;
iph = ip_hdr(skb); /* Still outer IP header... */
- gn = gs->rcv_data;
- geneve = geneve_lookup(gn, gs, iph, gnvh);
+
+ if (gs->collect_md) {
+ static u8 zero_vni[3];
+
+ v
Re: [PATCH net-next v2 7/9] geneve: Consolidate Geneve functionality in single module.
On Mon, Aug 17, 2015 at 2:11 PM, Pravin B Shelar wrote: > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > index e58468b..18ff83b 100644 > --- a/drivers/net/Kconfig > +++ b/drivers/net/Kconfig > @@ -181,7 +181,7 @@ config VXLAN > > config GENEVE > tristate "Generic Network Virtualization Encapsulation netdev" > - depends on INET && GENEVE_CORE > + depends on INET > select NET_IP_TUNNEL I think my comments on v1 one this patch were overlooked (about the UDP_TUNNEL dependency and the name). > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 5b43382..eb298ff 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > +static void geneve_build_header(struct genevehdr *geneveh, > + __be16 tun_flags, u8 vni[3], > + u8 options_len, u8 *options) [...] > +static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb, > + __be16 tun_flags, u8 vni[3], u8 opt_len, u8 *opt, > + bool csum) It seems like we could just merge these functions. I'm not sure that the role is all that different. In geneve_build_skb(), the error labels are somewhat confusing (for example, free_rt doesn't free the rt). Also, is it right that we don't free the rt if udp_tunnel_handle_offloads() fails()? It might be cleaner if the caller retains ownership of rt. My guess is that if the issue from the earlier patch about overlapping collect_md tunnels is fixed then that might allow us to simplify things a little further, since for those tunnels we can assume there is a 1:1 mapping between collect_md tunnels and sockets. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2 7/9] geneve: Consolidate Geneve functionality in single module.
On Wed, Aug 19, 2015 at 11:18 AM, Jesse Gross wrote: > On Mon, Aug 17, 2015 at 2:11 PM, Pravin B Shelar wrote: >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig >> index e58468b..18ff83b 100644 >> --- a/drivers/net/Kconfig >> +++ b/drivers/net/Kconfig >> @@ -181,7 +181,7 @@ config VXLAN >> >> config GENEVE >> tristate "Generic Network Virtualization Encapsulation netdev" >> - depends on INET && GENEVE_CORE >> + depends on INET >> select NET_IP_TUNNEL > > I think my comments on v1 one this patch were overlooked (about the > UDP_TUNNEL dependency and the name). > right, I missed it. >> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c >> index 5b43382..eb298ff 100644 >> --- a/drivers/net/geneve.c >> +++ b/drivers/net/geneve.c >> +static void geneve_build_header(struct genevehdr *geneveh, >> + __be16 tun_flags, u8 vni[3], >> + u8 options_len, u8 *options) > [...] >> +static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb, >> + __be16 tun_flags, u8 vni[3], u8 opt_len, u8 *opt, >> + bool csum) > > It seems like we could just merge these functions. I'm not sure that > the role is all that different. > ok. > In geneve_build_skb(), the error labels are somewhat confusing (for > example, free_rt doesn't free the rt). Also, is it right that we don't > free the rt if udp_tunnel_handle_offloads() fails()? It might be > cleaner if the caller retains ownership of rt. > ok. > My guess is that if the issue from the earlier patch about overlapping > collect_md tunnels is fixed then that might allow us to simplify > things a little further, since for those tunnels we can assume there > is a 1:1 mapping between collect_md tunnels and sockets. I dont see how it would be different. Can you elaborate on this ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2 7/9] geneve: Consolidate Geneve functionality in single module.
On Wed, Aug 19, 2015 at 11:29 AM, Pravin Shelar wrote: > On Wed, Aug 19, 2015 at 11:18 AM, Jesse Gross wrote: >> My guess is that if the issue from the earlier patch about overlapping >> collect_md tunnels is fixed then that might allow us to simplify >> things a little further, since for those tunnels we can assume there >> is a 1:1 mapping between collect_md tunnels and sockets. > > I dont see how it would be different. Can you elaborate on this ? Mostly just conceptually simpler. Right now it looks like we are doing some kind of refcounting between devices and tunnels in geneve_open/stop (I know it's not really but it appears like that in some ways.) We could just directly assign collect_md in geneve_open() and do nothing at all in geneve_stop(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2 7/9] geneve: Consolidate Geneve functionality in single module.
On Wed, Aug 19, 2015 at 11:37 AM, Jesse Gross wrote: > On Wed, Aug 19, 2015 at 11:29 AM, Pravin Shelar wrote: >> On Wed, Aug 19, 2015 at 11:18 AM, Jesse Gross wrote: >>> My guess is that if the issue from the earlier patch about overlapping >>> collect_md tunnels is fixed then that might allow us to simplify >>> things a little further, since for those tunnels we can assume there >>> is a 1:1 mapping between collect_md tunnels and sockets. >> >> I dont see how it would be different. Can you elaborate on this ? > > Mostly just conceptually simpler. Right now it looks like we are doing > some kind of refcounting between devices and tunnels in > geneve_open/stop (I know it's not really but it appears like that in > some ways.) We could just directly assign collect_md in geneve_open() > and do nothing at all in geneve_stop(). If you look at next patch, I have changed geneve_open and stop further. The change is geneve_open adds tunnel to hash table so that only device which are open are in hash table. Since geneve_open and stop is common for both type of tunnel I do not think there can be any changes even after avoiding overlapping tunnel types in given socket. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2 7/9] geneve: Consolidate Geneve functionality in single module.
On Wed, Aug 19, 2015 at 11:49 AM, Pravin Shelar wrote: > On Wed, Aug 19, 2015 at 11:37 AM, Jesse Gross wrote: >> On Wed, Aug 19, 2015 at 11:29 AM, Pravin Shelar wrote: >>> On Wed, Aug 19, 2015 at 11:18 AM, Jesse Gross wrote: My guess is that if the issue from the earlier patch about overlapping collect_md tunnels is fixed then that might allow us to simplify things a little further, since for those tunnels we can assume there is a 1:1 mapping between collect_md tunnels and sockets. >>> >>> I dont see how it would be different. Can you elaborate on this ? >> >> Mostly just conceptually simpler. Right now it looks like we are doing >> some kind of refcounting between devices and tunnels in >> geneve_open/stop (I know it's not really but it appears like that in >> some ways.) We could just directly assign collect_md in geneve_open() >> and do nothing at all in geneve_stop(). > > If you look at next patch, I have changed geneve_open and stop > further. The change is geneve_open adds tunnel to hash table so that > only device which are open are in hash table. Since geneve_open and > stop is common for both type of tunnel I do not think there can be any > changes even after avoiding overlapping tunnel types in given socket. I guess I'm not sure why with the later changes it would be incompatible. All I'm talking about is something pretty small: geneve_open: if (geneve->collect_md) gs->collect_md = true; to gs->collect_md = geneve->collect_md; geneve_close: remove if (geneve->collect_md) gs->collect_md = false; since the socket is about to be freed anyways. It's not very different in practice but it looks less like refcounting and more like a 1:1 mapping. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2 7/9] geneve: Consolidate Geneve functionality in single module.
On Wed, Aug 19, 2015 at 12:50 PM, Jesse Gross wrote: > On Wed, Aug 19, 2015 at 11:49 AM, Pravin Shelar wrote: >> On Wed, Aug 19, 2015 at 11:37 AM, Jesse Gross wrote: >>> On Wed, Aug 19, 2015 at 11:29 AM, Pravin Shelar wrote: On Wed, Aug 19, 2015 at 11:18 AM, Jesse Gross wrote: > My guess is that if the issue from the earlier patch about overlapping > collect_md tunnels is fixed then that might allow us to simplify > things a little further, since for those tunnels we can assume there > is a 1:1 mapping between collect_md tunnels and sockets. I dont see how it would be different. Can you elaborate on this ? >>> >>> Mostly just conceptually simpler. Right now it looks like we are doing >>> some kind of refcounting between devices and tunnels in >>> geneve_open/stop (I know it's not really but it appears like that in >>> some ways.) We could just directly assign collect_md in geneve_open() >>> and do nothing at all in geneve_stop(). >> >> If you look at next patch, I have changed geneve_open and stop >> further. The change is geneve_open adds tunnel to hash table so that >> only device which are open are in hash table. Since geneve_open and >> stop is common for both type of tunnel I do not think there can be any >> changes even after avoiding overlapping tunnel types in given socket. > > I guess I'm not sure why with the later changes it would be > incompatible. All I'm talking about is something pretty small: > > geneve_open: > if (geneve->collect_md) > gs->collect_md = true; > to > gs->collect_md = geneve->collect_md; > > geneve_close: > remove > if (geneve->collect_md) > gs->collect_md = false; > since the socket is about to be freed anyways. > > It's not very different in practice but it looks less like refcounting > and more like a 1:1 mapping. ok, I thought you were talking about socket refcounting. I will make the changes. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
