On Tue, Oct 3, 2023 at 2:28 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> On 10/3/23 10:33, Simon Horman wrote: > > On Tue, Oct 03, 2023 at 10:18:45AM +0200, Ales Musil wrote: > >> On Tue, Oct 3, 2023 at 10:14 AM Simon Horman <ho...@ovn.org> wrote: > >> > >>> On Tue, Oct 03, 2023 at 07:56:30AM +0200, 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. > >>>> > >>>> 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 > >>>> Signed-off-by: Ales Musil <amu...@redhat.com> > >>> > >>> ... > >>> > >>>> diff --git a/include/openvswitch/ofp-ct.h > b/include/openvswitch/ofp-ct.h > >>>> index c8023c309..d57b62678 100644 > >>>> --- a/include/openvswitch/ofp-ct.h > >>>> +++ b/include/openvswitch/ofp-ct.h > >>>> @@ -51,15 +51,21 @@ struct ofp_ct_match { > >>>> > >>>> struct ofp_ct_tuple tuple_orig; > >>>> struct ofp_ct_tuple tuple_reply; > >>> > >>> Hi Ales, > >>> > >> > >> Hi Simon, > >> > >> thank you for the review. > >> > >>> > >>> It's not related to this patch, but I notice that tuple_reply > >>> spans 2 cachelines (on x86_64). Depending on this access pattern > >>> this may not be ideal. > >>> > >> > >> I guess it should be fine, it shouldn't be in any hot loop as it's not > >> expected to run this command very often. > > > > Understood. Thanks for confirming. > > > >>>> + > >>>> + uint32_t mark; > >>>> + uint32_t mark_mask; > >>>> + > >>>> + ovs_u128 labels; > >>>> + ovs_u128 labels_mask; > >>>> }; > >>> > >>> ... > >>> > >>>> diff --git a/lib/dpctl.c b/lib/dpctl.c > >>>> index cd12625a1..15b04b7a2 100644 > >>>> --- a/lib/dpctl.c > >>>> +++ b/lib/dpctl.c > >>>> @@ -1773,48 +1773,17 @@ dpctl_flush_conntrack(int argc, const char > >>> *argv[], > >>>> struct dpif *dpif = NULL; > >>>> struct ofp_ct_match match = {0}; > >>>> struct ds ds = DS_EMPTY_INITIALIZER; > >>>> - uint16_t zone, *pzone = NULL; > >>>> + uint16_t zone; > >>>> int error; > >>>> int args = argc - 1; > >>>> - int zone_pos = 1; > >>>> + bool with_zone = false; > >>>> > >>>> if (dp_arg_exists(argc, argv)) { > >>>> args--; > >>>> - zone_pos = 2; > >>>> - } > >>>> - > >>>> - /* Parse zone. */ > >>>> - if (args && !strncmp(argv[zone_pos], "zone=", 5)) { > >>>> - if (!ovs_scan(argv[zone_pos], "zone=%"SCNu16, &zone)) { > >>>> - ds_put_cstr(&ds, "failed to parse zone"); > >>>> - error = EINVAL; > >>>> - goto error; > >>>> - } > >>>> - pzone = &zone; > >>>> - args--; > >>>> - } > >>>> - > >>>> - /* Parse ct tuples. */ > >>>> - for (int i = 0; i < 2; i++) { > >>>> - if (!args) { > >>>> - break; > >>>> - } > >>>> - > >>>> - struct ofp_ct_tuple *tuple = > >>>> - i ? &match.tuple_reply : &match.tuple_orig; > >>>> - const char *arg = argv[argc - args]; > >>>> - > >>>> - if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, &ds, > >>> &match.ip_proto, > >>>> - &match.l3_type)) { > >>>> - error = EINVAL; > >>>> - goto error; > >>>> - } > >>>> - args--; > >>>> } > >>>> > >>>> - /* Report error if there is more than one unparsed argument. */ > >>>> - if (args > 0) { > >>>> - ds_put_cstr(&ds, "invalid arguments"); > >>>> + if (args && !ofp_ct_match_parse(&argv[argc - args], args, &ds, > >>> &match, > >>>> + &with_zone ,&zone)) { > >>> > >>> It's very nice to factor out ofp_ct_match_parse(). > >>> I wonder if it would be worth separating that into a separate patch, > >>> with a second patch to add label and mark support. > >>> > >> > >> Let's see what others think, but yeah it might be possible to have it in > >> separate patch. > > > > Sure, let's. > > IMO, makes sense to split out. > > BTW, there are more unrelated whitespace changes in the patch, at least > 4 places. > > I'll give some more comments on the patch itself in a separate email. > > Best regards, Ilya Maximets. > > I know about the whitespace problems it will be fixed in the next iteration. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
