| Message ID | 163587314922.3737810.2741459698550214543.stgit@fed.void |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Native tunnel: Update neigh entries in tnl termination. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Tue, Nov 2, 2021, at 18:12, Paolo Valerio wrote: > This is a minor issue but visible e.g. when you try to flush the neigh > cache while the ARP flow is still present in the datapath, triggering > the revalidation of the datapath flows which subsequently > refreshes/adds the entry in the cache. > Hi, the change makes sense I think. Just a nit below. > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- > lib/tnl-neigh-cache.c | 20 +++++++++++++------- > lib/tnl-neigh-cache.h | 2 +- > ofproto/ofproto-dpif-xlate.c | 3 ++- > 3 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c > index 8eacaccd8..58dba5e5c 100644 > --- a/lib/tnl-neigh-cache.c > +++ b/lib/tnl-neigh-cache.c > @@ -174,7 +174,7 @@ tnl_arp_set(const char name[IFNAMSIZ], ovs_be32 dst, > > static int > tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc, > - const char name[IFNAMSIZ]) > + const char name[IFNAMSIZ], bool update) I did not understand the intention / meaning of 'update' until reading the last lines of this commit, so until I was able to see 'ctx->xin->allow_side_effects'. I think renaming this variable 'allow_update' would be clearer about how it's meant to be used. Same for other occurrences.
Gaëtan Rivet <grive@u256.net> writes: > On Tue, Nov 2, 2021, at 18:12, Paolo Valerio wrote: >> This is a minor issue but visible e.g. when you try to flush the neigh >> cache while the ARP flow is still present in the datapath, triggering >> the revalidation of the datapath flows which subsequently >> refreshes/adds the entry in the cache. >> > > Hi, the change makes sense I think. Just a nit below. > >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- >> lib/tnl-neigh-cache.c | 20 +++++++++++++------- >> lib/tnl-neigh-cache.h | 2 +- >> ofproto/ofproto-dpif-xlate.c | 3 ++- >> 3 files changed, 16 insertions(+), 9 deletions(-) >> >> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c >> index 8eacaccd8..58dba5e5c 100644 >> --- a/lib/tnl-neigh-cache.c >> +++ b/lib/tnl-neigh-cache.c >> @@ -174,7 +174,7 @@ tnl_arp_set(const char name[IFNAMSIZ], ovs_be32 dst, >> >> static int >> tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc, >> - const char name[IFNAMSIZ]) >> + const char name[IFNAMSIZ], bool update) > > I did not understand the intention / meaning of 'update' until reading the > last lines of this commit, so until I was able to see 'ctx->xin->allow_side_effects'. > I think renaming this variable 'allow_update' would be clearer about how it's meant > to be used. > > Same for other occurrences. ACK
diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c index 8eacaccd8..58dba5e5c 100644 --- a/lib/tnl-neigh-cache.c +++ b/lib/tnl-neigh-cache.c @@ -174,7 +174,7 @@ tnl_arp_set(const char name[IFNAMSIZ], ovs_be32 dst, static int tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc, - const char name[IFNAMSIZ]) + const char name[IFNAMSIZ], bool update) { /* Snoop normal ARP replies and gratuitous ARP requests/replies only */ if (!is_arp(flow) @@ -184,13 +184,17 @@ tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc, return EINVAL; } - tnl_arp_set(name, FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src), flow->arp_sha); + memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); + + if (update) { + tnl_arp_set(name, flow->nw_src, flow->arp_sha); + } return 0; } static int tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc, - const char name[IFNAMSIZ]) + const char name[IFNAMSIZ], bool update) { if (!is_nd(flow, wc) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) { return EINVAL; @@ -209,20 +213,22 @@ tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc, memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst); memset(&wc->masks.nd_target, 0xff, sizeof wc->masks.nd_target); - tnl_neigh_set__(name, &flow->nd_target, flow->arp_tha); + if (update) { + tnl_neigh_set__(name, &flow->nd_target, flow->arp_tha); + } return 0; } int tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc, - const char name[IFNAMSIZ]) + const char name[IFNAMSIZ], bool update) { int res; - res = tnl_arp_snoop(flow, wc, name); + res = tnl_arp_snoop(flow, wc, name, update); if (res != EINVAL) { return res; } - return tnl_nd_snoop(flow, wc, name); + return tnl_nd_snoop(flow, wc, name, update); } void diff --git a/lib/tnl-neigh-cache.h b/lib/tnl-neigh-cache.h index e4b42b059..10724d8b4 100644 --- a/lib/tnl-neigh-cache.h +++ b/lib/tnl-neigh-cache.h @@ -32,7 +32,7 @@ #include "util.h" int tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc, - const char dev_name[IFNAMSIZ]); + const char dev_name[IFNAMSIZ], bool update); int tnl_neigh_lookup(const char dev_name[IFNAMSIZ], const struct in6_addr *dst, struct eth_addr *mac); void tnl_neigh_cache_init(void); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 8723cb4e8..2f09bcca1 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4097,7 +4097,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow, (flow->dl_type == htons(ETH_TYPE_ARP) || flow->nw_proto == IPPROTO_ICMPV6) && is_neighbor_reply_correct(ctx, flow)) { - tnl_neigh_snoop(flow, wc, ctx->xbridge->name); + tnl_neigh_snoop(flow, wc, ctx->xbridge->name, + ctx->xin->allow_side_effects); } }
This is a minor issue but visible e.g. when you try to flush the neigh cache while the ARP flow is still present in the datapath, triggering the revalidation of the datapath flows which subsequently refreshes/adds the entry in the cache. Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/tnl-neigh-cache.c | 20 +++++++++++++------- lib/tnl-neigh-cache.h | 2 +- ofproto/ofproto-dpif-xlate.c | 3 ++- 3 files changed, 16 insertions(+), 9 deletions(-)