On 11/2/23 10:37, Ales Musil wrote: > > > On Wed, Oct 25, 2023 at 12:25 AM Ilya Maximets <i.maxim...@ovn.org > <mailto:i.maxim...@ovn.org>> wrote: > > On 10/18/23 08:28, Ales Musil wrote: > > Extend the current NX_CT_FLUSH with four additional fields, > > that allow to match on CT entry "mark" or "labels". This > > is encoded as separate TLV values which is backward compatible. > > Versions that do not support them will simply ignore it. > > Hmm. Just noticed that. This doesn't seem right. If unknown > property is passed, OVS should fail with OFPPROP_UNKNOWN(). > This probably should be a separate fix that we'll need to > backport to stable versions. If user requests flushing a > specific label, we should not flush everything just because > we do not understand the request. > > Some more comments inline. > > Best regards, Ilya Maximets. > > > Hi Ilya, > > thank you for the review. It makes sense to report unknown values, it's a bit > unfortunate because now we will probably need an additional feature flag to > indicate that this is supported WDYT?
But it's not new. If we don't fail than the requested fields will just be silently ignored, so the controller can't really use the feature anyway. You'll need a way to detect support regardless. If we fail though, the failure itself can be used as a probe. > I'll wait with v4 until the feature flag is resolved. > > > > > > > Extend also the ovs-dpctl and ovs-ofctl command line tools with > > option to specify those two matching parameters for the "ct-flush" > > command. > > > > Reported-at: https://issues.redhat.com/browse/FDP-55 > <https://issues.redhat.com/browse/FDP-55> > > Signed-off-by: Ales Musil <amu...@redhat.com <mailto:amu...@redhat.com>> > > --- > > v3: Rebase on top of current master. > > v2: Make sure that the mask decoding matches the dpctl/ovs-ofctl > interface. > > --- > > include/openflow/nicira-ext.h | 4 + > > include/openvswitch/ofp-ct.h | 9 +- > > lib/ct-dpif.c | 12 ++- > > lib/dpctl.c | 5 +- > > lib/ofp-ct.c | 151 +++++++++++++++++++++++++++++++++- > > tests/ofp-print.at <http://ofp-print.at> | 56 +++++++++++++ > > tests/ovs-ofctl.at <http://ovs-ofctl.at> | 32 +++++++ > > tests/system-traffic.at <http://system-traffic.at> | 112 > ++++++++++++++++--------- > > utilities/ovs-ofctl.8.in <http://ovs-ofctl.8.in> | 13 +-- > > utilities/ovs-ofctl.c | 5 +- > > 10 files changed, 344 insertions(+), 55 deletions(-) > > The change needs a NEWS entry. > > > > > diff --git a/include/openflow/nicira-ext.h > b/include/openflow/nicira-ext.h > > index 768775898..959845ce6 100644 > > --- a/include/openflow/nicira-ext.h > > +++ b/include/openflow/nicira-ext.h > > @@ -1075,6 +1075,10 @@ enum nx_ct_flush_tlv_type { > > * by 'enum > nx_ct_flush_tuple_tlv_type'*/ > > /* Primitive types. */ > > NXT_CT_ZONE_ID = 2, /* be16 zone id. */ > > + NXT_CT_MARK = 3, /* be32 mark. */ > > + NXT_CT_MARK_MASK = 4, /* be32 mark mask. */ > > + NXT_CT_LABELS = 5, /* be128 labels. */ > > + NXT_CT_LABELS_MASK = 6, /* be128 labels mask. */ > > }; > > > > /* CT flush nested TLVs. */ > > diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h > > index cd6192e6f..d57b62678 100644 > > --- a/include/openvswitch/ofp-ct.h > > +++ b/include/openvswitch/ofp-ct.h > > @@ -51,11 +51,16 @@ struct ofp_ct_match { > > > > struct ofp_ct_tuple tuple_orig; > > struct ofp_ct_tuple tuple_reply; > > + > > + uint32_t mark; > > + uint32_t mark_mask; > > + > > + ovs_u128 labels; > > + ovs_u128 labels_mask; > > }; > > > > bool ofp_ct_match_is_zero(const struct ofp_ct_match *); > > -bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *, uint8_t > ip_proto); > > -bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t > ip_proto); > > +bool ofp_ct_match_is_five_tuple(const struct ofp_ct_match *); > > > > void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *); > > bool ofp_ct_match_parse(const char **, int argc, struct ds *, > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > > index f59c6e560..0fd14b99f 100644 > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > @@ -269,6 +269,15 @@ ct_dpif_entry_cmp(const struct ct_dpif_entry > *entry, > > return false; > > } > > > > + if ((match->mark & match->mark_mask) != (entry->mark & > match->mark_mask)) { > > + return false; > > + } > > + > > + if (!ovs_u128_equals(ovs_u128_and(match->labels, > match->labels_mask), > > + ovs_u128_and(entry->labels, > match->labels_mask))) { > > + return false; > > + } > > + > > return true; > > } > > > > @@ -295,8 +304,7 @@ ct_dpif_flush_tuple(struct dpif *dpif, const > uint16_t *zone, > > > > /* If we have full five tuple in original and empty reply tuple > just > > * do the flush over original tuple directly. */ > > - if (ofp_ct_tuple_is_five_tuple(&match->tuple_orig, > match->ip_proto) && > > - ofp_ct_tuple_is_zero(&match->tuple_reply, match->ip_proto)) { > > + if (ofp_ct_match_is_five_tuple(match)) { > > struct ct_dpif_tuple tuple; > > > > ct_dpif_tuple_from_ofp_ct_tuple(&match->tuple_orig, &tuple, > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index bbab5881e..9d28a91ba 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -2981,8 +2981,9 @@ static const struct dpctl_command all_commands[] > = { > > 0, 4, dpctl_dump_conntrack, DP_RO }, > > { "dump-conntrack-exp", "[dp] [zone=N]", > > 0, 2, dpctl_dump_conntrack_exp, DP_RO }, > > - { "flush-conntrack", "[dp] [zone=N] [ct-orig-tuple] > [ct-reply-tuple]", > > - 0, 4, dpctl_flush_conntrack, DP_RW }, > > + { "flush-conntrack", "[dp] [zone=N] [mark=X[/M]] [labels=Y[/N]] " > > + "[ct-orig-tuple [ct-reply-tuple]]", > > + 0, 6, dpctl_flush_conntrack, DP_RW }, > > { "cache-get-size", "[dp]", 0, 1, dpctl_cache_get_size, DP_RO }, > > { "cache-set-size", "dp cache <size>", 3, 3, dpctl_cache_set_size, > DP_RW }, > > { "ct-stats-show", "[dp] [zone=N]", > > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c > > index 32aeb5455..344f7a0b2 100644 > > --- a/lib/ofp-ct.c > > +++ b/lib/ofp-ct.c > > @@ -50,7 +50,7 @@ ofp_ct_tuple_format(struct ds *ds, const struct > ofp_ct_tuple *tuple, > > } > > } > > > > -bool > > +static bool > > ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *tuple, uint8_t > ip_proto) > > { > > bool is_zero = ipv6_is_zero(&tuple->src) && > ipv6_is_zero(&tuple->dst); > > @@ -62,7 +62,7 @@ ofp_ct_tuple_is_zero(const struct ofp_ct_tuple > *tuple, uint8_t ip_proto) > > return is_zero; > > } > > > > -bool > > +static bool > > ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *tuple, uint8_t > ip_proto) > > { > > /* First check if we have address. */ > > @@ -75,17 +75,63 @@ ofp_ct_tuple_is_five_tuple(const struct > ofp_ct_tuple *tuple, uint8_t ip_proto) > > return five_tuple; > > } > > > > +static bool > > +ofp_ct_match_mark_is_zero(const struct ofp_ct_match *match) > > +{ > > + return !match->mark && !match->mark_mask; > > If the mask is zero, why do we care about the value? > > > We don't, but I don't see any harm in checking both. These checks do nothing useful and we intriduce 2 extra functions in order to have them. If we do not check the value we may remove these functions. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
