Re: [PATCH net-next v2 9/9] geneve: Implement rtnl changelink

2015-08-20 Thread Pravin Shelar 
On Wed, Aug 19, 2015 at 3:39 PM, Jesse Gross je...@nicira.com wrote:
 On Wed, Aug 19, 2015 at 1:12 PM, Pravin Shelar pshe...@nicira.com wrote:
 On Wed, Aug 19, 2015 at 12:40 PM, Jesse Gross je...@nicira.com wrote:
 On Mon, Aug 17, 2015 at 2:11 PM, Pravin B Shelar pshe...@nicira.com wrote:
 diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
 index e47cdd9..0d7fbef 100644
 --- a/drivers/net/geneve.c
 +++ b/drivers/net/geneve.c
 geneve-remote.sin_addr.s_addr = rem_addr;
 if (IN_MULTICAST(ntohl(geneve-remote.sin_addr.s_addr)))
 return -EINVAL;

 +   u32_to_vni(vni, geneve-vni);
 list_for_each_entry(t, gn-geneve_list, next) {
 if (!memcmp(geneve-vni, t-vni, sizeof(t-vni)) 
 rem_addr == t-remote.sin_addr.s_addr 

 I'm not sure that these types of operations are safe if the device is
 already running. We first overwrite the remote value and then we do
 error checking but that means that if there is an error, then the
 device will be left in a broken state. Don't we also need to update
 the hash table if some of these parameters change?

 ok, I will stop device before making changes. that way we can add it
 to hash table.

 I think we should still strive to make changes as minimally disruptive
 as possible. At least some changes can still be done safely at runtime
 so it would be nice to be able to handle those cleanly.

 +static int geneve_changelink(struct net_device *dev,
 +struct nlattr *tb[], struct nlattr *data[])
 +{
 [...]
 -   if (data[IFLA_GENEVE_PORT])
 -   dst_port = nla_get_u16(data[IFLA_GENEVE_PORT]);
 +   if (geneve-sock  (dst_port != ntohs(geneve-dst_port) ||
 +metadata != geneve-collect_md)) {

 It seems like in an ideal world, we wouldn't need to recreate the
 socket if metadata collection changed (assuming that there are no new
 conflicts).

 To keep changelink simple I am thinking of disallowing metadata changes.

 I guess I would rather allow it but make changes slower than disallow
 it. That way it is easier to improve in the future if necessary.

These changes need more refactoring in Geneve code. I will post it as
separate patch series once this series is in.
--
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 9/9] geneve: Implement rtnl changelink

2015-08-19 Thread Pravin Shelar 
On Wed, Aug 19, 2015 at 12:40 PM, Jesse Gross je...@nicira.com wrote:
 On Mon, Aug 17, 2015 at 2:11 PM, Pravin B Shelar pshe...@nicira.com wrote:
 diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
 index e47cdd9..0d7fbef 100644
 --- a/drivers/net/geneve.c
 +++ b/drivers/net/geneve.c
 -static int geneve_configure(struct net *net, struct net_device *dev,
 -   __be32 rem_addr, __u32 vni, __u8 ttl, __u8 tos,
 -   __u16 dst_port, bool metadata)
 +static int __geneve_configure(struct net *net, struct net_device *dev,
 + __be32 rem_addr, __u32 vni, __u8 ttl, __u8 tos,
 + __u16 dst_port, bool metadata)
  {
 [...]
 geneve-net = net;
 geneve-dev = dev;

 I guess this stuff should really be in geneve_configure() - it seems a
 bit odd to change it for a running device (even if it shouldn't
 change).

ok.

 geneve-remote.sin_addr.s_addr = rem_addr;
 if (IN_MULTICAST(ntohl(geneve-remote.sin_addr.s_addr)))
 return -EINVAL;

 +   u32_to_vni(vni, geneve-vni);
 list_for_each_entry(t, gn-geneve_list, next) {
 if (!memcmp(geneve-vni, t-vni, sizeof(t-vni)) 
 rem_addr == t-remote.sin_addr.s_addr 

 I'm not sure that these types of operations are safe if the device is
 already running. We first overwrite the remote value and then we do
 error checking but that means that if there is an error, then the
 device will be left in a broken state. Don't we also need to update
 the hash table if some of these parameters change?

ok, I will stop device before making changes. that way we can add it
to hash table.

 +static int geneve_changelink(struct net_device *dev,
 +struct nlattr *tb[], struct nlattr *data[])
 +{
 [...]
 -   if (data[IFLA_GENEVE_PORT])
 -   dst_port = nla_get_u16(data[IFLA_GENEVE_PORT]);
 +   if (geneve-sock  (dst_port != ntohs(geneve-dst_port) ||
 +metadata != geneve-collect_md)) {

 It seems like in an ideal world, we wouldn't need to recreate the
 socket if metadata collection changed (assuming that there are no new
 conflicts).

To keep changelink simple I am thinking of disallowing metadata 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

Re: [PATCH net-next v2 9/9] geneve: Implement rtnl changelink

2015-08-19 Thread Jesse Gross 
On Mon, Aug 17, 2015 at 2:11 PM, Pravin B Shelar pshe...@nicira.com wrote:
 diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
 index e47cdd9..0d7fbef 100644
 --- a/drivers/net/geneve.c
 +++ b/drivers/net/geneve.c
 -static int geneve_configure(struct net *net, struct net_device *dev,
 -   __be32 rem_addr, __u32 vni, __u8 ttl, __u8 tos,
 -   __u16 dst_port, bool metadata)
 +static int __geneve_configure(struct net *net, struct net_device *dev,
 + __be32 rem_addr, __u32 vni, __u8 ttl, __u8 tos,
 + __u16 dst_port, bool metadata)
  {
[...]
 geneve-net = net;
 geneve-dev = dev;

I guess this stuff should really be in geneve_configure() - it seems a
bit odd to change it for a running device (even if it shouldn't
change).

 geneve-remote.sin_addr.s_addr = rem_addr;
 if (IN_MULTICAST(ntohl(geneve-remote.sin_addr.s_addr)))
 return -EINVAL;

 +   u32_to_vni(vni, geneve-vni);
 list_for_each_entry(t, gn-geneve_list, next) {
 if (!memcmp(geneve-vni, t-vni, sizeof(t-vni)) 
 rem_addr == t-remote.sin_addr.s_addr 

I'm not sure that these types of operations are safe if the device is
already running. We first overwrite the remote value and then we do
error checking but that means that if there is an error, then the
device will be left in a broken state. Don't we also need to update
the hash table if some of these parameters change?

 +static int geneve_changelink(struct net_device *dev,
 +struct nlattr *tb[], struct nlattr *data[])
 +{
[...]
 -   if (data[IFLA_GENEVE_PORT])
 -   dst_port = nla_get_u16(data[IFLA_GENEVE_PORT]);
 +   if (geneve-sock  (dst_port != ntohs(geneve-dst_port) ||
 +metadata != geneve-collect_md)) {

It seems like in an ideal world, we wouldn't need to recreate the
socket if metadata collection changed (assuming that there are no new
conflicts).
--
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 9/9] geneve: Implement rtnl changelink

2015-08-19 Thread Jesse Gross 
On Wed, Aug 19, 2015 at 1:12 PM, Pravin Shelar pshe...@nicira.com wrote:
 On Wed, Aug 19, 2015 at 12:40 PM, Jesse Gross je...@nicira.com wrote:
 On Mon, Aug 17, 2015 at 2:11 PM, Pravin B Shelar pshe...@nicira.com wrote:
 diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
 index e47cdd9..0d7fbef 100644
 --- a/drivers/net/geneve.c
 +++ b/drivers/net/geneve.c
 geneve-remote.sin_addr.s_addr = rem_addr;
 if (IN_MULTICAST(ntohl(geneve-remote.sin_addr.s_addr)))
 return -EINVAL;

 +   u32_to_vni(vni, geneve-vni);
 list_for_each_entry(t, gn-geneve_list, next) {
 if (!memcmp(geneve-vni, t-vni, sizeof(t-vni)) 
 rem_addr == t-remote.sin_addr.s_addr 

 I'm not sure that these types of operations are safe if the device is
 already running. We first overwrite the remote value and then we do
 error checking but that means that if there is an error, then the
 device will be left in a broken state. Don't we also need to update
 the hash table if some of these parameters change?

 ok, I will stop device before making changes. that way we can add it
 to hash table.

I think we should still strive to make changes as minimally disruptive
as possible. At least some changes can still be done safely at runtime
so it would be nice to be able to handle those cleanly.

 +static int geneve_changelink(struct net_device *dev,
 +struct nlattr *tb[], struct nlattr *data[])
 +{
 [...]
 -   if (data[IFLA_GENEVE_PORT])
 -   dst_port = nla_get_u16(data[IFLA_GENEVE_PORT]);
 +   if (geneve-sock  (dst_port != ntohs(geneve-dst_port) ||
 +metadata != geneve-collect_md)) {

 It seems like in an ideal world, we wouldn't need to recreate the
 socket if metadata collection changed (assuming that there are no new
 conflicts).

 To keep changelink simple I am thinking of disallowing metadata changes.

I guess I would rather allow it but make changes slower than disallow
it. That way it is easier to improve in the future if necessary.
--
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

[PATCH net-next v2 9/9] geneve: Implement rtnl changelink

2015-08-17 Thread Pravin B Shelar 
Allow run time changes to Geneve device configuration.

Signed-off-by: Pravin B Shelar pshe...@nicira.com
---
 drivers/net/geneve.c | 159 +--
 1 file changed, 115 insertions(+), 44 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index e47cdd9..0d7fbef 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -70,12 +70,18 @@ struct geneve_sock {
struct hlist_head   vni_list[VNI_HASH_SIZE];
 };
 
-static inline __u32 geneve_net_vni_hash(u8 vni[3])
+/* Convert 64 bit tunnel ID to 24 bit VNI. */
+static void tunnel_id_to_vni(__be64 tun_id, __u8 *vni)
 {
-   __u32 vnid;
-
-   vnid = (vni[0]  16) | (vni[1]  8) | vni[2];
-   return hash_32(vnid, VNI_HASH_BITS);
+#ifdef __BIG_ENDIAN
+   vni[0] = (__force __u8)(tun_id  16);
+   vni[1] = (__force __u8)(tun_id  8);
+   vni[2] = (__force __u8)tun_id;
+#else
+   vni[0] = (__force __u8)((__force u64)tun_id  40);
+   vni[1] = (__force __u8)((__force u64)tun_id  48);
+   vni[2] = (__force __u8)((__force u64)tun_id  56);
+#endif
 }
 
 static __be64 vni_to_tunnel_id(const __u8 *vni)
@@ -89,8 +95,28 @@ static __be64 vni_to_tunnel_id(const __u8 *vni)
 #endif
 }
 
+static u32 vni_to_u32(const u8 vni[])
+{
+   return (vni[0]  16) | (vni[1]  8) | vni[2];
+}
+
+static void u32_to_vni(u32 id, u8 vni[])
+{
+   vni[0] = (id  0x00ff)  16;
+   vni[1] = (id  0xff00)  8;
+   vni[2] =  id  0x00ff;
+}
+
+static inline __u32 geneve_net_vni_hash(const u8 vni[])
+{
+   __u32 vnid;
+
+   vnid = vni_to_u32(vni);
+   return hash_32(vnid, VNI_HASH_BITS);
+}
+
 static struct geneve_dev *geneve_lookup(struct geneve_sock *gs,
-   __be32 addr, u8 vni[])
+   __be32 addr, const u8 vni[])
 {
struct hlist_head *vni_list_head;
struct geneve_dev *geneve;
@@ -120,7 +146,7 @@ static void geneve_rx(struct geneve_sock *gs, struct 
sk_buff *skb)
struct geneve_dev *geneve = NULL;
struct pcpu_sw_netstats *stats;
struct iphdr *iph;
-   u8 *vni;
+   const u8 *vni;
__be32 addr;
bool xnet;
int err;
@@ -519,6 +545,7 @@ static int geneve_stop(struct net_device *dev)
if (!hlist_unhashed(geneve-hlist))
hlist_del_rcu(geneve-hlist);
geneve_sock_release(gs);
+   geneve-sock = NULL;
return 0;
 }
 
@@ -613,20 +640,6 @@ static struct rtable *geneve_get_rt(struct sk_buff *skb,
return rt;
 }
 
-/* Convert 64 bit tunnel ID to 24 bit VNI. */
-static void tunnel_id_to_vni(__be64 tun_id, __u8 *vni)
-{
-#ifdef __BIG_ENDIAN
-   vni[0] = (__force __u8)(tun_id  16);
-   vni[1] = (__force __u8)(tun_id  8);
-   vni[2] = (__force __u8)tun_id;
-#else
-   vni[0] = (__force __u8)((__force u64)tun_id  40);
-   vni[1] = (__force __u8)((__force u64)tun_id  48);
-   vni[2] = (__force __u8)((__force u64)tun_id  56);
-#endif
-}
-
 static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev)
 {
struct geneve_dev *geneve = netdev_priv(dev);
@@ -800,26 +813,22 @@ static int geneve_validate(struct nlattr *tb[], struct 
nlattr *data[])
return 0;
 }
 
-static int geneve_configure(struct net *net, struct net_device *dev,
-   __be32 rem_addr, __u32 vni, __u8 ttl, __u8 tos,
-   __u16 dst_port, bool metadata)
+static int __geneve_configure(struct net *net, struct net_device *dev,
+ __be32 rem_addr, __u32 vni, __u8 ttl, __u8 tos,
+ __u16 dst_port, bool metadata)
 {
struct geneve_net *gn = net_generic(net, geneve_net_id);
struct geneve_dev *geneve = netdev_priv(dev);
struct geneve_dev *t;
-   int err;
 
geneve-net = net;
geneve-dev = dev;
 
-   geneve-vni[0] = (vni  0x00ff)  16;
-   geneve-vni[1] = (vni  0xff00)  8;
-   geneve-vni[2] =  vni  0x00ff;
-
geneve-remote.sin_addr.s_addr = rem_addr;
if (IN_MULTICAST(ntohl(geneve-remote.sin_addr.s_addr)))
return -EINVAL;
 
+   u32_to_vni(vni, geneve-vni);
list_for_each_entry(t, gn-geneve_list, next) {
if (!memcmp(geneve-vni, t-vni, sizeof(t-vni)) 
rem_addr == t-remote.sin_addr.s_addr 
@@ -832,6 +841,22 @@ static int geneve_configure(struct net *net, struct 
net_device *dev,
geneve-dst_port = htons(dst_port);
geneve-collect_md = metadata;
 
+   return 0;
+}
+
+static int geneve_configure(struct net *net, struct net_device *dev,
+   __be32 rem_addr, __u32 vni, __u8 ttl, __u8 tos,
+   __u32 dst_port, bool metadata)
+{
+   struct geneve_net *gn = net_generic(net, geneve_net_id);
+   struct geneve_dev *geneve = netdev_priv(dev);
+   int err;
+
+   err = __geneve_configure(net, dev,