[ovs-dev] [leaks 5/5] Rearrange structures to better fit valgrind's memory leak heuristics.
Ethan Jackson
ethan at nicira.com
Thu Mar 29 20:51:54 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.
Yep this makes sense to me. This approach wouldn't work if we had
structures with leading hmap_node's which weren't in use for periods
of time during the life of the structure, but that's probably
indicative of a bug (or at least a bad design), so this seems fine to
me.
Ethan
>
> (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