[ovs-dev] [PATCH RFC 2/5] Native tunnel: Add tnl/neigh/ageing command.

Paolo Valerio pvalerio at redhat.com
Mon Nov 1 08:04:22 UTC 2021


Flavio Leitner <fbl at sysclose.org> writes:

> On Thu, Oct 07, 2021 at 02:35:21PM +0200, Paolo Valerio wrote:
>> with the command is now possible to change the ageing time of the
>> cache entries.
>
> Please start with a normal sentence using a capital letter.
>

ACK

>> For the existing entries the ageing time is updated only if the
>> current expiration is greater than the new one. In any case, the next
>> refresh will set it to the new value.
>> 
>> This is intended mostly for debugging purpose.
>> 
>> Signed-off-by: Paolo Valerio <pvalerio at redhat.com>
>> ---
>>  NEWS                            |    3 ++
>>  lib/tnl-neigh-cache.c           |   77 ++++++++++++++++++++++++++++++++++-----
>>  ofproto/ofproto-tnl-unixctl.man |    5 +++
>>  3 files changed, 76 insertions(+), 9 deletions(-)
>> 
>> diff --git a/NEWS b/NEWS
>> index 90f4b1590..148dd5d61 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -10,6 +10,9 @@ Post-v2.16.0
>>         limiting behavior.
>>       * Add hardware offload support for matching IPv4/IPv6 frag types
>>         (experimental).
>> +   - Native tunnel:
>> +     * Added new ovs-appctl tnl/neigh/ageing to read/write the neigh ageing
>> +       time.
>>  
>>  
>>  v2.16.0 - 16 Aug 2021
>> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
>> index a37456e6d..c8a7b60cd 100644
>> --- a/lib/tnl-neigh-cache.c
>> +++ b/lib/tnl-neigh-cache.c
>> @@ -42,6 +42,7 @@
>>  #include "unixctl.h"
>>  #include "util.h"
>>  #include "openvswitch/vlog.h"
>> +#include "ovs-atomic.h"
>
> The previous patch is using atomic ops, so this shouldn't be needed
> here.
>

Right. I'll remove it in next respin.

