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? > 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
