Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-11 Thread Ales Musil 
On Wed, Jan 11, 2023 at 4:27 PM Ilya Maximets  wrote:

> On 1/11/23 16:09, Ales Musil wrote:
> >
> >
> > On Wed, Jan 11, 2023 at 3:52 PM Ilya Maximets  > wrote:
> >
> > On 1/11/23 14:09, Ales Musil wrote:
> > >
> > >
> > > On Wed, Jan 11, 2023 at 1:34 PM Ilya Maximets   >> wrote:
> > >
> > > On 1/11/23 07:53, Ales Musil wrote:
> > > >
> > > >
> > > > On Tue, Jan 10, 2023 at 11:08 PM Ilya Maximets <
> i.maxim...@ovn.org   >   wrote:
> > > >
> > > > On 12/16/22 13:30, Ales Musil wrote:
> > > > > Currently, the CT can be flushed by dpctl only be
> specifying
> > > >
> > > > *by
> > > >
> > > > > 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 
>  >>>
> > > > > Signed-off-by: Ales Musil  amu...@redhat.com> >
>   amu...@redhat.com  > > > > Acked-by: Paolo Valerio  pvale...@redhat.com> >   > > > > ---
> > > > > 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.
> > > > > ---
> > > >
> > > > Hi, Ales.  Thanks for working on this change.
> > > > Sorry for review taking so long.  I initially though
> there
> > > > will be mostly a few cosmetic/style changes needed.  But
> > > > longer I look at this code, more questions I have.  See
> below.
> > > >
> > > > Dumitru, maybe you can also help answer some of them?
> > > >
> > > > Best regards, Ilya Maximets.
> > > >
> > > >
> > > > Hi Ilya,
> > > >
> > > > thank you for the review. Before proceeding I tried to
> answer your concerns/questions.
> > > > Let me know what you think.
> > > >
> > > > Thanks,
> > > > Ales
> > > >
> > > >
> > > >
> > > > >  NEWS   |   2 +
> > > > >  include/openvswitch/ofp-util.h |  28 +++
> > > > >  lib/automake.mk  <
> http://automake.mk >  http://automake.mk> >>
> |   2 +
> > > > >  lib/ct-dpif.c  | 208
> +
> > > > >  lib/ct-dpif.h  |   4 +-
> > > > >  lib/dpctl.c|  42 ++---
> > > > >  lib/dpctl.man  |   3 +-
> > > > >  lib/ofp-ct-util.c  | 326
> +
> > > > >  lib/ofp-ct-util.h  |  36 
> > > > >  tests/system-traffic.at  <
> http://system-traffic.at > <
> http://system-traffic.at  <
> http://system-traffic.at >>|  82
> -
> > > > >  10 files changed, 586 insertions(+), 147 deletions(-)
> > > > >  create mode 100644 lib/ofp-ct-util.c
> > > > >  create mode 100644 lib/ofp-ct-util.h
> > > > >
> > > > > diff --git a/NEWS b/NEWS
> > > > > index 265375e1c..ff8904b02 100644
> > > > > --- a/NEWS
> > > >

Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-11 Thread Ilya Maximets 
On 1/11/23 16:09, Ales Musil wrote:
> 
> 
> On Wed, Jan 11, 2023 at 3:52 PM Ilya Maximets  > wrote:
> 
> On 1/11/23 14:09, Ales Musil wrote:
> >
> >
> > On Wed, Jan 11, 2023 at 1:34 PM Ilya Maximets    >> wrote:
> >
> >     On 1/11/23 07:53, Ales Musil wrote:
> >     >
> >     >
> >     > On Tue, Jan 10, 2023 at 11:08 PM Ilya Maximets 
> mailto:i.maxim...@ovn.org>  >     >     >
> >     >     On 12/16/22 13:30, Ales Musil wrote:
> >     >     > Currently, the CT can be flushed by dpctl only be specifying
> >     >
> >     >     *by
> >     >
> >     >     > 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 
>   >    >>
> >     >     > Signed-off-by: Ales Musil    >     >     >     > Acked-by: Paolo Valerio    >     >     >     > ---
> >     >     > 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.
> >     >     > ---
> >     >
> >     >     Hi, Ales.  Thanks for working on this change.
> >     >     Sorry for review taking so long.  I initially though there
> >     >     will be mostly a few cosmetic/style changes needed.  But
> >     >     longer I look at this code, more questions I have.  See below.
> >     >
> >     >     Dumitru, maybe you can also help answer some of them?
> >     >
> >     >     Best regards, Ilya Maximets.
> >     >
> >     >
> >     > Hi Ilya,
> >     >
> >     > thank you for the review. Before proceeding I tried to answer 
> your concerns/questions.
> >     > Let me know what you think.
> >     >
> >     > Thanks,
> >     > Ales
> >     >  
> >     >
> >     >
> >     >     >  NEWS                           |   2 +
> >     >     >  include/openvswitch/ofp-util.h |  28 +++
> >     >     >  lib/automake.mk   >  
> >>                |   2 +
> >     >     >  lib/ct-dpif.c                  | 208 +
> >     >     >  lib/ct-dpif.h                  |   4 +-
> >     >     >  lib/dpctl.c                    |  42 ++---
> >     >     >  lib/dpctl.man                  |   3 +-
> >     >     >  lib/ofp-ct-util.c              | 326 
> +
> >     >     >  lib/ofp-ct-util.h              |  36 
> >     >     >  tests/system-traffic.at  
> > 
>  
> >>        |  82 -
> >     >     >  10 files changed, 586 insertions(+), 147 deletions(-)
> >     >     >  create mode 100644 lib/ofp-ct-util.c
> >     >     >  create mode 100644 lib/ofp-ct-util.h
> >     >     >
> >     >     > diff --git a/NEWS b/NEWS
> >     >     > index 265375e1c..ff8904b02 100644
> >     >     > --- a/NEWS
> >     >     > +++ b/NEWS
> >     >     > @@ -14,6 +14,8 @@ Post-v3.0.0
> >     >     >       10 Gbps link speed by default in case the actual

Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-11 Thread Ales Musil 
On Wed, Jan 11, 2023 at 3:52 PM Ilya Maximets  wrote:

