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

[Girish Moodalbail <girish.moodalbail@oracle.com>]

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