Date: Mon, 5 Oct 2020 21:35:51 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Rick Macklem <rmacklem@uoguelph.ca>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r366429 - in head/sys: kern sys Message-ID: <CAGudoHEQ7ENrJQh0CQw0DTzgVY1qyuN49N0t6NECHmZaMZon6g@mail.gmail.com> In-Reply-To: <20201005011833.GI2643@kib.kiev.ua> References: <202010041633.094GXg4l044462@repo.freebsd.org> <CAGudoHEhqHgSvich0CfDeg_RkpEVkuB=6zQTwfQLjsM5_wmKWQ@mail.gmail.com> <YTBPR01MB39661E4155935AEAEAE71C78DD0F0@YTBPR01MB3966.CANPRD01.PROD.OUTLOOK.COM> <20201005011833.GI2643@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On 10/5/20, Konstantin Belousov <kostikbel@gmail.com> wrote: > On Sun, Oct 04, 2020 at 09:06:02PM +0000, 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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHEQ7ENrJQh0CQw0DTzgVY1qyuN49N0t6NECHmZaMZon6g>