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. > } > } > >>> 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
