diff mbox series

[ovs-dev,v1,1/2] ofproto-dpif: trigger revalidation when ipfix changes

Message ID 2022010921441915301290@chinatelecom.cn
State Changes Requested
Headers show
Series fix dpif backer revalidation | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Cheng Li Jan. 9, 2022, 1:44 p.m. UTC 
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 <lic121@chinatelecom.cn>
---
 ofproto/ofproto-dpif.c | 5 +++++
 1 file changed, 5 insertions(+)

--
1.8.3.1

Comments

Eelco Chaudron Jan. 14, 2022, 8:24 a.m. UTC | #1 
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 <lic121@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.

> +        ofproto->backer->need_revalidate = REV_RECONFIGURE;
> +    }
> +
>      return 0;
>  }
>
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Cheng Li Jan. 14, 2022, 8:58 a.m. UTC | #2 
>
>
>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 <lic121@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
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.

>> +        ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> +    }
>> +
>>      return 0;
>>  }
>>
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Eelco Chaudron Jan. 14, 2022, 9:22 a.m. UTC | #3 
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 <lic121@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?

> 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
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
Cheng Li Jan. 14, 2022, 9:38 a.m. UTC | #4 
>
>
>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 <lic121@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
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>
>
Eelco Chaudron Jan. 14, 2022, 10:15 a.m. UTC | #5 
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 <lic121@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.

>     }
> }
>
>>> 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
>>>>> dev@openvswitch.org
>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>>>>
>>
>>
Cheng Li Jan. 15, 2022, 2 a.m. UTC | #6 
>
>
>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 <lic121@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
>>>>>> dev@openvswitch.org
>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>>
>>>>>
>>>
>>>
>
>
Eelco Chaudron Jan. 17, 2022, 10:06 a.m. UTC | #7 
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 <lic121@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.

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
>>>>>>> dev@openvswitch.org
>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
diff mbox series

Patch

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) {
+        ofproto->backer->need_revalidate = REV_RECONFIGURE;
+    }
+
     return 0;
 }