Re: [PATCH net-next v2 1/1] geneve: add rtnl changelink support

2017-07-20 Thread Pravin Shelar 
On Thu, Jul 20, 2017 at 10:42 AM, Girish Moodalbail
 wrote:
> Hello Pravin,
>
>
>>> +/* Quiesces the geneve device data path for both TX and RX. */
>>> +static inline void geneve_quiesce(struct geneve_dev *geneve,
>>> + struct geneve_sock **gs4,
>>> + struct geneve_sock **gs6)
>>> +{
>>> +   *gs4 = rtnl_dereference(geneve->sock4);
>>> +   rcu_assign_pointer(geneve->sock4, NULL);
>>> +
>>> +#if IS_ENABLED(CONFIG_IPV6)
>>> +   *gs6 = rtnl_dereference(geneve->sock6);
>>> +   rcu_assign_pointer(geneve->sock6, NULL);
>>> +#else
>>> +   *gs6 = NULL;
>>> +#endif
>>> +   synchronize_net();
>>> +}
>>> +
>>> +/* Resumes the geneve device data path for both TX and RX. */
>>> +static inline void geneve_unquiesce(struct geneve_dev *geneve,
>>> +   struct geneve_sock *gs4,
>>> +   struct geneve_sock __maybe_unused
>>> *gs6)
>>> +{
>>> +   rcu_assign_pointer(geneve->sock4, gs4);
>>> +#if IS_ENABLED(CONFIG_IPV6)
>>> +   rcu_assign_pointer(geneve->sock6, gs6);
>>> +#endif
>>> +   synchronize_net();
>>> +}
>>> +
>>> +static int geneve_changelink(struct net_device *dev, struct nlattr
>>> *tb[],
>>> +struct nlattr *data[],
>>> +struct netlink_ext_ack *extack)
>>> +{
>>> +   struct geneve_dev *geneve = netdev_priv(dev);
>>> +   struct geneve_sock *gs4, *gs6;
>>> +   struct ip_tunnel_info info;
>>> +   bool metadata;
>>> +   bool use_udp6_rx_checksums;
>>> +   int err;
>>> +
>>> +   /* If the geneve device is configured for metadata (or externally
>>> +* controlled, for example, OVS), then nothing can be changed.
>>> +*/
>>> +   if (geneve->collect_md)
>>> +   return -EOPNOTSUPP;
>>> +
>>> +   /* Start with the existing info. */
>>> +   memcpy(, >info, sizeof(info));
>>> +   metadata = geneve->collect_md;
>>> +   use_udp6_rx_checksums = geneve->use_udp6_rx_checksums;
>>> +   err = geneve_nl2info(dev, tb, data, , ,
>>> +_udp6_rx_checksums, true);
>>> +   if (err)
>>> +   return err;
>>> +
>>> +   if (!geneve_dst_addr_equal(>info, ))
>>> +   dst_cache_reset(_cache);
>>> +
>>> +   geneve_quiesce(geneve, , );
>>> +   geneve->info = info;
>>> +   geneve->collect_md = metadata;
>>> +   geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
>>> +   geneve_unquiesce(geneve, gs4, gs6);
>>> +
>>
>> This is nice trick. But it adds check for the socket in datapath. did
>> you explore updating entire device state in single atomic transaction?
>
>
> I did explore, however what I have now seemed like a more concise method to
> perform changelink operation atomically w.r.t the datapath.
>
> That said, there is one thing I could do. Today we already check for the
> socket in datapath like below:
>
> (a) ipv4 datapath today:
> 
>
>   geneve_xmit_skb(...)
>   |
>   +->geneve_get_v4_rt()
>  |
>  +---> if (!rcu_dereference(geneve->sock4))
> return ERR_PTR(-EIO);
>
> (b) ipv4 datapath with my current patch:
> 
>
>   geneve_xmit_skb(...)
>   |
>   +->if (!rcu_dereference(geneve->sock4))
>   |  return -EIO;
>   |
>   +->geneve_get_v4_rt()
>  |
>  +---> if (!rcu_dereference(geneve->sock4))
> return ERR_PTR(-EIO);
>
> Perhaps, I could piggyback on the already existing check for NULL socket in
> geneve_get_v4_rt() and avoid the additional check I have added in datapath.
> (The situation is same for IPv6)
>
Sounds good to me. But I would like to see comment in code explaining
it bit since this is not standard RCU usage pattern.

Re: [PATCH net-next v2 1/1] geneve: add rtnl changelink support

2017-07-20 Thread Girish Moodalbail 

Hello Pravin,


+/* Quiesces the geneve device data path for both TX and RX. */
+static inline void geneve_quiesce(struct geneve_dev *geneve,
+ struct geneve_sock **gs4,
+ struct geneve_sock **gs6)
+{
+   *gs4 = rtnl_dereference(geneve->sock4);
+   rcu_assign_pointer(geneve->sock4, NULL);
+
+#if IS_ENABLED(CONFIG_IPV6)
+   *gs6 = rtnl_dereference(geneve->sock6);
+   rcu_assign_pointer(geneve->sock6, NULL);
+#else
+   *gs6 = NULL;
+#endif
+   synchronize_net();
+}
+
+/* Resumes the geneve device data path for both TX and RX. */
+static inline void geneve_unquiesce(struct geneve_dev *geneve,
+   struct geneve_sock *gs4,
+   struct geneve_sock __maybe_unused *gs6)
+{
+   rcu_assign_pointer(geneve->sock4, gs4);
+#if IS_ENABLED(CONFIG_IPV6)
+   rcu_assign_pointer(geneve->sock6, gs6);
+#endif
+   synchronize_net();
+}
+
+static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
+struct nlattr *data[],
+struct netlink_ext_ack *extack)
+{
+   struct geneve_dev *geneve = netdev_priv(dev);
+   struct geneve_sock *gs4, *gs6;
+   struct ip_tunnel_info info;
+   bool metadata;
+   bool use_udp6_rx_checksums;
+   int err;
+
+   /* If the geneve device is configured for metadata (or externally
+* controlled, for example, OVS), then nothing can be changed.
+*/
+   if (geneve->collect_md)
+   return -EOPNOTSUPP;
+
+   /* Start with the existing info. */
+   memcpy(, >info, sizeof(info));
+   metadata = geneve->collect_md;
+   use_udp6_rx_checksums = geneve->use_udp6_rx_checksums;
+   err = geneve_nl2info(dev, tb, data, , ,
+_udp6_rx_checksums, true);
+   if (err)
+   return err;
+
+   if (!geneve_dst_addr_equal(>info, ))
+   dst_cache_reset(_cache);
+
+   geneve_quiesce(geneve, , );
+   geneve->info = info;
+   geneve->collect_md = metadata;
+   geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
+   geneve_unquiesce(geneve, gs4, gs6);
+

This is nice trick. But it adds check for the socket in datapath. did
you explore updating entire device state in single atomic transaction?


I did explore, however what I have now seemed like a more concise method to 
perform changelink operation atomically w.r.t the datapath.


That said, there is one thing I could do. Today we already check for the socket 
in datapath like below:


(a) ipv4 datapath today:


  geneve_xmit_skb(...)
  |
  +->geneve_get_v4_rt()
 |
 +---> if (!rcu_dereference(geneve->sock4))
return ERR_PTR(-EIO);

(b) ipv4 datapath with my current patch:


  geneve_xmit_skb(...)
  |
  +->if (!rcu_dereference(geneve->sock4))
  |  return -EIO;
  |
  +->geneve_get_v4_rt()
 |
 +---> if (!rcu_dereference(geneve->sock4))
return ERR_PTR(-EIO);

Perhaps, I could piggyback on the already existing check for NULL socket in 
geneve_get_v4_rt() and avoid the additional check I have added in datapath. (The 
situation is same for IPv6)


regards,
~Girish

Re: [PATCH net-next v2 1/1] geneve: add rtnl changelink support

2017-07-19 Thread Pravin Shelar 
On Tue, Jul 18, 2017 at 4:33 PM, Girish Moodalbail
 wrote:
> This patch adds changelink rtnl operation support for geneve devices
> and the code changes involve:
>
>   - add geneve_quiesce() which quiesces the geneve device data path
> for both TX and RX. This lets us perform the changelink operation
> atomically w.r.t data path. Also add geneve_unquiesce() to
> reverse the operation of geneve_quiesce().
>
>   - refactor geneve_newlink into geneve_nl2info to be used by both
> geneve_newlink and geneve_changelink
>
>   - geneve_nl2info takes a changelink boolean argument to isolate
> changelink checks.
>
>   - Allow changing only a few attributes (ttl, tos, and remote tunnel
> endpoint IP address (within the same address family)):
> - return -EOPNOTSUPP for attributes that cannot be changed for
>   now. Incremental patches can make the non-supported one
>   available in the future if needed.
>

> Signed-off-by: Girish Moodalbail 
> ---
> v0 -> v1:
>- added geneve_quiesce() and geneve_unquiesce() functions to
>  perform the changelink operation atomically w.r.t data path
> ---

>  drivers/net/geneve.c | 192 
> +--
>  1 file changed, 157 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index de8156c..829f541 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
...
...

> +/* Quiesces the geneve device data path for both TX and RX. */
> +static inline void geneve_quiesce(struct geneve_dev *geneve,
> + struct geneve_sock **gs4,
> + struct geneve_sock **gs6)
> +{
> +   *gs4 = rtnl_dereference(geneve->sock4);
> +   rcu_assign_pointer(geneve->sock4, NULL);
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +   *gs6 = rtnl_dereference(geneve->sock6);
> +   rcu_assign_pointer(geneve->sock6, NULL);
> +#else
> +   *gs6 = NULL;
> +#endif
> +   synchronize_net();
> +}
> +
> +/* Resumes the geneve device data path for both TX and RX. */
> +static inline void geneve_unquiesce(struct geneve_dev *geneve,
> +   struct geneve_sock *gs4,
> +   struct geneve_sock __maybe_unused *gs6)
> +{
> +   rcu_assign_pointer(geneve->sock4, gs4);
> +#if IS_ENABLED(CONFIG_IPV6)
> +   rcu_assign_pointer(geneve->sock6, gs6);
> +#endif
> +   synchronize_net();
> +}
> +
> +static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
> +struct nlattr *data[],
> +struct netlink_ext_ack *extack)
> +{
> +   struct geneve_dev *geneve = netdev_priv(dev);
> +   struct geneve_sock *gs4, *gs6;
> +   struct ip_tunnel_info info;
> +   bool metadata;
> +   bool use_udp6_rx_checksums;
> +   int err;
> +
> +   /* If the geneve device is configured for metadata (or externally
> +* controlled, for example, OVS), then nothing can be changed.
> +*/
> +   if (geneve->collect_md)
> +   return -EOPNOTSUPP;
> +
> +   /* Start with the existing info. */
> +   memcpy(, >info, sizeof(info));
> +   metadata = geneve->collect_md;
> +   use_udp6_rx_checksums = geneve->use_udp6_rx_checksums;
> +   err = geneve_nl2info(dev, tb, data, , ,
> +_udp6_rx_checksums, true);
> +   if (err)
> +   return err;
> +
> +   if (!geneve_dst_addr_equal(>info, ))
> +   dst_cache_reset(_cache);
> +
> +   geneve_quiesce(geneve, , );
> +   geneve->info = info;
> +   geneve->collect_md = metadata;
> +   geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
> +   geneve_unquiesce(geneve, gs4, gs6);
> +
This is nice trick. But it adds check for the socket in datapath. did
you explore updating entire device state in single atomic transaction?


> +   return 0;
> +}
> +
>  static void geneve_dellink(struct net_device *dev, struct list_head *head)
>  {
> struct geneve_dev *geneve = netdev_priv(dev);
> @@ -1375,6 +1496,7 @@ static int geneve_fill_info(struct sk_buff *skb, const 
> struct net_device *dev)
> .setup  = geneve_setup,
> .validate   = geneve_validate,
> .newlink= geneve_newlink,
> +   .changelink = geneve_changelink,
> .dellink= geneve_dellink,
> .get_size   = geneve_get_size,
> .fill_info  = geneve_fill_info,
> --
> 1.8.3.1
>

Re: [PATCH net-next v2 1/1] geneve: add rtnl changelink support

2017-07-19 Thread Girish Moodalbail 

On 7/19/17 4:51 PM, David Miller wrote:

From: Girish Moodalbail 
Date: Tue, 18 Jul 2017 16:33:06 -0700


+static inline bool geneve_dst_addr_equal(struct ip_tunnel_info *a,

  ...

+static inline void geneve_quiesce(struct geneve_dev *geneve,

  ...

+static inline void geneve_unquiesce(struct geneve_dev *geneve,


Please no inline functions in foo.c files, let the compiler
decide.


Sure thing. Will do.

regards,
~Girish



Thanks.

Re: [PATCH net-next v2 1/1] geneve: add rtnl changelink support

2017-07-19 Thread David Miller 
From: Girish Moodalbail 
Date: Tue, 18 Jul 2017 16:33:06 -0700

> +static inline bool geneve_dst_addr_equal(struct ip_tunnel_info *a,
 ...
> +static inline void geneve_quiesce(struct geneve_dev *geneve,
 ...
> +static inline void geneve_unquiesce(struct geneve_dev *geneve,

Please no inline functions in foo.c files, let the compiler
decide.

Thanks.

[PATCH net-next v2 1/1] geneve: add rtnl changelink support

2017-07-18 Thread Girish Moodalbail 
This patch adds changelink rtnl operation support for geneve devices
and the code changes involve:

  - add geneve_quiesce() which quiesces the geneve device data path
for both TX and RX. This lets us perform the changelink operation
atomically w.r.t data path. Also add geneve_unquiesce() to
reverse the operation of geneve_quiesce().

  - refactor geneve_newlink into geneve_nl2info to be used by both
geneve_newlink and geneve_changelink

  - geneve_nl2info takes a changelink boolean argument to isolate
changelink checks.

  - Allow changing only a few attributes (ttl, tos, and remote tunnel
endpoint IP address (within the same address family)):
- return -EOPNOTSUPP for attributes that cannot be changed for
  now. Incremental patches can make the non-supported one
  available in the future if needed.

Signed-off-by: Girish Moodalbail 
---
v0 -> v1:
   - added geneve_quiesce() and geneve_unquiesce() functions to
 perform the changelink operation atomically w.r.t data path
---
 drivers/net/geneve.c | 192 +--
 1 file changed, 157 insertions(+), 35 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index de8156c..829f541 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -827,6 +827,9 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct 
net_device *dev,
__be16 df;
int err;
 
+   if (!gs4)
+   return -EIO;
+
rt = geneve_get_v4_rt(skb, dev, , info);
if (IS_ERR(rt))
return PTR_ERR(rt);
@@ -866,6 +869,9 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct 
net_device *dev,
__be16 sport;
int err;
 
+   if (!gs6)
+   return -EIO;
+
dst = geneve_get_v6_dst(skb, dev, , info);
if (IS_ERR(dst))
return PTR_ERR(dst);
@@ -1140,6 +1146,15 @@ static bool is_tnl_info_zero(const struct ip_tunnel_info 
*info)
return true;
 }
 
+static inline bool geneve_dst_addr_equal(struct ip_tunnel_info *a,
+struct ip_tunnel_info *b)
+{
+   if (ip_tunnel_info_af(a) == AF_INET)
+   return a->key.u.ipv4.dst == b->key.u.ipv4.dst;
+   else
+   return ipv6_addr_equal(>key.u.ipv6.dst, >key.u.ipv6.dst);
+}
+
 static int geneve_configure(struct net *net, struct net_device *dev,
const struct ip_tunnel_info *info,
bool metadata, bool ipv6_rx_csum)
@@ -1197,24 +1212,22 @@ static void init_tnl_info(struct ip_tunnel_info *info, 
__u16 dst_port)
info->key.tp_dst = htons(dst_port);
 }
 
-static int geneve_newlink(struct net *net, struct net_device *dev,
- struct nlattr *tb[], struct nlattr *data[],
- struct netlink_ext_ack *extack)
+static int geneve_nl2info(struct net_device *dev, struct nlattr *tb[],
+ struct nlattr *data[], struct ip_tunnel_info *info,
+ bool *metadata, bool *use_udp6_rx_checksums,
+ bool changelink)
 {
-   bool use_udp6_rx_checksums = false;
-   struct ip_tunnel_info info;
-   bool metadata = false;
-
-   init_tnl_info(, GENEVE_UDP_PORT);
-
if (data[IFLA_GENEVE_REMOTE] && data[IFLA_GENEVE_REMOTE6])
return -EINVAL;
 
if (data[IFLA_GENEVE_REMOTE]) {
-   info.key.u.ipv4.dst =
+   if (changelink && (ip_tunnel_info_af(info) == AF_INET6))
+   return -EOPNOTSUPP;
+
+   info->key.u.ipv4.dst =
nla_get_in_addr(data[IFLA_GENEVE_REMOTE]);
 
-   if (IN_MULTICAST(ntohl(info.key.u.ipv4.dst))) {
+   if (IN_MULTICAST(ntohl(info->key.u.ipv4.dst))) {
netdev_dbg(dev, "multicast remote is unsupported\n");
return -EINVAL;
}
@@ -1222,21 +1235,24 @@ static int geneve_newlink(struct net *net, struct 
net_device *dev,
 
if (data[IFLA_GENEVE_REMOTE6]) {
  #if IS_ENABLED(CONFIG_IPV6)
-   info.mode = IP_TUNNEL_INFO_IPV6;
-   info.key.u.ipv6.dst =
+   if (changelink && (ip_tunnel_info_af(info) == AF_INET))
+   return -EOPNOTSUPP;
+
+   info->mode = IP_TUNNEL_INFO_IPV6;
+   info->key.u.ipv6.dst =
nla_get_in6_addr(data[IFLA_GENEVE_REMOTE6]);
 
-   if (ipv6_addr_type() &
+   if (ipv6_addr_type(>key.u.ipv6.dst) &
IPV6_ADDR_LINKLOCAL) {
netdev_dbg(dev, "link-local remote is unsupported\n");
return -EINVAL;
}
-   if (ipv6_addr_is_multicast()) {
+   if (ipv6_addr_is_multicast(>key.u.ipv6.dst)) {
netdev_dbg(dev, "multicast remote is