On 15 Jan 2022, at 3:00, lic121 wrote:
>> >> >> On 14 Jan 2022, at 10:38, lic121 wrote: >> >>>> >>>> >>>> On 14 Jan 2022, at 9:58, lic121 wrote: >>>> >>>>>> >>>>>> >>>>>> On 9 Jan 2022, at 14:44, lic121 wrote: >>>>>> >>>>>>> Currently, ipfix creation/delection don't trigger dpif backer >>>>>>> revalidation. This is not expected, as we need the revalidation >>>>>>> to commit ipfix into xlate. So that xlate can generate ipfix >>>>>>> actions. >>>>>>> >>>>>>> Signed-off-by: lic121 <[email protected]> >>>>>>> --- >>>>>>> ofproto/ofproto-dpif.c | 5 +++++ >>>>>>> 1 file changed, 5 insertions(+) >>>>>>> >>>>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>>>>>> index bc3df8e..1cdef18 100644 >>>>>>> --- a/ofproto/ofproto-dpif.c >>>>>>> +++ b/ofproto/ofproto-dpif.c >>>>>>> @@ -2306,6 +2306,7 @@ set_ipfix( >>>>>>> { >>>>>>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); >>>>>>> struct dpif_ipfix *di = ofproto->ipfix; >>>>>>> + struct dpif_ipfix *old_ipfix = ofproto->ipfix; >>>>>>> bool has_options = bridge_exporter_options || >>>>>>> flow_exporters_options; >>>>>>> bool new_di = false; >>>>>>> >>>>>>> @@ -2335,6 +2336,10 @@ set_ipfix( >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> + if (old_ipfix != ofproto->ipfix) { >>>>>> >>>>>> This only works if ipfix was not configured earlier or disabled, i.e., >>>>>> ofproto->ipfix was/is NULL. >>>>>> If this was your intention, you could just have done “if (new_di || >>>>>> !ofproto->ipfix)”. >>>>>> >>>>>> However, I think there can also be changed in the configuration that >>>>>> requires a revalidate, what do you think? For example, >>>>>> enabling/disabling ingress/egress sampling. >>>>>> In this case dpif_ipfix_set_options() can be changed so it will return >>>>>> true if any configuration changes. >>>>>> >>>>> Actually, I had ever thought the same thing as you. But at last I didn't >>>>> do as that, for 3 reasons: >>>>> 1. I checked the history commit of ipfix, seems its not active in last >>>>> 2-3 years. So I guess not so many ovs users are using ipfix feature. >>>>> 2. In xlate_xbridge_set, the place where checks the change flag, it >>>>> doesn't check the ipfix options changes as well. >>>>> https://github.com/openvswitch/ovs/blob/eb1ab5357b6f3755f0ef1ee6d341ce24398d3bc1/ofproto/ofproto-dpif-xlate.c#L975-L978 >>>> >>>> But here it just set the pointer as a reference, it does not take care of >>>> acting on configuration changes, or do I miss something? >>>> >>> Assume that we checks the ipfix options changes in set_ipfix(): >>> 1. set_ipfix(ofproto, new_option,...) { >>> if (ofproto->ipfix->options != new_options) { >>> ipfix->options = new_options; >>> ofproto->backer->need_revalidate = REV_RECONFIGURE; >>> } >>> } >>> >>> 2. in xlate_xbridge_set, which is under revalidate context. >>> xlate_xbridge_set() { >>> ... >>> if (xbridge->ipfix != ipfix) { // here the ipfix options has changed, >>> but the "if test" will not aware that >>> dpif_ipfix_unref(xbridge->ipfix); >>> xbridge->ipfix = dpif_ipfix_ref(ipfix); >> >> But here the pointer does not change, so no need to update the reference to >> it. >> The actual configuration is taken in >> xlate_sample_action()/xlate_sample_action() used when creating/updating >> rules by the revalidator, kicked in by the succeeding udpif_revalidate() >> call. >> > After reading the code carefully again, I think you are right. > > I would like to divide the things into two parts. > Part #1 is to resolve unneccesary revalidate cased by set_lldp(). Part #2 is > the to take care of the ipfix/lldp content change more that enabling/diabling. > My team uses neither ipfix nor lldp, the problem we met is that > bridge_reconfigure() already trigger udpif revalidate because of lldp > problem(set_lldp() always trigger udpif revalidate). > So I would like to focus on the first part, but leave the second part for > users who really want to use ipfix/lldp features. > Please let me know your think, thanks. I see no reason why you could not split this patch up into two individual patches where the first one is sent out earlier than the other. >>> } >>> } >>> >>>>> 3. My main patch is the second one in this series, this patch is here >>>>> because it breaks two test cases. So I didn't spend muhc time on the >>>>> ipfix issue. >>>> >>>> See comments on the second patch. >>>> >>>>>>> + ofproto->backer->need_revalidate = REV_RECONFIGURE; >>>>>>> + } >>>>>>> + >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> -- >>>>>>> 1.8.3.1 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> dev mailing list >>>>>>> [email protected] >>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>> >>>>>> >>>> >>>> >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
