| Message ID | 20200513192532.4058934-1-andriin@fb.com |
|---|---|
| Headers | show |
| Series | BPF ring buffer | expand |
On Wed, May 13, 2020 at 12:25:26PM -0700, Andrii Nakryiko wrote: > Implement a new BPF ring buffer, as presented at BPF virtual conference ([0]). > It presents an alternative to perf buffer, following its semantics closely, > but allowing sharing same instance of ring buffer across multiple CPUs > efficiently. > > Most patches have extensive commentary explaining various aspects, so I'll > keep cover letter short. Overall structure of the patch set: > - patch #1 adds BPF ring buffer implementation to kernel and necessary > verifier support; > - patch #2 adds litmus tests validating all the memory orderings and locking > is correct; > - patch #3 is an optional patch that generalizes verifier's reference tracking > machinery to capture type of reference; > - patch #4 adds libbpf consumer implementation for BPF ringbuf; > - path #5 adds selftest, both for single BPF ring buf use case, as well as > using it with array/hash of maps; > - patch #6 adds extensive benchmarks and provide some analysis in commit > message, it build upon selftests/bpf's bench runner. > > [0] https://docs.google.com/presentation/d/18ITdg77Bj6YDOH2LghxrnFxiPWe0fAqcmJY95t_qr0w > > Cc: Paul E. McKenney <paulmck@kernel.org> > Cc: Jonathan Lemon <jonathan.lemon@gmail.com> Looks very nice! A few random questions: 1) Why not use a structure for the header, instead of 2 32bit ints? 2) Would it make sense to reserve X bytes, but only commit Y? the offset field could be used to write the record length. E.g.: reserve 512 bytes [BUSYBIT | 512][PG OFFSET] commit 400 bytes [ 512 ] [ 400 ] 3) Why have 2 separate pages for producer/consumer, instead of just aligning to a smp cache line (or even 1/2 page?) 4) The XOR of busybit makes me wonder if there is anything that prevents the system from calling commit twice? -- Jonathan
On Wed, May 13, 2020 at 3:49 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > On Wed, May 13, 2020 at 12:25:26PM -0700, Andrii Nakryiko wrote: > > Implement a new BPF ring buffer, as presented at BPF virtual conference ([0]). > > It presents an alternative to perf buffer, following its semantics closely, > > but allowing sharing same instance of ring buffer across multiple CPUs > > efficiently. > > > > Most patches have extensive commentary explaining various aspects, so I'll > > keep cover letter short. Overall structure of the patch set: > > - patch #1 adds BPF ring buffer implementation to kernel and necessary > > verifier support; > > - patch #2 adds litmus tests validating all the memory orderings and locking > > is correct; > > - patch #3 is an optional patch that generalizes verifier's reference tracking > > machinery to capture type of reference; > > - patch #4 adds libbpf consumer implementation for BPF ringbuf; > > - path #5 adds selftest, both for single BPF ring buf use case, as well as > > using it with array/hash of maps; > > - patch #6 adds extensive benchmarks and provide some analysis in commit > > message, it build upon selftests/bpf's bench runner. > > > > [0] https://docs.google.com/presentation/d/18ITdg77Bj6YDOH2LghxrnFxiPWe0fAqcmJY95t_qr0w > > > > Cc: Paul E. McKenney <paulmck@kernel.org> > > Cc: Jonathan Lemon <jonathan.lemon@gmail.com> > > Looks very nice! A few random questions: > > 1) Why not use a structure for the header, instead of 2 32bit ints? hm... no reason, just never occurred to me it's necessary :) > > 2) Would it make sense to reserve X bytes, but only commit Y? > the offset field could be used to write the record length. > > E.g.: > reserve 512 bytes [BUSYBIT | 512][PG OFFSET] > commit 400 bytes [ 512 ] [ 400 ] It could be done, though I had tentative plans to use those second 4 bytes for something useful eventually. But what's the use case? From ring buffer's perspective, X bytes were reserved and are gone already and subsequent writers might have already advanced producer counter with the assumption that all X bytes are going to be used. So there are no space savings, even if record is discarded or only portion of it is submitted. I can only see a bit of added convenience for an application, because it doesn't have to track amount of actual data in its record. But this doesn't seem to be a common case either, so not sure how it's worth supporting... Is there a particular case where this is extremely useful and extra 4 bytes in record payload is too much? > > 3) Why have 2 separate pages for producer/consumer, instead of > just aligning to a smp cache line (or even 1/2 page?) Access rights restrictions. Consumer page is readable/writable, producer page is read-only for user-space. If user-space had ability to write producer position, it could wreck a huge havoc for the ringbuf algorithm. > > 4) The XOR of busybit makes me wonder if there is anything that > prevents the system from calling commit twice? Yes, verifier checks this and will reject such BPF program. > -- > Jonathan
On Wed, May 13, 2020 at 11:08:46PM -0700, Andrii Nakryiko wrote: > On Wed, May 13, 2020 at 3:49 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > On Wed, May 13, 2020 at 12:25:26PM -0700, Andrii Nakryiko wrote: > > > Implement a new BPF ring buffer, as presented at BPF virtual conference ([0]). > > > It presents an alternative to perf buffer, following its semantics closely, > > > but allowing sharing same instance of ring buffer across multiple CPUs > > > efficiently. > > > > > > Most patches have extensive commentary explaining various aspects, so I'll > > > keep cover letter short. Overall structure of the patch set: > > > - patch #1 adds BPF ring buffer implementation to kernel and necessary > > > verifier support; > > > - patch #2 adds litmus tests validating all the memory orderings and locking > > > is correct; > > > - patch #3 is an optional patch that generalizes verifier's reference tracking > > > machinery to capture type of reference; > > > - patch #4 adds libbpf consumer implementation for BPF ringbuf; > > > - path #5 adds selftest, both for single BPF ring buf use case, as well as > > > using it with array/hash of maps; > > > - patch #6 adds extensive benchmarks and provide some analysis in commit > > > message, it build upon selftests/bpf's bench runner. > > > > > > [0] https://docs.google.com/presentation/d/18ITdg77Bj6YDOH2LghxrnFxiPWe0fAqcmJY95t_qr0w > > > > > > Cc: Paul E. McKenney <paulmck@kernel.org> > > > Cc: Jonathan Lemon <jonathan.lemon@gmail.com> > > > > Looks very nice! A few random questions: > > > > 1) Why not use a structure for the header, instead of 2 32bit ints? > > hm... no reason, just never occurred to me it's necessary :) It might be clearer to do this. Something like: struct ringbuf_record { union { struct { u32 size:30; bool busy:1; bool discard:1; }; u32 word1; }; union { u32 pgoff; u32 word2; }; }; While perhaps a bit overkill, makes it clear what is going on. > > 2) Would it make sense to reserve X bytes, but only commit Y? > > the offset field could be used to write the record length. > > > > E.g.: > > reserve 512 bytes [BUSYBIT | 512][PG OFFSET] > > commit 400 bytes [ 512 ] [ 400 ] > > It could be done, though I had tentative plans to use those second 4 > bytes for something useful eventually. > > But what's the use case? From ring buffer's perspective, X bytes were > reserved and are gone already and subsequent writers might have > already advanced producer counter with the assumption that all X bytes > are going to be used. So there are no space savings, even if record is > discarded or only portion of it is submitted. I can only see a bit of > added convenience for an application, because it doesn't have to track > amount of actual data in its record. But this doesn't seem to be a > common case either, so not sure how it's worth supporting... Is there > a particular case where this is extremely useful and extra 4 bytes in > record payload is too much? Not off the top of my head - it was just the first thing that came to mind when reading about the commit/discard paradigm. I was thinking about variable records, where the maximum is reserved, but less data is written. But there's no particular reason for the ringbuffer to track this either, it could be part of the application framing. > > 3) Why have 2 separate pages for producer/consumer, instead of > > just aligning to a smp cache line (or even 1/2 page?) > > Access rights restrictions. Consumer page is readable/writable, > producer page is read-only for user-space. If user-space had ability > to write producer position, it could wreck a huge havoc for the > ringbuf algorithm. Ah, thanks, that makes sense. Might want to add a comment to that effect, as it's different from other implementations.
On Thu, May 14, 2020 at 9:30 AM Jonathan Lemon <bsd@fb.com> wrote: > > On Wed, May 13, 2020 at 11:08:46PM -0700, Andrii Nakryiko wrote: > > On Wed, May 13, 2020 at 3:49 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > > > On Wed, May 13, 2020 at 12:25:26PM -0700, Andrii Nakryiko wrote: > > > > Implement a new BPF ring buffer, as presented at BPF virtual conference ([0]). > > > > It presents an alternative to perf buffer, following its semantics closely, > > > > but allowing sharing same instance of ring buffer across multiple CPUs > > > > efficiently. > > > > > > > > Most patches have extensive commentary explaining various aspects, so I'll > > > > keep cover letter short. Overall structure of the patch set: > > > > - patch #1 adds BPF ring buffer implementation to kernel and necessary > > > > verifier support; > > > > - patch #2 adds litmus tests validating all the memory orderings and locking > > > > is correct; > > > > - patch #3 is an optional patch that generalizes verifier's reference tracking > > > > machinery to capture type of reference; > > > > - patch #4 adds libbpf consumer implementation for BPF ringbuf; > > > > - path #5 adds selftest, both for single BPF ring buf use case, as well as > > > > using it with array/hash of maps; > > > > - patch #6 adds extensive benchmarks and provide some analysis in commit > > > > message, it build upon selftests/bpf's bench runner. > > > > > > > > [0] https://docs.google.com/presentation/d/18ITdg77Bj6YDOH2LghxrnFxiPWe0fAqcmJY95t_qr0w > > > > > > > > Cc: Paul E. McKenney <paulmck@kernel.org> > > > > Cc: Jonathan Lemon <jonathan.lemon@gmail.com> > > > > > > Looks very nice! A few random questions: > > > > > > 1) Why not use a structure for the header, instead of 2 32bit ints? > > > > hm... no reason, just never occurred to me it's necessary :) > > It might be clearer to do this. Something like: > > struct ringbuf_record { > union { > struct { > u32 size:30; > bool busy:1; > bool discard:1; > }; > u32 word1; > }; > union { > u32 pgoff; > u32 word2; > }; > }; > > While perhaps a bit overkill, makes it clear what is going on. I really want to avoid specifying bitfields, because that gets into endianness territory, and I don't want to do different order of bitfields depending on endianness of the platform. I can do struct ringbuf_record_header { u32 len; u32 pgoff; }; But that will be useful for kernel only and shouldn't be part of UAPI, because pgoff makes sense only inside the kernel. For user-space, first 4 bytes is length + busy&discard bits, second 4 bytes are reserved and shouldn't be used (at least yet). I guess I should put RINGBUF_META_SZ, RINGBUF_BUSY_BIT, RINGBUF_DISCARD_BIT from patch #1 into include/uapi/linux/bpf.h to make it a stable API, I suppose? > > > > > 2) Would it make sense to reserve X bytes, but only commit Y? > > > the offset field could be used to write the record length. > > > > > > E.g.: > > > reserve 512 bytes [BUSYBIT | 512][PG OFFSET] > > > commit 400 bytes [ 512 ] [ 400 ] > > > > It could be done, though I had tentative plans to use those second 4 > > bytes for something useful eventually. > > > > But what's the use case? From ring buffer's perspective, X bytes were > > reserved and are gone already and subsequent writers might have > > already advanced producer counter with the assumption that all X bytes > > are going to be used. So there are no space savings, even if record is > > discarded or only portion of it is submitted. I can only see a bit of > > added convenience for an application, because it doesn't have to track > > amount of actual data in its record. But this doesn't seem to be a > > common case either, so not sure how it's worth supporting... Is there > > a particular case where this is extremely useful and extra 4 bytes in > > record payload is too much? > > Not off the top of my head - it was just the first thing that came to > mind when reading about the commit/discard paradigm. I was thinking > about variable records, where the maximum is reserved, but less data > is written. But there's no particular reason for the ringbuffer to > track this either, it could be part of the application framing. Yeah, I'd defer to application doing that. People were asking about using reserve with variable-sized records, but I don't think it's possible to do. That what bpf_ringbuf_output() helper was added for: prepare variable-sized data outside of ringbuf, then reserve exact amount and copy over. Less performant, but allows to use ring buffer space more efficiently. > > > > > 3) Why have 2 separate pages for producer/consumer, instead of > > > just aligning to a smp cache line (or even 1/2 page?) > > > > Access rights restrictions. Consumer page is readable/writable, > > producer page is read-only for user-space. If user-space had ability > > to write producer position, it could wreck a huge havoc for the > > ringbuf algorithm. > > Ah, thanks, that makes sense. Might want to add a comment to > that effect, as it's different from other implementations. Yep, definitely, I knew I forgot to document something :) > -- > Jonathan