Ah, great. The original refactoring was mostly split out from the patchset
to get rid of the flow_dumper thread, which made it easier to split these
statistics fixes out. I hadn't realised there was a bit more that we could
do.
On 25 February 2014 15:51, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Feb 25, 2014 at 03:41:36PM -0800, Ben Pfaff wrote:
> > On Tue, Feb 25, 2014 at 03:25:35PM -0800, Ben Pfaff wrote:
> > > On Tue, Feb 11, 2014 at 01:55:34PM -0800, Joe Stringer wrote:
> > > > This splits out functions for re-use by later patches, and compacts
> the
> > > > udump revalidation code.
> > > >
> > > > Co-authored-by: Ethan Jackson <et...@nicira.com>
> > > > Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> > >
> > > I have this queued up to apply, thanks!
> >
> > It looks like we can avoid more code duplication in the following
> > patch by folding in the following. I'm tentatively doing that.
>
> Ditto for the following.
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 1bced67..343181b 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1412,8 +1412,10 @@ dump_op_init(struct dump_op *op, const struct
> nlattr *key, size_t key_len,
> }
>
> static void
> -push_dump_ops(struct udpif *udpif, struct dump_op *ops, size_t n_ops)
> +push_dump_ops(struct revalidator *revalidator,
> + struct dump_op *ops, size_t n_ops)
> {
> + struct udpif *udpif = revalidator->udpif;
> struct dpif_op *opsp[REVALIDATE_MAX_BATCH];
> size_t i;
>
> @@ -1463,6 +1465,18 @@ push_dump_ops(struct udpif *udpif, struct dump_op
> *ops, size_t n_ops)
> }
> }
> }
> +
> + for (i = 0; i < n_ops; i++) {
> + struct udpif_key *ukey = ops[i].ukey;
> +
> + /* Look up the ukey to prevent double-free in case 'ops' contains
> a
> + * given ukey more than once (which can happen if the datapath
> dumps a
> + * given flow more than once). */
> + ukey = ukey_lookup(revalidator, ops[i].udump);
> + if (ukey) {
> + ukey_delete(revalidator, ukey);
> + }
> + }
> }
>
> static void
> @@ -1472,7 +1486,7 @@ revalidate_udumps(struct revalidator *revalidator,
> struct list *udumps)
>
> struct dump_op ops[REVALIDATE_MAX_BATCH];
> struct udpif_flow_dump *udump, *next_udump;
> - size_t n_ops, i, n_flows;
> + size_t n_ops, n_flows;
> unsigned int flow_limit;
> long long int max_idle;
> bool must_del;
> @@ -1524,17 +1538,7 @@ revalidate_udumps(struct revalidator *revalidator,
> struct list *udumps)
> free(udump);
> }
>
> - push_dump_ops(udpif, ops, n_ops);
> - for (i = 0; i < n_ops; i++) {
> - struct udpif_key *ukey = ops[i].ukey;
> -
> - /* Look up the ukey to prevent double-free if the datapath dumps
> the
> - * same flow twice. */
> - ukey = ukey_lookup(revalidator, ops[i].udump);
> - if (ukey) {
> - ukey_delete(revalidator, ukey);
> - }
> - }
> + push_dump_ops(revalidator, ops, n_ops);
>
> LIST_FOR_EACH_SAFE (udump, next_udump, list_node, udumps) {
> list_remove(&udump->list_node);
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
