subject:"\[ovs\-dev\] \[PATCH RFC 1\/5\] Native tunnel\: Read\/write expires atomically."

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

2021-11-01 Thread Paolo Valerio

Flavio Leitner  writes:

> Hi Paolo,
>
> The lookup does not change cmap, but it changes the entry which can
> be used by multiple threads. In that case, we would need a mutex to
> modify the entry. However, in this specific case only 'expires' is
> required to change, and other fields are static. Therefore, going
> with atomic op makes sense to me.
>
> Since you're using atomic op, it would be great to include
> "ovs-atomic.h", though it is indirectly included by thread
> or rcu headers.
>
> What do you think?
>

ACK. Including it explicitly it's better to me too.
Will do.

> fbl
>
>
> On Thu, Oct 07, 2021 at 02:35:15PM +0200, Paolo Valerio wrote:
>> Signed-off-by: Paolo Valerio 
>> ---
>>  lib/tnl-neigh-cache.c |   31 ++-
>>  1 file changed, 22 insertions(+), 9 deletions(-)
>> 
>> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
>> index 5bda4af7e..a37456e6d 100644
>> --- a/lib/tnl-neigh-cache.c
>> +++ b/lib/tnl-neigh-cache.c
>> @@ -44,14 +44,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)
>>  
>>  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 +64,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;
>> +
>> +atomic_read_relaxed(>expires, );
>> +
>> +return expired <= time_msec();
>> +}
>> +
>>  static struct tnl_neigh_entry *
>>  tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const struct in6_addr *dst)
>>  {
>> @@ -73,11 +83,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, ) {
>>  if (ipv6_addr_equals(>ip, dst) && !strcmp(neigh->br_name, 
>> br_name)) {
>> -if (neigh->expires <= time_now()) {
> yy> +if (tnl_neigh_expired(neigh)) {
>>  return NULL;
>>  }
>>  
>> -neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
>> +atomic_store_relaxed(>expires, time_msec() +
>> + NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>>  return neigh;
>>  }
>>  }
>> @@ -121,7 +132,8 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct 
>> in6_addr *dst,
>>  struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst);
>>  if (neigh) {
>>  if (eth_addr_equals(neigh->mac, mac)) {
>> -neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
>> +atomic_store_relaxed(>expires, time_msec() +
>> + NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>>  ovs_mutex_unlock();
>>  return;
>>  }
>> @@ -133,7 +145,8 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct 
>> in6_addr *dst,
>>  
>>  neigh->ip = *dst;
>>  neigh->mac = mac;
>> -neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
>> +atomic_store_relaxed(>expires, time_msec() +
>> + NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>>  ovs_strlcpy(neigh->br_name, name, sizeof neigh->br_name);
>>  cmap_insert(, >cmap_node, tnl_neigh_hash(>ip));
>>  ovs_mutex_unlock();
>> @@ -208,7 +221,7 @@ tnl_neigh_cache_run(void)
>>  
>>  ovs_mutex_lock();
>>  CMAP_FOR_EACH(neigh, cmap_node, ) {
>> -if (neigh->expires <= time_now()) {
>> +if (tnl_neigh_expired(neigh)) {
>>  tnl_neigh_delete(neigh);
>>  changed = true;
>>  }
>> @@ -319,7 +332,7 @@ tnl_neigh_cache_show(struct unixctl_conn *conn, int argc 
>> OVS_UNUSED,
>>  
>>  ds_put_format(, ETH_ADDR_FMT"   %s",
>>ETH_ADDR_ARGS(neigh->mac), neigh->br_name);
>> -if (neigh->expires <= time_now()) {
>> +if (tnl_neigh_expired(neigh)) {
>>  ds_put_format(, " STALE");
>>  }
>>  ds_put_char(, '\n');
>> 
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> -- 
> fbl

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2021-10-25 Thread Flavio Leitner



Hi Paolo,

The lookup does not change cmap, but it changes the entry which can
be used by multiple threads. In that case, we would need a mutex to
modify the entry. However, in this specific case only 'expires' is
required to change, and other fields are static. Therefore, going
with atomic op makes sense to me.

Since you're using atomic op, it would be great to include
"ovs-atomic.h", though it is indirectly included by thread
or rcu headers.

What do you think?

fbl


On Thu, Oct 07, 2021 at 02:35:15PM +0200, Paolo Valerio wrote:
> Signed-off-by: Paolo Valerio 
> ---
>  lib/tnl-neigh-cache.c |   31 ++-
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
> index 5bda4af7e..a37456e6d 100644
> --- a/lib/tnl-neigh-cache.c
> +++ b/lib/tnl-neigh-cache.c
> @@ -44,14 +44,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)
>  
>  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 +64,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;
> +
> +atomic_read_relaxed(>expires, );
> +
> +return expired <= time_msec();
> +}
> +
>  static struct tnl_neigh_entry *
>  tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const struct in6_addr *dst)
>  {
> @@ -73,11 +83,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, ) {
>  if (ipv6_addr_equals(>ip, dst) && !strcmp(neigh->br_name, 
> br_name)) {
> -if (neigh->expires <= time_now()) {
yy> +if (tnl_neigh_expired(neigh)) {
>  return NULL;
>  }
>  
> -neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
> +atomic_store_relaxed(>expires, time_msec() +
> + NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>  return neigh;
>  }
>  }
> @@ -121,7 +132,8 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct 
> in6_addr *dst,
>  struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst);
>  if (neigh) {
>  if (eth_addr_equals(neigh->mac, mac)) {
> -neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
> +atomic_store_relaxed(>expires, time_msec() +
> + NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>  ovs_mutex_unlock();
>  return;
>  }
> @@ -133,7 +145,8 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct 
> in6_addr *dst,
>  
>  neigh->ip = *dst;
>  neigh->mac = mac;
> -neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
> +atomic_store_relaxed(>expires, time_msec() +
> + NEIGH_ENTRY_DEFAULT_IDLE_TIME);
>  ovs_strlcpy(neigh->br_name, name, sizeof neigh->br_name);
>  cmap_insert(, >cmap_node, tnl_neigh_hash(>ip));
>  ovs_mutex_unlock();
> @@ -208,7 +221,7 @@ tnl_neigh_cache_run(void)
>  
>  ovs_mutex_lock();
>  CMAP_FOR_EACH(neigh, cmap_node, ) {
> -if (neigh->expires <= time_now()) {
> +if (tnl_neigh_expired(neigh)) {
>  tnl_neigh_delete(neigh);
>  changed = true;
>  }
> @@ -319,7 +332,7 @@ tnl_neigh_cache_show(struct unixctl_conn *conn, int argc 
> OVS_UNUSED,
>  
>  ds_put_format(, ETH_ADDR_FMT"   %s",
>ETH_ADDR_ARGS(neigh->mac), neigh->br_name);
> -if (neigh->expires <= time_now()) {
> +if (tnl_neigh_expired(neigh)) {
>  ds_put_format(, " STALE");
>  }
>  ds_put_char(, '\n');
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

-- 
fbl
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2021-10-07 Thread Paolo Valerio

Signed-off-by: Paolo Valerio 
---
 lib/tnl-neigh-cache.c |   31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
index 5bda4af7e..a37456e6d 100644
--- a/lib/tnl-neigh-cache.c
+++ b/lib/tnl-neigh-cache.c
@@ -44,14 +44,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)
 
 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 +64,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;
+
+atomic_read_relaxed(>expires, );
+
+return expired <= time_msec();
+}
+
 static struct tnl_neigh_entry *
 tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const struct in6_addr *dst)
 {
@@ -73,11 +83,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, ) {
 if (ipv6_addr_equals(>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(>expires, time_msec() +
+ NEIGH_ENTRY_DEFAULT_IDLE_TIME);
 return neigh;
 }
 }
@@ -121,7 +132,8 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct 
in6_addr *dst,
 struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst);
 if (neigh) {
 if (eth_addr_equals(neigh->mac, mac)) {
-neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
+atomic_store_relaxed(>expires, time_msec() +
+ NEIGH_ENTRY_DEFAULT_IDLE_TIME);
 ovs_mutex_unlock();
 return;
 }
@@ -133,7 +145,8 @@ tnl_neigh_set__(const char name[IFNAMSIZ], const struct 
in6_addr *dst,
 
 neigh->ip = *dst;
 neigh->mac = mac;
-neigh->expires = time_now() + NEIGH_ENTRY_DEFAULT_IDLE_TIME;
+atomic_store_relaxed(>expires, time_msec() +
+ NEIGH_ENTRY_DEFAULT_IDLE_TIME);
 ovs_strlcpy(neigh->br_name, name, sizeof neigh->br_name);
 cmap_insert(, >cmap_node, tnl_neigh_hash(>ip));
 ovs_mutex_unlock();
@@ -208,7 +221,7 @@ tnl_neigh_cache_run(void)
 
 ovs_mutex_lock();
 CMAP_FOR_EACH(neigh, cmap_node, ) {
-if (neigh->expires <= time_now()) {
+if (tnl_neigh_expired(neigh)) {
 tnl_neigh_delete(neigh);
 changed = true;
 }
@@ -319,7 +332,7 @@ tnl_neigh_cache_show(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
 
 ds_put_format(, ETH_ADDR_FMT"   %s",
   ETH_ADDR_ARGS(neigh->mac), neigh->br_name);
-if (neigh->expires <= time_now()) {
+if (tnl_neigh_expired(neigh)) {
 ds_put_format(, " STALE");
 }
 ds_put_char(, '\n');

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information