[ovs-dev] Skip processing actions when batch is emptied.

Message ID AM5PR0701MB29617AF52B1039F4C4B01BCEA4560@AM5PR0701MB2961.eurprd07.prod.outlook.com
State New
Headers show
Series
  • [ovs-dev] Skip processing actions when batch is emptied.
Related show

Commit Message

Vishal Deep Ajmera Nov. 8, 2017, 9:44 a.m.
Today in OVS, when errors are encountered during the execution of an action the entire batch of packets may be deleted (for e.g. in processing push_tnl_action, if the port is not found in the port_cache of PMD). The remaining actions continue to be executed even though there are no packets to be processed. It is assumed that the code dealing with each action checks that the batch is not empty before executing. Crashes may occur if the assumption is not met.

The patch makes OVS skip processing of further actions from the action-set once a batch is emptied. Doing so centralizes the check in one place and avoids the possibility of crashes.

Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
---
lib/odp-execute.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

dev@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Comments

Vishal Deep Ajmera Nov. 15, 2017, 5:44 a.m. | #1
Hi,

Do anyone see any issues with the approach below of skipping ovs actions when batch is emptied ?

Warm Regards,
Vishal

-----Original Message-----
From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Vishal Deep Ajmera
Sent: Wednesday, November 08, 2017 3:14 PM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH] Skip processing actions when batch is emptied.

Today in OVS, when errors are encountered during the execution of an action the entire batch of packets may be deleted (for e.g. in processing push_tnl_action, if the port is not found in the port_cache of PMD). The remaining actions continue to be executed even though there are no packets to be processed. It is assumed that the code dealing with each action checks that the batch is not empty before executing. Crashes may occur if the assumption is not met.

The patch makes OVS skip processing of further actions from the action-set once a batch is emptied. Doing so centralizes the check in one place and avoids the possibility of crashes.

Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
---
lib/odp-execute.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 3109f39..8697727 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -685,9 +685,12 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
                 dp_execute_action(dp, batch, a, may_steal);
-                if (last_action) {
-                    /* We do not need to free the packets. dp_execute_actions()
-                     * has stolen them */
+                if (last_action || (batch->count == 0)) {
+                    /* We do not need to free the packets.
+                     * Either dp_execute_actions() has stolen them
+                     * or the batch is freed due to errors. In either
+                     * case we do not need to execute further actions.
+                     */
                     return;
                 }
             }
William Tu Nov. 15, 2017, 3:48 p.m. | #2
On Wed, Nov 8, 2017 at 1:44 AM, Vishal Deep Ajmera
<vishal.deep.ajmera@ericsson.com> wrote:
> Today in OVS, when errors are encountered during the execution of an action the entire batch of packets may be deleted (for e.g. in processing push_tnl_action, if the port is not found in the port_cache of PMD). The remaining actions continue to be executed even though there are no packets to be processed. It is assumed that the code dealing with each action checks that the batch is not empty before executing. Crashes may occur if the assumption is not met.
>
> The patch makes OVS skip processing of further actions from the action-set once a batch is emptied. Doing so centralizes the check in one place and avoids the possibility of crashes.
>
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> ---

nit: please wrap your commit messages to about 75 columns

otherwise looks good to me.

Acked-by: William Tu <u9012063@gmail.com>

> lib/odp-execute.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 3109f39..8697727 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -685,9 +685,12 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>                  dp_execute_action(dp, batch, a, may_steal);
> -                if (last_action) {
> -                    /* We do not need to free the packets. dp_execute_actions()
> -                     * has stolen them */
> +                if (last_action || (batch->count == 0)) {
> +                    /* We do not need to free the packets.
> +                     * Either dp_execute_actions() has stolen them
> +                     * or the batch is freed due to errors. In either
> +                     * case we do not need to execute further actions.
> +                     */
>                      return;
>                  }
>              }
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Vishal Deep Ajmera Nov. 16, 2017, 6:04 a.m. | #3
Thank you William Tu for reviewing the patch. I have fixed the commit message & corrected subject line (by including file name) in V2 version of the patch.

Warm Regards,
Vishal

-----Original Message-----
From: William Tu [mailto:u9012063@gmail.com] 
Sent: Wednesday, November 15, 2017 9:19 PM
To: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Skip processing actions when batch is emptied.

On Wed, Nov 8, 2017 at 1:44 AM, Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com> wrote:
> Today in OVS, when errors are encountered during the execution of an action the entire batch of packets may be deleted (for e.g. in processing push_tnl_action, if the port is not found in the port_cache of PMD). The remaining actions continue to be executed even though there are no packets to be processed. It is assumed that the code dealing with each action checks that the batch is not empty before executing. Crashes may occur if the assumption is not met.
>
> The patch makes OVS skip processing of further actions from the action-set once a batch is emptied. Doing so centralizes the check in one place and avoids the possibility of crashes.
>
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> ---

nit: please wrap your commit messages to about 75 columns

otherwise looks good to me.

Acked-by: William Tu <u9012063@gmail.com>

> lib/odp-execute.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 
> 3109f39..8697727 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -685,9 +685,12 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>                  dp_execute_action(dp, batch, a, may_steal);
> -                if (last_action) {
> -                    /* We do not need to free the packets. dp_execute_actions()
> -                     * has stolen them */
> +                if (last_action || (batch->count == 0)) {
> +                    /* We do not need to free the packets.
> +                     * Either dp_execute_actions() has stolen them
> +                     * or the batch is freed due to errors. In either
> +                     * case we do not need to execute further actions.
> +                     */
>                      return;
>                  }
>              }
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 3109f39..8697727 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -685,9 +685,12 @@  odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
                 dp_execute_action(dp, batch, a, may_steal);
-                if (last_action) {
-                    /* We do not need to free the packets. dp_execute_actions()
-                     * has stolen them */
+                if (last_action || (batch->count == 0)) {
+                    /* We do not need to free the packets.
+                     * Either dp_execute_actions() has stolen them
+                     * or the batch is freed due to errors. In either
+                     * case we do not need to execute further actions.
+                     */
                     return;
                 }
             }
_______________________________________________
dev mailing list