svn commit: r366429 - in head/sys: kern sys

[Scott Long]

> On Oct 5, 2020, at 1:35 PM, Mateusz Guzik <mjguzik at gmail.com> wrote:
> 
> On 10/5/20, Konstantin Belousov <kostikbel at 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.
> 
> -- 
> 

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