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