On Thu, May 14, 2020 at 9:30 AM Jonathan Lemon <bsd@xxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx> > > > > Cc: Jonathan Lemon <jonathan.lemon@xxxxxxxxx> > > > > > > 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
