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

[Eelco Chaudron]

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

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

[lic121]

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

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

[Eelco Chaudron]

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

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

[lic121]

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

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

[Eelco Chaudron]

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

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

[lic121]

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

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

[Eelco Chaudron]

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

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

[lic121]

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 
---
 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) {
+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