|
From: <sv...@va...> - 2015-01-31 00:30:03
|
Author: florian
Date: Sat Jan 31 00:29:50 2015
New Revision: 14898
Log:
Replace the SegName array with a simple string table.
The validity of this change follows from the following observations:
(1) There is a single source for allocating and storing segment names,
namely allocate_segname.
(2) For all invocations of allocate_segname the returned value (which
represents the segmant name) is assigned to NSegment::fnIdx.
(3) All but one assignments to NSegment::fnIdx assign allocate_segname.
The single exception assigns -1 in init_nsegment. That function is
called whenever a new segment (named or unnamed) is allocated.
For a segment name to become unused there must be an assignment to
NSegment::fnIdx which was previously assigned a return value from
allocate_segname. There is no such assignment.
It follows that all segment names are in use at all times, hence
SegName::inUse == True for all SegNames. So we can constant fold it
and don't need to represent it.
Pass 3 in preen_nsegments is obsolete as there are no segment names to
garbage collect.
Modified:
trunk/coregrind/m_aspacemgr/aspacemgr-linux.c
trunk/include/pub_tool_aspacemgr.h
Modified: trunk/coregrind/m_aspacemgr/aspacemgr-linux.c
==============================================================================
--- trunk/coregrind/m_aspacemgr/aspacemgr-linux.c (original)
+++ trunk/coregrind/m_aspacemgr/aspacemgr-linux.c Sat Jan 31 00:29:50 2015
@@ -1,3 +1,4 @@
+/* -*- mode: C; c-basic-offset: 3; -*- */
/*--------------------------------------------------------------------*/
/*--- The address space manager: segment initialisation and ---*/
@@ -290,25 +291,15 @@
# define VG_N_SEGNAMES 6000
#endif
-/* Max length of a segment file name. */
+/* Max length of a segment file name. FIXME: to be removed */
#define VG_MAX_SEGNAMELEN 1000
+/* String table for segment names */
-typedef
- struct {
- Bool inUse;
- Bool mark;
- HChar fname[VG_MAX_SEGNAMELEN];
- }
- SegName;
-
-/* Filename table. _used is the high water mark; an entry is only
- valid if its index >= 0, < _used, and its .inUse field == True.
- The .mark field is used to garbage-collect dead entries.
-*/
-static SegName segnames[VG_N_SEGNAMES];
-static Int segnames_used = 0;
-
+/* FIXME: This is just for backward compatibility for now. To be adjusted. */
+static HChar segnames[VG_N_SEGNAMES * VG_MAX_SEGNAMELEN];
+static SizeT segnames_used = 0; /* number of characters used */
+static UInt num_segnames = 0; /* number of names in string table */
/* Array [0 .. nsegments_used-1] of all mappings. */
/* Sorted by .addr field. */
@@ -390,60 +381,44 @@
/*-----------------------------------------------------------------*/
/*--- ---*/
-/*--- SegName array management. ---*/
+/*--- Segment name management. ---*/
/*--- ---*/
/*-----------------------------------------------------------------*/
-/* Searches the filename table to find an index for the given name.
- If none is found, an index is allocated and the name stored. If no
- space is available we just give up. If the string is too long to
- store, return -1.
+/* Searches the string table to find an index for the given name.
+ If none is found, an index is allocated and the name stored.
+ If the string is too long to store, return -1.
*/
static Int allocate_segname ( const HChar* name )
{
- Int i, j, len;
+ SizeT len, l, ix;
aspacem_assert(name);
if (0) VG_(debugLog)(0,"aspacem","allocate_segname %s\n", name);
len = VG_(strlen)(name);
- if (len + 1 > VG_MAX_SEGNAMELEN) {
- return -1;
- }
/* first see if we already have the name. */
- Int free_slot = -1;
- for (i = 0; i < segnames_used; i++) {
- if (!segnames[i].inUse) {
- free_slot = i;
- continue;
- }
- if (0 == VG_(strcmp)(name, &segnames[i].fname[0])) {
- return i;
- }
+ for (ix = 0; ix < segnames_used; ix += l + 1) {
+ l = VG_(strlen)(segnames + ix);
+ if (l == len && VG_(strcmp)(name, segnames + ix) == 0) return ix;
}
- /* no we don't. */
- if (free_slot >= 0) {
- /* we have a free slot; use it. */
- i = free_slot;
- } else {
- /* no free slots .. advance the high-water mark. */
- if (segnames_used < VG_N_SEGNAMES) {
- i = segnames_used;
- segnames_used++;
- } else {
- ML_(am_barf_toolow)("VG_N_SEGNAMES");
- }
+ /* Is there enough room in the string table? */
+ if (len + 1 > (sizeof segnames) - segnames_used) {
+ return -1;
}
+ ++num_segnames;
+
/* copy it in */
- segnames[i].inUse = True;
- for (j = 0; j < len; j++)
- segnames[i].fname[j] = name[j];
- segnames[i].fname[len] = 0;
- return i;
+ ix = segnames_used;
+
+ VG_(strcpy)(segnames + segnames_used, name);
+ segnames_used += len + 1;
+
+ return ix;
}
@@ -513,9 +488,8 @@
const HChar* name = "(none)";
if (seg->fnIdx >= 0 && seg->fnIdx < segnames_used
- && segnames[seg->fnIdx].inUse
- && segnames[seg->fnIdx].fname[0] != 0)
- name = segnames[seg->fnIdx].fname;
+ && segnames[seg->fnIdx] != 0)
+ name = segnames + seg->fnIdx;
show_len_concisely(len_buf, seg->start, seg->end);
@@ -606,14 +580,14 @@
void VG_(am_show_nsegments) ( Int logLevel, const HChar* who )
{
Int i;
+ SizeT ix;
VG_(debugLog)(logLevel, "aspacem",
- "<<< SHOW_SEGMENTS: %s (%d segments, %d segnames)\n",
- who, nsegments_used, segnames_used);
- for (i = 0; i < segnames_used; i++) {
- if (!segnames[i].inUse)
- continue;
+ "<<< SHOW_SEGMENTS: %s (%d segments, %u segnames)\n",
+ who, nsegments_used, num_segnames);
+ i = 0;
+ for (ix = 0; ix < segnames_used; ix += VG_(strlen)(segnames + ix) + 1) {
VG_(debugLog)(logLevel, "aspacem",
- "(%2d) %s\n", i, segnames[i].fname);
+ "(%2d) %s\n", i++, segnames + ix);
}
for (i = 0; i < nsegments_used; i++)
show_nsegment( logLevel, i, &nsegments[i] );
@@ -623,18 +597,13 @@
/* Get the filename corresponding to this segment, if known and if it
- has one. The returned name's storage cannot be assumed to be
- persistent, so the caller should immediately copy the name
- elsewhere. */
+ has one. */
const HChar* VG_(am_get_filename)( NSegment const * seg )
{
Int i;
aspacem_assert(seg);
i = seg->fnIdx;
- if (i < 0 || i >= segnames_used || !segnames[i].inUse)
- return NULL;
- else
- return &segnames[i].fname[0];
+ return (i < 0) ? NULL : segnames + i;
}
/* Collect up the start addresses of all non-free, non-resvn segments.
@@ -725,8 +694,7 @@
return
s->smode == SmFixed
&& (s->fnIdx == -1 ||
- (s->fnIdx >= 0 && s->fnIdx < segnames_used
- && segnames[s->fnIdx].inUse))
+ (s->fnIdx >= 0 && s->fnIdx < segnames_used))
&& !s->isCH;
case SkResvn:
@@ -807,7 +775,7 @@
static Bool preen_nsegments ( void )
{
- Int i, j, r, w, nsegments_used_old = nsegments_used;
+ Int i, r, w, nsegments_used_old = nsegments_used;
/* Pass 1: check the segment array covers the entire address space
exactly once, and also that each segment is sane. */
@@ -837,27 +805,6 @@
aspacem_assert(w > 0 && w <= nsegments_used);
nsegments_used = w;
- /* Pass 3: free up unused string table slots */
- /* clear mark bits */
- for (i = 0; i < segnames_used; i++)
- segnames[i].mark = False;
- /* mark */
- for (i = 0; i < nsegments_used; i++) {
- j = nsegments[i].fnIdx;
- aspacem_assert(j >= -1 && j < segnames_used);
- if (j >= 0) {
- aspacem_assert(segnames[j].inUse);
- segnames[j].mark = True;
- }
- }
- /* release */
- for (i = 0; i < segnames_used; i++) {
- if (segnames[i].mark == False) {
- segnames[i].inUse = False;
- segnames[i].fname[0] = 0;
- }
- }
-
return nsegments_used != nsegments_used_old;
}
Modified: trunk/include/pub_tool_aspacemgr.h
==============================================================================
--- trunk/include/pub_tool_aspacemgr.h (original)
+++ trunk/include/pub_tool_aspacemgr.h Sat Jan 31 00:29:50 2015
@@ -137,11 +137,7 @@
extern NSegment const * VG_(am_find_nsegment) ( Addr a );
/* Get the filename corresponding to this segment, if known and if it
- has one. The returned name's storage cannot be assumed to be
- persistent, so the caller should immediately copy the name
- elsewhere. This may return NULL if the file name is not known or
- for arbitrary other implementation-dependent reasons, so callers
- need to be able to handle a NULL return value. */
+ has one. The function may return NULL if the file name is not known. */
extern const HChar* VG_(am_get_filename)( NSegment const * );
/* Is the area [start .. start+len-1] validly accessible by the
|
|
From: Florian K. <fl...@ei...> - 2015-02-02 17:21:53
|
On 31.01.2015 01:29, sv...@va... wrote:
>
> For a segment name to become unused there must be an assignment to
> NSegment::fnIdx which was previously assigned a return value from
> allocate_segname.
Yes.
> There is no such assignment.
Wrong. I missed this assignment (in pass #2 of preen_segments):
nsegments[w] = nsegments[r];
This happens when two named segments can be merged. One would expect (at
least I would) that only named segments with the same name can be merged
but that is not the case. See thread entitled "a question about
maybe_merge_segments" of Nov 11, 2014.
So with r14898 we could have an occasional segment name leak. I did a
few experiments. No named segments were merged (I have never see that
happen).
#segments #segnames SegName size string table size
abiword 1175 290 290580 14203
vlc 960 665 666330 33171
avidemux 1049 243 243486 12338
All sizes in bytes.
The SegName size is sizeof(SegName) * #segment names.
For all experiments the savings is approx 95%
That looks like significant enough improvement to me to allow an
occasional, if any, leak of a segment name.
Florian
|
|
From: Julian S. <js...@ac...> - 2015-02-03 13:39:25
|
On 02/02/15 18:21, Florian Krohm wrote: > Wrong. I missed this assignment (in pass #2 of preen_segments): > > nsegments[w] = nsegments[r]; I did wonder about this, yes. > That looks like significant enough improvement to me to allow an > occasional, if any, leak of a segment name. Hmm, the problem is, that gives us a new, if obscure, failure mode that didn't exist before. If a long running application dlopens and dlcloses some .so, is it possible to run out of string table space now? J |
|
From: Florian K. <fl...@ei...> - 2015-02-03 16:18:46
|
On 03.02.2015 14:39, Julian Seward wrote: > > Hmm, the problem is, that gives us a new, if obscure, failure mode that > didn't exist before. If a long running application dlopens and dlcloses > some .so, is it possible to run out of string table space now? Sure. On the other hand there is no longer a limit in the number of segnames that you can have (other than exhausting the string table). The use case you describe is perhaps a corner case which will have more pressing issues other than running out of segment names; see https://bugs.kde.org/show_bug.cgi?id=79362 But your point is an interesting one. And I will keep it in mind. I began looking at the aspace mgr because I would like to get rid of those hard limits: VG_N_SEGMENTS and string table size. I tend to think that should be doable because we can dynamically allocate memory once we're done with VG_(am_startup) but I need to learn more about it first. Florian |
|
From: Philippe W. <phi...@sk...> - 2015-02-04 20:17:13
|
On Tue, 2015-02-03 at 17:18 +0100, Florian Krohm wrote: > On 03.02.2015 14:39, Julian Seward wrote: > > > > Hmm, the problem is, that gives us a new, if obscure, failure mode that > > didn't exist before. If a long running application dlopens and dlcloses > > some .so, is it possible to run out of string table space now? > > Sure. On the other hand there is no longer a limit in the number of > segnames that you can have (other than exhausting the string table). > The use case you describe is perhaps a corner case which will have more > pressing issues other than running out of segment names; see > https://bugs.kde.org/show_bug.cgi?id=79362 > But your point is an interesting one. And I will keep it in mind. If I understood well, the leak happens only when merging 2 named segments having a different name, but same device nr/inode nr. It looks to me it might be better to just not merge these segments. Not merging these has the following advantages: 1. The linux kernel already behaves like that : it does not merge such segments in /proc/<pid>/maps In other words, /proc/<pid>/maps consider different filenames as different 'entries' in the map 2. in case an error is reported for such a mmap-ed address, then it will be somewhat wrongly/surprisingly shown as belonging to another filename than the mmaped filename, if some merging was done. Such merging might or might not happen, depending on how valgrind choosed where to mmap the 2 filenames. 3. not merging solves this memleak. If we do not do the merge, then we have to ensure aspacemgr selfcheck uses the same checking rules as the rules used for merging. If not, it will assert, and we fix :) Philippe |
|
From: Florian K. <fl...@ei...> - 2015-02-05 17:20:49
|
On 04.02.2015 21:18, Philippe Waroquiers wrote: > 2. in case an error is reported for such a mmap-ed address, > then it will be somewhat wrongly/surprisingly shown as belonging > to another filename than the mmaped filename, if some merging was > done. Such merging might or might not happen, depending > on how valgrind choosed where to mmap the 2 filenames. Yes, that was my argument. The dead-beat argument against it was that the code might rely on segments being maximally merged - of which we do not know whether this is so. > 3. not merging solves this memleak. Actually, if we ever want to fix https://bugs.kde.org/show_bug.cgi?id=79362 then we will have to store the segment name somewhere, because we might want it after the shared object has been unloaded. Florian |