[ovs-dev] [leaks 5/5] Rearrange structures to better fit valgrind's memory leak heuristics.
Ben Pfaff
blp at nicira.com
Thu Mar 29 20:46:46 UTC 2012
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 <blp at 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 <blp at 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 at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list