I pushed this series to master, thank you for looking it over.
On Thu, Mar 29, 2012 at 01:46:46PM -0700, Ben Pfaff wrote:
> valgrind basically acts like a mark/sweep garbage collector. If it
> can arrive at memory block M by at least one path that follows only
> pointers to the start of memory blocks, then it considers M to
> definitely be reachable. If it can arrive at memory block M only by
> paths that include a pointer to the interior of a memory block, then
> it considers M to be "possibly leaked". So we don't need every
> hmap_node or list to be at the beginning of a block, only ones along a
> tree starting from the root set.
>
> (I don't know whether that helps enough, but it was meant to.)
>
> On Thu, Mar 29, 2012 at 12:22:47PM -0700, Ethan Jackson wrote:
> > I'm not sure I fully understand this one. Does valgrind always report
> > a warning for structures with hmap_nodes or list_nodes that aren't at
> > the front? If not what are the conditions cause the leak to be
> > reported? If so, how can this possibly work for structures that have
> > both an hmap_node or list_node?
> >
> > At any rate, the patch seems fine to me. I'm just curious.
> >
> > Ethan
> >
> > On Wed, Mar 28, 2012 at 14:58, Ben Pfaff <b...@nicira.com> wrote:
> > > valgrind's memory leak detector considers a pointer to the head of a
> > > memory
> > > block to be "definitely" a pointer to that memory block but a pointer to
> > > the interior of a memory block only "possibly" a pointer to that memory
> > > block. Open vSwitch hmap_node and list data structures can go anywhere
> > > inside a structure; if they are in the middle of a structure then valgrind
> > > considers pointers to them to be possible leaks. Therefore, this commit
> > > moves some of these from the middle of data structures to the head, to
> > > reduce valgrind's uncertainty.
> > >
> > > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > > ---
> > > ofproto/ofproto-dpif.c | 2 +-
> > > ofproto/ofproto-provider.h | 6 +++---
> > > ovsdb/jsonrpc-server.c | 4 ++--
> > > ovsdb/row.h | 4 ++--
> > > ovsdb/server.h | 12 ++++++------
> > > vswitchd/bridge.c | 2 +-
> > > 6 files changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > > index f2b9339..51b847f 100644
> > > --- a/ofproto/ofproto-dpif.c
> > > +++ b/ofproto/ofproto-dpif.c
> > > @@ -145,8 +145,8 @@ static void update_mirror_stats(struct ofproto_dpif
> > > *ofproto,
> > > uint64_t packets, uint64_t bytes);
> > >
> > > struct ofbundle {
> > > - struct ofproto_dpif *ofproto; /* Owning ofproto. */
> > > struct hmap_node hmap_node; /* In struct ofproto's "bundles" hmap. */
> > > + struct ofproto_dpif *ofproto; /* Owning ofproto. */
> > > void *aux; /* Key supplied by ofproto's client. */
> > > char *name; /* Identifier for log messages. */
> > >
> > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > > index e540850..26904ef 100644
> > > --- a/ofproto/ofproto-provider.h
> > > +++ b/ofproto/ofproto-provider.h
> > > @@ -36,10 +36,10 @@ struct ofputil_flow_mod;
> > > * With few exceptions, ofproto implementations may look at these fields
> > > but
> > > * should not modify them. */
> > > struct ofproto {
> > > + struct hmap_node hmap_node; /* In global 'all_ofprotos' hmap. */
> > > const struct ofproto_class *ofproto_class;
> > > char *type; /* Datapath type. */
> > > char *name; /* Datapath name. */
> > > - struct hmap_node hmap_node; /* In global 'all_ofprotos' hmap. */
> > >
> > > /* Settings. */
> > > uint64_t fallback_dpid; /* Datapath ID if no better choice found.
> > > */
> > > @@ -94,8 +94,8 @@ struct ofport *ofproto_get_port(const struct ofproto *,
> > > uint16_t ofp_port);
> > > * With few exceptions, ofproto implementations may look at these fields
> > > but
> > > * should not modify them. */
> > > struct ofport {
> > > - struct ofproto *ofproto; /* The ofproto that contains this port.
> > > */
> > > struct hmap_node hmap_node; /* In struct ofproto's "ports" hmap. */
> > > + struct ofproto *ofproto; /* The ofproto that contains this port.
> > > */
> > > struct netdev *netdev;
> > > struct ofputil_phy_port pp;
> > > uint16_t ofp_port; /* OpenFlow port number. */
> > > @@ -156,8 +156,8 @@ struct oftable {
> > > * With few exceptions, ofproto implementations may look at these fields
> > > but
> > > * should not modify them. */
> > > struct rule {
> > > - struct ofproto *ofproto; /* The ofproto that contains this rule.
> > > */
> > > struct list ofproto_node; /* Owned by ofproto base code. */
> > > + struct ofproto *ofproto; /* The ofproto that contains this rule.
> > > */
> > > struct cls_rule cr; /* In owning ofproto's classifier. */
> > >
> > > struct ofoperation *pending; /* Operation now in progress, if
> > > nonnull. */
> > > diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> > > index 9e6ed25..1848bb9 100644
> > > --- a/ovsdb/jsonrpc-server.c
> > > +++ b/ovsdb/jsonrpc-server.c
> > > @@ -1,4 +1,4 @@
> > > -/* Copyright (c) 2009, 2010, 2011 Nicira Networks
> > > +/* Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks
> > > *
> > > * Licensed under the Apache License, Version 2.0 (the "License");
> > > * you may not use this file except in compliance with the License.
> > > @@ -295,9 +295,9 @@ ovsdb_jsonrpc_server_wait(struct ovsdb_jsonrpc_server
> > > *svr)
> > > /* JSON-RPC database server session. */
> > >
> > > struct ovsdb_jsonrpc_session {
> > > + struct list node; /* Element in remote's sessions list. */
> > > struct ovsdb_session up;
> > > struct ovsdb_jsonrpc_remote *remote;
> > > - struct list node; /* Element in remote's sessions list. */
> > >
> > > /* Triggers. */
> > > struct hmap triggers; /* Hmap of "struct
> > > ovsdb_jsonrpc_trigger"s. */
> > > diff --git a/ovsdb/row.h b/ovsdb/row.h
> > > index 2fdc72e..306a56d 100644
> > > --- a/ovsdb/row.h
> > > +++ b/ovsdb/row.h
> > > @@ -1,4 +1,4 @@
> > > -/* Copyright (c) 2009, 2010, 2011 Nicira Networks
> > > +/* Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks
> > > *
> > > * Licensed under the Apache License, Version 2.0 (the "License");
> > > * you may not use this file except in compliance with the License.
> > > @@ -43,8 +43,8 @@ struct ovsdb_weak_ref {
> > >
> > > /* A row in a database table. */
> > > struct ovsdb_row {
> > > - struct ovsdb_table *table; /* Table to which this belongs. */
> > > struct hmap_node hmap_node; /* Element in ovsdb_table's 'rows'
> > > hmap. */
> > > + struct ovsdb_table *table; /* Table to which this belongs. */
> > > struct ovsdb_txn_row *txn_row; /* Transaction that row is in, if any.
> > > */
> > >
> > > /* Weak references. */
> > > diff --git a/ovsdb/server.h b/ovsdb/server.h
> > > index a9285f7..17e4222 100644
> > > --- a/ovsdb/server.h
> > > +++ b/ovsdb/server.h
> > > @@ -1,4 +1,4 @@
> > > -/* Copyright (c) 2011 Nicira Networks
> > > +/* Copyright (c) 2011, 2012 Nicira Networks
> > > *
> > > * Licensed under the Apache License, Version 2.0 (the "License");
> > > * you may not use this file except in compliance with the License.
> > > @@ -39,9 +39,9 @@ struct ovsdb_lock_waiter *ovsdb_session_get_lock_waiter(
> > > * A lock always has one or more "lock waiters" kept on a list. The
> > > waiter at
> > > * the head of the list owns the lock. */
> > > struct ovsdb_lock {
> > > + struct hmap_node hmap_node; /* In ovsdb_server's "locks" hmap. */
> > > struct ovsdb_server *server; /* The containing server. */
> > > char *name; /* Unique name. */
> > > - struct hmap_node hmap_node; /* In ovsdb_server's "locks" hmap. */
> > > struct list waiters; /* Contains "struct ovsdb_lock_waiter"s.
> > > */
> > > };
> > >
> > > @@ -55,14 +55,14 @@ enum ovsdb_lock_mode {
> > >
> > > /* A session's request for a database lock. */
> > > struct ovsdb_lock_waiter {
> > > + struct hmap_node session_node; /* In ->session->locks's hmap. */
> > > + struct ovsdb_lock *lock; /* The lock being waited for. */
> > > +
> > > enum ovsdb_lock_mode mode;
> > > char *lock_name;
> > >
> > > - struct ovsdb_lock *lock; /* The lock being waited for. */
> > > - struct list lock_node; /* In ->lock->waiters's list. */
> > > -
> > > struct ovsdb_session *session;
> > > - struct hmap_node session_node; /* In ->session->locks's hmap. */
> > > + struct list lock_node; /* In ->lock->waiters's list. */
> > > };
> > >
> > > struct ovsdb_session *ovsdb_lock_waiter_remove(struct ovsdb_lock_waiter
> > > *);
> > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > > index e15d57b..adc3b47 100644
> > > --- a/vswitchd/bridge.c
> > > +++ b/vswitchd/bridge.c
> > > @@ -87,8 +87,8 @@ struct mirror {
> > > };
> > >
> > > struct port {
> > > - struct bridge *bridge;
> > > struct hmap_node hmap_node; /* Element in struct bridge's "ports"
> > > hmap. */
> > > + struct bridge *bridge;
> > > char *name;
> > >
> > > const struct ovsrec_port *cfg;
> > > --
> > > 1.7.2.5
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
