Skip to content

Commit 99a3529

Browse files
dalehamelflorianl
andcommitted
Apply suggestions from code review
Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>
1 parent e151da4 commit 99a3529

File tree

2 files changed

+23
-28
lines changed

2 files changed

+23
-28
lines changed

interpreter/ruby/ruby.go

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -381,8 +381,8 @@ func (r *rubyData) Attach(ebpf interpreter.EbpfHandler, pid libpf.PID, bias libp
381381
procInfo: &cdata,
382382
globalSymbolsAddr: r.globalSymbolsAddr + bias,
383383
addrToString: addrToString,
384-
mappings: make(map[process.RawMapping]*uint32),
385-
prefixes: make(map[lpm.Prefix]*uint32),
384+
mappings: make(map[process.RawMapping]uint32),
385+
prefixes: make(map[lpm.Prefix]uint32),
386386
memPool: sync.Pool{
387387
New: func() any {
388388
buf := make([]byte, 512)
@@ -447,9 +447,9 @@ type rubyInstance struct {
447447
maxSize atomic.Uint32
448448

449449
// mappings is indexed by the Mapping to its generation
450-
mappings map[process.RawMapping]*uint32
450+
mappings map[process.RawMapping]uint32
451451
// prefixes is indexed by the prefix added to ebpf maps (to be cleaned up) to its generation
452-
prefixes map[lpm.Prefix]*uint32
452+
prefixes map[lpm.Prefix]uint32
453453
// mappingGeneration is the current generation (so old entries can be pruned)
454454
mappingGeneration uint32
455455
}
@@ -1343,32 +1343,26 @@ func (r *rubyInstance) SynchronizeMappings(ebpf interpreter.EbpfHandler,
13431343
continue
13441344
}
13451345

1346-
if gen, exists := r.mappings[*m]; exists {
1347-
*gen = r.mappingGeneration
1348-
continue
1346+
isNew := false
1347+
if _, exists := r.mappings[*m]; !exists {
1348+
isNew = true
1349+
log.Debugf("Enabling Ruby interpreter for %#x/%#x", m.Vaddr, m.Length)
13491350
}
1350-
1351-
// Generate a new uint32 pointer which is shared for mapping and the prefixes it owns
1352-
// so updating the mapping above will reflect to prefixes also.
1353-
mappingGeneration := r.mappingGeneration
1354-
r.mappings[*m] = &mappingGeneration
1355-
1356-
log.Debugf("Enabling Ruby interpreter for %#x/%#x", m.Vaddr, m.Length)
1351+
r.mappings[*m] = r.mappingGeneration
13571352

13581353
prefixes, err := lpm.CalculatePrefixList(m.Vaddr, m.Vaddr+m.Length)
13591354
if err != nil {
13601355
return fmt.Errorf("new anonymous mapping lpm failure %#x/%#x: %w", m.Vaddr, m.Length, err)
13611356
}
13621357

13631358
for _, prefix := range prefixes {
1364-
_, exists := r.prefixes[prefix]
1365-
if !exists {
1366-
err := ebpf.UpdatePidInterpreterMapping(pid, prefix, support.ProgUnwindRuby, 0, 0)
1367-
if err != nil {
1359+
if isNew {
1360+
if err := ebpf.UpdatePidInterpreterMapping(pid, prefix,
1361+
support.ProgUnwindRuby, 0, 0); err != nil {
13681362
return err
13691363
}
13701364
}
1371-
r.prefixes[prefix] = &mappingGeneration
1365+
r.prefixes[prefix] = r.mappingGeneration
13721366
}
13731367
}
13741368
// Detect JIT region from all mappings and update proc data if changed.
@@ -1382,17 +1376,17 @@ func (r *rubyInstance) SynchronizeMappings(ebpf interpreter.EbpfHandler,
13821376
log.Debugf("Updated JIT region %#x-%#x in ruby proc info", jitStart, jitEnd)
13831377
}
13841378
// Remove prefixes not seen
1385-
for prefix, generationPtr := range r.prefixes {
1386-
if *generationPtr == r.mappingGeneration {
1379+
for prefix, gen := range r.prefixes {
1380+
if gen == r.mappingGeneration {
13871381
continue
13881382
}
13891383
if err := ebpf.DeletePidInterpreterMapping(pid, prefix); err != nil {
13901384
log.Debugf("Failed to delete Ruby prefix %#v: %v", prefix, err)
13911385
}
13921386
delete(r.prefixes, prefix)
13931387
}
1394-
for m, generationPtr := range r.mappings {
1395-
if *generationPtr == r.mappingGeneration {
1388+
for m, gen := range r.mappings {
1389+
if gen == r.mappingGeneration {
13961390
continue
13971391
}
13981392
log.Debugf("Disabling Ruby for %#x/%#x", m.Vaddr, m.Length)

process/process.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -275,17 +275,18 @@ func iterateMappings(mapsFile io.Reader, callback func(m RawMapping) bool) (uint
275275

276276
var path string
277277
if inode == 0 {
278-
if fields[5] == "[vdso]" {
278+
switch fieldValue := fields[5]; {
279+
case fieldValue == "[vdso]":
279280
// Map to something filename looking with synthesized inode
280281
path = VdsoPathName
281282
device = 0
282283
inode = vdsoInode
283-
} else if fields[5] == "" {
284+
case fieldValue == "":
284285
// This is an anonymous mapping, keep it
285-
} else if strings.HasPrefix(fields[5], "[anon:") {
286+
case strings.HasPrefix(fieldValue, "[anon:"):
286287
// This is an anonymous mapping named with prctl(PR_SET_VMA), keep the name
287-
path = trimMappingPath(fields[5])
288-
} else {
288+
path = trimMappingPath(fieldValue)
289+
default:
289290
// Ignore other mappings that are invalid, non-existent or are special pseudo-files
290291
continue
291292
}

0 commit comments

Comments
 (0)