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: injectglist(glist *gList) npidle may not necessarily be the latest value. Could it cause scheduling starvation issues? #63555

Closed
sxstd001 opened this issue Oct 15, 2023 · 3 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@sxstd001
Copy link

sxstd001 commented Oct 15, 2023

injectglist(glist *gList) npidle may not necessarily be the latest value.
Could it cause scheduling starvation issues?

func injectglist(glist *gList) {
        ..............
	startIdle := func(n int) {
		for i := 0; i < n; i++ {
			mp := acquirem() // See comment in startm.
			lock(&sched.lock)
			pp, _ := pidlegetSpinning(0)
			if pp == nil {
				unlock(&sched.lock)
				releasem(mp)
				break
			}
			
			startm(pp, false, true)
			unlock(&sched.lock)
			releasem(mp)
		}
	}
	
       // there no problem;
	pp := getg().m.p.ptr()
	if pp == nil {
		lock(&sched.lock)
		globrunqputbatch(&q, int32(qsize))
		unlock(&sched.lock)
		startIdle(qsize)
		return
	}
	

	npidle := int(sched.npidle.Load())

       // !!!!!!!!   npidle may not necessarily be the latest value

	var globq gQueue
	var n int
	for n = 0; n < npidle && !q.empty(); n++ {
		g := q.pop()
		globq.pushBack(g)
	}
	if n > 0 {
		lock(&sched.lock)
		globrunqputbatch(&globq, int32(n))
		unlock(&sched.lock)
		startIdle(n)
		qsize -= n
	}
	
	if !q.empty() {
		runqputbatch(pp, &q, qsize)
	}

       **//  Should wakeP be called here???????????"**

}
``
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 15, 2023
@ianlancetaylor
Copy link
Contributor

I think you're right. I think there is a very short race in which some other P becomes idle after injectglist loads npidle but before it adds any goroutines to the queue. That could lead to an idle P when there is work available on the global run queue. That could potentially last until some goroutine becomes ready to run.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 16, 2023
@prattmic
Copy link
Member

I agree, this looks like a minor work conversation issue. Thanks for the report!

@prattmic prattmic added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 16, 2023
@prattmic prattmic self-assigned this Oct 16, 2023
@mknyszek mknyszek added this to the Backlog milestone Nov 1, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/550175 mentions this issue: runtime: fix potential race of idle P's during injectglist

@panjf2000 panjf2000 modified the milestones: Backlog, Go1.23 Jan 23, 2024
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. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants