perf add timeout argument #736

xpu22 · 2022-07-14T11:32:59Z

No description provided.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

florianl · 2022-07-14T15:01:25Z

perf/reader.go

@@ -162,6 +163,7 @@ type ReaderOptions struct {
 	// Read will process data. Must be smaller than PerCPUBuffer.
 	// The default is to start processing as soon as data is available.
 	Watermark int
+	TimeOut   int


I think this option should have at least document what it is used for so users can reason about its impact. Also if there is a default value, it should be mentioned here as well I think.

With SetDeadline this Option isn't used anymore.

lmb · 2022-07-14T16:36:55Z

Hi @xpu22, thanks for your PR!

There was an earlier proposal for this in #523 The gist of the discussion is:

Instead of a timeout option, there should be a Reader.SetDeadline function.
You can take the implementation on Poller from 26fab34#diff-1f29559e5dfacc2fda670f413123949365e356c977391255950d29a89a6e983bR124-R141

Please also add some unit tests and take a look at the failing CI: https://ebpf.semaphoreci.com/jobs/b80f08ab-6ffd-4473-8887-27ad0c4c0508#L572

xpu22 · 2022-07-15T01:40:05Z

Hi @xpu22, thanks for your PR!

There was an earlier proposal for this in #523 The gist of the discussion is:

Instead of a timeout option, there should be a Reader.SetDeadline function.
Looks good to me, Thanks!

You can take the implementation on Poller from 26fab34#diff-1f29559e5dfacc2fda670f413123949365e356c977391255950d29a89a6e983bR124-R141

Please also add some unit tests and take a look at the failing CI: https://ebpf.semaphoreci.com/jobs/b80f08ab-6ffd-4473-8887-27ad0c4c0508#L572
Ok

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

lmb · 2022-07-18T15:54:04Z

https://ebpf.semaphoreci.com/jobs/690fd5ce-e7d0-4400-9f58-3da6f915e3a0#L573 is still failing. Please ping me explicitly when you're ready for another round of review.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

xpu22 · 2022-07-19T02:07:25Z

https://ebpf.semaphoreci.com/jobs/690fd5ce-e7d0-4400-9f58-3da6f915e3a0#L573 is still failing. Please ping me explicitly when you're ready for another round of review.

Hi @lmb please review

internal/epoll/poller.go

lmb

Looks good, thanks for following up so quickly!

lmb · 2022-07-19T12:03:36Z

internal/epoll/poller.go

-		n, err := unix.EpollWait(p.epollFd, events, -1)
+		msec := int(-1)
+		if !deadline.IsZero() {
+			msec = int(time.Until(deadline).Milliseconds())


Bug in my own code: this doesn't handle deadlines in the past or so far in the future that msec is overflowed.

I'll fix that up in a follow up.

lmb · 2022-07-19T12:10:38Z

perf/reader.go

-	ErrClosed = os.ErrClosed
-	errEOR    = errors.New("end of ring")
+	ErrClosed  = os.ErrClosed
+	ErrTimeOut = os.ErrDeadlineExceeded


This alias isn't necessary. Just document that os.ErrDeadlineExceeded is returned.

lmb · 2022-07-19T12:11:28Z

perf/reader.go

@@ -162,6 +163,7 @@ type ReaderOptions struct {
 	// Read will process data. Must be smaller than PerCPUBuffer.
 	// The default is to start processing as soon as data is available.
 	Watermark int
+	TimeOut   int


With SetDeadline this Option isn't used anymore.

lmb · 2022-07-19T12:11:46Z

perf/reader.go

@@ -133,7 +135,8 @@ func readRawSample(rd io.Reader, buf, sampleBuf []byte) ([]byte, error) {
 // Reader allows reading bpf_perf_event_output
 // from user space.
 type Reader struct {
-	poller *epoll.Poller
+	poller  *epoll.Poller
+	timeOut time.Time


Nit: deadline time.Time.

lmb · 2022-07-19T12:13:41Z

perf/reader.go

@@ -299,14 +316,15 @@ func (pr *Reader) Read() (Record, error) {
 func (pr *Reader) ReadInto(rec *Record) error {
 	pr.mu.Lock()
 	defer pr.mu.Unlock()
+	defer func() { pr.timeOut = time.Time{} }()


So net.Conn does not automatically clear the deadline: https://pkg.go.dev/net#Conn Unless there is a strong reason to do this here I'd suggest that we also don't clear it.

lmb · 2022-07-19T12:14:34Z

perf/reader.go

@@ -280,6 +285,18 @@ func (pr *Reader) Close() error {
 	return nil
 }

+// SetDeadline the Read timeout


Suggested change

// SetDeadline the Read timeout

// SetDeadline controls how long Read and ReadInto will block waiting for samples.

//

// Passing a zero time.Time will remove the deadline.

lmb · 2022-07-19T12:15:25Z

perf/reader.go

+// SetDeadline the Read timeout
+//
+// If this function was not invoked, the Read, ReadInto
+// will be blocked until there are at least Watermark


I think this is slightly misleading: adding a timeout / deadline doesn't change how Watermark works.

perf/reader.go

lmb · 2022-07-19T12:17:15Z

perf/reader_test.go

@@ -58,6 +58,10 @@ func TestPerfReader(t *testing.T) {
 	if record.CPU < 0 {
 		t.Error("Record has invalid CPU number")
 	}
+
+	rd.SetDeadline(time.Now().Add(100 * time.Millisecond))


Nit: you can make the timeout very short, like a millisecond or so. It'll save us a bit of time when running ht test.

lmb · 2022-07-19T12:19:02Z

internal/epoll/poller.go

@@ -138,6 +144,10 @@ func (p *Poller) Wait(events []unix.EpollEvent) (int, error) {
 			return 0, err
 		}

+		if n == 0 && msec != -1 {
+			return 0, os.ErrDeadlineExceeded


I think we should wrap the error here, otherwise we return a naked ErrDeadlineExceeded.

Suggested change

return 0, os.ErrDeadlineExceeded

return 0, fmt.Errorf("poll: %w", os.ErrDeadlineExceeded)

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

nashuiliang · 2022-07-27T07:03:04Z

Cool, adding the timeout of ebpf-perf is very necessary!
Hi, @lmb @florianl @xpu22, have you considered using context.Context WithCancel/WithDeadline for this?

The new Read() API is as follows:

func (pr *Reader) ReadWithContext(ctx context.Context) (Record, error)

ti-mo · 2022-07-28T09:52:51Z

have you considered using context.Context WithCancel/WithDeadline for this?

We have, but IMO the performance characteristics of context.Context aren't very well-suited to this kind of API. Granted, maybe that's not our call to make and we could just offer the functionality. If the ring isn't that busy, the overhead of Context could be acceptable if it makes it much more ergonomic to use.

Could you give a practical use case for it? In any case, I would suggest we take this discussion to a dedicated issue.

lmb

Thanks for finishing this work, I was waiting for you to ping me for another review. Sorry it took a while.

xpu22 added 2 commits July 14, 2022 19:26

epoll: wait add timeout argument

66031f8

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

perf: add timeout option

1633d0e

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

florianl reviewed Jul 14, 2022

View reviewed changes

perf: update

3ce013c

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

lmb self-requested a review July 18, 2022 15:53

ringbuf: remove white space

d1a6a02

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

florianl reviewed Jul 19, 2022

View reviewed changes

internal/epoll/poller.go Show resolved Hide resolved

lmb requested changes Jul 19, 2022

View reviewed changes

perf: update

646342b

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

lmb approved these changes Aug 10, 2022

View reviewed changes

lmb merged commit 9f500f3 into cilium:master Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf add timeout argument #736

perf add timeout argument #736

xpu22 commented Jul 14, 2022

florianl Jul 14, 2022

xpu22 Jul 15, 2022

lmb Jul 19, 2022

lmb commented Jul 14, 2022

xpu22 commented Jul 15, 2022

lmb commented Jul 18, 2022

xpu22 commented Jul 19, 2022

lmb left a comment

lmb Jul 19, 2022

lmb Jul 19, 2022

lmb Jul 19, 2022

lmb Jul 19, 2022

lmb Jul 19, 2022

lmb Jul 19, 2022

lmb Jul 19, 2022

lmb Jul 19, 2022

lmb Jul 19, 2022

nashuiliang commented Jul 27, 2022

ti-mo commented Jul 28, 2022

lmb left a comment

-// SetDeadline the Read timeout
+// SetDeadline controls how long Read and ReadInto will block waiting for samples.
+//
+// Passing a zero time.Time will remove the deadline.

	return 0, os.ErrDeadlineExceeded
	return 0, fmt.Errorf("poll: %w", os.ErrDeadlineExceeded)

perf add timeout argument #736

perf add timeout argument #736

Conversation

xpu22 commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb commented Jul 14, 2022

xpu22 commented Jul 15, 2022

lmb commented Jul 18, 2022

xpu22 commented Jul 19, 2022

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashuiliang commented Jul 27, 2022

ti-mo commented Jul 28, 2022

lmb left a comment

Choose a reason for hiding this comment