| Message ID | 1522049787-24486-1-git-send-email-jan.scheurich@ericsson.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofp-actions: Correct execution of encap/decap actions in action set | expand |
On Mon, Mar 26, 2018 at 09:36:27AM +0200, Jan Scheurich wrote: > The actions encap, decap and dec_nsh_ttl were wrongly flagged as set_field > actions in ofpact_is_set_or_move_action(). This caused them to be executed > twice in the action set or a group bucket, once explicitly in > ofpacts_execute_action_set() and once again as part of the list of > set_field or move actions. > > Fixes: f839892a ("OF support and translation of generic encap and decap") > Fixes: 491e05c2 ("nsh: add dec_nsh_ttl action") > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > > --- > > The fix should be backported to OVS 2.9 and OVS 2.8 (without the case > for OFPACT_DEC_NSH_TTL introduced in 2.9). Thanks, I applied this to master and backported to branch-2.9 and branch-2.8. I want to encourage you to add a test that uses one or both of these actions in an action set, so that we get some test coverage. Thanks, Ben.
> -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Tuesday, 03 April, 2018 20:36 > > On Mon, Mar 26, 2018 at 09:36:27AM +0200, Jan Scheurich wrote: > > The actions encap, decap and dec_nsh_ttl were wrongly flagged as set_field > > actions in ofpact_is_set_or_move_action(). This caused them to be executed > > twice in the action set or a group bucket, once explicitly in > > ofpacts_execute_action_set() and once again as part of the list of > > set_field or move actions. > > > > Fixes: f839892a ("OF support and translation of generic encap and decap") > > Fixes: 491e05c2 ("nsh: add dec_nsh_ttl action") > > > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > > > > --- > > > > The fix should be backported to OVS 2.9 and OVS 2.8 (without the case > > for OFPACT_DEC_NSH_TTL introduced in 2.9). > > Thanks, I applied this to master and backported to branch-2.9 and > branch-2.8. Thanks! > I want to encourage you to add a test that uses one or both of these > actions in an action set, so that we get some test coverage. Will do when I find the time. I thought about that but didn't want to delay the fix. Regards, Jan
Hi, Jan Sangfor guy tried this one, he still encountered assert issue after ovs ran for about 20 minutes, moreover it appeared periodically. I'm not sure if https://patchwork.ozlabs.org/patch/895405/ is helpful for this issue. Do you think what the root cause is? -----Original Message----- From: Jan Scheurich [mailto:jan.scheurich@ericsson.com] Sent: Monday, March 26, 2018 3:36 PM To: dev@openvswitch.org Cc: Yang, Yi Y <yi.y.yang@intel.com>; Jan Scheurich <jan.scheurich@ericsson.com> Subject: [PATCH] ofp-actions: Correct execution of encap/decap actions in action set The actions encap, decap and dec_nsh_ttl were wrongly flagged as set_field actions in ofpact_is_set_or_move_action(). This caused them to be executed twice in the action set or a group bucket, once explicitly in ofpacts_execute_action_set() and once again as part of the list of set_field or move actions. Fixes: f839892a ("OF support and translation of generic encap and decap") Fixes: 491e05c2 ("nsh: add dec_nsh_ttl action") Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> --- The fix should be backported to OVS 2.9 and OVS 2.8 (without the case for OFPACT_DEC_NSH_TTL introduced in 2.9). lib/ofp-actions.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index db85716..87797bc 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -6985,9 +6985,6 @@ ofpact_is_set_or_move_action(const struct ofpact *a) case OFPACT_SET_TUNNEL: case OFPACT_SET_VLAN_PCP: case OFPACT_SET_VLAN_VID: - case OFPACT_ENCAP: - case OFPACT_DECAP: - case OFPACT_DEC_NSH_TTL: return true; case OFPACT_BUNDLE: case OFPACT_CLEAR_ACTIONS: @@ -7025,6 +7022,9 @@ ofpact_is_set_or_move_action(const struct ofpact *a) case OFPACT_WRITE_METADATA: case OFPACT_DEBUG_RECIRC: case OFPACT_DEBUG_SLOW: + case OFPACT_ENCAP: + case OFPACT_DECAP: + case OFPACT_DEC_NSH_TTL: return false; default: OVS_NOT_REACHED(); -- 1.9.1
Hi Yi, The assertion failure is indeed caused by the incorrect implementation of double encap() and should be fixed by the patch you mention (which is merged to master by now). Prior to the below fix this happened with every encap(nsh) in an group bucket. I can't say why it still happens periodically every few minutes in your test. You'd need to carefully analyze a crash dump to try to understand the packet processing history that leads to a double encap() or perhaps decap(). It is definitely worth trying whether the problem is already resolved on the latest master. BR, Jan > -----Original Message----- > From: Yang, Yi Y [mailto:yi.y.yang@intel.com] > Sent: Sunday, 08 April, 2018 10:27 > To: Jan Scheurich <jan.scheurich@ericsson.com>; dev@openvswitch.org > Subject: RE: [PATCH] ofp-actions: Correct execution of encap/decap actions in action set > > Hi, Jan > > Sangfor guy tried this one, he still encountered assert issue after ovs ran for about 20 minutes, moreover it appeared periodically. I'm > not sure if https://patchwork.ozlabs.org/patch/895405/ is helpful for this issue. Do you think what the root cause is? > > -----Original Message----- > From: Jan Scheurich [mailto:jan.scheurich@ericsson.com] > Sent: Monday, March 26, 2018 3:36 PM > To: dev@openvswitch.org > Cc: Yang, Yi Y <yi.y.yang@intel.com>; Jan Scheurich <jan.scheurich@ericsson.com> > Subject: [PATCH] ofp-actions: Correct execution of encap/decap actions in action set > > The actions encap, decap and dec_nsh_ttl were wrongly flagged as set_field actions in ofpact_is_set_or_move_action(). This caused > them to be executed twice in the action set or a group bucket, once explicitly in > ofpacts_execute_action_set() and once again as part of the list of set_field or move actions. > > Fixes: f839892a ("OF support and translation of generic encap and decap") > Fixes: 491e05c2 ("nsh: add dec_nsh_ttl action") > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > > --- > > The fix should be backported to OVS 2.9 and OVS 2.8 (without the case for OFPACT_DEC_NSH_TTL introduced in 2.9). > > > lib/ofp-actions.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index db85716..87797bc 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -6985,9 +6985,6 @@ ofpact_is_set_or_move_action(const struct ofpact *a) > case OFPACT_SET_TUNNEL: > case OFPACT_SET_VLAN_PCP: > case OFPACT_SET_VLAN_VID: > - case OFPACT_ENCAP: > - case OFPACT_DECAP: > - case OFPACT_DEC_NSH_TTL: > return true; > case OFPACT_BUNDLE: > case OFPACT_CLEAR_ACTIONS: > @@ -7025,6 +7022,9 @@ ofpact_is_set_or_move_action(const struct ofpact *a) > case OFPACT_WRITE_METADATA: > case OFPACT_DEBUG_RECIRC: > case OFPACT_DEBUG_SLOW: > + case OFPACT_ENCAP: > + case OFPACT_DECAP: > + case OFPACT_DEC_NSH_TTL: > return false; > default: > OVS_NOT_REACHED(); > -- > 1.9.1
Jan, sorry for late reply, I have been waiting for Sangfor guy's confirmation. With this patch https://patchwork.ozlabs.org/patch/895405/, they won't have any issue any more. So now it can work well, thank you so much. -----Original Message----- From: Jan Scheurich [mailto:jan.scheurich@ericsson.com] Sent: Monday, April 9, 2018 3:12 PM To: Yang, Yi Y <yi.y.yang@intel.com>; dev@openvswitch.org Subject: RE: [PATCH] ofp-actions: Correct execution of encap/decap actions in action set Hi Yi, The assertion failure is indeed caused by the incorrect implementation of double encap() and should be fixed by the patch you mention (which is merged to master by now). Prior to the below fix this happened with every encap(nsh) in an group bucket. I can't say why it still happens periodically every few minutes in your test. You'd need to carefully analyze a crash dump to try to understand the packet processing history that leads to a double encap() or perhaps decap(). It is definitely worth trying whether the problem is already resolved on the latest master. BR, Jan > -----Original Message----- > From: Yang, Yi Y [mailto:yi.y.yang@intel.com] > Sent: Sunday, 08 April, 2018 10:27 > To: Jan Scheurich <jan.scheurich@ericsson.com>; dev@openvswitch.org > Subject: RE: [PATCH] ofp-actions: Correct execution of encap/decap > actions in action set > > Hi, Jan > > Sangfor guy tried this one, he still encountered assert issue after > ovs ran for about 20 minutes, moreover it appeared periodically. I'm not sure if https://patchwork.ozlabs.org/patch/895405/ is helpful for this issue. Do you think what the root cause is? > > -----Original Message----- > From: Jan Scheurich [mailto:jan.scheurich@ericsson.com] > Sent: Monday, March 26, 2018 3:36 PM > To: dev@openvswitch.org > Cc: Yang, Yi Y <yi.y.yang@intel.com>; Jan Scheurich > <jan.scheurich@ericsson.com> > Subject: [PATCH] ofp-actions: Correct execution of encap/decap actions > in action set > > The actions encap, decap and dec_nsh_ttl were wrongly flagged as > set_field actions in ofpact_is_set_or_move_action(). This caused them > to be executed twice in the action set or a group bucket, once > explicitly in > ofpacts_execute_action_set() and once again as part of the list of set_field or move actions. > > Fixes: f839892a ("OF support and translation of generic encap and > decap") > Fixes: 491e05c2 ("nsh: add dec_nsh_ttl action") > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > > --- > > The fix should be backported to OVS 2.9 and OVS 2.8 (without the case for OFPACT_DEC_NSH_TTL introduced in 2.9). > > > lib/ofp-actions.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index > db85716..87797bc 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -6985,9 +6985,6 @@ ofpact_is_set_or_move_action(const struct ofpact *a) > case OFPACT_SET_TUNNEL: > case OFPACT_SET_VLAN_PCP: > case OFPACT_SET_VLAN_VID: > - case OFPACT_ENCAP: > - case OFPACT_DECAP: > - case OFPACT_DEC_NSH_TTL: > return true; > case OFPACT_BUNDLE: > case OFPACT_CLEAR_ACTIONS: > @@ -7025,6 +7022,9 @@ ofpact_is_set_or_move_action(const struct ofpact *a) > case OFPACT_WRITE_METADATA: > case OFPACT_DEBUG_RECIRC: > case OFPACT_DEBUG_SLOW: > + case OFPACT_ENCAP: > + case OFPACT_DECAP: > + case OFPACT_DEC_NSH_TTL: > return false; > default: > OVS_NOT_REACHED(); > -- > 1.9.1
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index db85716..87797bc 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -6985,9 +6985,6 @@ ofpact_is_set_or_move_action(const struct ofpact *a) case OFPACT_SET_TUNNEL: case OFPACT_SET_VLAN_PCP: case OFPACT_SET_VLAN_VID: - case OFPACT_ENCAP: - case OFPACT_DECAP: - case OFPACT_DEC_NSH_TTL: return true; case OFPACT_BUNDLE: case OFPACT_CLEAR_ACTIONS: @@ -7025,6 +7022,9 @@ ofpact_is_set_or_move_action(const struct ofpact *a) case OFPACT_WRITE_METADATA: case OFPACT_DEBUG_RECIRC: case OFPACT_DEBUG_SLOW: + case OFPACT_ENCAP: + case OFPACT_DECAP: + case OFPACT_DEC_NSH_TTL: return false; default: OVS_NOT_REACHED();
The actions encap, decap and dec_nsh_ttl were wrongly flagged as set_field actions in ofpact_is_set_or_move_action(). This caused them to be executed twice in the action set or a group bucket, once explicitly in ofpacts_execute_action_set() and once again as part of the list of set_field or move actions. Fixes: f839892a ("OF support and translation of generic encap and decap") Fixes: 491e05c2 ("nsh: add dec_nsh_ttl action") Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> --- The fix should be backported to OVS 2.9 and OVS 2.8 (without the case for OFPACT_DEC_NSH_TTL introduced in 2.9). lib/ofp-actions.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)