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

syscall: swap use of read-write locks for ForkLock #54162

Open
junchuan-tzh opened this issue Aug 1, 2022 · 16 comments
Open

syscall: swap use of read-write locks for ForkLock #54162

junchuan-tzh opened this issue Aug 1, 2022 · 16 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@junchuan-tzh
Copy link

junchuan-tzh commented Aug 1, 2022

The ForkLock disables fork while creating new fd and marking it close-on-exec. Fd ops hold read lock, and fork holds write lock.

In kernels newer than 2.6, the two fd ops can be done in one step, and thus the ForkLock is no need. Actually, the ForkLock brings a bottleneck to concurrent forks.

Therefore, we exchange read-write locks: Fd ops hold write lock, and fork holds read lock. It is reasonable since fd ops "modify" process state, and fork just "reads" state. With this change, the newer kernels can remove the concurrent bottlenect, and the older kernels can still ensure the safety of fd close-on-exec.

@gopherbot gopherbot added this to the Proposal milestone Aug 1, 2022
@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Aug 2, 2022

This is not an API change so it doesn't have to go through the proposal process.

@ianlancetaylor ianlancetaylor changed the title proposal: syscall: exchange read-write locks for ForkLock syscall: use read-write locks for ForkLock Aug 2, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 2, 2022
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed compiler/runtime Issues related to the Go compiler and/or runtime. Proposal labels Aug 2, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Aug 2, 2022
@ianlancetaylor ianlancetaylor changed the title syscall: use read-write locks for ForkLock syscall: swap use of read-write locks for ForkLock Aug 2, 2022
@rittneje
Copy link

rittneje commented Aug 2, 2022

This is a breaking change. Consumers that have to set close-on-exec on fds themselves may already be using syscall.ForkLock, and you cannot force everyone to replace Lock with RLock and vice versa.

@junchuan-tzh
Copy link
Author

junchuan-tzh commented Aug 2, 2022

syscall.ForkLock is only used in several cases (Pipe, Socket, Accept, Open, Dup), and swapping Lock and RLock in these cases that already are using syscall.ForkLock is enough. The upper APIs/consumers are not aware of this.

@rittneje
Copy link

rittneje commented Aug 2, 2022

No, syscall.ForkLock is a public property that anyone can use. And this is required when you are working with lower-level APIs (e.g., syscall.Socketpair) and have to set close-on-exec yourself.

Also, your statement that kernels newer that 2.6 can do this atomically (via O_CLOEXEC, etc.) only applies to Linux. For example, macOS does not support that flag in all cases. In other words, this change would cause performance issues for macOS applications that create new fds more often than they fork, which personally I suspect is the majority case.

@junchuan-tzh
Copy link
Author

junchuan-tzh commented Aug 5, 2022

Using an either-or lock is ok? See issues/23558

Like this (only the first fork holds the write lock):

close-on-exec:

    ForkLock.RLock()
    set_fd_close_on_exec()
    ForkLock.RUnlock()

fork: refcnt
    Mutex.Lock()
    if refcnt == 0:
        // get write lock for the first fork (if a non-fork op holds wirte lock, it is also ok)
        FockLock.Lock()
    refcnt += 1
    Mutex..Unlock()

    do_forkexec()

    Mutex..Lock()
        refcnt -= 1
        if refcnt == 0:
            // release write lock for the last fork
            ForkLock.UnLock()
    Mutex.Unlock()

@rittneje
Copy link

rittneje commented Aug 5, 2022

I'm assuming you meant to write ForkLock.Unlock() for the second block. I think that will work.

@junchuan-tzh
Copy link
Author

junchuan-tzh commented Aug 5, 2022

Yes, a typo error.

There is a potential problem. The latency to get ForkLock.RLock() can be very long if there are too many forks.
Is there a better way to do either-or lock?

@rittneje
Copy link

rittneje commented Aug 5, 2022

Unfortunately, I think you'd have to be able to ask the ForkLock whether there are pending readers in order to avoid that, and currently that information is not exposed.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Aug 6, 2022

Perhaps we should close this issue as a dup of #23558, as they are about the same problem.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Aug 6, 2022

Let me add that both this and #23558 have the same problem: we unfortunately exported syscall.ForkLock, and it is at least possible that people are using it, and that makes it difficult to change.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Aug 6, 2022

OK, I think I see a way: https://go.dev/cl/421441. Anybody see a way to improve that code?

@gopherbot
Copy link

gopherbot commented Aug 6, 2022

Change https://go.dev/cl/421441 mentions this issue: syscall: avoid serializing forks on ForkLock

@rittneje
Copy link

rittneje commented Aug 6, 2022

@ianlancetaylor This is the approach that @junchuan-tzh mentioned above. As discussed, it has an issue where if forkExec is continually being invoked it will prevent syscall.ForkLock.RLock() from ever unblocking.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Aug 6, 2022

@rittneje My apologies for not reading more carefully.

I've updated https://go.dev/cl/421441 to avoid that problem. But it may be too complicated now. What do you think of that approach? Thanks.

@rittneje
Copy link

rittneje commented Aug 6, 2022

Indeed, it is fairly complex. The choice of 10 as the concurrency limit (sort of) also seems very arbitrary.

Perhaps sync.Cond would be a better choice than adding a channel? Just to reduce the number of players.

var forkingCond = sync.NewCond(new(sync.Mutex))

...

forkingCond.L.Lock()
if forking > 10 {
    forkingCond.Wait()
}
if forking == 0 {
    syscall.ForkLock.Lock()
}
forking++
forkingCond.L.Unlock()

defer func() {
    forkingCond.L.Lock()
    forking--
    if forking == 0 {
        syscall.ForkLock.Unlock()
        forkingCond.Broadcast()
    }
    forkingCond.L.Unlock()
}()

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Aug 6, 2022

My personal feeling is that sync.Cond never makes things clearer. See also #21165.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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