monitor: reduce overhead

[aalemayhu]

Reduce the number of allocations from the monitor.

[rlenglet]

This is a good idea, thanks!

[rlenglet]

I would revert this change. Even if you have no listeners you must keep reading from the perf buffer, otherwise you'll start losing events and incorrectly report a high lost event count when the first reader connects.
Plus, it's always interesting to collect and report the "lost event" metric even when there are no readers.

However, you could optimize the no-listener case by not decoding the monitor events, just read and discard them.

[tgraf]

You want to disconnect from the perf ring buffer to stop triggering event generation in the data path. We don't want to pay the datapath cost as a penalty if no-one is listening.

[rlenglet]

How about renaming into nlisteners for consistency with npages above, or just listeners for consistency with cpus, lost, and unknown above?

[aalemayhu]

nlisteners sounds good.

[rlenglet]

epoll is used in its default edge-triggered mode here. Therefore, if you don't call monitorEvents.ReadAll every time monitorEvents.Poll returns todo > 0, the next call to monitorEvents.Poll will not block, because there's still data to be read from the file descriptor. Therefore, this loop will spin and consume 100% of one CPU, and will also result in the perf ring buffer filling up.

In short, this if todo > 0 test CAN'T be conditioned on anything, e.g. numListeners() > 0.
You must revert this change.

You should either:

• Move your optimization into m.receiveEvent, where you could just ignore read events (don't decode them) if there are no listeners, as I previously suggested.
• Or close the file descriptor until numListeners() > 0, as @tgraf suggested.

[aalemayhu]

Please wait till I request a new review. Thanks!

[aanm]

What is the purpose for this channel?

[aalemayhu]

To wait for listeners which are added in the handle connection. Am I using it wrong?

[raybejjani]

There are a few patterns you might use that would be more canonical. The most useful would be that you can make a buffered channel (make(chan bool, 1000)) and then the writes you have to it below wouldn't need the goroutine wrappers. Since you're using main_event_loop as a normal label, instead of one for the loop, you can replace the for ok := <-waitForListeners; !ok; with a simple for <-waitForListeners but then only write to the channel when you would have written a true. You currently always write true in handleConnection so that doesn't change, and you would have to use an if statement in the send case.

[aalemayhu]

Looks cleaner. The only other change I did was drop the loop because it seemed to lock forever. Please take one more pass in case I misunderstood you.

[aalemayhu]

Or maybe wait looks like I missed something.

[raybejjani]

Sort of. My goal was to avoid the busy loop on the waitForListeners channel. The thing is, we're mixing concerns a bit, in that we want the loop to pause until there are listeners but the only way we have to do that is by blocking on the channel every iteration, thus, we also need to bypass this channel read :/

Would it be simpler to coordinate this a different way?

• Spawn a goroutine that runs the logic in main_event_loop when we handleConnection and we go from numListeners==0 to numListeners==1
• In this main_event_loop goroutine, exit it completely when there are no more listeners
  after purgeInactiveListeners (or just check at the end/start of the loop).
• We can also defer closing sockets and things in this goroutine, so we know the cleanups happen at the right time.
• This version would not have this waitForListeners channel at all.

My reasoning is that it is simple to not run the perf reading parts at all if there are no listeners. I think this was the motivation for these changes right?

If you want to stick with what you have now, then it might be easier to split the gotos and labels:

• add a new label waiting_for_listers. jump to it when we have no more listeners (so, where we currently jump to main_event_loop)
• main_event_loop is moved to after the waitForListeners check. We jump to it in the case where there are listeners still, and we need to poll again and keep reading.

[aalemayhu]

You are right. Will do it as purposed, just need to test it a little bit locally and will push it out. Thanks, Ray.

[joestringer]

When there are no monitors, is that what produces an API response with size 0 here?

[aalemayhu]

The intention of writing an empty meta is so that we can detect a write failure when any of the known connections are gone / inactive. If we are not aware of any connections we never write anything and these lines are not reached. The connections are added to a list of listeners, but the Go API has no good way for us to know when a connection is inactive. So we need to try to write in order to detect a failure. Does this answer your question?

[tgraf]

The explanation makes sense, can you add it as a comment to the code? It's not obvious otherwise.

[aalemayhu]

sure

[aanm]

        t := time.NewTicker(5 * time.Second)
        for range t.C {
                if monitorEvents != nil {
                        m.dumpState()
                }
        }

[aalemayhu]

Fixed

[tgraf]

Is e.state. guaranteed to never be NULL?

[aalemayhu]

No. We are using malloc so there is a chance it can return NULL. What is the proper way to handle this. Do you want to do a null check first?

[raybejjani]

I guess we'd have to check on line 293 and return an error there. To be safe, we could make this no-op on e.state being nil too

[aalemayhu]

Will leave this as is unless @tgraf really wants it to change.

[tgraf]

I'm taking my comment back, calling free() on NULL is perfectly fine so we can keep this.

[tgraf]

The explanation makes sense, can you add it as a comment to the code? It's not obvious otherwise.

[tgraf]

Convert this to // comments

[tgraf]

Expand this to multiple lines, it will become easier to read and you don't need to store num first.

[tgraf]

Convert to //

[tgraf]

Move this into a function:

func generateHealthCheckMsg() {
	meta := &payload.Meta{Size: uint32(0)}
 	metaBuf, err := meta.MarshalBinary()
 	if err != nil {
 		log.WithError(err).Fatal("meta encode")
 	}

        return metaBuf
}

[tgraf]

Can we safely continue here?

[aalemayhu]

We should continue will change it to a info.

[raybejjani]

Mostly small things that you can ignore, with one logic order thing that might be worth revisiting.

[raybejjani]

This if-else chain can be a switch statement, if you want. That would allow you to handle the last pl.Type == payload.RecordLost case explicitly and then have default report an error or whatever.

	switch {
	case meta.Size == 0:
		continue
	case pl.Type == payload.EventSample:
		receiveEvent(pl.Data, pl.CPU)
	case pl.Type == payload.RecordLost:
		lostEvent(pl.Lost, pl.CPU)
	default:
		log.WithError(err).Fatal("unsupported type in meta object")
	}

[aalemayhu]

Looks better

[raybejjani]

I personally prefer to create the Ticker as you have here but if you wanted a more compact form you can do:

for range time.Tick(5 * time.Second)

The assumption here is that this function is never intended to return (which it can't, since we loop infinitely and have no returns or breaks) and so the Ticker providing the channel returned by Tick does not need to Close called on it (this would normally cause it to leak (https://golang.org/pkg/time/#Tick)
If you keep the explicit t := thing then could you also add a defer t.Close(). It isn't necessary but if someone ever refactors this code and it returns, we won't leak the timer.

[aalemayhu]

Will add a comment but otherwise fine as is.

[raybejjani]

This could be `if first := listeners.Len() == 0; first {``` since we don't use first otherwise

[aalemayhu]

This does not do what you think it does. We only want to start the event loop if this is really our first listener. If you move the initialization further down after the list is being modified the meaning of first is not the same anymore.

[raybejjani]

Oh! I see, yes. We also can't start the main loop since it would then exit because there are no listeners. You are totally right!

[raybejjani]

Should we do the numListeners() == 0 check (the else on line 151 below) here? this then means we can return before the Poll if there are no listeners, and also means that if we do the Poll call we always read data in the todo > 0 case (I think this was rlenglet's concern earlier).
The general structure of the function would then look like:

bpf.NewCpuEvents()
for {
	purgeInactiveListeners()
	if numListeners == 0 {
		CloseAll()
		return
	}
	Poll()
	if todo > 0 {
		ReadAll()
	}
}

An alternative would be to defer monitorEventsCloseAll() and the error handling for it and if numListeners == 0 simply returns from the function/breaks the loop to allow the function to exit.

[aalemayhu]

Will change it to the suggested but deferring the closing of the perf event side might lead to an invalid memory address or nil pointer dereference so we can't do that.

[raybejjani]

A nit, but you could assign monitorEvents directly on line 128 (I suspect this is a holdover from a complicated if statement that didn't create monitorEvents in the outer scope).

[raybejjani]

The implied behaviour here is that payload's contents are undefined in the meta.Size==0 case. We can zero out payload or add a comment about this in the function comment (line 102)

[raybejjani]

I guess we'd have to check on line 293 and return an error there. To be safe, we could make this no-op on e.state being nil too

[raybejjani]

Oh, it just occurred to me that since we just keep sending this same empty meta, and only in purgeInactiveListeners, we can store the []byte and just keep reusing that. It's not a big deal, though, this isn't a big memory use or anything.

Dismissing review, since there's been changes since last time