| Message ID | 20170906221252.17257-1-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-upcall: Transition ukey on dp_ops error. | expand |
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>
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.
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. >
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; }
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(+)