[ovs-dev,v2] odp-execute: Skip processing actions when batch is emptied
diff mbox series

Message ID AM5PR0701MB296171E2A20B8156CEA39804A42E0@AM5PR0701MB2961.eurprd07.prod.outlook.com
State Changes Requested
Headers show
Series
  • [ovs-dev,v2] odp-execute: Skip processing actions when batch is emptied
Related show

Commit Message

Vishal Deep Ajmera Nov. 16, 2017, 6:01 a.m. UTC
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(-)

--
1.9.1

Comments

Ben Pfaff Nov. 28, 2017, 6:47 p.m. UTC | #1
On Thu, Nov 16, 2017 at 06:01:54AM +0000, Vishal Deep Ajmera 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>

Is this a bug fix or a precaution?  That is, are there actual cases in
the code today where such a crash occurs?

In the code, please do not add extra () where they are not needed:
                if (last_action || (batch->count == 0)) {


Thanks,

Ben.
Vishal Deep Ajmera Nov. 29, 2017, 6:35 a.m. UTC | #2
Thanks Ben for reviewing the patch. My comments inline below.

Warm Regards,
Vishal

-----Original Message-----
From: Ben Pfaff [mailto:blp@ovn.org] 
Sent: Wednesday, November 29, 2017 12:17 AM
To: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v2] odp-execute: Skip processing actions when batch is emptied

On Thu, Nov 16, 2017 at 06:01:54AM +0000, Vishal Deep Ajmera 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>

Is this a bug fix or a precaution?  That is, are there actual cases in the code today where such a crash occurs?

[Vishal] This is only a precaution. As of now, I do not see a case where a crash 
can happen if we continue to execute the actions on an empty batch. 
However any new actions we add in future will need to take care of empty batches.

In the code, please do not add extra () where they are not needed:
                if (last_action || (batch->count == 0)) {

[Vishal] Should I spin a V2 patch addressing your comment ?

Thanks,

Ben.
Ben Pfaff Nov. 29, 2017, 5:12 p.m. UTC | #3
On Wed, Nov 29, 2017 at 06:35:35AM +0000, Vishal Deep Ajmera wrote:
> On Thu, Nov 16, 2017 at 06:01:54AM +0000, Vishal Deep Ajmera 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>
> 
> Is this a bug fix or a precaution?  That is, are there actual cases in the code today where such a crash occurs?
> 
> [Vishal] This is only a precaution. As of now, I do not see a case where a crash 
> can happen if we continue to execute the actions on an empty batch. 
> However any new actions we add in future will need to take care of empty batches.

OK, thanks.  Will you please make that clear in the commit message?  It
can be important when reading a commit message later to know whether it
fixes a bug.

> In the code, please do not add extra () where they are not needed:
>                 if (last_action || (batch->count == 0)) {
> 
> [Vishal] Should I spin a V2 patch addressing your comment ?

Please do, with an updated commit message too.

Thanks,

Ben.

Patch
diff mbox series

diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 3011479..887246d 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -686,9 +686,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;
                 }
             }