The comment over the trylock exceeds the line limit.
I'm not sure if this is the right patch to fix it or not, but I don't
like how we're overloading the meaning of the 'mark'. I think the
code would be a lot clearer if we had both a "already_dumped" flag and
a "delete" flag on every ukey. What do you think?
Acked-by: Ethan Jackson <et...@nicira.com>
On Thu, May 29, 2014 at 6:38 PM, Joe Stringer <joestrin...@nicira.com> wrote:
> A series of bugs have been identified recently that are caused by a
> combination of the awkward flow dump API, possibility of duplicate flows
> in a flow dump, and premature optimisation of the revalidator logic.
> This patch attempts to simplify the revalidator logic by combining
> multiple critical sections into one, which should make the state more
> consistent.
>
> The new flow of logic is:
> + Lookup the ukey.
> + If the ukey doesn't exist, create it.
> + Insert the ukey into the udpif. If we can't insert it, skip this flow.
> + Lock the ukey. If we can't lock it, skip it.
> + Determine if the ukey was already handled. If it has, skip it.
> + Revalidate.
> + Update ukey's fields (mark, flow_exists).
> + Unlock the ukey.
>
> Previously, we would attempt process a flow without creating a ukey if
> it hadn't been dumped before and it was due to be deleted. This patch
> changes this to always create a ukey, allowing the ukey's
> mutex to be used as the basis for preventing a flow from being handled
> twice. This improves code correctness and readability.
>
> Bug #1252997.
>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> The original version of this patch has been merged to branch-2.3, and
> noted a potential performance drop. After testing this patch against
> master in netperf TCP_CRR, there was no measurable difference so I have
> dropped that comment from the commit message.
> ---
> ofproto/ofproto-dpif-upcall.c | 74
> ++++++++++++++++++-----------------------
> 1 file changed, 33 insertions(+), 41 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index db0f17e..1c82b6b 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1097,6 +1097,7 @@ should_revalidate(uint64_t packets, long long int used)
> static bool
> revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
> const struct dpif_flow *f)
> + OVS_REQUIRES(ukey->mutex)
> {
> uint64_t slow_path_buf[128 / 8];
> struct xlate_out xout, *xoutp;
> @@ -1117,7 +1118,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> xoutp = NULL;
> netflow = NULL;
>
> - ovs_mutex_lock(&ukey->mutex);
> last_used = ukey->stats.used;
> push.used = f->stats.used;
> push.tcp_flags = f->stats.tcp_flags;
> @@ -1128,11 +1128,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> ? f->stats.n_bytes - ukey->stats.n_bytes
> : 0);
>
> - if (!ukey->flow_exists) {
> - /* Don't bother revalidating if the flow was already deleted. */
> - goto exit;
> - }
> -
> if (udpif->need_revalidate && last_used
> && !should_revalidate(push.n_packets, last_used)) {
> ok = false;
> @@ -1212,7 +1207,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> ok = true;
>
> exit:
> - ovs_mutex_unlock(&ukey->mutex);
> if (netflow) {
> if (!ok) {
> netflow_expire(netflow, &flow);
> @@ -1371,54 +1365,52 @@ revalidate(struct revalidator *revalidator)
> uint32_t hash = hash_bytes(f->key, f->key_len, udpif->secret);
> struct udpif_key *ukey = ukey_lookup(udpif, f->key, f->key_len,
> hash);
> - bool mark;
> -
> - if (ukey) {
> - bool already_dumped;
> -
> - ovs_mutex_lock(&ukey->mutex);
> - already_dumped = ukey->mark || !ukey->flow_exists;
> - if (!used) {
> - used = ukey->created;
> - }
> - ovs_mutex_unlock(&ukey->mutex);
> -
> - if (already_dumped) {
> - /* The flow has already been dumped. This can
> occasionally
> - * occur if the datapath is changed in the middle of a
> flow
> - * dump. Rather than perform the same work twice, skip
> the
> - * flow this time. */
> + bool already_dumped, mark;
> +
> + if (!ukey) {
> + ukey = ukey_create(key, key_len, used);
> + if (!udpif_insert_ukey(udpif, ukey, hash)) {
> + /* The same ukey has already been created. This means
> that
> + * another revalidator is processing this flow
> + * concurrently, so don't bother processing it. */
> COVERAGE_INC(upcall_duplicate_flow);
> + ukey_delete(NULL, ukey);
> continue;
> }
> }
>
> + if (ovs_mutex_trylock(&ukey->mutex)) {
> + /* The flow has been dumped, and is being handled by another
> + * revalidator concurrently. This can occasionally occur if
> the
> + * datapath is changed in the middle of a flow dump. Rather
> than
> + * perform the same work twice, skip the flow this time. */
> + COVERAGE_INC(upcall_duplicate_flow);
> + continue;
> + }
> +
> + already_dumped = ukey->mark || !ukey->flow_exists;
> + if (already_dumped) {
> + /* The flow has already been dumped and handled by another
> + * revalidator during this flow dump operation. Skip it. */
> + COVERAGE_INC(upcall_duplicate_flow);
> + ovs_mutex_unlock(&ukey->mutex);
> + continue;
> + }
> +
> + if (!used) {
> + used = ukey->created;
> + }
> if (kill_them_all || (used && used < now - max_idle)) {
> mark = false;
> } else {
> - if (!ukey) {
> - ukey = ukey_create(f->key, f->key_len, used);
> - if (!udpif_insert_ukey(udpif, ukey, hash)) {
> - /* The same ukey has already been created. This means
> - * that another revalidator is processing this flow
> - * concurrently, so don't bother processing it. */
> - ukey_delete(NULL, ukey);
> - continue;
> - }
> - }
> -
> mark = revalidate_ukey(udpif, ukey, f);
> }
> -
> - if (ukey) {
> - ovs_mutex_lock(&ukey->mutex);
> - ukey->mark = ukey->flow_exists = mark;
> - ovs_mutex_unlock(&ukey->mutex);
> - }
> + ukey->mark = ukey->flow_exists = mark;
>
> if (!mark) {
> dump_op_init(&ops[n_ops++], f->key, f->key_len, ukey);
> }
> + ovs_mutex_unlock(&ukey->mutex);
> }
>
> if (n_ops) {
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
