Date: Mon, 5 Oct 2020 20:03:31 -0600 From: Scott Long <scottl@samsco.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Konstantin Belousov <kostikbel@gmail.com>, 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: <33690A29-E145-44B7-AA9B-5FDE05F5C459@samsco.org> In-Reply-To: <CAGudoHEQ7ENrJQh0CQw0DTzgVY1qyuN49N0t6NECHmZaMZon6g@mail.gmail.com> References: <202010041633.094GXg4l044462@repo.freebsd.org> <CAGudoHEhqHgSvich0CfDeg_RkpEVkuB=6zQTwfQLjsM5_wmKWQ@mail.gmail.com> <YTBPR01MB39661E4155935AEAEAE71C78DD0F0@YTBPR01MB3966.CANPRD01.PROD.OUTLOOK.COM> <20201005011833.GI2643@kib.kiev.ua> <CAGudoHEQ7ENrJQh0CQw0DTzgVY1qyuN49N0t6NECHmZaMZon6g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Oct 5, 2020, at 1:35 PM, Mateusz Guzik <mjguzik@gmail.com> wrote: >=20 > 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. >>>=20 >> Yes, the function should not be on any frequent path. >>=20 >> 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. >>=20 >> 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; >>=20 >> td =3D curthread; >> + if ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) =3D=3D = 0) >> + return (0); >> + >> p =3D td->td_proc; >>=20 >> PROC_LOCK(p); >>=20 >=20 > I presume copy_file_range will not be the only consumer going forward. >=20 > 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. >=20 > So happens process lock is already quite contended (e.g., when running > poudriere) and adding to it is the wrong way to go. >=20 > --=20 >=20 Agreed. After all of the work that=E2=80=99s been done in the last 20 = years to make SMP work well on FreeBSD, I=E2=80=99m not sure why it=E2=80=99s ok to = wave ones hand and say that serializing locks are still ok, or that they don=E2=80=99t = matter. The bias needs to be against adding more serialization points, even if it=E2=80=99s not = convenient. Scott
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?33690A29-E145-44B7-AA9B-5FDE05F5C459>