[ovs-dev] [PATCH 1/4] Native tunnel: Read/write expires atomically.

Gaëtan Rivet grive at u256.net
Wed Nov 3 18:49:32 UTC 2021


On Tue, Nov 2, 2021, at 18:12, Paolo Valerio wrote:

Hi Paolo,

I think this commit needs more details.
I'm guessing the threads involved are the PMDs and main, are there others?

Coherency is implicit on x86 cores, but those parallel read/writes are
undefined behavior & possibly incorrect on weaker memory model.

So this patch seems good to me, but it would be nice to have more context
in the log.

IIRC in the cover letter there is a short description of what this patch does,
it might be sufficient.

> Signed-off-by: Paolo Valerio <pvalerio at redhat.com>
> ---
>  lib/tnl-neigh-cache.c |   32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
> index 5bda4af7e..2769e5c3d 100644
> --- a/lib/tnl-neigh-cache.c
> +++ b/lib/tnl-neigh-cache.c
> @@ -32,6 +32,7 @@
>  #include "errno.h"
>  #include "flow.h"
>  #include "netdev.h"
> +#include "ovs-atomic.h"
>  #include "ovs-thread.h"
>  #include "packets.h"
>  #include "openvswitch/poll-loop.h"
> @@ -44,14 +45,14 @@
>  #include "openvswitch/vlog.h"
> 
> 
> -/* In seconds */
> -#define NEIGH_ENTRY_DEFAULT_IDLE_TIME  (15 * 60)
> +/* In milliseconds */
> +#define NEIGH_ENTRY_DEFAULT_IDLE_TIME  (15 * 60 * 1000)

It is easier to check correctness of macro usage if the unit is part of
the name. I think here using 'xxx_MS' would be helpful.

> 
>  struct tnl_neigh_entry {
>      struct cmap_node cmap_node;
>      struct in6_addr ip;
>      struct eth_addr mac;
> -    time_t expires;             /* Expiration time. */
> +    atomic_llong expires;       /* Expiration time in ms. */
>      char br_name[IFNAMSIZ];
>  };
> 
> @@ -64,6 +65,16 @@ tnl_neigh_hash(const struct in6_addr *ip)
>      return hash_bytes(ip->s6_addr, 16, 0);
>  }
> 
> +static bool
> +tnl_neigh_expired(struct tnl_neigh_entry *neigh)
> +{
> +    long long expired;

This is a nit but I would use 'expires', to align with the field name.

> +
> +    atomic_read_relaxed(&neigh->expires, &expired);

I'm having doubts about unfenced read / writes on expires.
Technically on lookup there would be reads then writes without barriers.
I'm not convinced it makes a difference, WDYT?

Otherwise looks good to me.

> +
> +    return expired <= time_msec();
> +}
> +
>  static struct tnl_neigh_entry *
>  tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const struct in6_addr 
> *dst)
>  {
> @@ -73,11 +84,12 @@ tnl_neigh_lookup__(const char br_name[IFNAMSIZ], 
> const struct in6_addr *dst)
>      hash = tnl_neigh_hash(dst);
>      CMAP_FOR_EACH_WITH_HASH (neigh, cmap_node, hash, &table) {
>          if (ipv6_addr_equals(&neigh->ip, dst) && 
> !strcmp(neigh->br_name, br_name)) {
> -            if (neigh->expires <= time_now()) {
> +            if (tnl_neigh_expired(neigh)) {
>                  return NULL;
>              }
> 
> -            neigh->expires = time_now() + 
> NEIGH_ENTRY_DEFAULT_IDLE_TIME;
> +            atomic_store_relaxed(&neigh->expires, time_msec() +
> +                                 NEIGH_ENTRY_DEFAULT_IDLE_TIME);

Here without lock there would be no fence.
(this write could be re-ordered before tnl_neigh_expired().)

-- 
Gaetan Rivet


More information about the dev mailing list