> On 1/11/23 14:09, Ales Musil wrote:
> >
> >
> > On Wed, Jan 11, 2023 at 1:34 PM Ilya Maximets  > wrote:
> >
> > On 1/11/23 07:53, Ales Musil wrote:
> > >
> > >
> > > On Tue, Jan 10, 2023 at 11:08 PM Ilya Maximets   >> wrote:
> > >
> > > On 12/16/22 13:30, Ales Musil wrote:
> > > > Currently, the CT can be flushed by dpctl only be specifying
> > >
> > > *by
> > >
> > > > 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>  >
> > > > Signed-off-by: Ales Musil  amu...@redhat.com> >>
> > > > Acked-by: Paolo Valerio  pvale...@redhat.com> >>
> > > > ---
> > > > 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.
> > > > ---
> > >
> > > Hi, Ales.  Thanks for working on this change.
> > > Sorry for review taking so long.  I initially though there
> > > will be mostly a few cosmetic/style changes needed.  But
> > > longer I look at this code, more questions I have.  See below.
> > >
> > > Dumitru, maybe you can also help answer some of them?
> > >
> > > Best regards, Ilya Maximets.
> > >
> > >
> > > Hi Ilya,
> > >
> > > thank you for the review. Before proceeding I tried to answer your
> concerns/questions.
> > > Let me know what you think.
> > >
> > > Thanks,
> > > Ales
> > >
> > >
> > >
> > > >  NEWS   |   2 +
> > > >  include/openvswitch/ofp-util.h |  28 +++
> > > >  lib/automake.mk   http://automake.mk>>|   2 +
> > > >  lib/ct-dpif.c  | 208 +
> > > >  lib/ct-dpif.h  |   4 +-
> > > >  lib/dpctl.c|  42 ++---
> > > >  lib/dpctl.man  |   3 +-
> > > >  lib/ofp-ct-util.c  | 326
> +
> > > >  lib/ofp-ct-util.h  |  36 
> > > >  tests/system-traffic.at  <
> http://system-traffic.at >|  82
> -
> > > >  10 files changed, 586 insertions(+), 147 deletions(-)
> > > >  create mode 100644 lib/ofp-ct-util.c
> > > >  create mode 100644 lib/ofp-ct-util.h
> > > >
> > > > diff --git a/NEWS b/NEWS
> > > > index 265375e1c..ff8904b02 100644
> > > > --- a/NEWS
> > > > +++ b/NEWS
> > > > @@ -14,6 +14,8 @@ Post-v3.0.0
> > > >   10 Gbps link speed by default in case the actual link
> speed cannot be
> > > >   determined.  Previously it was 10 Mbps.  Values can
> still be overridden
> > > >   by specifying 'max-rate' or '[r]stp-path-cost'
> accordingly.
> > > > +   - ovs-dpctl and related ovs-appctl commands:
> > >
> > > "ovs-dpctl and 'ovs-appctl dpctl/'" might be a better header.
> > >
> > > > + * "flush-conntrack" is capable of handling partial
> 5-tuple.
> > > >
> > > >
> > > >  v3.0.0 - 15 Aug 2022
> > > > diff --git a/include/openvswitch/ofp-util.h
> b/include/openvswitch/ofp-util.h
> > > > index 091a09cad..84937ae26 100644
> > > > --- a/include/openvswitch/ofp-util.h
> > > > +++ b/include/openvswitch/ofp-util.h
> > > > @@ -19,6 +19,9 @@
> > > >
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > > +#include 
> > > > +
> > > >  #include "openvswitch/ofp-protocol.h"
> > > >
> > > >  struct ofp_header;
> > > > @@ -27,6 +30,31 @@ struct ofp_header;
> > > >  extern "C" {
> > > >  #endif
> > > >
> > > > +struct

Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-11 Thread Ilya Maximets 
On 1/11/23 14:09, Ales Musil wrote:
> 
> 
> On Wed, Jan 11, 2023 at 1:34 PM Ilya Maximets  > wrote:
> 
> On 1/11/23 07:53, Ales Musil wrote:
> >
> >
> > On Tue, Jan 10, 2023 at 11:08 PM Ilya Maximets    >> wrote:
> >
> >     On 12/16/22 13:30, Ales Musil wrote:
> >     > Currently, the CT can be flushed by dpctl only be specifying
> >
> >     *by
> >
> >     > 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 
>   >
> >     > Signed-off-by: Ales Musil    >>
> >     > Acked-by: Paolo Valerio    >>
> >     > ---
> >     > 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.
> >     > ---
> >
> >     Hi, Ales.  Thanks for working on this change.
> >     Sorry for review taking so long.  I initially though there
> >     will be mostly a few cosmetic/style changes needed.  But
> >     longer I look at this code, more questions I have.  See below.
> >
> >     Dumitru, maybe you can also help answer some of them?
> >
> >     Best regards, Ilya Maximets.
> >
> >
> > Hi Ilya,
> >
> > thank you for the review. Before proceeding I tried to answer your 
> concerns/questions.
> > Let me know what you think.
> >
> > Thanks,
> > Ales
> >  
> >
> >
> >     >  NEWS                           |   2 +
> >     >  include/openvswitch/ofp-util.h |  28 +++
> >     >  lib/automake.mk   >                |   2 +
> >     >  lib/ct-dpif.c                  | 208 +
> >     >  lib/ct-dpif.h                  |   4 +-
> >     >  lib/dpctl.c                    |  42 ++---
> >     >  lib/dpctl.man                  |   3 +-
> >     >  lib/ofp-ct-util.c              | 326 
> +
> >     >  lib/ofp-ct-util.h              |  36 
> >     >  tests/system-traffic.at  
> >        |  82 -
> >     >  10 files changed, 586 insertions(+), 147 deletions(-)
> >     >  create mode 100644 lib/ofp-ct-util.c
> >     >  create mode 100644 lib/ofp-ct-util.h
> >     >
> >     > diff --git a/NEWS b/NEWS
> >     > index 265375e1c..ff8904b02 100644
> >     > --- a/NEWS
> >     > +++ b/NEWS
> >     > @@ -14,6 +14,8 @@ Post-v3.0.0
> >     >       10 Gbps link speed by default in case the actual link speed 
> cannot be
> >     >       determined.  Previously it was 10 Mbps.  Values can still 
> be overridden
> >     >       by specifying 'max-rate' or '[r]stp-path-cost' accordingly.
> >     > +   - ovs-dpctl and related ovs-appctl commands:
> >
> >     "ovs-dpctl and 'ovs-appctl dpctl/'" might be a better header.
> >
> >     > +     * "flush-conntrack" is capable of handling partial 5-tuple.
> >     > 
> >     > 
> >     >  v3.0.0 - 15 Aug 2022
> >     > diff --git a/include/openvswitch/ofp-util.h 
> b/include/openvswitch/ofp-util.h
> >     > index 091a09cad..84937ae26 100644
> >     > --- a/include/openvswitch/ofp-util.h
> >     > +++ b/include/openvswitch/ofp-util.h
> >     > @@ -19,6 +19,9 @@
> >     > 
> >     >  #include 
> >     >  #include 
> >     > +#include 
> >     > +#include 
> >     > +
> >     >  #include "openvswitch/ofp-protocol.h"
> >     > 
> >     >  struct ofp_header;
> >     > @@ -27,6 +30,31 @@ struct ofp_header;
> >     >  extern "C" {
> >     >  #endif
> >     > 
> >     > +struct ofputil_ct_tuple {
> >     > +    struct in6_addr src;
> >     > +    struct in6_addr dst;
> >     > +
> >     > +    union {
> >     > +        ovs_be16 src_port;
> >     > +        ovs_be16 icmp_id;

Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-11 Thread Ales Musil 
On Wed, Jan 11, 2023 at 1:34 PM Ilya Maximets  wrote:

> On 1/11/23 07:53, Ales Musil wrote:
> >
> >
> > On Tue, Jan 10, 2023 at 11:08 PM Ilya Maximets  > wrote:
> >
> > On 12/16/22 13:30, Ales Musil wrote:
> > > Currently, the CT can be flushed by dpctl only be specifying
> >
> > *by
> >
> > > 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>
> > > Signed-off-by: Ales Musil  amu...@redhat.com>>
> > > Acked-by: Paolo Valerio  pvale...@redhat.com>>
> > > ---
> > > 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.
> > > ---
> >
> > Hi, Ales.  Thanks for working on this change.
> > Sorry for review taking so long.  I initially though there
> > will be mostly a few cosmetic/style changes needed.  But
> > longer I look at this code, more questions I have.  See below.
> >
> > Dumitru, maybe you can also help answer some of them?
> >
> > Best regards, Ilya Maximets.
> >
> >
> > Hi Ilya,
> >
> > thank you for the review. Before proceeding I tried to answer your
> concerns/questions.
> > Let me know what you think.
> >
> > Thanks,
> > Ales
> >
> >
> >
> > >  NEWS   |   2 +
> > >  include/openvswitch/ofp-util.h |  28 +++
> > >  lib/automake.mk |   2 +
> > >  lib/ct-dpif.c  | 208 +
> > >  lib/ct-dpif.h  |   4 +-
> > >  lib/dpctl.c|  42 ++---
> > >  lib/dpctl.man  |   3 +-
> > >  lib/ofp-ct-util.c  | 326
> +
> > >  lib/ofp-ct-util.h  |  36 
> > >  tests/system-traffic.at |  82
> -
> > >  10 files changed, 586 insertions(+), 147 deletions(-)
> > >  create mode 100644 lib/ofp-ct-util.c
> > >  create mode 100644 lib/ofp-ct-util.h
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 265375e1c..ff8904b02 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -14,6 +14,8 @@ Post-v3.0.0
> > >   10 Gbps link speed by default in case the actual link speed
> cannot be
> > >   determined.  Previously it was 10 Mbps.  Values can still be
> overridden
> > >   by specifying 'max-rate' or '[r]stp-path-cost' accordingly.
> > > +   - ovs-dpctl and related ovs-appctl commands:
> >
> > "ovs-dpctl and 'ovs-appctl dpctl/'" might be a better header.
> >
> > > + * "flush-conntrack" is capable of handling partial 5-tuple.
> > >
> > >
> > >  v3.0.0 - 15 Aug 2022
> > > diff --git a/include/openvswitch/ofp-util.h
> b/include/openvswitch/ofp-util.h
> > > index 091a09cad..84937ae26 100644
> > > --- a/include/openvswitch/ofp-util.h
> > > +++ b/include/openvswitch/ofp-util.h
> > > @@ -19,6 +19,9 @@
> > >
> > >  #include 
> > >  #include 
> > > +#include 
> > > +#include 
> > > +
> > >  #include "openvswitch/ofp-protocol.h"
> > >
> > >  struct ofp_header;
> > > @@ -27,6 +30,31 @@ struct ofp_header;
> > >  extern "C" {
> > >  #endif
> > >
> > > +struct ofputil_ct_tuple {
> > > +struct in6_addr src;
> > > +struct in6_addr dst;
> > > +
> > > +union {
> > > +ovs_be16 src_port;
> > > +ovs_be16 icmp_id;
> > > +};
> > > +union {
> > > +ovs_be16 dst_port;
> > > +struct {
> > > +uint8_t icmp_code;
> > > +uint8_t icmp_type;
> > > +};
> > > +};
> > > +};
> > > +
> > > +struct ofputil_ct_match {
> > > +uint8_t ip_proto;
> > > +uint16_t l3_type;
> > > +
> > > +struct ofputil_ct_tuple tuple_orig;
> > > +struct ofputil_ct_tuple tuple_reply;
> > > +};
> >
> > These structures are very similar to existing ct_dpif_tuple.
> > We can keep them for now, but I think, we'll need to replace
> > ct_dpif_tuple with the new ofputil_ct_tuple in the future.
> > We may need some changes in ofputil_ct_tuple for that though.
> >
> >
> > I was thinking about replacing them, however it would require a much
> > bigger change, which was out

Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-11 Thread Ilya Maximets 
On 1/11/23 07:53, Ales Musil wrote:
> 
> 
> On Tue, Jan 10, 2023 at 11:08 PM Ilya Maximets  > wrote:
> 
> On 12/16/22 13:30, Ales Musil wrote:
> > Currently, the CT can be flushed by dpctl only be specifying
> 
> *by
> 
> > 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 
> 
> > Signed-off-by: Ales Musil mailto:amu...@redhat.com>>
> > Acked-by: Paolo Valerio  >
> > ---
> > 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.
> > ---
> 
> Hi, Ales.  Thanks for working on this change.
> Sorry for review taking so long.  I initially though there
> will be mostly a few cosmetic/style changes needed.  But
> longer I look at this code, more questions I have.  See below.
> 
> Dumitru, maybe you can also help answer some of them?
> 
> Best regards, Ilya Maximets.
> 
> 
> Hi Ilya,
> 
> thank you for the review. Before proceeding I tried to answer your 
> concerns/questions.
> Let me know what you think.
> 
> Thanks,
> Ales
>  
> 
> 
> >  NEWS                           |   2 +
> >  include/openvswitch/ofp-util.h |  28 +++
> >  lib/automake.mk                 |   2 +
> >  lib/ct-dpif.c                  | 208 +
> >  lib/ct-dpif.h                  |   4 +-
> >  lib/dpctl.c                    |  42 ++---
> >  lib/dpctl.man                  |   3 +-
> >  lib/ofp-ct-util.c              | 326 +
> >  lib/ofp-ct-util.h              |  36 
> >  tests/system-traffic.at         |  82 
> -
> >  10 files changed, 586 insertions(+), 147 deletions(-)
> >  create mode 100644 lib/ofp-ct-util.c
> >  create mode 100644 lib/ofp-ct-util.h
> >
> > diff --git a/NEWS b/NEWS
> > index 265375e1c..ff8904b02 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -14,6 +14,8 @@ Post-v3.0.0
> >       10 Gbps link speed by default in case the actual link speed 
> cannot be
> >       determined.  Previously it was 10 Mbps.  Values can still be 
> overridden
> >       by specifying 'max-rate' or '[r]stp-path-cost' accordingly.
> > +   - ovs-dpctl and related ovs-appctl commands:
> 
> "ovs-dpctl and 'ovs-appctl dpctl/'" might be a better header.
> 
> > +     * "flush-conntrack" is capable of handling partial 5-tuple.
> > 
> > 
> >  v3.0.0 - 15 Aug 2022
> > diff --git a/include/openvswitch/ofp-util.h 
> b/include/openvswitch/ofp-util.h
> > index 091a09cad..84937ae26 100644
> > --- a/include/openvswitch/ofp-util.h
> > +++ b/include/openvswitch/ofp-util.h
> > @@ -19,6 +19,9 @@
> > 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +
> >  #include "openvswitch/ofp-protocol.h"
> > 
> >  struct ofp_header;
> > @@ -27,6 +30,31 @@ struct ofp_header;
> >  extern "C" {
> >  #endif
> > 
> > +struct ofputil_ct_tuple {
> > +    struct in6_addr src;
> > +    struct in6_addr dst;
> > +
> > +    union {
> > +        ovs_be16 src_port;
> > +        ovs_be16 icmp_id;
> > +    };
> > +    union {
> > +        ovs_be16 dst_port;
> > +        struct {
> > +            uint8_t icmp_code;
> > +            uint8_t icmp_type;
> > +        };
> > +    };
> > +};
> > +
> > +struct ofputil_ct_match {
> > +    uint8_t ip_proto;
> > +    uint16_t l3_type;
> > +
> > +    struct ofputil_ct_tuple tuple_orig;
> > +    struct ofputil_ct_tuple tuple_reply;
> > +};
> 
> These structures are very similar to existing ct_dpif_tuple.
> We can keep them for now, but I think, we'll need to replace
> ct_dpif_tuple with the new ofputil_ct_tuple in the future.
> We may need some changes in ofputil_ct_tuple for that though.
> 
> 
> I was thinking about replacing them, however it would require a much
> bigger change, which was out of scope of this RFE. Also I was not entirely
> sure if there wasn't any specific reason for the ct_dpif_tuple specifically
> the address part. It seems like optimization to me.
> 
> Anyway it should be possible to unify those if needed.
>  
> 
> 
> 
> OTOH, I'm not sure

Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-10 Thread Ales Musil 
On Tue, Jan 10, 2023 at 11:08 PM Ilya Maximets  wrote:

> On 12/16/22 13:30, Ales Musil wrote:
> > Currently, the CT can be flushed by dpctl only be specifying
>
> *by
>
> > 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
> > Signed-off-by: Ales Musil 
> > Acked-by: Paolo Valerio 
> > ---
> > 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.
> > ---
>
> Hi, Ales.  Thanks for working on this change.
> Sorry for review taking so long.  I initially though there
> will be mostly a few cosmetic/style changes needed.  But
> longer I look at this code, more questions I have.  See below.
>
> Dumitru, maybe you can also help answer some of them?
>
> Best regards, Ilya Maximets.
>

Hi Ilya,

thank you for the review. Before proceeding I tried to answer your
concerns/questions.
Let me know what you think.

Thanks,
Ales


>
> >  NEWS   |   2 +
> >  include/openvswitch/ofp-util.h |  28 +++
> >  lib/automake.mk|   2 +
> >  lib/ct-dpif.c  | 208 +
> >  lib/ct-dpif.h  |   4 +-
> >  lib/dpctl.c|  42 ++---
> >  lib/dpctl.man  |   3 +-
> >  lib/ofp-ct-util.c  | 326 +
> >  lib/ofp-ct-util.h  |  36 
> >  tests/system-traffic.at|  82 -
> >  10 files changed, 586 insertions(+), 147 deletions(-)
> >  create mode 100644 lib/ofp-ct-util.c
> >  create mode 100644 lib/ofp-ct-util.h
> >
> > diff --git a/NEWS b/NEWS
> > index 265375e1c..ff8904b02 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -14,6 +14,8 @@ Post-v3.0.0
> >   10 Gbps link speed by default in case the actual link speed cannot
> be
> >   determined.  Previously it was 10 Mbps.  Values can still be
> overridden
> >   by specifying 'max-rate' or '[r]stp-path-cost' accordingly.
> > +   - ovs-dpctl and related ovs-appctl commands:
>
> "ovs-dpctl and 'ovs-appctl dpctl/'" might be a better header.
>
> > + * "flush-conntrack" is capable of handling partial 5-tuple.
> >
> >
> >  v3.0.0 - 15 Aug 2022
> > diff --git a/include/openvswitch/ofp-util.h
> b/include/openvswitch/ofp-util.h
> > index 091a09cad..84937ae26 100644
> > --- a/include/openvswitch/ofp-util.h
> > +++ b/include/openvswitch/ofp-util.h
> > @@ -19,6 +19,9 @@
> >
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +
> >  #include "openvswitch/ofp-protocol.h"
> >
> >  struct ofp_header;
> > @@ -27,6 +30,31 @@ struct ofp_header;
> >  extern "C" {
> >  #endif
> >
> > +struct ofputil_ct_tuple {
> > +struct in6_addr src;
> > +struct in6_addr dst;
> > +
> > +union {
> > +ovs_be16 src_port;
> > +ovs_be16 icmp_id;
> > +};
> > +union {
> > +ovs_be16 dst_port;
> > +struct {
> > +uint8_t icmp_code;
> > +uint8_t icmp_type;
> > +};
> > +};
> > +};
> > +
> > +struct ofputil_ct_match {
> > +uint8_t ip_proto;
> > +uint16_t l3_type;
> > +
> > +struct ofputil_ct_tuple tuple_orig;
> > +struct ofputil_ct_tuple tuple_reply;
> > +};
>
> These structures are very similar to existing ct_dpif_tuple.
> We can keep them for now, but I think, we'll need to replace
> ct_dpif_tuple with the new ofputil_ct_tuple in the future.
> We may need some changes in ofputil_ct_tuple for that though.
>

I was thinking about replacing them, however it would require a much
bigger change, which was out of scope of this RFE. Also I was not entirely
sure if there wasn't any specific reason for the ct_dpif_tuple specifically
the address part. It seems like optimization to me.

Anyway it should be possible to unify those if needed.


>
>
> OTOH, I'm not sure I understand why we need both tuples here.
> Can we have a tuple and a flag that indicates a direction
> instead?
>

That is also a possibility. However that wouldn't fit too that nicely in the
whole match struct, we could probably duplicate the proto field and get rid
of the
l3_type. That way we could supply just two tuples instead, getting rid of
the match completely.


>
> There are several issues with having both original and reply
> tuples, IIUC:
>
> 1. None of the command line interfaces support specifying
>reply tuple.
>

That was the intention of the original command AFAICT, we could modify the
commands
to support that. However I'm not sure if there is any benefit in that.


>
> 2.

Re: [ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2023-01-10 Thread Ilya Maximets 
On 12/16/22 13:30, Ales Musil wrote:
> Currently, the CT can be flushed by dpctl only be specifying

*by

> 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
> Signed-off-by: Ales Musil 
> Acked-by: Paolo Valerio 
> ---
> 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.
> ---

Hi, Ales.  Thanks for working on this change.
Sorry for review taking so long.  I initially though there
will be mostly a few cosmetic/style changes needed.  But
longer I look at this code, more questions I have.  See below.

Dumitru, maybe you can also help answer some of them?

Best regards, Ilya Maximets.

>  NEWS   |   2 +
>  include/openvswitch/ofp-util.h |  28 +++
>  lib/automake.mk|   2 +
>  lib/ct-dpif.c  | 208 +
>  lib/ct-dpif.h  |   4 +-
>  lib/dpctl.c|  42 ++---
>  lib/dpctl.man  |   3 +-
>  lib/ofp-ct-util.c  | 326 +
>  lib/ofp-ct-util.h  |  36 
>  tests/system-traffic.at|  82 -
>  10 files changed, 586 insertions(+), 147 deletions(-)
>  create mode 100644 lib/ofp-ct-util.c
>  create mode 100644 lib/ofp-ct-util.h
> 
> diff --git a/NEWS b/NEWS
> index 265375e1c..ff8904b02 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,8 @@ Post-v3.0.0
>   10 Gbps link speed by default in case the actual link speed cannot be
>   determined.  Previously it was 10 Mbps.  Values can still be overridden
>   by specifying 'max-rate' or '[r]stp-path-cost' accordingly.
> +   - ovs-dpctl and related ovs-appctl commands:

"ovs-dpctl and 'ovs-appctl dpctl/'" might be a better header.

> + * "flush-conntrack" is capable of handling partial 5-tuple.
>  
>  
>  v3.0.0 - 15 Aug 2022
> diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
> index 091a09cad..84937ae26 100644
> --- a/include/openvswitch/ofp-util.h
> +++ b/include/openvswitch/ofp-util.h
> @@ -19,6 +19,9 @@
>  
>  #include 
>  #include 
> +#include 
> +#include 
> +
>  #include "openvswitch/ofp-protocol.h"
>  
>  struct ofp_header;
> @@ -27,6 +30,31 @@ struct ofp_header;
>  extern "C" {
>  #endif
>  
> +struct ofputil_ct_tuple {
> +struct in6_addr src;
> +struct in6_addr dst;
> +
> +union {
> +ovs_be16 src_port;
> +ovs_be16 icmp_id;
> +};
> +union {
> +ovs_be16 dst_port;
> +struct {
> +uint8_t icmp_code;
> +uint8_t icmp_type;
> +};
> +};
> +};
> +
> +struct ofputil_ct_match {
> +uint8_t ip_proto;
> +uint16_t l3_type;
> +
> +struct ofputil_ct_tuple tuple_orig;
> +struct ofputil_ct_tuple tuple_reply;
> +};

These structures are very similar to existing ct_dpif_tuple.
We can keep them for now, but I think, we'll need to replace
ct_dpif_tuple with the new ofputil_ct_tuple in the future.
We may need some changes in ofputil_ct_tuple for that though.


OTOH, I'm not sure I understand why we need both tuples here.
Can we have a tuple and a flag that indicates a direction
instead?

There are several issues with having both original and reply
tuples, IIUC:

1. None of the command line interfaces support specifying
   reply tuple.

2. If the reply tuple will be specified via OpenFlow, it
   will be ignored if the original direction tuple is
   fully specified.

3. Netfilter kernel interface allows specifying only one
   tuple, not both, AFAICT.

I'm not a conntrack expert, so I have a question: Is it possible
that addresses and ports in original and reply directions are
not mirrored copies?  Or do we care about these cases?

If we want to flush all entries related to the same IP:PORT,
we will not be able to do that in a single OpenFlow request,
because we'll need to supply the same IP and PORT as src
and dst, or as src in both original and reply directions.
None of these can fit into a single request.

At this point is there actually any value in flushing basing
on reply direction?  Shouldn't we be able to flush the same
entries by flushing by dst in original direction, if we will
need to issue multiple requests anyway?

Did you start working on the OVN side of things?  Which
commands OVN will actually use?

> +
>  bool ofputil_decode_hello(const struct ofp_header *,
>uint32_t *allowed_versions);
>  struct ofpbuf *ofputil_encode_hello(uint32_t version_bitmap);
> diff

[ovs-dev] [PATCH v5 1/2] ofp, dpif: Allow CT flush based on partial match

2022-12-16 Thread Ales Musil 
Currently, the CT can be flushed by dpctl only be 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
Signed-off-by: Ales Musil 
Acked-by: Paolo Valerio 
---
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   |   2 +
 include/openvswitch/ofp-util.h |  28 +++
 lib/automake.mk|   2 +
 lib/ct-dpif.c  | 208 +
 lib/ct-dpif.h  |   4 +-
 lib/dpctl.c|  42 ++---
 lib/dpctl.man  |   3 +-
 lib/ofp-ct-util.c  | 326 +
 lib/ofp-ct-util.h  |  36 
 tests/system-traffic.at|  82 -
 10 files changed, 586 insertions(+), 147 deletions(-)
 create mode 100644 lib/ofp-ct-util.c
 create mode 100644 lib/ofp-ct-util.h

diff --git a/NEWS b/NEWS
index 265375e1c..ff8904b02 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,8 @@ Post-v3.0.0
  10 Gbps link speed by default in case the actual link speed cannot be
  determined.  Previously it was 10 Mbps.  Values can still be overridden
  by specifying 'max-rate' or '[r]stp-path-cost' accordingly.
+   - ovs-dpctl and related ovs-appctl commands:
+ * "flush-conntrack" is capable of handling partial 5-tuple.
 
 
 v3.0.0 - 15 Aug 2022
diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
index 091a09cad..84937ae26 100644
--- a/include/openvswitch/ofp-util.h
+++ b/include/openvswitch/ofp-util.h
@@ -19,6 +19,9 @@
 
 #include 
 #include 
+#include 
+#include 
+
 #include "openvswitch/ofp-protocol.h"
 
 struct ofp_header;
@@ -27,6 +30,31 @@ struct ofp_header;
 extern "C" {
 #endif
 
+struct ofputil_ct_tuple {
+struct in6_addr src;
+struct in6_addr dst;
+
+union {
+ovs_be16 src_port;
+ovs_be16 icmp_id;
+};
+union {
+ovs_be16 dst_port;
+struct {
+uint8_t icmp_code;
+uint8_t icmp_type;
+};
+};
+};
+
+struct ofputil_ct_match {
+uint8_t ip_proto;
+uint16_t l3_type;
+
+struct ofputil_ct_tuple tuple_orig;
+struct ofputil_ct_tuple tuple_reply;
+};
+
 bool ofputil_decode_hello(const struct ofp_header *,
   uint32_t *allowed_versions);
 struct ofpbuf *ofputil_encode_hello(uint32_t version_bitmap);
diff --git a/lib/automake.mk b/lib/automake.mk
index a0fabe38f..37135f118 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -227,6 +227,8 @@ lib_libopenvswitch_la_SOURCES = \
lib/ofp-actions.c \
lib/ofp-bundle.c \
lib/ofp-connection.c \
+   lib/ofp-ct-util.c \
+   lib/ofp-ct-util.h \
lib/ofp-ed-props.c \
lib/ofp-errors.c \
lib/ofp-flow.c \
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 6f17a26b5..9b324ff79 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -20,6 +20,7 @@
 #include 
 
 #include "ct-dpif.h"
+#include "ofp-ct-util.h"
 #include "openvswitch/ofp-parse.h"
 #include "openvswitch/vlog.h"
 
@@ -71,6 +72,31 @@ ct_dpif_dump_start(struct dpif *dpif, struct 
ct_dpif_dump_state **dump,
 return err;
 }
 
+static void
+ct_dpif_tuple_from_ofputil_ct_tuple(const struct ofputil_ct_tuple *ofp_tuple,
+struct ct_dpif_tuple *tuple,
+uint16_t l3_type, uint8_t ip_proto)
+{
+if (l3_type == AF_INET) {
+tuple->src.ip = in6_addr_get_mapped_ipv4(_tuple->src);
+tuple->dst.ip = in6_addr_get_mapped_ipv4(_tuple->dst);
+} else {
+tuple->src.in6 = ofp_tuple->src;
+tuple->dst.in6 = ofp_tuple->dst;
+}
+
+tuple->l3_type = l3_type;
+tuple->ip_proto = ip_proto;
+tuple->src_port = ofp_tuple->src_port;
+
+if (ip_proto == IPPROTO_ICMP || ip_proto == IPPROTO_ICMPV6) {
+tuple->icmp_code = ofp_tuple->icmp_code;
+tuple->icmp_type = ofp_tuple->icmp_type;
+} else {
+tuple->dst_port = ofp_tuple->dst_port;
+}
+}
+
 /* Dump one connection from a tracker, and put it in 'entry'.
  *
  * 'dump' should have been initialized by ct_dpif_dump_start().
@@ -100,25 +126,77 @@ ct_dpif_dump_done(struct ct_dpif_dump_state *dump)
 ? dpif->dpif_class->ct_dump_done(dpif, dump)
 : EOPNOTSUPP);
 }
-
+
+static int
+ct_dpif_flush_tuple(struct dpif *dpif, const uint16_t *zone,
+const struct ofputil_ct_match *match) {
+struct ct_dpif_dump_state *dump;
+struct ct_dpif_entry cte;
+