curproc vs MP vs locking

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

curproc vs MP vs locking

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?

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'?

Reply | Threaded
Open this post in threaded view
|

Re: curproc vs MP vs locking

Mark Kettenis
> Date: Tue, 15 Sep 2020 12:34:07 +0200
> From: Martin Pieuchot <[hidden email]>
>
> 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.

Reply | Threaded
Open this post in threaded view
|

Re: curproc vs MP vs locking

Claudio Jeker
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 <[hidden email]>
> >
> > 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