Re: svn commit: r366429 - in head/sys: kern sys
> On Oct 5, 2020, at 1:35 PM, Mateusz Guzik wrote: > > On 10/5/20, Konstantin Belousov wrote: >> On Sun, Oct 04, 2020 at 09:06:02PM +, Rick Macklem wrote: >>> Mateusz Guzik wrote: Why is the process lock always taken? It looks like both routines just check a thread-local flag, so perhaps this can get away without serializing this process-wide? >>> I did spot this slight difference between the initial version of >>> sig_intr() and >>> this one. At least w.r.t. copy_file_range(2), the call happens >>> infrequently >>> enough that the overhead of acquiring the lock is not significant. >>> >> Yes, the function should not be on any frequent path. >> >> That said, all signal delivery to process is covered by the process lock, >> so checks under process lock make the advisory answer provide less false >> negatives. If considered too importand in some cases (when ?), the >> following >> patch can be applied. >> >> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c >> index 8108d4cb3a5..ed4dd52b66d 100644 >> --- a/sys/kern/kern_sig.c >> +++ b/sys/kern/kern_sig.c >> @@ -3212,6 +3212,9 @@ sig_intr(void) >> int ret; >> >> td = curthread; >> +if ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) == 0) >> +return (0); >> + >> p = td->td_proc; >> >> PROC_LOCK(p); >> > > I presume copy_file_range will not be the only consumer going forward. > > The default for all new code should be to avoid locks or other atomics > if it can be helped, otherwise it's just never ending whack-a-mole and > this is bound to become a bottleneck at some point. > > So happens process lock is already quite contended (e.g., when running > poudriere) and adding to it is the wrong way to go. > > -- > Agreed. After all of the work that’s been done in the last 20 years to make SMP work well on FreeBSD, I’m not sure why it’s ok to wave ones hand and say that serializing locks are still ok, or that they don’t matter. The bias needs to be against adding more serialization points, even if it’s not convenient. Scott ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366429 - in head/sys: kern sys
On 10/5/20, Konstantin Belousov wrote: > On Sun, Oct 04, 2020 at 09:06:02PM +, Rick Macklem wrote: >> Mateusz Guzik wrote: >> >Why is the process lock always taken? It looks like both routines just >> >check a thread-local flag, so perhaps this can get away without >> >serializing this process-wide? >> I did spot this slight difference between the initial version of >> sig_intr() and >> this one. At least w.r.t. copy_file_range(2), the call happens >> infrequently >> enough that the overhead of acquiring the lock is not significant. >> > Yes, the function should not be on any frequent path. > > That said, all signal delivery to process is covered by the process lock, > so checks under process lock make the advisory answer provide less false > negatives. If considered too importand in some cases (when ?), the > following > patch can be applied. > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c > index 8108d4cb3a5..ed4dd52b66d 100644 > --- a/sys/kern/kern_sig.c > +++ b/sys/kern/kern_sig.c > @@ -3212,6 +3212,9 @@ sig_intr(void) > int ret; > > td = curthread; > + if ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) == 0) > + return (0); > + > p = td->td_proc; > > PROC_LOCK(p); > I presume copy_file_range will not be the only consumer going forward. The default for all new code should be to avoid locks or other atomics if it can be helped, otherwise it's just never ending whack-a-mole and this is bound to become a bottleneck at some point. So happens process lock is already quite contended (e.g., when running poudriere) and adding to it is the wrong way to go. -- Mateusz Guzik ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366429 - in head/sys: kern sys
On Sun, Oct 04, 2020 at 09:06:02PM +, Rick Macklem wrote: > Mateusz Guzik wrote: > >Why is the process lock always taken? It looks like both routines just > >check a thread-local flag, so perhaps this can get away without > >serializing this process-wide? > I did spot this slight difference between the initial version of sig_intr() > and > this one. At least w.r.t. copy_file_range(2), the call happens infrequently > enough that the overhead of acquiring the lock is not significant. > Yes, the function should not be on any frequent path. That said, all signal delivery to process is covered by the process lock, so checks under process lock make the advisory answer provide less false negatives. If considered too importand in some cases (when ?), the following patch can be applied. diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 8108d4cb3a5..ed4dd52b66d 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -3212,6 +3212,9 @@ sig_intr(void) int ret; td = curthread; + if ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) == 0) + return (0); + p = td->td_proc; PROC_LOCK(p); ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366429 - in head/sys: kern sys
Mateusz Guzik wrote:
>Why is the process lock always taken? It looks like both routines just
>check a thread-local flag, so perhaps this can get away without
>serializing this process-wide?
I did spot this slight difference between the initial version of sig_intr() and
this one. At least w.r.t. copy_file_range(2), the call happens infrequently
enough that the overhead of acquiring the lock is not significant.
rick
On 10/4/20, Konstantin Belousov wrote:
> Author: kib
> Date: Sun Oct 4 16:33:42 2020
> New Revision: 366429
> URL: https://svnweb.freebsd.org/changeset/base/366429
>
> Log:
> Add sig_intr(9).
>
> It gives the answer would the thread sleep according to current state
> of signals and suspensions. Of course the answer is racy and allows
> for false-negatives (no sleep when signal is delivered after process
> lock is dropped). Also the answer might change due to signal
> rescheduling among threads in multi-threaded process.
>
> Still it is the best approximation I can provide, to answering the
> question was the thread interrupted.
>
> Reviewed by:markj
> Tested by: pho, rmacklem
> Sponsored by: The FreeBSD Foundation
> MFC after: 2 weeks
> Differential revision: https://reviews.freebsd.org/D26628
>
> Modified:
> head/sys/kern/kern_sig.c
> head/sys/sys/signalvar.h
>
> Modified: head/sys/kern/kern_sig.c
> ==
> --- head/sys/kern/kern_sig.c Sun Oct 4 16:30:05 2020(r366428)
> +++ head/sys/kern/kern_sig.c Sun Oct 4 16:33:42 2020(r366429)
> @@ -3204,6 +3204,24 @@ sig_ast_needsigchk(struct thread *td)
> return (ret);
> }
>
> +int
> +sig_intr(void)
> +{
> + struct thread *td;
> + struct proc *p;
> + int ret;
> +
> + td = curthread;
> + p = td->td_proc;
> +
> + PROC_LOCK(p);
> + ret = sig_ast_checksusp(td);
> + if (ret == 0)
> + ret = sig_ast_needsigchk(td);
> + PROC_UNLOCK(p);
> + return (ret);
> +}
> +
> void
> proc_wkilled(struct proc *p)
> {
>
> Modified: head/sys/sys/signalvar.h
> ==
> --- head/sys/sys/signalvar.h Sun Oct 4 16:30:05 2020(r366428)
> +++ head/sys/sys/signalvar.h Sun Oct 4 16:33:42 2020(r366429)
> @@ -408,6 +408,7 @@ int sig_ffs(sigset_t *set);
> void sigfastblock_clear(struct thread *td);
> void sigfastblock_fetch(struct thread *td);
> void sigfastblock_setpend(struct thread *td, bool resched);
> +int sig_intr(void);
> void siginit(struct proc *p);
> void signotify(struct thread *td);
> void sigqueue_delete(struct sigqueue *queue, int sig);
> ___
> svn-src-all@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
>
--
Mateusz Guzik
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366429 - in head/sys: kern sys
Why is the process lock always taken? It looks like both routines just
check a thread-local flag, so perhaps this can get away without
serializing this process-wide?
On 10/4/20, Konstantin Belousov wrote:
> Author: kib
> Date: Sun Oct 4 16:33:42 2020
> New Revision: 366429
> URL: https://svnweb.freebsd.org/changeset/base/366429
>
> Log:
> Add sig_intr(9).
>
> It gives the answer would the thread sleep according to current state
> of signals and suspensions. Of course the answer is racy and allows
> for false-negatives (no sleep when signal is delivered after process
> lock is dropped). Also the answer might change due to signal
> rescheduling among threads in multi-threaded process.
>
> Still it is the best approximation I can provide, to answering the
> question was the thread interrupted.
>
> Reviewed by:markj
> Tested by: pho, rmacklem
> Sponsored by: The FreeBSD Foundation
> MFC after: 2 weeks
> Differential revision: https://reviews.freebsd.org/D26628
>
> Modified:
> head/sys/kern/kern_sig.c
> head/sys/sys/signalvar.h
>
> Modified: head/sys/kern/kern_sig.c
> ==
> --- head/sys/kern/kern_sig.c Sun Oct 4 16:30:05 2020(r366428)
> +++ head/sys/kern/kern_sig.c Sun Oct 4 16:33:42 2020(r366429)
> @@ -3204,6 +3204,24 @@ sig_ast_needsigchk(struct thread *td)
> return (ret);
> }
>
> +int
> +sig_intr(void)
> +{
> + struct thread *td;
> + struct proc *p;
> + int ret;
> +
> + td = curthread;
> + p = td->td_proc;
> +
> + PROC_LOCK(p);
> + ret = sig_ast_checksusp(td);
> + if (ret == 0)
> + ret = sig_ast_needsigchk(td);
> + PROC_UNLOCK(p);
> + return (ret);
> +}
> +
> void
> proc_wkilled(struct proc *p)
> {
>
> Modified: head/sys/sys/signalvar.h
> ==
> --- head/sys/sys/signalvar.h Sun Oct 4 16:30:05 2020(r366428)
> +++ head/sys/sys/signalvar.h Sun Oct 4 16:33:42 2020(r366429)
> @@ -408,6 +408,7 @@ int sig_ffs(sigset_t *set);
> void sigfastblock_clear(struct thread *td);
> void sigfastblock_fetch(struct thread *td);
> void sigfastblock_setpend(struct thread *td, bool resched);
> +int sig_intr(void);
> void siginit(struct proc *p);
> void signotify(struct thread *td);
> void sigqueue_delete(struct sigqueue *queue, int sig);
> ___
> svn-src-all@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
>
--
Mateusz Guzik
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r366429 - in head/sys: kern sys
Author: kib
Date: Sun Oct 4 16:33:42 2020
New Revision: 366429
URL: https://svnweb.freebsd.org/changeset/base/366429
Log:
Add sig_intr(9).
It gives the answer would the thread sleep according to current state
of signals and suspensions. Of course the answer is racy and allows
for false-negatives (no sleep when signal is delivered after process
lock is dropped). Also the answer might change due to signal
rescheduling among threads in multi-threaded process.
Still it is the best approximation I can provide, to answering the
question was the thread interrupted.
Reviewed by: markj
Tested by:pho, rmacklem
Sponsored by: The FreeBSD Foundation
MFC after:2 weeks
Differential revision:https://reviews.freebsd.org/D26628
Modified:
head/sys/kern/kern_sig.c
head/sys/sys/signalvar.h
Modified: head/sys/kern/kern_sig.c
==
--- head/sys/kern/kern_sig.cSun Oct 4 16:30:05 2020(r366428)
+++ head/sys/kern/kern_sig.cSun Oct 4 16:33:42 2020(r366429)
@@ -3204,6 +3204,24 @@ sig_ast_needsigchk(struct thread *td)
return (ret);
}
+int
+sig_intr(void)
+{
+ struct thread *td;
+ struct proc *p;
+ int ret;
+
+ td = curthread;
+ p = td->td_proc;
+
+ PROC_LOCK(p);
+ ret = sig_ast_checksusp(td);
+ if (ret == 0)
+ ret = sig_ast_needsigchk(td);
+ PROC_UNLOCK(p);
+ return (ret);
+}
+
void
proc_wkilled(struct proc *p)
{
Modified: head/sys/sys/signalvar.h
==
--- head/sys/sys/signalvar.hSun Oct 4 16:30:05 2020(r366428)
+++ head/sys/sys/signalvar.hSun Oct 4 16:33:42 2020(r366429)
@@ -408,6 +408,7 @@ int sig_ffs(sigset_t *set);
void sigfastblock_clear(struct thread *td);
void sigfastblock_fetch(struct thread *td);
void sigfastblock_setpend(struct thread *td, bool resched);
+intsig_intr(void);
void siginit(struct proc *p);
void signotify(struct thread *td);
void sigqueue_delete(struct sigqueue *queue, int sig);
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
