proposal: remove unowned fragment support from Buffer API

[brian-pane]

Description: The Buffer API has an addBufferFragment method that inserts a reference to an externally owned blob of data into a Buffer::Instance. This is used mostly in test code.

I propose that we should remove this method, because its semantics are inherently dangerous. The caller is responsible for ensuring that the fragment's lifetime lasts at least as long as the earlier of: 1) the buffer being destructed or 2) the buffer no longer referencing the fragment (which can happen as a result of certain move or drain operations). That's an accident waiting to happen, and I've run into it a couple of times while writing otherwise simple unit tests.

Does anybody have a use case (e.g., supporting alternate network stacks) that depends on maintaining support for unowned buffer fragments? If not, I'll make a PR to remove this method.

[mattklein123]

@brian-pane I think it's fine to remove if there is no real usage. I thought this was used at some point in prod code but I can't recall the history.

[htuch]

I think we need this for internal Google use @genioshelo can comment further.

[mattklein123]

I think we need this for internal Google use @genioshelo can comment further.

Oh right, sorry, I forgot about this.

[alyssawilk]

Maybe worth adding a comment to the code that it's used downstream?

[brian-pane]

Would it be feasible for the downstream Google use case to make a subclass and move the addBufferFragment method there?

If not, a comment explaining the situation may be the best we can do.

[alyssawilk]

Also tagging @ianswett @mpwarres to see if the QUIC folk are likely to make use of it upstream - I know they've done lots of zero-copy work for QUIC CPU improvements and it'd be a shame to remove if we'll need to add it right back in 6-9 months

[genioshelo]

Yeah we still use this for ustreamer integration to do zero-copy between Google and Envoy buffers.

Best, Shalom

On Thu, Jan 31, 2019, 11:31 AM alyssawilk <notifications@github.com wrote:

Also tagging @ianswett https://github.com/ianswett @mpwarres https://github.com/mpwarres to see if the QUIC folk are likely to make use of it upstream - I know they've done lots of zero-copy work for QUIC CPU improvements and it'd be a shame to remove if we'll need to add it right back in 6-9 months

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/envoyproxy/envoy/issues/5754#issuecomment-459433112, or mute the thread https://github.com/notifications/unsubscribe-auth/AhTkE5wlbW0hxY0DuvAmIpVFeylVXna7ks5vIyhhgaJpZM4aYsvJ .

[brian-pane]

I’m interested in learning more about the QUIC zero-copy needs, too. Depending on the specific needs, there are alternate ways we could support zero-copy in the buffer API while still maintaining safer lifetime semantics than addBufferFragment.

[mpwarres]

In some deployments we receive packets via an mmap'ed ring buffer (using PACKET_RX_RING), in which case the memory backing the packet payload belongs to the ring buffer, rather than being caller supplied.

[ianswett]

As @mpwarres commented, I believe unowned buffers are necessary for using mmap'ed PACKET_RX_RING efficiently.

[brian-pane]

Thanks for the context. For the mmap'ed ring buffer case, if we could redesign the buffer API from scratch, I'd propose using something like this:

addBufferFragment(std::shared_ptr<Fragment>)

where Fragment is an interface that describes a span of bytes. Then we could make a subclass of Fragment, called PacketRxRingFragment for example, where the destructor returns the memory slot to kernel space. That would give us zero-copy with some extra reference-counting protection.

For now, though, I'll just make some changes in PR 5441 to provide a safer alternative to addBufferFragment in test code. There are some unit tests that call that method as a way to control the internal partitioning of a buffer's content into slices, and it's straightforward to replace those with a new method appendAsSeparateSliceForTest(absl::string_view data).