Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 30 Jul 2019 19:18:56 -0700
From:      Bryan Drewery <bdrewery@FreeBSD.org>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r350005 - head/sys/kern
Message-ID:  <47623274-f1be-7134-2f29-4941776df39f@FreeBSD.org>
In-Reply-To: <20190730232714.GM2731@kib.kiev.ua>
References:  <201907151918.x6FJIPFo077975@repo.freebsd.org> <d281e1dc-1931-20a5-9f41-7c1f66d4f667@FreeBSD.org> <20190730231256.GL2731@kib.kiev.ua> <20190730232714.GM2731@kib.kiev.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--8uyUCaTumRMJ4zEOnwjMI9AZI8qMzKRIp
Content-Type: multipart/mixed; boundary="H026HVpn5qTcefLIsmZMlKeUwrSTao0fy";
 protected-headers="v1"
From: Bryan Drewery <bdrewery@FreeBSD.org>
To: Konstantin Belousov <kostikbel@gmail.com>
Cc: src-committers@freebsd.org, svn-src-all@freebsd.org,
 svn-src-head@freebsd.org
Message-ID: <47623274-f1be-7134-2f29-4941776df39f@FreeBSD.org>
Subject: Re: svn commit: r350005 - head/sys/kern
References: <201907151918.x6FJIPFo077975@repo.freebsd.org>
 <d281e1dc-1931-20a5-9f41-7c1f66d4f667@FreeBSD.org>
 <20190730231256.GL2731@kib.kiev.ua> <20190730232714.GM2731@kib.kiev.ua>
In-Reply-To: <20190730232714.GM2731@kib.kiev.ua>

--H026HVpn5qTcefLIsmZMlKeUwrSTao0fy
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

On 7/30/2019 4:27 PM, Konstantin Belousov wrote:
> On Wed, Jul 31, 2019 at 02:13:02AM +0300, Konstantin Belousov wrote:
>> On Tue, Jul 30, 2019 at 03:04:57PM -0700, Bryan Drewery wrote:
>>> On 7/15/2019 12:18 PM, Konstantin Belousov wrote:
>>>> Author: kib
>>>> Date: Mon Jul 15 19:18:25 2019
>>>> New Revision: 350005
>>>> URL: https://svnweb.freebsd.org/changeset/base/350005
>>>>
>>>> Log:
>>>>   In do_sem2_wait(), balance umtx_key_get() with umtx_key_release() =
on retry.
>>>>  =20
>>>
>>> Is this also needed in do_sem_wait()? A similar pattern seems to be t=
here.
>> No, I do not think do_sem_wait() has similar issue, because the again =
label
>> does not re-get the key.
>>
>>>
>>> I ask because of what I referenced on IRC. I have some processes stuc=
k
>>> in here from a 10.4 jail.
>>>
>>>> ~/git/poudriere # procstat -kk 1498
>>>>   PID    TID COMM                TDNAME              KSTACK
>>>>  1498 100710 python2.7           -                   mi_switch+0x174=
 sleepq_switch+0x110 sleepq_catch_signals+0x417 sleepq_wait_sig+0xf _slee=
p+0x2d0 umtxq_sleep+0x153 do_sem_wait+0x42c __umtx_op_sem_wait+0x6e amd64=
_syscall+0x2bb fast_syscall_common+0x101
>>>>  1498 101575 python2.7           -                   mi_switch+0x174=
 sleepq_switch+0x110 sleepq_catch_signals+0x417 sleepq_wait_sig+0xf _slee=
p+0x2d0 umtxq_sleep+0x153 do_sem_wait+0x42c __umtx_op_sem_wait+0x6e amd64=
_syscall+0x2bb fast_syscall_common+0x101
>>>>  1498 101657 python2.7           -                   <running>
>>> ...
>>>> ~/git/poudriere # procstat -kk 1498
>>>>   PID    TID COMM                TDNAME              KSTACK
>>>>  1498 100710 python2.7           -                   mi_switch+0x174=
 sleepq_switch+0x110 sleepq_catch_signals+0x417 sleepq_wait_sig+0xf _slee=
