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, &geneve->info, sizeof(info));
+ metadata = geneve->collect_md;
+ use_udp6_rx_checksums = geneve->use_udp6_rx_checksums;
+ err = geneve_nl2info(dev, tb, data, &info, &metadata,
+ &use_udp6_rx_checksums, true);
+ if (err)
+ return err;
+
+ if (!geneve_dst_addr_equal(&geneve->info, &info))
+ dst_cache_reset(&info.dst_cache);
+
+ geneve_quiesce(geneve, &gs4, &gs6);
+ 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
