buffer: separate the BufferFragement release and drain tracker by soulxu · Pull Request #28770 · envoyproxy/envoy

soulxu · 2023-08-02T07:32:38Z

Commit Message: buffer: separate the BufferFragement release and drain tracker
Additional Description:
There is a requirement for the user space io handle and iouring io handle to move the write buffer to its own buffer and invoke the drain trackers. But becomes an issue for the BufferFragment, the BufferFragement's releasor is invoked by the drain tracker also. After moving with invoke drain trackers, the memory in the BufferFragment will be released too, which leads to the Buffer reference some memory already released.
Risk Level: high
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes #28760
Related to issue: #28395

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only · 2023-08-02T07:32:42Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #28770 was opened by soulxu.

see: more, trace.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2023-08-04T13:34:37Z

/wait

I want to give a test with IoUring first.

soulxu · 2023-08-15T08:56:49Z

@KBaichoo @yanavlasov This should be ready for the review. I also tested this with #28824, it didn't see any fail related to using the BufferFragment.

KBaichoo

/wait

would be awesome if we could have a reproduction in an integration test similar to what @roelfdutoit reported in #28760

cc @pianiststickman since this seems like a regression related to your change in #27499

KBaichoo · 2023-08-23T19:12:48Z

source/common/buffer/buffer_impl.h

  void transferDrainTrackersTo(Slice& destination) {
    destination.drain_trackers_.splice(destination.drain_trackers_.end(), drain_trackers_);
    ASSERT(drain_trackers_.empty());
+    destination.releasor_.swap(releasor_);


what happens with the original releasor that the destination has? Is it fine to toss it as we're doing? ISTM that we don't need to do this especially as the only place transferDrainTrackersTo is called is when we copy the current slice into destination in coalesceOrAddSlice.

yes, you are right, I shouldn't change releasor here.

soulxu · 2023-08-28T07:05:14Z

would be awesome if we could have a reproduction in an integration test similar to what @roelfdutoit reported in #28760

I will look at how can I reproduce that in an integration test

Thanks for the review!

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2023-09-14T09:10:13Z

@KBaichoo I added the tests for UserSpace::IoHandle, would you like take a look this again? thanks in advance!

soulxu · 2023-09-15T05:25:37Z

/retest

KBaichoo

Thanks for working on this

/wait

KBaichoo · 2023-09-15T18:50:30Z

source/common/buffer/buffer_impl.h

    ASSERT(drain_trackers_.empty());
+    // The releasor needn't to be transferred, and actually if there is releasor, this
+    // slice can't coalesce. Then there won't be a chance to calling this method.
+    ASSERT(releasor_ == nullptr);


I might be missing something on my end but how can we guarantee no slice that we're transferring (copying) has no releasor?

There is condition control that

envoy/source/common/buffer/buffer_impl.cc

Lines 298 to 303 in 782bb0c

if (other_slice.canCoalesce() && !slices_.empty() && slice_size < CopyThreshold &&

slices_.back().reservableSize() >= slice_size) {

// Copy content of the `other_slice`. The `move` methods which call this method effectively

// drain the source buffer.

addImpl(other_slice.data(), slice_size);

other_slice.transferDrainTrackersTo(slices_.back());

The other_slice.canCoalesce() is false for the Slice created from BufferFragement.

envoy/source/common/buffer/buffer_impl.h

Lines 89 to 90 in 782bb0c

Slice(BufferFragment& fragment)

: capacity_(fragment.size()), storage_(nullptr),

That makes sense, thanks @soulxu

KBaichoo

/assign @ggreenway

For additional buffer expertise

KBaichoo · 2023-09-19T20:27:46Z

source/common/buffer/buffer_impl.h

    ASSERT(drain_trackers_.empty());
+    // The releasor needn't to be transferred, and actually if there is releasor, this
+    // slice can't coalesce. Then there won't be a chance to calling this method.
+    ASSERT(releasor_ == nullptr);


That makes sense, thanks @soulxu

soulxu · 2023-10-09T08:15:36Z

@ggreenway gentle ping :)

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

KBaichoo · 2023-11-06T16:20:42Z

/retest

ggreenway

Sorry for the delay on this. It was marked waiting so it didn't show up my list.

/wait

ggreenway · 2023-11-06T22:00:00Z

source/common/buffer/buffer_impl.h

      reservable_ = rhs.reservable_;
      drain_trackers_ = std::move(rhs.drain_trackers_);
      account_ = std::move(rhs.account_);
+      releasor_.swap(rhs.releasor_);


I don't think swap is correct in this case. Putting the old releasor into rhs is incorrect, isn't it?

You are right, let me fix that

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2023-11-07T06:25:51Z

/retest

soulxu · 2023-11-07T06:26:32Z

Sorry for the delay on this. It was marked waiting so it didn't show up my list.

/wait

no worries! I didn't pay attention on this recently also

soulxu · 2023-11-08T03:08:35Z

It seems stuck at DCO test.

soulxu · 2023-11-08T03:08:56Z

/retest

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2023-11-08T10:59:35Z

/retest

soulxu · 2023-11-08T13:13:01Z

@ggreenway ci is passed, would you think it is ok to merge?

…proxy#28770) Fixes envoyproxy#28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Fixes #28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

…proxy#28770) Fixes envoyproxy#28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>

buffer: separate the BufferFragement release and drain tracker

8bd0264

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu added 2 commits August 2, 2023 07:44

Add tests and fix format

86813b9

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

remove debug log

0256f20

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu marked this pull request as ready for review August 2, 2023 07:59

htuch assigned KBaichoo Aug 2, 2023

KBaichoo added the waiting label Aug 4, 2023

soulxu mentioned this pull request Aug 17, 2023

IoUring support #28395

Closed

KBaichoo removed the waiting label Aug 22, 2023

KBaichoo reviewed Aug 23, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 23, 2023

add test and fix

09d3d04

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested a review from kyessenov as a code owner September 14, 2023 09:05

repokitteh-read-only bot removed the waiting label Sep 14, 2023

KBaichoo reviewed Sep 15, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 15, 2023

KBaichoo previously approved these changes Sep 19, 2023

View reviewed changes

roelfdutoit mentioned this pull request Oct 9, 2023

Envoy 1.27.0: Regression in UserSpace::IoHandleImpl #28760

Closed

ggreenway self-assigned this Oct 9, 2023

Merge branch 'main' into fix_fragment_release

a2a1f69

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Nov 6, 2023

ggreenway requested changes Nov 6, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 6, 2023

soulxu dismissed KBaichoo’s stale review via ee7cb9c November 7, 2023 01:03

repokitteh-read-only bot removed the waiting label Nov 7, 2023

soulxu added 2 commits November 7, 2023 01:26

fix typo

9edef2f

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

cleanup debug log

80fab89

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway approved these changes Nov 7, 2023

View reviewed changes

trigger ci

7dcfdef

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway merged commit c124a78 into envoyproxy:main Nov 8, 2023

soulxu mentioned this pull request Nov 9, 2023

[bp/1.28] buffer: separate the BufferFragement release and drain tracker (#28770) #30787

Merged

soulxu added a commit to soulxu/envoy that referenced this pull request Nov 9, 2023

buffer: separate the BufferFragement release and drain tracker (envoy…

ee63330

…proxy#28770) Fixes envoyproxy#28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

This was referenced Nov 9, 2023

[bp/1.27] buffer: separate the BufferFragement release and drain tracker (#28770) #30788

Merged

[bp/1.26] buffer: separate the BufferFragement release and drain tracker (#28770) #30789

Closed

soulxu added a commit to soulxu/envoy that referenced this pull request Nov 9, 2023

buffer: separate the BufferFragement release and drain tracker (envoy…

28501b9

…proxy#28770) Fixes envoyproxy#28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu mentioned this pull request Nov 9, 2023

[bp/1.26] buffer: separate the BufferFragement release and drain tracker (#28770) #30790

Merged

soulxu added a commit to soulxu/envoy that referenced this pull request Nov 9, 2023

buffer: separate the BufferFragement release and drain tracker (envoy…

afc5ca2

…proxy#28770) Fixes envoyproxy#28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

This was referenced Nov 9, 2023

[bp/1.25] buffer: separate the BufferFragement release and drain tracker (#28770) #30791

Closed

[bp/1.25] buffer: separate the BufferFragement release and drain tracker (#28770) #30792

Merged

soulxu added a commit to soulxu/envoy that referenced this pull request Nov 17, 2023

buffer: separate the BufferFragement release and drain tracker (envoy…

79cc4a5

…proxy#28770) Fixes envoyproxy#28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

phlax pushed a commit that referenced this pull request Nov 17, 2023

buffer: separate the BufferFragement release and drain tracker (#28770)

b7bf0c3

Fixes #28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

phlax pushed a commit that referenced this pull request Nov 21, 2023

buffer: separate the BufferFragement release and drain tracker (#28770)

502913c

Fixes #28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

phlax pushed a commit that referenced this pull request Nov 21, 2023

buffer: separate the BufferFragement release and drain tracker (#28770)

d64d9e4

Fixes #28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

phlax pushed a commit that referenced this pull request Nov 21, 2023

buffer: separate the BufferFragement release and drain tracker (#28770)

3a3d158

Fixes #28760 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu mentioned this pull request Nov 22, 2023

IoUring: add IoUringServerSocket #30773

Merged

	if (other_slice.canCoalesce() && !slices_.empty() && slice_size < CopyThreshold &&
	slices_.back().reservableSize() >= slice_size) {
	// Copy content of the `other_slice`. The `move` methods which call this method effectively
	// drain the source buffer.
	addImpl(other_slice.data(), slice_size);
	other_slice.transferDrainTrackersTo(slices_.back());

	Slice(BufferFragment& fragment)
	: capacity_(fragment.size()), storage_(nullptr),

Conversation

soulxu commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 2, 2023

Uh oh!

soulxu commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented Sep 14, 2023

Uh oh!

soulxu commented Sep 15, 2023

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu commented Oct 9, 2023

Uh oh!

KBaichoo commented Nov 6, 2023

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu commented Nov 7, 2023

Uh oh!

soulxu commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented Nov 8, 2023

Uh oh!

soulxu commented Nov 8, 2023

Uh oh!

soulxu commented Nov 8, 2023

Uh oh!

soulxu commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

soulxu commented Aug 2, 2023 •

edited

Loading

soulxu commented Aug 4, 2023 •

edited

Loading

soulxu commented Aug 15, 2023 •

edited

Loading

soulxu commented Aug 28, 2023 •

edited

Loading

soulxu commented Nov 7, 2023 •

edited

Loading