On Mon, Jan 16, 2023 at 12:20 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> On 1/16/23 12:08, Ales Musil wrote: > > > > > > On Mon, Jan 16, 2023 at 10:53 AM Ilya Maximets <i.maxim...@ovn.org > <mailto:i.maxim...@ovn.org>> wrote: > > > > On 1/16/23 07:21, Ales Musil wrote: > > > > > > > > > On Fri, Jan 13, 2023 at 9:58 PM Ilya Maximets <i.maxim...@ovn.org > <mailto:i.maxim...@ovn.org> <mailto:i.maxim...@ovn.org <mailto: > i.maxim...@ovn.org>>> wrote: > > > > > > On 1/12/23 18:17, Ales Musil wrote: > > > > Currently, the CT can be flushed by dpctl only by specifying > > > > the whole 5-tuple. This is not very convenient when there are > > > > only some fields known to the user of CT flush. Add new > struct > > > > ofputil_ct_match which represents the generic filtering that > can > > > > be done for CT flush. The match is done only on fields that > are > > > > non-zero with exception to the icmp fields. > > > > > > > > This allows the filtering just within dpctl, however > > > > it is a preparation for OpenFlow extension. > > > > > > > > Reported-at: https://bugzilla.redhat.com/2120546 < > https://bugzilla.redhat.com/2120546> <https://bugzilla.redhat.com/2120546 > <https://bugzilla.redhat.com/2120546>> > > > > Signed-off-by: Ales Musil <amu...@redhat.com <mailto: > amu...@redhat.com> <mailto:amu...@redhat.com <mailto:amu...@redhat.com>>> > > > > --- > > > > v6: Rebase on top of current master. > > > > Address comments from Ilya. > > > > > > Nit: this kind of version history is not really useful. It > should say > > > what actually changed. I don't remember every comment I made, > other > > > people have no idea what I was asking for. > > > > > > > v5: Add ack from Paolo. > > > > v4: Fix a flush all scenario. > > > > v3: Rebase on top of master. > > > > Address the C99 comment and missing dpif_close call. > > > > v2: Rebase on top of master. > > > > Address comments from Paolo. > > > > --- > > > > NEWS | 3 + > > > > include/openvswitch/ofp-util.h | 28 +++ > > > > lib/automake.mk <http://automake.mk> <http://automake.mk < > http://automake.mk>> | 2 + > > > > lib/ct-dpif.c | 210 ++++++++++------------- > > > > lib/ct-dpif.h | 4 +- > > > > lib/dpctl.c | 43 +++-- > > > > lib/dpctl.man | 15 +- > > > > lib/ofp-ct-util.c | 303 > +++++++++++++++++++++++++++++++++ > > > > lib/ofp-ct-util.h | 40 +++++ > > > > tests/system-traffic.at <http://system-traffic.at> < > http://system-traffic.at <http://system-traffic.at>> | 102 > ++++++++++- > > > > 10 files changed, 611 insertions(+), 139 deletions(-) > > > > create mode 100644 lib/ofp-ct-util.c > > > > create mode 100644 lib/ofp-ct-util.h > > > > > > > > <snip> > > > > > > +bool > > > > +ofputil_ct_tuple_is_zero(const struct ofputil_ct_tuple > *tuple, uint8_t ip_proto) > > > > +{ > > > > + bool is_zero = ipv6_is_zero(&tuple->src) && > ipv6_is_zero(&tuple->dst); > > > > + > > > > + if (!(ip_proto == IPPROTO_ICMP || ip_proto == > IPPROTO_ICMPV6)) { > > > > + is_zero = is_zero && !tuple->src_port && > !tuple->dst_port; > > > > + } > > > > + > > > > + return is_zero; > > > > > > Do we require ICMP tuples to always have at least one address > specified? > > > Or should the 'if' above have an 'else { return false; }' ? > > > > > > > > > The ICMP is very problematic, if we do the 'else { return false; > }' the shortcut for > > > the full 5-tuple stops working. Without it if user specifies reply > with only ICMP parameters > > > we won't flush anything. I'm not sure what the correct approach > here would be. > > > Maybe documenting the limitation? Adding note that for ICMP in > reply direction to work > > > user needs to specify at least one IP? > > > > I think, the correct solution to construct a mask of what the user > > have specified while parsing and use the mask instead of values for > > is_zero(). And also use it during comparison to con compare fields > > that are not specified. Will that solve the problem? > > > > Since partial matches are not supported, it can be a bitmask of > > NXT_CT_TUPLE_* values. > > > > It should be OK to just document for now. This also should be > > mentioned in the header along with the definition of the struct > > nx_ct_flush. > > > > A bitmap thing can go as a fixup after the branching, if it works. > > > > Best regards, Ilya Maximets. > > > > > > > > Yes, that would work and it would be probably faster. > > I will update the NXT_CT_TUPLE_* values, so they can be included in > bitmask > > without breaking compatibility with the current approach. > > They can be included in the mask as long as they are unique, > i.e. with (1 << NXTC_CT_TUPLE_*). > > What do you have in mind? > True that is enough, I've confused myself a bit. I'll run the last round of tests and send the v7. Thanks, Ales > > > > > Thanks, > > Ales > > > > -- > > > > Ales Musil > > > > Senior Software Engineer - OVN Core > > > > Red Hat EMEA <https://www.redhat.com> > > > > amu...@redhat.com <mailto:amu...@redhat.com> IM: amusil > > > > <https://red.ht/sig> > > > > -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
