Message ID | 20220128144019858375129@chinatelecom.cn |
---|---|
State | Superseded |
Headers | show |
Series | fix dpif backer revalidation | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 28 Jan 2022, at 7:40, lic121 wrote: > Currently, ipfix conf creation/deletion 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. > > This patch covers only new creation/deletion of ipfix config. > Will upload one more patch to cover ipfix option changes. I think you could drop this patch from the series until you have the full fix, but I let the maintainers decide how they would like to handle this. Ilya/Ian/William? > Signed-off-by: lic121 <lic121@chinatelecom.cn> > --- > ofproto/ofproto-dpif.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index bc3df8e..5737615 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -2333,6 +2333,12 @@ set_ipfix( > dpif_ipfix_unref(di); > ofproto->ipfix = NULL; > } > + > + /* TODO: need to test ipfix option changes more than > + * enable/disable */ > + if (new_di || !ofproto->ipfix) { > + 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
>On 28 Jan 2022, at 7:40, lic121 wrote: > >> Currently, ipfix conf creation/deletion 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. >> >> This patch covers only new creation/deletion of ipfix config. >> Will upload one more patch to cover ipfix option changes. > >I think you could drop this patch from the series until you have the full fix, but I let the maintainers decide how they would like to handle this. Without this patch, two ipfix test cases fails with my second patch. With my second patch, revalidator is not triggerd when no lldp changes. As the result, ipfix config changes is not commited into xlate layer, because ipfix doesn't trigger revalidator by itself. > >Ilya/Ian/William? > >> Signed-off-by: lic121 <lic121@chinatelecom.cn> >> --- >> ofproto/ofproto-dpif.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index bc3df8e..5737615 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -2333,6 +2333,12 @@ set_ipfix( >> dpif_ipfix_unref(di); >> ofproto->ipfix = NULL; >> } >> + >> + /* TODO: need to test ipfix option changes more than >> + * enable/disable */ >> + if (new_di || !ofproto->ipfix) { >> + 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 > >
On 4 Feb 2022, at 15:25, lic121 wrote: >> On 28 Jan 2022, at 7:40, lic121 wrote: >> >>> Currently, ipfix conf creation/deletion 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. >>> >>> This patch covers only new creation/deletion of ipfix config. >>> Will upload one more patch to cover ipfix option changes. >> >> I think you could drop this patch from the series until you have the full fix, but I let the maintainers decide how they would like to handle this. > > Without this patch, two ipfix test cases fails with my second patch. Which test cases are they, as I see no newly introduced test cases for ipfix? > With my second patch, revalidator is not triggerd when no lldp changes. > As the result, ipfix config changes is not commited into xlate layer, because ipfix doesn't trigger revalidator by itself. > >> >> Ilya/Ian/William? >> >>> Signed-off-by: lic121 <lic121@chinatelecom.cn> >>> --- >>> ofproto/ofproto-dpif.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index bc3df8e..5737615 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -2333,6 +2333,12 @@ set_ipfix( >>> dpif_ipfix_unref(di); >>> ofproto->ipfix = NULL; >>> } >>> + >>> + /* TODO: need to test ipfix option changes more than >>> + * enable/disable */ >>> + if (new_di || !ofproto->ipfix) { >>> + 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 >> >>
>On 4 Feb 2022, at 15:25, lic121 wrote: > >>> On 28 Jan 2022, at 7:40, lic121 wrote: >>> >>>> Currently, ipfix conf creation/deletion 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. >>>> >>>> This patch covers only new creation/deletion of ipfix config. >>>> Will upload one more patch to cover ipfix option changes. >>> >>> I think you could drop this patch from the series until you have the full fix, but I let the maintainers decide how they would like to handle this. >> >> Without this patch, two ipfix test cases fails with my second patch. > >Which test cases are they, as I see no newly introduced test cases for ipfix? They are not new test cases: "Bridge IPFIX sanity check" and "Bridge IPFIX statistics check". I copied the first test case failure log: ########## test log ################# --- - 2022-02-05 02:07:11.901471095 +0000 +++ /root/ovs/tests/testsuite.dir/at-groups/1190/stdout 2022-02-05 02:07:11.899001638 +0000 @@ -1,3 +1,3 @@ flow-dump from the main thread: -packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295)) +packets:2, bytes:68, used:0.001s, actions:drop ########## log end ################# It fails because ipfix creating doesn't trigger revalidator. So no ipfix action is generated. It didn't fail becase lldp always trigger revalidator in bridge_reconfigure() even if lldp config is empty In my second patch, lldp issue is fixed, which avoid unneccesary revalidator triggered by lldp. > >> With my second patch, revalidator is not triggerd when no lldp changes. >> As the result, ipfix config changes is not commited into xlate layer, because ipfix doesn't trigger revalidator by itself. >> >>> >>> Ilya/Ian/William? >>> >>>> Signed-off-by: lic121 <lic121@chinatelecom.cn> >>>> --- >>>> ofproto/ofproto-dpif.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>>> index bc3df8e..5737615 100644 >>>> --- a/ofproto/ofproto-dpif.c >>>> +++ b/ofproto/ofproto-dpif.c >>>> @@ -2333,6 +2333,12 @@ set_ipfix( >>>> dpif_ipfix_unref(di); >>>> ofproto->ipfix = NULL; >>>> } >>>> + >>>> + /* TODO: need to test ipfix option changes more than >>>> + * enable/disable */ >>>> + if (new_di || !ofproto->ipfix) { >>>> + 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 --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index bc3df8e..5737615 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2333,6 +2333,12 @@ set_ipfix( dpif_ipfix_unref(di); ofproto->ipfix = NULL; } + + /* TODO: need to test ipfix option changes more than + * enable/disable */ + if (new_di || !ofproto->ipfix) { + ofproto->backer->need_revalidate = REV_RECONFIGURE; + } } return 0;
Currently, ipfix conf creation/deletion 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. This patch covers only new creation/deletion of ipfix config. Will upload one more patch to cover ipfix option changes. Signed-off-by: lic121 <lic121@chinatelecom.cn> --- ofproto/ofproto-dpif.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 1.8.3.1