p+0x2d0 umtxq_sleep+0x153 do_sem_wait+0x42c __umtx_op_sem_wait+0x6e amd64=
_syscall+0x2bb fast_syscall_common+0x101
>>>>  1498 101575 python2.7           -                   mi_switch+0x174=
 sleepq_switch+0x110 sleepq_catch_signals+0x417 sleepq_wait_sig+0xf _slee=
p+0x2d0 umtxq_sleep+0x153 do_sem_wait+0x42c __umtx_op_sem_wait+0x6e amd64=
_syscall+0x2bb fast_syscall_common+0x101
>>>>  1498 101657 python2.7           -                   do_sem_wait+0x1=
b6 __umtx_op_sem_wait+0x6e amd64_syscall+0x2bb fast_syscall_common+0x101
>>> ...
>>>> ~/git/poudriere # procstat -kk 94392
>>>>   PID    TID COMM                TDNAME              KSTACK
>>>> 94392 101815 python2.7           -                   mi_switch+0x174=
 sleepq_switch+0x110 sleepq_catch_signals+0x417 sleepq_wait_sig+0xf _slee=
p+0x2d0 umtxq_sleep+0x153 do_sem_wait+0x42c __umtx_op_sem_wait+0x6e amd64=
_syscall+0x2bb fast_syscall_common+0x101
>>>> 94392 101816 python2.7           -                   __mtx_lock_slee=
p+0x118 __mtx_lock_flags+0x102 _sleep+0x334 umtxq_busy+0xb7 do_sem_wait+0=
x161 __umtx_op_sem_wait+0x6e amd64_syscall+0x2bb fast_syscall_common+0x10=
1
>>>> 94392 102076 python2.7           -                   __mtx_lock_flag=
s+0x94 do_sem_wait+0x228 __umtx_op_sem_wait+0x6e amd64_syscall+0x2bb fast=
_syscall_common+0x101
>>
>> Try this.  We should only retry casueword if it failed spuriously.
>>
>> diff --git a/sys/kern/kern_umtx.c b/sys/kern/kern_umtx.c
>> index bb998457975..6c914ab6f3e 100644
>> --- a/sys/kern/kern_umtx.c
>> +++ b/sys/kern/kern_umtx.c
>> @@ -3229,7 +3229,8 @@ do_sem_wait(struct thread *td, struct _usem *sem=
, struct _umtx_time *timeout)
>>  	rv =3D casueword32(&sem->_has_waiters, 0, &count1, 1);
>>  	if (rv =3D=3D 0)
>>  		rv1 =3D fueword32(&sem->_count, &count);
>> -	if (rv =3D=3D -1 || (rv =3D=3D 0 && (rv1 =3D=3D -1 || count !=3D 0))=
 || rv =3D=3D 1) {
>> +	if (rv =3D=3D -1 || (rv =3D=3D 0 && (rv1 =3D=3D -1 || count !=3D 0))=
 ||
>> +	    (rv =3D=3D 1 && count1 =3D=3D 0)) {
>>  		umtxq_lock(&uq->uq_key);
>>  		umtxq_unbusy(&uq->uq_key);
>>  		umtxq_remove(uq);
>=20
> I think there is another problem, since even despite our intent of loop=
ing
> just because of casueword returned 1, the umtxq_check_susp() should hav=
e
> terminated the loop.  I believe the following update would fix that.
>=20
> If you have time, can you please apply only the umtxq_check_susp() chun=
k
> and see if it helps as well ? [Both chunks are needed for correctness, =
but
> the umtxq_check_susp() is almost impossible to test in combination]

What is the expected "working" test for the umtxq_check_susp() change? I
get the 100% CPU and kill -9 does not kill it.

> ~ # procstat -kk 19150
>   PID    TID COMM                TDNAME              KSTACK
> 19150 101524 python2.7           -                   do_sem_wait+0x150 =
__umtx_op_sem_wait+0x6e amd64_syscall+0x2bb fast_syscall_common+0x101
> 19150 101553 python2.7           -                   mi_switch+0x174 sl=
eepq_switch+0x110 sleepq_catch_signals+0x417 sleepq_wait_sig+0xf _sleep+0=
x2d0 umtxq_sleep+0x153 do_sem_wait+0x41c __umtx_op_sem_wait+0x6e amd64_sy=
scall+0x2bb fast_syscall_common+0x101
> 19150 101554 python2.7           -                   __mtx_lock_sleep+0=
x118 __mtx_lock_flags+0x102 _sleep+0x334 umtxq_busy+0xb7 do_sem_wait+0x15=
0 __umtx_op_sem_wait+0x6e amd64_syscall+0x2bb fast_syscall_common+0x101


>=20
> diff --git a/sys/kern/kern_umtx.c b/sys/kern/kern_umtx.c
> index bb998457975..08bdd1a1a9a 100644
> --- a/sys/kern/kern_umtx.c
> +++ b/sys/kern/kern_umtx.c
> @@ -723,13 +723,11 @@ umtxq_check_susp(struct thread *td, bool sleep)
>  	error =3D 0;
>  	p =3D td->td_proc;
>  	PROC_LOCK(p);
> -	if (P_SHOULDSTOP(p) ||
> -	    ((p->p_flag & P_TRACED) && (td->td_dbgflags & TDB_SUSPEND))) {
> -		if (p->p_flag & P_SINGLE_EXIT)
> -			error =3D EINTR;
> -		else
> -			error =3D sleep ? thread_suspend_check(0) : ERESTART;
> -	}
> +	if (p->p_flag & P_SINGLE_EXIT)
> +		error =3D EINTR;
> +	else if (P_SHOULDSTOP(p) ||
> +	    ((p->p_flag & P_TRACED) && (td->td_dbgflags & TDB_SUSPEND)))
> +		error =3D sleep ? thread_suspend_check(0) : ERESTART;
>  	PROC_UNLOCK(p);
>  	return (error);
>  }
> @@ -3229,7 +3227,8 @@ do_sem_wait(struct thread *td, struct _usem *sem,=
 struct _umtx_time *timeout)
>  	rv =3D casueword32(&sem->_has_waiters, 0, &count1, 1);
>  	if (rv =3D=3D 0)
>  		rv1 =3D fueword32(&sem->_count, &count);
> -	if (rv =3D=3D -1 || (rv =3D=3D 0 && (rv1 =3D=3D -1 || count !=3D 0)) =
|| rv =3D=3D 1) {
> +	if (rv =3D=3D -1 || (rv =3D=3D 0 && (rv1 =3D=3D -1 || count !=3D 0)) =
||
> +	    (rv =3D=3D 1 && count1 =3D=3D 0)) {
>  		umtxq_lock(&uq->uq_key);
>  		umtxq_unbusy(&uq->uq_key);
>  		umtxq_remove(uq);
>=20


--=20
Regards,
Bryan Drewery


--H026HVpn5qTcefLIsmZMlKeUwrSTao0fy--

--8uyUCaTumRMJ4zEOnwjMI9AZI8qMzKRIp
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGTBAEBCgB9FiEE+Rc8ssOq6npcih8JNddxu25Gl88FAl1A+pFfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldEY5
MTczQ0IyQzNBQUVBN0E1QzhBMUYwOTM1RDc3MUJCNkU0Njk3Q0YACgkQNddxu25G
l89ouwgAj8TT0zduz0lz1I388JEkfuITnm0qhBwXdNQGAixRzLCLn61GJtFgXnDC
j/7MBqC23he6mS+hx1XnzCV+6fLfQlG4Tyr1Ae3ywfTYF5aB4t4xZzXi0bHlMmeQ
aaVU0cY5Q7MmIe65JPQY8uZw+Jg2b8kYxMYTGZwtRYFcbyo5Ql7mxZIPTt+dkPVp
d5+bmMf0HYoqcF9f4rrFLfLoD5FeN9vw2/87st+rf3A2JZu2bOMJRa8SvChh/ydC
NjaO1RLoHQ01/fJDO6kZaUiMNDGC/ZiaZ66ilI3lJwyUYhoquth3XyXzRJtiF0Hw
bCzc35dBAJYiNnllMzYRXZPfOMzqHA==
=btpK
-----END PGP SIGNATURE-----

--8uyUCaTumRMJ4zEOnwjMI9AZI8qMzKRIp--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47623274-f1be-7134-2f29-4941776df39f>