[ovs-dev] [PATCH 2/5] upcall: Defer ukey deletion until after pushing stats.

[Joe Stringer]

I suspect you may have found that the later patches make use of this
structure in more locations?

Either way, fine by me.


On 25 February 2014 15:16, Ben Pfaff <blp at nicira.com> wrote:

> On Tue, Feb 11, 2014 at 01:55:33PM -0800, Joe Stringer wrote:
> > It is possible for a datapath to dump the same flow twice, for instance
> > if the flow is the last in a batch of flows to be dumped, then a new
> > flow is inserted into the same bucket before the flow dumper fetches
> > another batch.
> >
> > In this case, datapath flow stats may be duplicated: The revalidator
> > records the stats from the first flow, using the ukey to get the stats
> > delta. The ukey is deleted, then the revalidator reads the second
> > (duplicate) flow and cannot lookup the ukey for the delta. As such, it
> > will push the stats as-is.
> >
> > This patch reduces the likelihood of such stats duplications by
> > deferring ukey deletion until after stats are pushed for deleted flows.
> >
> > Signed-off-by: Joe Stringer <joestringer at nicira.com>
>
> Thanks, I'm adding this to my queue for application.
>
> Since struct dump_op is only used inside revalidate_udumps() I decided
> to move the declaration inside revalidate_udumps() too, for clarity:
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 254a59d..18784ec 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1376,18 +1376,18 @@ exit:
>      return ok;
>  }
>
> -struct dump_op {
> -    struct udpif_key *ukey;
> -    struct udpif_flow_dump *udump;
> -    struct dpif_flow_stats stats;         /* Stats for 'op'. */
> -    struct dpif_op op;                    /* Flow del operation. */
> -};
> -
>  static void
>  revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
>  {
>      struct udpif *udpif = revalidator->udpif;
>
> +    struct dump_op {
> +        struct udpif_key *ukey;
> +        struct udpif_flow_dump *udump;
> +        struct dpif_flow_stats stats; /* Stats for 'op'. */
> +        struct dpif_op op;            /* Flow del operation. */
> +    };
> +
>      struct dump_op ops[REVALIDATE_MAX_BATCH];
>      struct dpif_op *opsp[REVALIDATE_MAX_BATCH];
>      struct udpif_flow_dump *udump, *next_udump;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140225/8dbdae4c/attachment-0005.html>