[ovs-dev] [ovs-dev v11] ofproto-dpif-upcall: fix push_dp_ops
push_dp_ops only handles delete ops errors but ignores the modify
ops results. It's better to handle all the dp operation errors in
a consistent way.
This patch prevents the inconsistency by considering modify failure
in revalidators.
To note, we cannot perform two state transitions and change ukey_state
into UKEY_EVICTED directly here, because, if we do so, the
sweep will remove the ukey alone and leave dp flow alive. Later, the
dump will retrieve the dp flow and might even recover it. This will
contribute the stats of this dp flow twice.
v8->v9: add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
v9->v10: change the commit message and refine the test case.
v10->v11: fix indentation and refine the test case.
Signed-off-by: Peng He
---
ofproto/ofproto-dpif-upcall.c | 51 +--
tests/dpif-netdev.at | 44 ++
2 files changed, 81 insertions(+), 14 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index cd57fdbd9..c920c749c 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -62,6 +62,7 @@ COVERAGE_DEFINE(upcall_flow_limit_hit);
COVERAGE_DEFINE(upcall_flow_limit_kill);
COVERAGE_DEFINE(upcall_ukey_contention);
COVERAGE_DEFINE(upcall_ukey_replace);
+COVERAGE_DEFINE(dumped_inconsistent_flow);
/* A thread that reads upcalls from dpif, forwards each upcall's packet,
* and possibly sets up a kernel flow as a cache. */
@@ -258,6 +259,7 @@ enum ukey_state {
UKEY_CREATED = 0,
UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is queued. */
UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */
+UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is inconsistent. */
UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued. */
UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */
UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */
@@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
ukey_state dst,
* UKEY_VISIBLE -> UKEY_EVICTED
* A handler attempts to install the flow, but the datapath rejects it.
* Consider that the datapath has already destroyed it.
+ * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
+ * A revalidator modifies the flow with error returns.
+ * UKEY_INCONSISTENT -> UKEY_EVICTING
+ * A revalidator decides to evict the datapath flow.
* UKEY_OPERATIONAL -> UKEY_EVICTING
* A revalidator decides to evict the datapath flow.
* UKEY_EVICTING-> UKEY_EVICTED
@@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum
ukey_state dst,
* UKEY_EVICTED -> UKEY_DELETED
* A revalidator has removed the ukey from the umap and is deleting it.
*/
-if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
- dst < UKEY_DELETED)) {
+if (ukey->state == dst - 1 ||
+ (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
+ (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
ukey->state = dst;
} else {
struct ds ds = DS_EMPTY_INITIALIZER;
@@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops,
size_t n_ops)
for (i = 0; i < n_ops; i++) {
struct ukey_op *op = &ops[i];
-struct dpif_flow_stats *push, *stats, push_buf;
-
-stats = op->dop.flow_del.stats;
-push = &push_buf;
-
-if (op->dop.type != DPIF_OP_FLOW_DEL) {
-/* Only deleted flows need their stats pushed. */
-continue;
-}
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);
+if (op->dop.type == DPIF_OP_FLOW_DEL) {
+transition_ukey(op->ukey, UKEY_EVICTED);
+} else {
+/* Modification of the flow failed */
+transition_ukey(op->ukey, UKEY_INCONSISTENT);
+}
ovs_mutex_unlock(&op->ukey->mutex);
}
continue;
}
+if (op->dop.type != DPIF_OP_FLOW_DEL) {
+/* Only deleted flows need their stats pushed. */
+continue;
+}
+
+struct dpif_flow_stats *push, *stats, push_buf;
+
+stats = op->dop.flow_del.stats;
+push = &push_buf;
+
if (op->ukey) {
ovs_mutex_lock(&op->ukey->mutex);
transition_ukey(op->ukey, UKEY_EVICTED);
@@ -2839,6 +2851,16 @@ revalidate(struct revalidator *revalidator)
continue;
}
+if (ukey->state == UKEY_INCONSISTENT) {
+ukey->dump_seq = dump_seq;
+COVERAGE_INC(dumped_inconsistent_flow);
+reval_op_init(&
Re: [ovs-dev] [ovs-dev v11] ofproto-dpif-upcall: fix push_dp_ops
On 9 Jun 2023, at 17:03, Peng He wrote:
> push_dp_ops only handles delete ops errors but ignores the modify
> ops results. It's better to handle all the dp operation errors in
> a consistent way.
>
> This patch prevents the inconsistency by considering modify failure
> in revalidators.
>
> To note, we cannot perform two state transitions and change ukey_state
> into UKEY_EVICTED directly here, because, if we do so, the
> sweep will remove the ukey alone and leave dp flow alive. Later, the
> dump will retrieve the dp flow and might even recover it. This will
> contribute the stats of this dp flow twice.
>
> v8->v9: add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
> v9->v10: change the commit message and refine the test case.
> v10->v11: fix indentation and refine the test case.
>
> Signed-off-by: Peng He
Thanks for making these changes, there is a merge conflict on the tests case
and some small comments below.
So maybe you can send a v12 and I’ll do a quick check and ACK it. What do you
think?
Cheers,
Eelco
> ---
> ofproto/ofproto-dpif-upcall.c | 51 +--
> tests/dpif-netdev.at | 44 ++
> 2 files changed, 81 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index cd57fdbd9..c920c749c 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -62,6 +62,7 @@ COVERAGE_DEFINE(upcall_flow_limit_hit);
> COVERAGE_DEFINE(upcall_flow_limit_kill);
> COVERAGE_DEFINE(upcall_ukey_contention);
> COVERAGE_DEFINE(upcall_ukey_replace);
> +COVERAGE_DEFINE(dumped_inconsistent_flow);
Can you add this in alphabetical order?
>
> /* A thread that reads upcalls from dpif, forwards each upcall's packet,
> * and possibly sets up a kernel flow as a cache. */
> @@ -258,6 +259,7 @@ enum ukey_state {
> UKEY_CREATED = 0,
> UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is queued.
> */
> UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */
> +UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is inconsistent. */
> UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued.
> */
> UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */
> UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */
> @@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> * UKEY_VISIBLE -> UKEY_EVICTED
> * A handler attempts to install the flow, but the datapath rejects it.
> * Consider that the datapath has already destroyed it.
> + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
> + * A revalidator modifies the flow with error returns.
> + * UKEY_INCONSISTENT -> UKEY_EVICTING
> + * A revalidator decides to evict the datapath flow.
> * UKEY_OPERATIONAL -> UKEY_EVICTING
> * A revalidator decides to evict the datapath flow.
> * UKEY_EVICTING-> UKEY_EVICTED
> @@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> * UKEY_EVICTED -> UKEY_DELETED
> * A revalidator has removed the ukey from the umap and is deleting it.
> */
> -if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
> - dst < UKEY_DELETED)) {
> +if (ukey->state == dst - 1 ||
> + (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
> + (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
> ukey->state = dst;
> } else {
> struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops,
> size_t n_ops)
>
> for (i = 0; i < n_ops; i++) {
> struct ukey_op *op = &ops[i];
> -struct dpif_flow_stats *push, *stats, push_buf;
> -
> -stats = op->dop.flow_del.stats;
> -push = &push_buf;
> -
> -if (op->dop.type != DPIF_OP_FLOW_DEL) {
> -/* Only deleted flows need their stats pushed. */
> -continue;
> -}
>
> 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);
> +if (op->dop.type == DPIF_OP_FLOW_DEL) {
> +transition_ukey(op->ukey, UKEY_EVICTED);
> +} else {
> +/* Modification of the flow failed */
You forgot to copy the dot.
> +transition_ukey(op->ukey, UKEY_INCONSISTENT);
> +}
> ovs_mutex_unlock(&op->ukey->mutex);
> }
> continue;
> }
>
> +if (op->dop.type != DPIF_OP_FLOW_DEL) {
> +/* Only deleted flows need their stats pushed. */
> +continue;
> +}
> +
> +struct dpi
Re: [ovs-dev] [ovs-dev v11] ofproto-dpif-upcall: fix push_dp_ops
On 6/23/23 14:20, Eelco Chaudron wrote:
>
>
> On 9 Jun 2023, at 17:03, Peng He wrote:
>
>> push_dp_ops only handles delete ops errors but ignores the modify
>> ops results. It's better to handle all the dp operation errors in
>> a consistent way.
>>
>> This patch prevents the inconsistency by considering modify failure
>> in revalidators.
>>
>> To note, we cannot perform two state transitions and change ukey_state
>> into UKEY_EVICTED directly here, because, if we do so, the
>> sweep will remove the ukey alone and leave dp flow alive. Later, the
>> dump will retrieve the dp flow and might even recover it. This will
>> contribute the stats of this dp flow twice.
>>
>> v8->v9: add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
>> v9->v10: change the commit message and refine the test case.
>> v10->v11: fix indentation and refine the test case.
>>
>> Signed-off-by: Peng He
>
> Thanks for making these changes, there is a merge conflict on the tests case
> and some small comments below.
>
> So maybe you can send a v12 and I’ll do a quick check and ACK it. What do you
> think?
>
> Cheers,
>
> Eelco
>
>
>> ---
>> ofproto/ofproto-dpif-upcall.c | 51 +--
>> tests/dpif-netdev.at | 44 ++
>> 2 files changed, 81 insertions(+), 14 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index cd57fdbd9..c920c749c 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -62,6 +62,7 @@ COVERAGE_DEFINE(upcall_flow_limit_hit);
>> COVERAGE_DEFINE(upcall_flow_limit_kill);
>> COVERAGE_DEFINE(upcall_ukey_contention);
>> COVERAGE_DEFINE(upcall_ukey_replace);
>> +COVERAGE_DEFINE(dumped_inconsistent_flow);
>
> Can you add this in alphabetical order?
>
>>
>> /* A thread that reads upcalls from dpif, forwards each upcall's packet,
>> * and possibly sets up a kernel flow as a cache. */
>> @@ -258,6 +259,7 @@ enum ukey_state {
>> UKEY_CREATED = 0,
>> UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is
>> queued. */
>> UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */
>> +UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is inconsistent.
>> */
>> UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued.
>> */
>> UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */
>> UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */
>> @@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
>> ukey_state dst,
>> * UKEY_VISIBLE -> UKEY_EVICTED
>> * A handler attempts to install the flow, but the datapath rejects it.
>> * Consider that the datapath has already destroyed it.
>> + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
>> + * A revalidator modifies the flow with error returns.
>> + * UKEY_INCONSISTENT -> UKEY_EVICTING
>> + * A revalidator decides to evict the datapath flow.
>> * UKEY_OPERATIONAL -> UKEY_EVICTING
>> * A revalidator decides to evict the datapath flow.
>> * UKEY_EVICTING-> UKEY_EVICTED
>> @@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum
>> ukey_state dst,
>> * UKEY_EVICTED -> UKEY_DELETED
>> * A revalidator has removed the ukey from the umap and is deleting it.
>> */
>> -if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
>> - dst < UKEY_DELETED)) {
>> +if (ukey->state == dst - 1 ||
>> + (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
>> + (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
>> ukey->state = dst;
>> } else {
>> struct ds ds = DS_EMPTY_INITIALIZER;
>> @@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
>> *ops, size_t n_ops)
>>
>> for (i = 0; i < n_ops; i++) {
>> struct ukey_op *op = &ops[i];
>> -struct dpif_flow_stats *push, *stats, push_buf;
>> -
>> -stats = op->dop.flow_del.stats;
>> -push = &push_buf;
>> -
>> -if (op->dop.type != DPIF_OP_FLOW_DEL) {
>> -/* Only deleted flows need their stats pushed. */
>> -continue;
>> -}
>>
>> 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);
>> +if (op->dop.type == DPIF_OP_FLOW_DEL) {
>> +transition_ukey(op->ukey, UKEY_EVICTED);
>> +} else {
>> +/* Modification of the flow failed */
>
> You forgot to copy the dot.
>
>> +transition_ukey(op->ukey, UKEY_INCONSISTENT);
>> +}
>> ovs_mutex_unlock(&op->ukey->mutex);
>> }
>> continue;
>>
Re: [ovs-dev] [ovs-dev v11] ofproto-dpif-upcall: fix push_dp_ops
Ilya Maximets 于2023年6月23日周五 20:40写道：
> On 6/23/23 14:20, Eelco Chaudron wrote:
> >
> >
> > On 9 Jun 2023, at 17:03, Peng He wrote:
> >
> >> push_dp_ops only handles delete ops errors but ignores the modify
> >> ops results. It's better to handle all the dp operation errors in
> >> a consistent way.
> >>
> >> This patch prevents the inconsistency by considering modify failure
> >> in revalidators.
> >>
> >> To note, we cannot perform two state transitions and change ukey_state
> >> into UKEY_EVICTED directly here, because, if we do so, the
> >> sweep will remove the ukey alone and leave dp flow alive. Later, the
> >> dump will retrieve the dp flow and might even recover it. This will
> >> contribute the stats of this dp flow twice.
> >>
> >> v8->v9: add testsuite and delete INCONSISTENT ukey at
> revalidate_sweep.
> >> v9->v10: change the commit message and refine the test case.
> >> v10->v11: fix indentation and refine the test case.
> >>
> >> Signed-off-by: Peng He
> >
> > Thanks for making these changes, there is a merge conflict on the tests
> case and some small comments below.
> >
> > So maybe you can send a v12 and I’ll do a quick check and ACK it. What
> do you think?
> >
> > Cheers,
> >
> > Eelco
> >
> >
> >> ---
> >> ofproto/ofproto-dpif-upcall.c | 51 +--
> >> tests/dpif-netdev.at | 44 ++
> >> 2 files changed, 81 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> >> index cd57fdbd9..c920c749c 100644
> >> --- a/ofproto/ofproto-dpif-upcall.c
> >> +++ b/ofproto/ofproto-dpif-upcall.c
> >> @@ -62,6 +62,7 @@ COVERAGE_DEFINE(upcall_flow_limit_hit);
> >> COVERAGE_DEFINE(upcall_flow_limit_kill);
> >> COVERAGE_DEFINE(upcall_ukey_contention);
> >> COVERAGE_DEFINE(upcall_ukey_replace);
> >> +COVERAGE_DEFINE(dumped_inconsistent_flow);
> >
> > Can you add this in alphabetical order?
> >
> >>
> >> /* A thread that reads upcalls from dpif, forwards each upcall's
> packet,
> >> * and possibly sets up a kernel flow as a cache. */
> >> @@ -258,6 +259,7 @@ enum ukey_state {
> >> UKEY_CREATED = 0,
> >> UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is
> queued. */
> >> UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is
> installed. */
> >> +UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is
> inconsistent. */
> >> UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is
> queued. */
> >> UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted.
> */
> >> UKEY_DELETED, /* Ukey removed from umap, ukey free is
> deferred. */
> >> @@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> >> * UKEY_VISIBLE -> UKEY_EVICTED
> >> * A handler attempts to install the flow, but the datapath
> rejects it.
> >> * Consider that the datapath has already destroyed it.
> >> + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
> >> + * A revalidator modifies the flow with error returns.
> >> + * UKEY_INCONSISTENT -> UKEY_EVICTING
> >> + * A revalidator decides to evict the datapath flow.
> >> * UKEY_OPERATIONAL -> UKEY_EVICTING
> >> * A revalidator decides to evict the datapath flow.
> >> * UKEY_EVICTING-> UKEY_EVICTED
> >> @@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> >> * UKEY_EVICTED -> UKEY_DELETED
> >> * A revalidator has removed the ukey from the umap and is
> deleting it.
> >> */
> >> -if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
> >> - dst < UKEY_DELETED)) {
> >> +if (ukey->state == dst - 1 ||
> >> + (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
> >> + (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
> >> ukey->state = dst;
> >> } else {
> >> struct ds ds = DS_EMPTY_INITIALIZER;
> >> @@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
> *ops, size_t n_ops)
> >>
> >> for (i = 0; i < n_ops; i++) {
> >> struct ukey_op *op = &ops[i];
> >> -struct dpif_flow_stats *push, *stats, push_buf;
> >> -
> >> -stats = op->dop.flow_del.stats;
> >> -push = &push_buf;
> >> -
> >> -if (op->dop.type != DPIF_OP_FLOW_DEL) {
> >> -/* Only deleted flows need their stats pushed. */
> >> -continue;
> >> -}
> >>
> >> 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);
> >> +if (op->dop.type == DPIF_OP_FLOW_DEL) {
> >> +transition_ukey(op->ukey, UKEY_EVICTED);
> >> +} else {
> >> +/* Modification of the flow f