>>  
>>  
>>  /* In milliseconds */
>> @@ -57,6 +58,7 @@ struct tnl_neigh_entry {
>>  
>>  static struct cmap table = CMAP_INITIALIZER;
>>  static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
>> +static atomic_uint32_t neigh_ageing;
>>  
>>  static uint32_t
>>  tnl_neigh_hash(const struct in6_addr *ip)
>> @@ -74,6 +76,15 @@ tnl_neigh_expired(struct tnl_neigh_entry *neigh)
>>      return expired <= time_msec();
>>  }
>>  
>> +static uint32_t
>> +tnl_neigh_get_ageing(void)
>> +{
>> +    unsigned int ageing;
>> +
>> +    atomic_read_relaxed(&neigh_ageing, &ageing);
>> +    return ageing;
>> +}
>> +
>>  static struct tnl_neigh_entry *
>>  tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const struct in6_addr *dst)
>>  {
>> @@ -88,7 +99,7 @@ tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const struct in6_addr *dst)
>>              }
>>  
>>              atomic_store_relaxed(&neigh->expires, time_msec() +
>> -                                 NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>> +                                 tnl_neigh_get_ageing());
>>              return neigh;
>>          }
>>      }
>> @@ -133,7 +144,7 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct in6_addr *dst,
>>      if (neigh) {
>>          if (eth_addr_equals(neigh->mac, mac)) {
>>              atomic_store_relaxed(&neigh->expires, time_msec() +
>> -                                 NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>> +                                 tnl_neigh_get_ageing());
>>              ovs_mutex_unlock(&mutex);
>>              return;
>>          }
>> @@ -146,7 +157,7 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct in6_addr *dst,
>>      neigh->ip = *dst;
>>      neigh->mac = mac;
>>      atomic_store_relaxed(&neigh->expires, time_msec() +
>> -                         NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>> +                         tnl_neigh_get_ageing());
>>      ovs_strlcpy(neigh->br_name, name, sizeof neigh->br_name);
>>      cmap_insert(&table, &neigh->cmap_node, tnl_neigh_hash(&neigh->ip));
>>      ovs_mutex_unlock(&mutex);
>> @@ -272,6 +283,43 @@ tnl_neigh_cache_flush(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>      unixctl_command_reply(conn, "OK");
>>  }
>>  
>> +static void
>> +tnl_neigh_cache_ageing(struct unixctl_conn *conn, int argc,
>> +                        const char *argv[], void *aux OVS_UNUSED)
>> +{
>> +    long long int new_exp, curr_exp;
>> +    struct tnl_neigh_entry *neigh;
>> +    uint32_t ageing;
>> +
>> +    if (argc == 1) {
>> +        struct ds ds = DS_EMPTY_INITIALIZER;
>> +        ds_put_format(&ds, "%u", tnl_neigh_get_ageing() / 1000);
>
> Shouldn't that be PRIu32?
>

ACK

>> +        unixctl_command_reply(conn, ds_cstr(&ds));
>> +        ds_destroy(&ds);
>> +
>> +        return;
>> +    }
>> +
>> +    if (!ovs_scan(argv[1], "%"SCNu32, &ageing) ||
>> +        !ageing || ageing > 10000) {
>
> Why 10000? Perhaps it needs to be defined and mentioned in the
> documentation. I suggest 3600 secs (1 hour) as an arbitrary upper
> limit.
>

ACK

>> +        unixctl_command_reply_error(conn, "bad ageing value");
>> +        return;
>> +    }
>> +
>> +    ageing *= 1000;
>> +    atomic_store_relaxed(&neigh_ageing, ageing);
>> +    new_exp = time_msec() + ageing;
>> +
>> +    CMAP_FOR_EACH (neigh, cmap_node, &table) {
>> +        atomic_read_relaxed(&neigh->expires, &curr_exp);
>> +        if (new_exp < curr_exp) {
>> +            atomic_store_relaxed(&neigh->expires, new_exp);
>> +        }
>> +    }
>> +
>> +    unixctl_command_reply(conn, "OK");
>> +}
>> +
>>  static int
>>  lookup_any(const char *host_name, struct in6_addr *address)
>>  {
>> @@ -346,10 +394,21 @@ tnl_neigh_cache_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>  void
>>  tnl_neigh_cache_init(void)
>>  {
>> -    unixctl_command_register("tnl/arp/show", "", 0, 0, tnl_neigh_cache_show, NULL);
>> -    unixctl_command_register("tnl/arp/set", "BRIDGE IP MAC", 3, 3, tnl_neigh_cache_add, NULL);
>> -    unixctl_command_register("tnl/arp/flush", "", 0, 0, tnl_neigh_cache_flush, NULL);
>> -    unixctl_command_register("tnl/neigh/show", "", 0, 0, tnl_neigh_cache_show, NULL);
>> -    unixctl_command_register("tnl/neigh/set", "BRIDGE IP MAC", 3, 3, tnl_neigh_cache_add, NULL);
>> -    unixctl_command_register("tnl/neigh/flush", "", 0, 0, tnl_neigh_cache_flush, NULL);
>> +    atomic_init(&neigh_ageing, NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>> +    unixctl_command_register("tnl/arp/show", "", 0, 0,
>> +                             tnl_neigh_cache_show, NULL);
>> +    unixctl_command_register("tnl/arp/set", "BRIDGE IP MAC", 3, 3,
>> +                             tnl_neigh_cache_add, NULL);
>> +    unixctl_command_register("tnl/arp/flush", "", 0, 0,
>> +                             tnl_neigh_cache_flush, NULL);
>> +    unixctl_command_register("tnl/arp/ageing", "[SECS]", 0, 1,
>> +                             tnl_neigh_cache_ageing, NULL);
>> +    unixctl_command_register("tnl/neigh/show", "", 0, 0,
>> +                             tnl_neigh_cache_show, NULL);
>> +    unixctl_command_register("tnl/neigh/set", "BRIDGE IP MAC", 3, 3,
>> +                             tnl_neigh_cache_add, NULL);
>> +    unixctl_command_register("tnl/neigh/flush", "", 0, 0,
>> +                             tnl_neigh_cache_flush, NULL);
>> +    unixctl_command_register("tnl/neigh/ageing", "[SECS]", 0, 1,
>> +                             tnl_neigh_cache_ageing, NULL);
>>  }
>> diff --git a/ofproto/ofproto-tnl-unixctl.man b/ofproto/ofproto-tnl-unixctl.man
>> index c70cca539..d17fea1aa 100644
>> --- a/ofproto/ofproto-tnl-unixctl.man
>> +++ b/ofproto/ofproto-tnl-unixctl.man
>> @@ -27,6 +27,11 @@ to \fImac\fR.
>>  .IP "\fBtnl/arp/flush\fR"
>>  Flush ARP table.
>>  .
>> +.IP "\fBtnl/neigh/ageing [seconds]\fR"
>> +.IP "\fBtnl/arp/ageing [seconds]\fR"
>> +Changes the ageing time. If used without arguments, it prints
>> +the current ageing value.
>
> It needs to mention the upper limit and also that it doesn't
> change if the current ageing value is greater than the new
> value.
>

Yes, this is needed. Will do in next respin.

>> +.
>>  .IP "\fBtnl/egress_port_range [num1] [num2]\fR"
>>  Set range for UDP source port used for UDP based Tunnels. For
>>  example VxLAN. If case of zero arguments this command prints
>> 
>
> I see that you wrote unit tests, so it would be great to
> include them as part of this patch instead.
>

ACK, will do.

> Thanks,
> fbl
>
>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> -- 
> fbl



More information about the dev mailing list