| Message ID | 163361011562.2049658.18335425151080048726.stgit@fed.void |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Native tunnel: Update neigh entries in tnl termination. | expand |
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 <pvalerio@redhat.com> > --- > 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(&neigh->expires, &expired); > + > + 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, &table) { > if (ipv6_addr_equals(&neigh->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(&neigh->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(&neigh->expires, time_msec() + > + NEIGH_ENTRY_DEFAULT_IDLE_TIME); > ovs_mutex_unlock(&mutex); > 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(&neigh->expires, time_msec() + > + NEIGH_ENTRY_DEFAULT_IDLE_TIME); > 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); > @@ -208,7 +221,7 @@ tnl_neigh_cache_run(void) > > ovs_mutex_lock(&mutex); > CMAP_FOR_EACH(neigh, cmap_node, &table) { > - 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(&ds, 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(&ds, " STALE"); > } > ds_put_char(&ds, '\n'); > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Flavio Leitner <fbl@sysclose.org> 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 <pvalerio@redhat.com> >> --- >> 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(&neigh->expires, &expired); >> + >> + 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, &table) { >> if (ipv6_addr_equals(&neigh->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(&neigh->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(&neigh->expires, time_msec() + >> + NEIGH_ENTRY_DEFAULT_IDLE_TIME); >> ovs_mutex_unlock(&mutex); >> 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(&neigh->expires, time_msec() + >> + NEIGH_ENTRY_DEFAULT_IDLE_TIME); >> 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); >> @@ -208,7 +221,7 @@ tnl_neigh_cache_run(void) >> >> ovs_mutex_lock(&mutex); >> CMAP_FOR_EACH(neigh, cmap_node, &table) { >> - 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(&ds, 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(&ds, " STALE"); >> } >> ds_put_char(&ds, '\n'); >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > fbl
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(&neigh->expires, &expired); + + 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, &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); 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(&neigh->expires, time_msec() + + NEIGH_ENTRY_DEFAULT_IDLE_TIME); ovs_mutex_unlock(&mutex); 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(&neigh->expires, time_msec() + + NEIGH_ENTRY_DEFAULT_IDLE_TIME); 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); @@ -208,7 +221,7 @@ tnl_neigh_cache_run(void) ovs_mutex_lock(&mutex); CMAP_FOR_EACH(neigh, cmap_node, &table) { - 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(&ds, 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(&ds, " STALE"); } ds_put_char(&ds, '\n');
Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/tnl-neigh-cache.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)