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