>
>
>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 <lic...@chinatelecom.cn>
>>>> ---
>>>> 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);
}
}
>> 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
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
