chore(tiering): External alloc free page chaining #3237

dranikpg · 2024-06-28T05:41:16Z

The intrusive free list I suggested, please review the design first.... I might have missed some detail from reading the current code that might break everything 😓

Signed-off-by: Vladislav Oleshko <[email protected]>

src/server/tiering/external_alloc.cc

romange · 2024-06-29T08:30:10Z

src/server/tiering/external_alloc.cc

@@ -478,12 +496,23 @@ void ExternalAllocator::FreePage(Page* page, SegmentDescr* owner, size_t block_s
  BinIdx bidx = ToBinIdx(block_size);

  // Remove fast allocation reference.
-  if (free_pages_[bidx] == page) {
-    free_pages_[bidx] = &empty_page;
+  if (page->free_chained) {


According to your change any page at this point must be part of free_pages_ list. Moreover, seems reasonable to add this invariant to class Page description:

Page can be exactly in either these 3 states: 1. unitialized - with no blocks being allocated - segment_inuse will be 0 in that case, 1 otherwise. 2. Partly utilized by 1 or more blocks, with available > 0 in that case. It must be present in free_pages_ list then. 3. Fully utilized, with available==0, in that case it's not part of free_pages_ list.

If what I wrote is correct, you do not need free_chained variable.

Looks like it, I just made sure it's versatile and won't break. However worst we do with this is just do a pointless search, so nothing critical

romange · 2024-06-29T08:35:08Z

src/server/tiering/external_alloc.cc

+    if (free_pages_[bidx] == page) {
+      free_pages_[bidx] = &empty_page;
+    } else {
+      for (auto* cur = free_pages_[bidx]; cur != nullptr; cur = cur->next_free) {


i do not mind. just a suggestion - another way to do it - is lazily, so when you actually pull a full page from the free_pages_ list, you pop all the "empty" ones, though one can argue it's a premature optimisation :)

Also a very fragile approach, we return the page to the segment and might decide to use with a different bin index. We can't keep the pointer

src/server/tiering/external_alloc.cc

romange

in general looks good, added a few comments.

src/server/tiering/external_alloc.cc

romange · 2024-06-29T14:01:17Z

src/server/tiering/external_alloc.cc

  if (page->available == blocks_num) {
    FreePage(page, seg, block_size);
+  } else if (page->available == 1) {
+    page->next_free = free_pages_[page->bin_idx];


note, this may introduce a jerking behaviour where a page with a single block will be restored,removed and so on (no action items).

romange · 2024-06-29T14:03:56Z

src/server/tiering/external_alloc.cc

@@ -479,11 +496,19 @@ void ExternalAllocator::FreePage(Page* page, SegmentDescr* owner, size_t block_s

  // Remove fast allocation reference.
  if (free_pages_[bidx] == page) {
-    free_pages_[bidx] = &empty_page;
+    free_pages_[bidx] = page->next_free ? page->next_free : &empty_page;


I am not sure if page->next_free can ever be null in this case.
I thought free_pages_ list always terminates with &empty_page.

no, empty_page ends with nullptr. I don't see a reason to fight with it because it allows us easily to place empty_page into the free_pages_ slot without additional checks

romange · 2024-06-29T14:05:06Z

src/server/tiering/external_alloc.cc

-    free_pages_[bidx] = &empty_page;
+    free_pages_[bidx] = page->next_free ? page->next_free : &empty_page;
+  } else {
+    for (auto* cur = free_pages_[bidx]; cur != nullptr; cur = cur->next_free) {


cur != &empty_page ?

dranikpg · 2024-06-30T06:28:43Z

According to your change any page at this point must be part of free_pages_ list. Moreover, seems reasonable to add this invariant to class Page description:

Seems like not, it doesn't break because of &empty_page, but because we get a circle in the list 😱

UPD: because we delete lazily

dranikpg mentioned this pull request Jun 28, 2024

chore(tiering): Fixes #3225

Merged

chore(tiering): External alloc free page chaining

0be2fad

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg force-pushed the fix-external-alloc branch from 294627e to 0be2fad Compare June 28, 2024 13:55

dranikpg requested a review from romange June 28, 2024 13:56