Skip to content

Commit

Permalink
Remove nil error return while adding mapping
Browse files Browse the repository at this point in the history 
commit_hash:7a9ba588997108d0094d8027acf18084ed811b96
  • Loading branch information
@pashagoose
pashagoose committed Feb 13, 2025
1 parent d5a29ef commit 3afadd9
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 65 deletions.
26 changes: 10 additions & 16 deletions perforator/agent/collector/pkg/dso/map.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,20 +153,17 @@ func (d *Registry) getMappingCount() int {
return len(d.dsos)
}

func (d *Registry) trackingFetch(id string, ttl time.Duration, cb func() (*dso, error)) (ccache.TrackedItem[*dso], error) {
func (d *Registry) trackingFetch(id string, ttl time.Duration, cb func() *dso) ccache.TrackedItem[*dso] {
item := d.cache.TrackingGet(id)
if item != nil {
// item can be .Expired() now.
// It is ok to return stale unwind tables.
return item, nil
return item
}

value, err := cb()
if err != nil {
return nil, err
}
value := cb()

return d.cache.TrackingSet(id, value, ttl), nil
return d.cache.TrackingSet(id, value, ttl)
}

func (d *Registry) acquireIfExists(buildID string) *refCountedDSO {
Expand Down Expand Up @@ -194,23 +191,20 @@ func (d *Registry) ensure(buildID string, newDSO *refCountedDSO) (*refCountedDSO
}

// Return existing DSO if it exists, otherwise build unwind table and store new DSO
func (d *Registry) register(ctx context.Context, buildInfo *xelf.BuildInfo, file binary.UnsealedFile) (*dso, error) {
func (d *Registry) register(ctx context.Context, buildInfo *xelf.BuildInfo, file binary.UnsealedFile) *dso {
buildID := buildInfo.BuildID

rcdso := d.acquireIfExists(buildID)
if rcdso != nil {
return rcdso.dso, nil
return rcdso.dso
}

item, err := d.trackingFetch(buildID, 10*time.Minute, func() (*dso, error) {
item := d.trackingFetch(buildID, 10*time.Minute, func() *dso {
return &dso{
ID: d.nextid.Add(1) - 1,
buildInfo: buildInfo,
}, nil
}
})
if err != nil {
return nil, err
}

newDSO := &refCountedDSO{
dso: item.Value(),
Expand All @@ -221,7 +215,7 @@ func (d *Registry) register(ctx context.Context, buildInfo *xelf.BuildInfo, file
dso, inserted := d.ensure(buildID, newDSO)
if !inserted {
item.Release()
return dso.dso, nil
return dso.dso
}

if file != nil {
Expand All @@ -234,7 +228,7 @@ func (d *Registry) register(ctx context.Context, buildInfo *xelf.BuildInfo, file
log.UInt64("id", newDSO.ID),
)

return newDSO.dso, nil
return newDSO.dso
}

func (d *Registry) release(ctx context.Context, buildID string) {
Expand Down
10 changes: 3 additions & 7 deletions perforator/agent/collector/pkg/dso/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,23 +132,19 @@ func (d *Storage) removeMapping(ctx context.Context, vmap *versionedMapping) {
// Add mapping for the process.
// Thread safe.
// Atomic relative to RemoveProcess
func (d *Storage) AddMapping(ctx context.Context, pid linux.ProcessID, mapping Mapping, binary binary.UnsealedFile) (*dso, error) {
func (d *Storage) AddMapping(ctx context.Context, pid linux.ProcessID, mapping Mapping, binary binary.UnsealedFile) *dso {
var dso *dso
var err error

if mapping.BuildInfo != nil {
dso, err = d.registry.register(ctx, mapping.BuildInfo, binary)
if err != nil {
return nil, err
}
dso = d.registry.register(ctx, mapping.BuildInfo, binary)
mapping.DSO = dso
}

maps := d.ensureProcess(pid, true /*=lock*/)
defer maps.lock.Unlock()
maps.addMappingLocked(mapping)

return dso, nil
return dso
}

// Remove unused process mappings.
Expand Down
47 changes: 13 additions & 34 deletions perforator/agent/collector/pkg/dso/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,10 @@ func TestRegistry_Simple(t *testing.T) {
buildID := "abacaba"
buildInfo := &xelf.BuildInfo{BuildID: buildID}

_, err = registry.register(ctx, buildInfo, nil)
require.NoError(t, err)

registry.register(ctx, buildInfo, nil)
require.Equal(t, buildID, registry.get(buildID).buildInfo.BuildID)

_, err = registry.register(ctx, buildInfo, nil)
require.NoError(t, err)
registry.register(ctx, buildInfo, nil)
require.Equal(t, 1, registry.getMappingCount())

registry.release(ctx, buildID)
Expand Down Expand Up @@ -77,10 +74,7 @@ func TestRegistry_Concurrent(t *testing.T) {
for i := 0; i < iterations; i++ {
buildID := buildIDs[i%len(buildIDs)]
buildInfo := &xelf.BuildInfo{BuildID: buildID}
_, err := registry.register(ctx, buildInfo, nil)
if err != nil {
return err
}
registry.register(ctx, buildInfo, nil)
}

return nil
Expand Down Expand Up @@ -174,7 +168,7 @@ func TestStorage_OneMapping(t *testing.T) {
)
require.NoError(t, err)

_, err = storage.AddMapping(
storage.AddMapping(
ctx,
123,
Mapping{Mapping: procfs.Mapping{
Expand All @@ -186,7 +180,6 @@ func TestStorage_OneMapping(t *testing.T) {
}},
nil,
)
require.NoError(t, err)
var loc *Location

_, err = storage.ResolveAddress(ctx, 123, 0xdeadadd7e55)
Expand Down Expand Up @@ -235,7 +228,7 @@ func TestStorage_OverlappingMappings(t *testing.T) {
)
require.NoError(t, err)

_, err = storage.AddMapping(
storage.AddMapping(
ctx,
linux.ProcessID(0),
Mapping{
Expand All @@ -250,8 +243,7 @@ func TestStorage_OverlappingMappings(t *testing.T) {
},
nil,
)
require.NoError(t, err)
_, err = storage.AddMapping(
storage.AddMapping(
ctx,
linux.ProcessID(0),
Mapping{
Expand All @@ -266,9 +258,8 @@ func TestStorage_OverlappingMappings(t *testing.T) {
},
nil,
)
require.NoError(t, err)

_, err = storage.AddMapping(
storage.AddMapping(
ctx,
linux.ProcessID(0),
Mapping{
Expand All @@ -283,7 +274,6 @@ func TestStorage_OverlappingMappings(t *testing.T) {
},
nil,
)
require.NoError(t, err)

loc, err := storage.ResolveAddress(ctx, linux.ProcessID(0), 600)
require.NoError(t, err)
Expand Down Expand Up @@ -348,8 +338,7 @@ func TestStorage_MultipleProcsAndMappings(t *testing.T) {
},
}
for _, mapping := range mappings {
_, err := storage.AddMapping(ctx, linux.ProcessID(0), mapping, nil)
require.NoError(t, err)
storage.AddMapping(ctx, linux.ProcessID(0), mapping, nil)
}

loc, err := storage.ResolveAddress(ctx, linux.ProcessID(0), 8400)
Expand All @@ -368,8 +357,7 @@ func TestStorage_MultipleProcsAndMappings(t *testing.T) {

newMappings := []Mapping{mappings[0], mappings[1]}
for _, mapping := range newMappings {
_, err = storage.AddMapping(ctx, linux.ProcessID(1), mapping, nil)
require.NoError(t, err)
storage.AddMapping(ctx, linux.ProcessID(1), mapping, nil)
}

loc, err = storage.ResolveAddress(ctx, linux.ProcessID(1), 3012)
Expand Down Expand Up @@ -434,11 +422,8 @@ func TestStorage_SameMappings(t *testing.T) {
}

for range 5 {
_, err = storage.AddMapping(ctx, linux.ProcessID(0), mapping, nil)
require.NoError(t, err)

_, err = storage.AddMapping(ctx, linux.ProcessID(0), mapping2, nil)
require.NoError(t, err)
storage.AddMapping(ctx, linux.ProcessID(0), mapping, nil)
storage.AddMapping(ctx, linux.ProcessID(0), mapping2, nil)
}

logger.Debug(ctx, "Compactify process", log.Int("pid", 0))
Expand Down Expand Up @@ -490,10 +475,7 @@ func TestStorage_Concurrent(t *testing.T) {
continue
}

_, err := storage.AddMapping(ctx, linux.ProcessID(pid), mappings[j], nil)
if err != nil {
return err
}
storage.AddMapping(ctx, linux.ProcessID(pid), mappings[j], nil)

if storage.registry.get(mappings[j].BuildInfo.BuildID) == nil {
return fmt.Errorf("no mapping `%s` found in dso map", mappings[j].BuildInfo.BuildID)
Expand Down Expand Up @@ -529,10 +511,7 @@ func TestStorage_Concurrent(t *testing.T) {
continue
}

_, err := storage.AddMapping(ctx, linux.ProcessID(pid), mappings[j], nil)
if err != nil {
return err
}
storage.AddMapping(ctx, linux.ProcessID(pid), mappings[j], nil)
}

return nil
Expand Down
13 changes: 5 additions & 8 deletions perforator/agent/collector/pkg/process/map.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,13 +498,13 @@ func (a *processAnalyzer) processMapping(ctx context.Context, m *procfs.Mapping)
if mapping.Path == "" {
// Probably JITed mapping.
mapping.Path = "[JIT]"
_, err := a.reg.dsoStorage.AddMapping(ctx, a.proc.id, mapping, nil)
return err
a.reg.dsoStorage.AddMapping(ctx, a.proc.id, mapping, nil)
return nil
}

if vdso.IsUnsymbolizableVDSOMapping(&mapping.Mapping) {
_, err := a.reg.dsoStorage.AddMapping(ctx, a.proc.id, mapping, nil)
return err
a.reg.dsoStorage.AddMapping(ctx, a.proc.id, mapping, nil)
return nil
}

binary := binary.NewProcessMappingBinary(a.proc.id, a.reg.mounts, m)
Expand Down Expand Up @@ -578,15 +578,12 @@ func (a *processAnalyzer) processMapping(ctx context.Context, m *procfs.Mapping)
return err
}

dso, err := a.reg.dsoStorage.AddMapping(
dso := a.reg.dsoStorage.AddMapping(
ctx,
a.proc.id,
mapping,
binary,
)
if err != nil {
l.Error(ctx, "Failed to register mapping", log.Error(err))
}

mapping.DSO = dso
a.registerMapping(&mapping)
Expand Down

0 comments on commit 3afadd9

Please sign in to comment.