[ovs-dev] ofproto-dpif-upcall: Transition ukey on dp_ops error.

Message ID 20170906221252.17257-1-joe@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] ofproto-dpif-upcall: Transition ukey on dp_ops error.
Related show

Commit Message

Joe Stringer Sept. 6, 2017, 10:12 p.m.
In most situations, we don't expect that a flow we've successfully
dumped, which we intend to delete, cannot be deleted. However, to make
this code more resilient to ensure that ukeys *will* transition in all
cases (including an error at this stage), grab the lock and transition
this ukey forward to the evicted state, effectively treating a failure
to delete as "this flow is already gone".

If we subsequently find out that it wasn't deleted, then that's ok - we
will re-dump, and validate at that stage, which should lead to creating
a new ukey or deleting the datapath flow when that happens.

Signed-off-by: Joe Stringer <joe@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Gregory Rose Sept. 18, 2017, 9:56 p.m. | #1
On 09/06/2017 03:12 PM, Joe Stringer wrote:
> In most situations, we don't expect that a flow we've successfully
> dumped, which we intend to delete, cannot be deleted. However, to make
> this code more resilient to ensure that ukeys *will* transition in all
> cases (including an error at this stage), grab the lock and transition
> this ukey forward to the evicted state, effectively treating a failure
> to delete as "this flow is already gone".
> 
> If we subsequently find out that it wasn't deleted, then that's ok - we
> will re-dump, and validate at that stage, which should lead to creating
> a new ukey or deleting the datapath flow when that happens.
> 
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
>   ofproto/ofproto-dpif-upcall.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 4a71bbe258df..bd324fbb6323 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2227,6 +2227,11 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
>   
>           if (op->dop.error) {
>               /* flow_del error, 'stats' is unusable. */
> +            if (op->ukey) {
> +                ovs_mutex_lock(&op->ukey->mutex);
> +                transition_ukey(op->ukey, UKEY_EVICTED);
> +                ovs_mutex_unlock(&op->ukey->mutex);
> +            }
>               continue;
>           }
>   
> 

Compile tested only - I didn't see of any good way to force the error

Code looks good to me.

Reviewed-by: Greg Rose <gvrose8192@gmail.com>
Ben Pfaff Nov. 2, 2017, 10:02 p.m. | #2
On Mon, Sep 18, 2017 at 02:56:35PM -0700, Greg Rose wrote:
> On 09/06/2017 03:12 PM, Joe Stringer wrote:
> >In most situations, we don't expect that a flow we've successfully
> >dumped, which we intend to delete, cannot be deleted. However, to make
> >this code more resilient to ensure that ukeys *will* transition in all
> >cases (including an error at this stage), grab the lock and transition
> >this ukey forward to the evicted state, effectively treating a failure
> >to delete as "this flow is already gone".
> >
> >If we subsequently find out that it wasn't deleted, then that's ok - we
> >will re-dump, and validate at that stage, which should lead to creating
> >a new ukey or deleting the datapath flow when that happens.
> >
> >Signed-off-by: Joe Stringer <joe@ovn.org>
> >---
> >  ofproto/ofproto-dpif-upcall.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> >diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> >index 4a71bbe258df..bd324fbb6323 100644
> >--- a/ofproto/ofproto-dpif-upcall.c
> >+++ b/ofproto/ofproto-dpif-upcall.c
> >@@ -2227,6 +2227,11 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
> >          if (op->dop.error) {
> >              /* flow_del error, 'stats' is unusable. */
> >+            if (op->ukey) {
> >+                ovs_mutex_lock(&op->ukey->mutex);
> >+                transition_ukey(op->ukey, UKEY_EVICTED);
> >+                ovs_mutex_unlock(&op->ukey->mutex);
> >+            }
> >              continue;
> >          }
> >
> 
> Compile tested only - I didn't see of any good way to force the error
> 
> Code looks good to me.
> 
> Reviewed-by: Greg Rose <gvrose8192@gmail.com>

I applied this to master, although I don't fully understand it so that
seems like living dangerously.  That's EXTREME programming.
Joe Stringer Nov. 5, 2017, 8:47 p.m. | #3
On Thu, 2 Nov 2017, 15:02 Ben Pfaff, <blp@ovn.org> wrote:

> On Mon, Sep 18, 2017 at 02:56:35PM -0700, Greg Rose wrote:
> > On 09/06/2017 03:12 PM, Joe Stringer wrote:
> > >In most situations, we don't expect that a flow we've successfully
> > >dumped, which we intend to delete, cannot be deleted. However, to make
> > >this code more resilient to ensure that ukeys *will* transition in all
> > >cases (including an error at this stage), grab the lock and transition
> > >this ukey forward to the evicted state, effectively treating a failure
> > >to delete as "this flow is already gone".
> > >
> > >If we subsequently find out that it wasn't deleted, then that's ok - we
> > >will re-dump, and validate at that stage, which should lead to creating
> > >a new ukey or deleting the datapath flow when that happens.
> > >
> > >Signed-off-by: Joe Stringer <joe@ovn.org>
> > >---
> > >  ofproto/ofproto-dpif-upcall.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > >diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > >index 4a71bbe258df..bd324fbb6323 100644
> > >--- a/ofproto/ofproto-dpif-upcall.c
> > >+++ b/ofproto/ofproto-dpif-upcall.c
> > >@@ -2227,6 +2227,11 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
> *ops, size_t n_ops)
> > >          if (op->dop.error) {
> > >              /* flow_del error, 'stats' is unusable. */
> > >+            if (op->ukey) {
> > >+                ovs_mutex_lock(&op->ukey->mutex);
> > >+                transition_ukey(op->ukey, UKEY_EVICTED);
> > >+                ovs_mutex_unlock(&op->ukey->mutex);
> > >+            }
> > >              continue;
> > >          }
> > >
> >
> > Compile tested only - I didn't see of any good way to force the error
> >
> > Code looks good to me.
> >
> > Reviewed-by: Greg Rose <gvrose8192@gmail.com>
>
> I applied this to master, although I don't fully understand it so that
> seems like living dangerously.  That's EXTREME programming.
>

Here was I, thinking that extreme programming involved at least one of a
volcano, parachute, or snowboard...

Thanks for applying this.

>

Patch

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4a71bbe258df..bd324fbb6323 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2227,6 +2227,11 @@  push_dp_ops(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
 
         if (op->dop.error) {
             /* flow_del error, 'stats' is unusable. */
+            if (op->ukey) {
+                ovs_mutex_lock(&op->ukey->mutex);
+                transition_ukey(op->ukey, UKEY_EVICTED);
+                ovs_mutex_unlock(&op->ukey->mutex);
+            }
             continue;
         }