Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: heap pageAlloc.searchAddr strategy may be wrong #41989

Open
dreamerjackson opened this issue Oct 15, 2020 · 2 comments
Open

runtime: heap pageAlloc.searchAddr strategy may be wrong #41989

dreamerjackson opened this issue Oct 15, 2020 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dreamerjackson
Copy link
Contributor

dreamerjackson commented Oct 15, 2020

i read the heap alloc code. and find the pageAlloc.searchAddr may have a problem, as the comment say
i hope someone can review it @mknyszek

type pageAlloc struct {
	// The address to start an allocation search with. It must never
	// point to any memory that is not contained in inUse, i.e.
	// inUse.contains(searchAddr) must always be true.
	//
	// When added with arenaBaseOffset, we guarantee that
	// all valid heap addresses (when also added with
	// arenaBaseOffset) below this value are allocated and
	// not worth searching.
	//
	// Note that adding in arenaBaseOffset transforms addresses
	// to a new address space with a linear view of the full address
	// space on architectures with segmented address spaces.
	searchAddr offAddr

why i think it may have problem? when we find the continue npages in radix tree(code is below), the searchAddr will increase when j++ into new entry in the same level. but it only mean below searchAddr just not enough contiguous space to alloc npages。but it may can alloc mpages(mpages < npages) below searchAddr。 so next time when we want to alloc mpages, we give up the change to alloc mpages which address below searchAddr, so we can't use the heap Efficiently。
and we may alloc more memory because when we can't find mpages we will grow the memory.

func (s *pageAlloc) find(npages uintptr) (uintptr, offAddr) {
   for j := j0; j < len(entries); j++ {
			..
			// We've encountered a non-zero summary which means
			// free memory, so update firstFree.
			foundFree(levelIndexToOffAddr(l, i+j), (uintptr(1)<<logMaxPages)*pageSize)
			....
		if size >= uint(npages) {
			addr := levelIndexToOffAddr(l, i).add(uintptr(base) * pageSize).addr()
			return addr, firstFree.base
		}
}

and , the searchAddr can only recover when free npages

func (s *pageAlloc) free(base, npages uintptr) {
	// If we're freeing pages below the s.searchAddr, update searchAddr.
	if b := (offAddr{base}); b.lessThan(s.searchAddr) {
		s.searchAddr = b
	}
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 15, 2020
@ALTree ALTree added this to the Unplanned milestone Oct 15, 2020
@mknyszek
Copy link
Contributor

I'm not sure I follow. The invariant of searchAddr is that every page below it is allocated. find makes its best guess as to what the first free page is (that's what the firstFree struct is tracking). It only knows that with certainty when npages=1. It then returns that guess, and alloc decides whether its better than the allocator's current searchAddr. In this way, we'll never miss the lowest-address contiguous block of pages that fits npages because we're only pruning the parts of the search space that we know have zero free memory (or should be, anyway).

That being said, there have been bugs here before. I've written tests for as many cases as I could think of in mpagealloc_test.go. If you can show in a test that the allocator is going to miss some memory, that would be very helpful.

@dreamerjackson
Copy link
Contributor Author

dreamerjackson commented Oct 16, 2020

@mknyszek I tested it carefully and you are right. before i mistake the foundFree in find function.

i just found another detail. in the allocfunction. does we should judge the addr( find function return) near the s.searchAddr?
if addr near s.searchAddr, we should just s.searchAddr += size, so we can get the accurate s.searchAddr. i find we have no change to handle this , if the searchAddr not accurate, it will cause to search in radix tree rather than find in pallocBits in quick path

func (s *pageAlloc) alloc(npages uintptr) (addr uintptr, scav uintptr) {
	 ....
	addr, searchAddr = s.find(npages)
	if s.searchAddr.lessThan(searchAddr) {
		s.searchAddr = searchAddr
	}

       //  here  this is my thought
      if addr nearby  s.searchAddr{
          s.searchAddr += npages size
      }

	return addr, scav
}

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants