Re: curproc vs MP vs locking
On Tue, Sep 15, 2020 at 04:38:45PM +0200, Mark Kettenis wrote: > > Date: Tue, 15 Sep 2020 12:34:07 +0200 > > From: Martin Pieuchot > > > > Many functions in the kernel take a "struct proc *" as argument. When > > reviewing diffs or reading the signature of such functions it is not > > clear if this pointer can be any thread or if it is, like in many cases, > > pointing to `curproc'. > > > > This distinction matters when it comes to reading/writing members of > > this "struct proc" and that's why a growing number of functions start > > with the following idiom: > > > > KASSERT(p == curproc); > > > > This is verbose and redundant, so I suggested to always use `curproc' > > and stop passing a "struct proc *" as argument when a function isn't > > meant to modify any thread. claudio@ raised a concern of performance > > claiming that `curproc' isn't always cheap. Is it still true? Does > > the KASSERT()s make us pay the cost anyhow? > > Right, because our kernel has DIAGNOSTIC enabled. > > > If that's the case can we adopt a convention to help review functions > > that take a "struct proc *" but only mean `curproc'? What about naming > > this parameter `curp' instead of `p'? > > That'll result in quite a bit of churn. I'd really like to avoid > doing that. I think the conclusion between the people here at k2k20 is that we should drop the argument from the function and use curproc in the functions (assigning curproc = p early on). This way there will be no way of using the wrong proc. -- :wq Claudio
Re: curproc vs MP vs locking
> Date: Tue, 15 Sep 2020 12:34:07 +0200 > From: Martin Pieuchot > > Many functions in the kernel take a "struct proc *" as argument. When > reviewing diffs or reading the signature of such functions it is not > clear if this pointer can be any thread or if it is, like in many cases, > pointing to `curproc'. > > This distinction matters when it comes to reading/writing members of > this "struct proc" and that's why a growing number of functions start > with the following idiom: > > KASSERT(p == curproc); > > This is verbose and redundant, so I suggested to always use `curproc' > and stop passing a "struct proc *" as argument when a function isn't > meant to modify any thread. claudio@ raised a concern of performance > claiming that `curproc' isn't always cheap. Is it still true? Does > the KASSERT()s make us pay the cost anyhow? Right, because our kernel has DIAGNOSTIC enabled. > If that's the case can we adopt a convention to help review functions > that take a "struct proc *" but only mean `curproc'? What about naming > this parameter `curp' instead of `p'? That'll result in quite a bit of churn. I'd really like to avoid doing that.
curproc vs MP vs locking
Many functions in the kernel take a "struct proc *" as argument. When reviewing diffs or reading the signature of such functions it is not clear if this pointer can be any thread or if it is, like in many cases, pointing to `curproc'. This distinction matters when it comes to reading/writing members of this "struct proc" and that's why a growing number of functions start with the following idiom: KASSERT(p == curproc); This is verbose and redundant, so I suggested to always use `curproc' and stop passing a "struct proc *" as argument when a function isn't meant to modify any thread. claudio@ raised a concern of performance claiming that `curproc' isn't always cheap. Is it still true? Does the KASSERT()s make us pay the cost anyhow? If that's the case can we adopt a convention to help review functions that take a "struct proc *" but only mean `curproc'? What about naming this parameter `curp' instead of `p'?
