changes to vmem physical page allocation policy#666
changes to vmem physical page allocation policy#666eshaanmandal wants to merge 5 commits intoChampSim:developfrom
Conversation
ngober
left a comment
There was a problem hiding this comment.
This looks like it will do what the issue suggests. I have some code cleanliness comments.
| // void VirtualMemory::populate_pages() | ||
| // { | ||
| // assert(dram.size() > 1_MiB); | ||
| // ppage_free_list.resize(((dram.size() - 1_MiB) / PAGE_SIZE).count()); | ||
| // assert(ppage_free_list.size() != 0); | ||
| // champsim::page_number base_address = | ||
| // champsim::page_number{champsim::lowest_address_for_size(std::max<champsim::data::mebibytes>(champsim::data::bytes{PAGE_SIZE}, 1_MiB))}; | ||
| // for (auto it = ppage_free_list.begin(); it != ppage_free_list.end(); it++) { | ||
| // *it = base_address; | ||
| // base_address++; | ||
| // } | ||
| // } | ||
|
|
There was a problem hiding this comment.
Please remove all of the commented code here. The source code is under version control, so we can easily go back and see old behavior.
| // } | ||
|
|
||
|
|
||
| std::deque<std::pair<champsim::page_number, bool>>::iterator |
There was a problem hiding this comment.
This should just be auto. There's no point in spelling it out.
There was a problem hiding this comment.
I will apply the changes, also I had one more thing to report. So when doing make test, one of the test fails. The test started with the number 802. The reason being that the original algo removed the pages from the free_pa_list, and this algo doesn't. And the test checks for the fact if x pages have been removed from the free list after allocation. So, if we go ahead with this change, do we need to modify the test as
well?
test/cpp/src/801-vmem-duplicated.cc:51: FAILED: REQUIRE( original_size - 2 == uut.available_ppages() ) with expansion: 130814 (0x1fefe) == 130816 (0x1ff00)
There was a problem hiding this comment.
Also could i keep the return type the way it is rather than "auto", as it makes it easier to understand what data type is being returned?
There was a problem hiding this comment.
The return type is a contract with the calling code. If you say "this function returns a std::deque<std::pair<champsim::page_number, bool>>::iterator", then the calling code can depend on any of the following:
- The container is a
std::deque, and not astd::vector. - The value type is a
std::pairand not a structural type with two members - The pair contains a
champsim::page_numberand not achampsim::address. - The return type gives a view into an underlying range structure, that is, that I can increment it and maybe decrement it to move around in the range.
None of that is what the function is meant to do. What the function actually does is return a handle to a value that can be popped. Maybe it should just return a std::pair<champsim::page_number, bool>&.
There was a problem hiding this comment.
I will apply the changes, also I had one more thing to report. So when doing make test, one of the test fails. The test started with the number 802. The reason being that the original algo removed the pages from the free_pa_list, and this algo doesn't. And the test checks for the fact if x pages have been removed from the free list after allocation. So, if we go ahead with this change, do we need to modify the test as well?
test/cpp/src/801-vmem-duplicated.cc:51: FAILED: REQUIRE( original_size - 2 == uut.available_ppages() ) with expansion: 130814 (0x1fefe) == 130816 (0x1ff00)
All of the tests should pass before we can merge the changes. If you make changes to the tests, we should all understand why these changes reflect more correct behavior.
There was a problem hiding this comment.
That makes sense, i did change it to auto. Regarding the test, all the other tests pass except one (the one for vmem). Since the list size remains fixed. Should I update the ppages_available() function so that it returns how many unoccupied locations are there in the dequeue?
There was a problem hiding this comment.
Yes. You have changed how the underlying structures work. You will need to make sure that ppages_available() reports the number of available pages remain. That's what the tests are there to catch.
There was a problem hiding this comment.
You could just use a counter to track pages that have been allocated so far. Initialize to total free pages, then decrement when a page is allocated. Report that from those functions.
| auto it = ppage_free_list.begin() + idx; | ||
|
|
||
| // for keeping track of the original ptr | ||
| auto start_itr = it; | ||
| while(it != ppage_free_list.end() && it->second){ | ||
| it++; | ||
| } |
There was a problem hiding this comment.
| auto it = ppage_free_list.begin() + idx; | |
| // for keeping track of the original ptr | |
| auto start_itr = it; | |
| while(it != ppage_free_list.end() && it->second){ | |
| it++; | |
| } | |
| auto start_itr = ppage_free_list.begin() + idx; | |
| auto it = std::find_if(start_itr, ppage_free_list.end(), [](const auto& free_list_entry) { return !free_list_entry.second; }); |
There's a standard library function for what you're doing here.
This PR updates the virtual-to-physical page allocation policy to use a hash-based index instead of sequential allocation. This makes physical page selection deterministic and reproducible across runs, addressing variability discussed in #656
Sequential allocation caused differences in mappings depending on timing/order of page faults, leading to inconsistent behavior even with identical seeds. A hash-based mapping stabilizes this.