On 28 Sep 2026, at 15:11, Tim Rozet wrote: > The native userspace tunnel neighbor cache extends a complete entry's > lifetime whenever it is used. An idle entry is instead removed when > its aging timer expires. The first packet after removal is dropped > while OVS resolves the neighbor again. > > Refresh complete entries before they expire while keeping the cached > MAC usable. Start up to three retransmission intervals before expiration, > capped at half the aging interval, and retry at the retransmission > interval while the entry remains valid. The cap prevents immediate > repeated probes when aging and retransmission intervals are equal. > Unanswered entries still expire at their original deadline. > > Track the next cache event and queue due refreshes. Normal main-loop > iterations only compare the cached deadline; scan the table when an > event is due. Send ARP or Neighbor Discovery requests through the bridge > pipeline outside the neighbor mutex. Retain the IP interface on which > the neighbor was learned so refresh preserves policy-routing context. > For manually inserted entries, select a suitable IP interface on their > bridge. > > Renew the aging deadline on neighbor learning, rather than cache lookup. > Remove the now-unused XC_TNL_NEIGH statistics bookkeeping. Cold entries, > flushed entries, and entries lost on restart still require resolution > and can drop their triggering packet. > > Add IPv4 and IPv6 tests covering proactive refresh, retries, actual tunnel > forwarding while refresh is pending and after renewal, and expiration > without a reply. Also cover equal timers, source-based routing with two > VTEPs, and learned and manual entries on a non-local tunnel endpoint. > Verify that a changed neighbor MAC invalidates existing datapath actions > and that unanswered expiration invalidates them even with ongoing traffic. > > Assisted-by: GPT-6, OpenAI Codex > Signed-off-by: Tim Rozet <[email protected]>
Hi Tim, I think in general the code flow looks good to me. I do have some comments below. In addition, I would like Mike, who worked on this before, also to take a look. So maybe wait with a new revision until he got a chance to take a look. Cheers, Eelco > --- > v3: > - Replace stale-entry retention with proactive ARP/ND refresh. > - Schedule up to three probes before expiration, retaining the > cached MAC while awaiting a reply. > - Preserve the learned interface for policy-routed tunnels. > - Remove obsolete neighbor lookup statistics bookkeeping. > - Test cached datapath forwarding after MAC changes and invalidate > cached actions on unanswered expiration, addressing Eelco's concern. > - Freeze test clocks and avoid immediate wakeups for queued probes. > > Previous discussion: > https://mail.openvswitch.org/pipermail/ovs-dev/2026-September/436265.html > > lib/dpif-netdev.c | 1 + > lib/tnl-neigh-cache.c | 191 ++++++++++++++++++++++++----- > lib/tnl-neigh-cache.h | 6 +- > ofproto/ofproto-dpif-xlate-cache.c | 10 -- > ofproto/ofproto-dpif-xlate-cache.h | 5 - > ofproto/ofproto-dpif-xlate.c | 80 ++++++++++-- > ofproto/ofproto-dpif-xlate.h | 1 + > ofproto/ofproto-dpif.c | 2 +- > tests/tunnel-push-pop-ipv6.at | 111 ++++++++++++++++- > tests/tunnel-push-pop.at | 164 ++++++++++++++++++++++++- > 10 files changed, 510 insertions(+), 61 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 4151ea056..7d397887a 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -5946,6 +5946,7 @@ dpif_netdev_wait(struct dpif *dpif) > } > ovs_rwlock_unlock(&dp->port_rwlock); > ovs_mutex_unlock(&dp_netdev_mutex); > + tnl_neigh_cache_wait(); tnl_neigh_cache_run() and tnl_neigh_cache_wait() are called from dpif_netdev_run()/dpif_netdev_wait() in lib/dpif-netdev.c, but xlate_tnl_neigh_refresh() is called from type_run() in ofproto-dpif.c. The timer management and the work it triggers are split across two layers. Moving tnl_neigh_cache_run() and tnl_neigh_cache_wait() into type_run()/type_wait() next to xlate_tnl_neigh_refresh() would keep the whole mechanism in one place. I think this should work. > seq_wait(tnl_conf_seq, dp->last_tnl_conf_seq); > } > > diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c > index fbefc3d51..5d46b3135 100644 > --- a/lib/tnl-neigh-cache.c > +++ b/lib/tnl-neigh-cache.c > @@ -33,6 +33,7 @@ > #include "flow.h" > #include "netdev.h" > #include "ovs-atomic.h" > +#include "openvswitch/list.h" > #include "ovs-thread.h" > #include "packets.h" > #include "openvswitch/poll-loop.h" > @@ -51,17 +52,23 @@ > > struct tnl_neigh_entry { > struct cmap_node cmap_node; > + struct ovs_list refresh_node; > struct in6_addr ip; > struct eth_addr mac; > atomic_llong expires; /* Expiration time in ms. */ > char br_name[IFNAMSIZ]; > atomic_bool complete; > + char dev_name[IFNAMSIZ]; /* IP interface used to learn the neighbor. > */ > + long long refresh_at; /* Next probe, protected by mutex. */ > + bool refresh_pending; /* Queued for transmission. */ The struct fields could be grouped more logically and the comments aligned. Timers together, names together, booleans together: struct tnl_neigh_entry { struct cmap_node cmap_node; struct ovs_list refresh_node; struct in6_addr ip; struct eth_addr mac; atomic_llong expires; /* Expiration time in ms. */ long long refresh_at; /* Next probe, protected by mutex. */ char br_name[IFNAMSIZ]; char dev_name[IFNAMSIZ]; /* IP interface used to learn the neighbor. */ atomic_bool complete; bool refresh_pending; /* Queued for transmission. */ }; > }; > > static struct cmap table = CMAP_INITIALIZER; > +static struct ovs_list refresh_list = OVS_LIST_INITIALIZER(&refresh_list); refresh_list is accessed only under the mutex. Please add the OVS_GUARDED_BY(mutex) annotation: static struct ovs_list refresh_list OVS_GUARDED_BY(mutex) = OVS_LIST_INITIALIZER(&refresh_list); The new functions tnl_neigh_cancel_refresh() and tnl_neigh_get_refresh() are missing mutex annotations. tnl_neigh_cancel_refresh() and tnl_neigh_delete() are called with the mutex held and should have OVS_REQUIRES(mutex), consistent with tnl_neigh_schedule(). tnl_neigh_get_refresh() acquires the mutex internally and should have OVS_EXCLUDED(mutex). Also, refresh_list should be annotated with OVS_GUARDED_BY(mutex). There are other existing functions that should get mutex annotations, but that can be a separate cleanup. Mike anything you could take a look at? > static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER; > static atomic_uint32_t neigh_aging; > static atomic_uint32_t neigh_retrans_time; > +static atomic_llong next_event; > > static uint32_t > tnl_neigh_hash(const struct in6_addr *ip) > @@ -98,6 +105,29 @@ tnl_neigh_get_retrans_time(void) > return retrans_time; > } > > +static void > +tnl_neigh_schedule(long long deadline) > + OVS_REQUIRES(mutex) > +{ > + long long next; > + > + atomic_read_explicit(&next_event, &next, memory_order_acquire); > + if (deadline < next) { > + atomic_store_explicit(&next_event, deadline, memory_order_release); memory_order_relaxed would be sufficient here for both reads and writes. And all other places in the code. > + } > +} > + > +static long long > +tnl_neigh_refresh_deadline(long long expires) > +{ > + uint32_t refresh_time = tnl_neigh_get_retrans_time(); > + > + /* Leave time for two retries when the intervals permit, but always give > + * a learned entry at least half its lifetime before probing again, > + * including when aging == retrans. */ > + return expires - MIN(3 * refresh_time, tnl_neigh_get_aging() / 2); 3 * refresh_time can overflow uint32_t when refresh_time exceeds ~1.4 billion ms. Unlikely with real values, but Coverity will flag it. Using 3ULL would stop the report. > +} > + > static bool > tnl_neigh_is_complete(struct tnl_neigh_entry *neigh) > { > @@ -120,12 +150,6 @@ tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const > struct in6_addr *dst) > return NULL; > } > > - if (tnl_neigh_is_complete(neigh)) { > - atomic_store_explicit(&neigh->expires, > - time_msec() + tnl_neigh_get_aging(), > - memory_order_release); > - } > - > return neigh; > } > } > @@ -135,6 +159,8 @@ tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const > struct in6_addr *dst) > static void > tnl_neigh_set_partial(const char name[IFNAMSIZ], const struct in6_addr *dst) > { > + long long expires; I would just set it here in place; long long expires = time_msec() + tnl_neigh_get_retrans_time(); > + > ovs_mutex_lock(&mutex); > struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst); > if (neigh) { > @@ -145,12 +171,16 @@ tnl_neigh_set_partial(const char name[IFNAMSIZ], const > struct in6_addr *dst) > } > neigh = xmalloc(sizeof *neigh); > > + ovs_list_init(&neigh->refresh_node); nit; but I would move it right before the cmap_insert(). > neigh->ip = *dst; > atomic_store_relaxed(&neigh->complete, false); > - atomic_store_relaxed(&neigh->expires, > - time_msec() + tnl_neigh_get_retrans_time()); > + expires = time_msec() + tnl_neigh_get_retrans_time(); > + atomic_store_relaxed(&neigh->expires, expires); > + neigh->refresh_at = LLONG_MAX; > + neigh->refresh_pending = false; > ovs_strlcpy(neigh->br_name, name, sizeof neigh->br_name); dev_name is not initialised here (xmalloc does not zero). So it should be cleared: neigh->dev_name[0] = 0; ovs_list_init(&neigh->refresh_node); > cmap_insert(&table, &neigh->cmap_node, tnl_neigh_hash(&neigh->ip)); > + tnl_neigh_schedule(expires); > > ovs_mutex_unlock(&mutex); > seq_change(tnl_conf_seq); > @@ -185,18 +215,31 @@ neigh_entry_free(struct tnl_neigh_entry *neigh) > free(neigh); > } > > +static void > +tnl_neigh_cancel_refresh(struct tnl_neigh_entry *neigh) > +{ > + if (neigh->refresh_pending) { > + ovs_list_remove(&neigh->refresh_node); After removing refresh_node from the list (both in tnl_neigh_cancel_refresh() and tnl_neigh_get_refresh()), please reinitialise it with ovs_list_init() so that a stray ovs_list_remove() is a harmless no-op rather than list corruption. > + neigh->refresh_pending = false; > + } > +} > + > static void > tnl_neigh_delete(struct tnl_neigh_entry *neigh) > { > uint32_t hash = tnl_neigh_hash(&neigh->ip); > + > + tnl_neigh_cancel_refresh(neigh); > cmap_remove(&table, &neigh->cmap_node, hash); > ovsrcu_postpone(neigh_entry_free, neigh); > } > > void > tnl_neigh_set(const char name[IFNAMSIZ], const struct in6_addr *dst, > - const struct eth_addr mac) > + const struct eth_addr mac, const char *dev_name) dev_name should use the same const char [IFNAMSIZ] convention as name, and move to the second parameter to group the two name strings together, consistent with tnl_neigh_snoop() and the other functions. void tnl_neigh_set(const char name[IFNAMSIZ], const char dev_name[IFNAMSIZ], const struct in6_addr *dst, const struct eth_addr mac); > { > + long long expires long long expires = time_msec() + tnl_neigh_get_aging(); Expires is set to time_msec() + tnl_neigh_get_aging() in both branches (line 251 and line 275). Setting it once before the if-block avoids the duplication and gives both paths a consistent timestamp. > + > ovs_mutex_lock(&mutex); > struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst); > bool insert = true; > @@ -205,8 +248,14 @@ tnl_neigh_set(const char name[IFNAMSIZ], const struct > in6_addr *dst, > if (!tnl_neigh_is_complete(neigh)) { > insert = false; > } else if (eth_addr_equals(neigh->mac, mac)) { > - atomic_store_relaxed(&neigh->expires, > - time_msec() + tnl_neigh_get_aging()); > + expires = time_msec() + tnl_neigh_get_aging(); > + atomic_store_relaxed(&neigh->expires, expires); > + tnl_neigh_cancel_refresh(neigh); > + if (dev_name[0]) { > + ovs_strlcpy(neigh->dev_name, dev_name, sizeof > neigh->dev_name); > + } I guess this should be; if (dev_name && dev_name[0]) > + neigh->refresh_at = tnl_neigh_refresh_deadline(expires); > + tnl_neigh_schedule(neigh->refresh_at); > ovs_mutex_unlock(&mutex); > return; > } else { > @@ -217,19 +266,25 @@ tnl_neigh_set(const char name[IFNAMSIZ], const struct > in6_addr *dst, > if (insert) { > neigh = xmalloc(sizeof *neigh); > > + ovs_list_init(&neigh->refresh_node); > neigh->ip = *dst; > + neigh->refresh_pending = false; > ovs_strlcpy(neigh->br_name, name, sizeof neigh->br_name); nit: Would move the list_init here. > } > > + expires = time_msec() + tnl_neigh_get_aging(); > neigh->mac = mac; > - atomic_store_explicit(&neigh->expires, > - time_msec() + tnl_neigh_get_aging(), > + tnl_neigh_cancel_refresh(neigh); > + ovs_strlcpy(neigh->dev_name, dev_name, sizeof neigh->dev_name); See above; if (dev_name && dev_name[0]) > + neigh->refresh_at = tnl_neigh_refresh_deadline(expires); > + atomic_store_explicit(&neigh->expires, expires, > memory_order_release); Will fit on one line now. > atomic_store_explicit(&neigh->complete, true, memory_order_release); > > if (insert) { > cmap_insert(&table, &neigh->cmap_node, tnl_neigh_hash(&neigh->ip)); > } > + tnl_neigh_schedule(neigh->refresh_at); > > ovs_mutex_unlock(&mutex); > seq_change(tnl_conf_seq); > @@ -237,15 +292,16 @@ tnl_neigh_set(const char name[IFNAMSIZ], const struct > in6_addr *dst, > > static void > tnl_arp_set(const char name[IFNAMSIZ], ovs_be32 dst, > - const struct eth_addr mac) > + const struct eth_addr mac, const char *dev_name) > { > struct in6_addr dst6 = in6_addr_mapped_ipv4(dst); > - tnl_neigh_set(name, &dst6, mac); > + tnl_neigh_set(name, &dst6, mac, dev_name); > } > > static int > tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc, > - const char name[IFNAMSIZ], bool allow_update) > + const char name[IFNAMSIZ], const char dev_name[IFNAMSIZ], > + bool allow_update) > { > /* Snoop normal ARP replies and gratuitous ARP requests/replies only */ > if (!is_arp(flow) > @@ -258,14 +314,15 @@ tnl_arp_snoop(const struct flow *flow, struct > flow_wildcards *wc, > memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); > > if (allow_update) { > - tnl_arp_set(name, flow->nw_src, flow->arp_sha); > + tnl_arp_set(name, flow->nw_src, flow->arp_sha, dev_name); > } > return 0; > } > > static int > tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc, > - const char name[IFNAMSIZ], bool allow_update) > + const char name[IFNAMSIZ], const char dev_name[IFNAMSIZ], > + bool allow_update) > { > if (!is_nd(flow, wc) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) { > return EINVAL; > @@ -285,36 +342,66 @@ tnl_nd_snoop(const struct flow *flow, struct > flow_wildcards *wc, > memset(&wc->masks.nd_target, 0xff, sizeof wc->masks.nd_target); > > if (allow_update) { > - tnl_neigh_set(name, &flow->nd_target, flow->arp_tha); > + tnl_neigh_set(name, &flow->nd_target, flow->arp_tha, dev_name); > } > return 0; > } > > int > tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc, > - const char name[IFNAMSIZ], bool allow_update) > + const char name[IFNAMSIZ], const char dev_name[IFNAMSIZ], > + bool allow_update) > { > int res; > - res = tnl_arp_snoop(flow, wc, name, allow_update); > + res = tnl_arp_snoop(flow, wc, name, dev_name, allow_update); > if (res != EINVAL) { > return res; > } > - return tnl_nd_snoop(flow, wc, name, allow_update); > + return tnl_nd_snoop(flow, wc, name, dev_name, allow_update); > } > > void > tnl_neigh_cache_run(void) > { > struct tnl_neigh_entry *neigh; > + long long wakeup = LLONG_MAX; > + long long now = time_msec(); > + long long scheduled; > + uint32_t refresh_time; > bool changed = false; > > + atomic_read_explicit(&next_event, &scheduled, memory_order_acquire); > + if (scheduled > now) { > + return; > + } > + > + refresh_time = tnl_neigh_get_retrans_time(); > ovs_mutex_lock(&mutex); > CMAP_FOR_EACH(neigh, cmap_node, &table) { > - if (tnl_neigh_expired(neigh)) { > + long long deadline; > + long long expires; > + > + atomic_read_explicit(&neigh->expires, &expires, > + memory_order_acquire); > + if (expires <= now) { > tnl_neigh_delete(neigh); > changed = true; > + continue; > } > + > + deadline = expires; We do not need the deadline variable, use expires below directly. > + if (tnl_neigh_is_complete(neigh) && refresh_time > + && !neigh->refresh_pending) { > + if (neigh->refresh_at <= now) { > + neigh->refresh_pending = true; > + ovs_list_push_back(&refresh_list, &neigh->refresh_node); > + } else { > + deadline = MIN(expires, neigh->refresh_at); > + } > + } > + wakeup = MIN(wakeup, deadline); > } > + atomic_store_explicit(&next_event, wakeup, memory_order_release); > ovs_mutex_unlock(&mutex); > > if (changed) { > @@ -322,6 +409,46 @@ tnl_neigh_cache_run(void) > } > } > > +/* Removes one pending refresh and copies its bridge, IP interface and > + * destination. Returns false if no refresh is pending. */ > +bool > +tnl_neigh_get_refresh(char br_name[IFNAMSIZ], char dev_name[IFNAMSIZ], > + struct in6_addr *dst) > +{ > + struct tnl_neigh_entry *neigh; > + bool found = false; > + > + ovs_mutex_lock(&mutex); > + if (!ovs_list_is_empty(&refresh_list)) { > + neigh = CONTAINER_OF(ovs_list_pop_front(&refresh_list), > + struct tnl_neigh_entry, refresh_node); > + neigh->refresh_pending = false; New line > + /* A queued request may wait for the translation configuration. Arm > + * the retry only when dequeuing it, rather than repeatedly waking > for > + * an overdue request that is still pending. */ Took me a while to get the comment, would the below be more clear? /* Arm the next retry from the time the probe is actually sent, so the * full retransmission interval elapses between sends. */ > + neigh->refresh_at = time_msec() + tnl_neigh_get_retrans_time(); > + tnl_neigh_schedule(neigh->refresh_at); > + ovs_strlcpy(br_name, neigh->br_name, IFNAMSIZ); > + ovs_strlcpy(dev_name, neigh->dev_name, IFNAMSIZ); > + *dst = neigh->ip; > + found = true; > + } > + ovs_mutex_unlock(&mutex); > + > + return found; > +} > + > +void > +tnl_neigh_cache_wait(void) > +{ > + long long wakeup; > + > + atomic_read_explicit(&next_event, &wakeup, memory_order_acquire); > + if (wakeup != LLONG_MAX) { > + poll_timer_wait_until(wakeup); > + } > +} > + > void > tnl_neigh_flush(const char br_name[IFNAMSIZ]) > { > @@ -395,14 +522,19 @@ tnl_neigh_cache_aging(struct unixctl_conn *conn, int > argc, > atomic_store_explicit(&neigh_aging, aging, memory_order_release); > new_exp = time_msec() + aging; > > + ovs_mutex_lock(&mutex); > CMAP_FOR_EACH (neigh, cmap_node, &table) { > atomic_read_explicit(&neigh->expires, &curr_exp, > memory_order_acquire); > if (new_exp < curr_exp) { > atomic_store_explicit(&neigh->expires, new_exp, > memory_order_release); > + curr_exp = new_exp; > } > + neigh->refresh_at = tnl_neigh_refresh_deadline(curr_exp); > } > + tnl_neigh_schedule(time_msec()); tnl_neigh_schedule(time_msec()) on an empty table causes a spurious wakeup and scan. Should guard with a cmap_is_empty() check. Same for retrans_time below. > + ovs_mutex_unlock(&mutex); > > unixctl_command_reply(conn, "OK"); > } > @@ -438,17 +570,19 @@ tnl_neigh_cache_retrans_time(struct unixctl_conn *conn, > int argc, > memory_order_release); > new_exp = time_msec() + retrans_time; > > + ovs_mutex_lock(&mutex); > CMAP_FOR_EACH (neigh, cmap_node, &table) { > - if (tnl_neigh_is_complete(neigh)) { > - continue; > - } > atomic_read_explicit(&neigh->expires, &curr_exp, > memory_order_acquire); > - if (new_exp < curr_exp) { > + if (!tnl_neigh_is_complete(neigh) && new_exp < curr_exp) { > atomic_store_explicit(&neigh->expires, new_exp, > memory_order_release); > + curr_exp = new_exp; > } > + neigh->refresh_at = tnl_neigh_refresh_deadline(curr_exp); > } > + tnl_neigh_schedule(time_msec()); > + ovs_mutex_unlock(&mutex); > > unixctl_command_reply(conn, "OK"); > } > @@ -488,7 +622,7 @@ tnl_neigh_cache_add(struct unixctl_conn *conn, int argc > OVS_UNUSED, > return; > } > > - tnl_neigh_set(br_name, &ip6, mac); > + tnl_neigh_set(br_name, &ip6, mac, ""); We should use NULL instead of "", see other comment. > unixctl_command_reply(conn, "OK"); > } > > @@ -534,6 +668,7 @@ tnl_neigh_cache_init(void) > { > atomic_init(&neigh_aging, NEIGH_ENTRY_DEFAULT_IDLE_TIME_MS); > atomic_init(&neigh_retrans_time, NEIGH_ENTRY_LOOKUP_RETRANS_TIME); > + atomic_init(&next_event, LLONG_MAX); > unixctl_command_register("tnl/arp/show", "", 0, 0, > tnl_neigh_cache_show, NULL); > unixctl_command_register("tnl/arp/set", "BRIDGE IP MAC", 3, 3, > diff --git a/lib/tnl-neigh-cache.h b/lib/tnl-neigh-cache.h > index e16155b4d..a4e27294a 100644 > --- a/lib/tnl-neigh-cache.h > +++ b/lib/tnl-neigh-cache.h > @@ -32,13 +32,17 @@ > #include "util.h" > > int tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc, > + const char br_name[IFNAMSIZ], > const char dev_name[IFNAMSIZ], bool allow_update); > void tnl_neigh_set(const char name[IFNAMSIZ], const struct in6_addr *dst, > - const struct eth_addr mac); > + const struct eth_addr mac, const char *dev_name); > int tnl_neigh_lookup(const char dev_name[IFNAMSIZ], const struct in6_addr > *dst, > struct eth_addr *mac, bool insert_partial); > void tnl_neigh_cache_init(void); > void tnl_neigh_cache_run(void); > +bool tnl_neigh_get_refresh(char br_name[IFNAMSIZ], char dev_name[IFNAMSIZ], > + struct in6_addr *dst); > +void tnl_neigh_cache_wait(void); > void tnl_neigh_flush(const char dev_name[IFNAMSIZ]); > > #endif > diff --git a/ofproto/ofproto-dpif-xlate-cache.c > b/ofproto/ofproto-dpif-xlate-cache.c > index cb37e2462..9b06fdf6b 100644 > --- a/ofproto/ofproto-dpif-xlate-cache.c > +++ b/ofproto/ofproto-dpif-xlate-cache.c > @@ -43,7 +43,6 @@ > #include "openvswitch/vlog.h" > #include "ovs-router.h" > #include "packets.h" > -#include "tnl-neigh-cache.h" > #include "util.h" > > VLOG_DEFINE_THIS_MODULE(ofproto_xlate_cache); > @@ -92,7 +91,6 @@ void > xlate_push_stats_entry(struct xc_entry *entry, > struct dpif_flow_stats *stats, bool offloaded) > { > - struct eth_addr dmac; > Remove one more new line > switch (entry->type) { > case XC_TABLE: > @@ -149,11 +147,6 @@ xlate_push_stats_entry(struct xc_entry *entry, > group_dpif_credit_stats(entry->group.group, entry->group.bucket, > stats); > break; > - case XC_TNL_NEIGH: > - /* Lookup neighbor to avoid timeout. */ > - tnl_neigh_lookup(entry->tnl_neigh_cache.br_name, > - &entry->tnl_neigh_cache.d_ipv6, &dmac, false); > - break; > case XC_TUNNEL_HEADER: > if (entry->tunnel_hdr.operation == ADD) { > stats->n_bytes += stats->n_packets * entry->tunnel_hdr.hdr_size; > @@ -241,8 +234,6 @@ xlate_cache_clear_entry(struct xc_entry *entry) > case XC_GROUP: > ofproto_group_unref(&entry->group.group->up); > break; > - case XC_TNL_NEIGH: > - break; > case XC_TUNNEL_HEADER: > break; > default: > @@ -348,7 +339,6 @@ xlate_xcache_format(struct ds *s, const struct > xlate_cache *xcache) > case XC_LEARN: > case XC_NORMAL: > case XC_FIN_TIMEOUT: > - case XC_TNL_NEIGH: > case XC_TUNNEL_HEADER: > break; > } > diff --git a/ofproto/ofproto-dpif-xlate-cache.h > b/ofproto/ofproto-dpif-xlate-cache.h > index e701734d7..adcd6856f 100644 > --- a/ofproto/ofproto-dpif-xlate-cache.h > +++ b/ofproto/ofproto-dpif-xlate-cache.h > @@ -51,7 +51,6 @@ enum xc_type { > XC_NORMAL, > XC_FIN_TIMEOUT, /* Calls back to ofproto. */ > XC_GROUP, > - XC_TNL_NEIGH, > XC_TUNNEL_HEADER, > }; > > @@ -111,10 +110,6 @@ struct xc_entry { > struct group_dpif *group; > struct ofputil_bucket *bucket; > } group; > - struct { > - char br_name[IFNAMSIZ]; > - struct in6_addr d_ipv6; > - } tnl_neigh_cache; > struct { > struct ofproto_dpif *ofproto; > struct ofproto_async_msg *am; > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 4e7d6fb40..d31667ccc 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3767,7 +3767,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct > xport *out_dev, > struct ofpact_output output; > struct flow flow; > > - if (!xlate_resubmit_resource_check(ctx)) { > + if (ctx && !xlate_resubmit_resource_check(ctx)) { Calling this with a null ctx does not make sense to me. Maybe we should add a function in line with the existing xlate_send_packet(), called xlate_send_packet_pipeline(), and call it directly from xlate_tnl_neigh_refresh(), and here from compose_table_xlate(). But not sure how to handle it trough the nb/arp sent functions. > return 0; > } > > @@ -3779,7 +3779,8 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct > xport *out_dev, > > return ofproto_dpif_execute_actions__(xbridge->ofproto, version, &flow, > NULL, &output.ofpact, sizeof > output, > - ctx->depth + 1, ctx->resubmits, > + ctx ? ctx->depth + 1 : 0, > + ctx ? ctx->resubmits : 0, > packet); > } > > @@ -3811,6 +3812,68 @@ tnl_send_arp_request(struct xlate_ctx *ctx, const > struct xport *out_dev, > dp_packet_uninit(&packet); > } > > +/* Sends all tunnel neighbor refresh requests queued by the cache. */ > +void > +xlate_tnl_neigh_refresh(void) > +{ > + struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); > + char br_name[IFNAMSIZ], out_dev_name[IFNAMSIZ]; > + struct in6_addr dst; > + > + if (!xcfg) { > + return; > + } > + > + while (tnl_neigh_get_refresh(br_name, out_dev_name, &dst)) { > + char dst_s[INET6_ADDRSTRLEN]; > + struct in6_addr src; > + struct eth_addr smac; > + struct xport *out_dev = NULL; > + struct xbridge *xbridge = NULL; > + struct xbridge *iter; > + struct xport *port; Variable declarations should be in reverse Christmas tree order. > + > + HMAP_FOR_EACH (iter, hmap_node, &xcfg->xbridges) { > + if (!strcmp(iter->name, br_name)) { > + xbridge = iter; > + break; > + } > + } New line > + if (!xbridge) { > + continue; > + } > + > + ipv6_string_mapped(dst_s, &dst); > + VLOG_DBG("refreshing tunnel neighbor %s on bridge %s", > + dst_s, xbridge->name); > + > + /* Manually inserted entries have no learned IP interface. Select > + * an interface on their bridge that can address this neighbor. */ > + HMAP_FOR_EACH (port, ofp_node, &xbridge->xports) { > + const char *name = netdev_get_name(port->netdev); > + > + if ((!out_dev_name[0] || !strncmp(name, out_dev_name, IFNAMSIZ)) > + && !ovs_router_get_netdev_source_address(&dst, name, &src)) { > + out_dev = port; > + break; If the learned device no longer exists, the refresh silently fails and retries until expiry. Should we consider falling back to scanning all bridge ports, the same as the empty out_dev_name case, so the entry can still be refreshed through an alternative interface. > + } > + } New line > + if (!out_dev || netdev_get_etheraddr(out_dev->netdev, &smac)) { > + VLOG_DBG("no output port for tunnel neighbor %s", dst_s); > + continue; > + } Wondering if any of the failures (continues) should delete the entry? Because if it can not be reached, the tunnels should probably fail. Thoughts? > + > + COVERAGE_INC(xlate_actions_neigh_sent); > + if (IN6_IS_ADDR_V4MAPPED(&dst)) { > + tnl_send_arp_request(NULL, out_dev, smac, > + in6_addr_get_mapped_ipv4(&src), > + in6_addr_get_mapped_ipv4(&dst)); > + } else {C > + tnl_send_nd_request(NULL, out_dev, smac, &src, &dst); > + } > + } > +} > + > static void > propagate_tunnel_data_to_flow__(struct flow *dst_flow, > const struct flow *src_flow, > @@ -4001,15 +4064,6 @@ native_tunnel_output(struct xlate_ctx *ctx, const > struct xport *xport, > return err; > } > > - if (ctx->xin->xcache) { > - struct xc_entry *entry; > - > - entry = xlate_cache_add_entry(ctx->xin->xcache, XC_TNL_NEIGH); > - ovs_strlcpy(entry->tnl_neigh_cache.br_name, out_dev->xbridge->name, > - sizeof entry->tnl_neigh_cache.br_name); > - entry->tnl_neigh_cache.d_ipv6 = d_ip6; > - } > - > xlate_report(ctx, OFT_DETAIL, "tunneling from "ETH_ADDR_FMT" %s" > " to "ETH_ADDR_FMT" %s", > ETH_ADDR_ARGS(smac), ipv6_string_mapped(buf_sip6, &s_ip6), > @@ -4476,6 +4530,7 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const > struct xport *xport, > if (*tnl_port == ODPP_NONE && > (check_neighbor_reply(ctx, flow) || is_garp(flow, wc))) { > tnl_neigh_snoop(flow, wc, ctx->xbridge->name, > + netdev_get_name(xport->netdev), > ctx->xin->allow_side_effects); > } else if (*tnl_port != ODPP_NONE && > ctx->xin->allow_side_effects && > @@ -4489,7 +4544,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const > struct xport *xport, > s_ip6 = flow->ipv6_src; > } > > - tnl_neigh_set(ctx->xbridge->name, &s_ip6, mac); > + tnl_neigh_set(ctx->xbridge->name, &s_ip6, mac, > + netdev_get_name(xport->netdev)); > } > } > > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h > index d973a634a..7b87ed73c 100644 > --- a/ofproto/ofproto-dpif-xlate.h > +++ b/ofproto/ofproto-dpif-xlate.h > @@ -231,6 +231,7 @@ enum ofperr xlate_resume(struct ofproto_dpif *, > struct ofpbuf *odp_actions, enum slow_path_reason *, > struct flow *, struct xlate_cache *); > int xlate_send_packet(const struct ofport_dpif *, bool oam, struct dp_packet > *); > +void xlate_tnl_neigh_refresh(void); > > void xlate_mac_learning_update(const struct ofproto_dpif *ofproto, > ofp_port_t in_port, struct eth_addr dl_src, > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 0f0f71d14..8ade83656 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -372,6 +372,7 @@ type_run(const char *type) > if (dpif_run(backer->dpif)) { > backer->need_revalidate = REV_RECONFIGURE; > } Add a new line here. > + xlate_tnl_neigh_refresh(); > > udpif_run(backer->udpif); > > @@ -5172,7 +5173,6 @@ ofproto_dpif_xcache_execute(struct ofproto_dpif > *ofproto, > case XC_MIRROR: > case XC_NORMAL: > case XC_GROUP: > - case XC_TNL_NEIGH: > case XC_TUNNEL_HEADER: > xlate_push_stats_entry(entry, stats, false); > break; > diff --git a/tests/tunnel-push-pop-ipv6.at b/tests/tunnel-push-pop-ipv6.at Due to time contraints I did not go over the unit test changes. I'll do it the next revision. [...] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
