From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751946AbdLFNHA (ORCPT ); Wed, 6 Dec 2017 08:07:00 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:33360 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751443AbdLFNG4 (ORCPT ); Wed, 6 Dec 2017 08:06:56 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20171206130653euoutp026df98221631cc306e5dc913f87fb8db9~9tmlRIWfd0743007430euoutp02d X-AuditID: cbfec7ef-f79ee6d000003120-db-5a27eb6ce853 From: =?utf-8?Q?=C5=81ukasz_Stelmach?= To: Krzysztof Kozlowski Cc: Stephan Mueller , robh+dt@kernel.org, Herbert Xu , "David S. Miller" , Kukjin Kim , linux-crypto@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Marek Szyprowski , =?utf-8?Q?Bart=C5=82omiej_=C5=BBo?= =?utf-8?Q?=C5=82nierkiewicz?= Subject: Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG Date: Wed, 06 Dec 2017 14:06:50 +0100 In-reply-to: Message-id: <87h8t49f9x.fsf%l.stelmach@samsung.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-version: 1.0 Content-type: multipart/signed; boundary="=-=-="; micalg="pgp-sha256"; protocol="application/pgp-signature" X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrGKsWRmVeSWpSXmKPExsWy7djP87o5r9WjDDZ9FLDYOGM9q8Wc8y0s Ft2vZCz6H79mtjh/fgO7xf17P5ksLu+aw2Yx4/w+Jou1R+6yW7TuPcJuMfX0WhYHbo9PV64w eWxZeZPJY9sBVY9NqzrZPPq2rGL0+LxJLoAtissmJTUnsyy1SN8ugStj5dTXLAWfNCseHnjF 2MA4X6mLkZNDQsBE4vnCGYwQtpjEhXvr2boYuTiEBJYxSiy+cpsRwvnMKDF9x3UmmI63Ew6x wFXNu3kDquoLo8T5W/+ZQarYBOwl+o/sYwGxRQQ0Ja7//c4KUsQssJhZ4t+CY6wgCWEBV4lZ lzeDNbAIqEqsWNQOZnMKBEvMvXSDHcTmFTCWmHnsMthqUQFLiXt9d9kg4oISPybfA1vALJAr 8anpPzvIAgmBdnaJ682zWSFudZHoXT+XGcIWlnh1fAs7hC0jcXlyNwtEQz+jxOH536ESU4De XugAYVtL/Fk1kQ1iA5/EpG3TgQZxAMV5JTrahCBKPCQeL34CDT1HifY/X5kgQbEEaMzXfpYJ jLKzkBw7C8mxs4BGMQNDZv0ufYiwtsSyha+ZIWxbiXXr3rMsYGRdxSiSWlqcm55abKhXnJhb XJqXrpecn7uJEZiCTv87/n4H49PmkEOMAhyMSjy8F16qRwmxJpYVV+YeYlQBGvNow+oLjFIs efl5qUoivJcvA6V5UxIrq1KL8uOLSnNSiw8xSnOwKInz2ka1RQoJpCeWpGanphakFsFkmTg4 pRoYt0jdXGV+d87F2JwHHZO6xSxdXnL5cLDeCzh2PHmlQ6xZQftV58VbnI/JzpYQ3i29/F/1 JCW9Y1X3pzjZKlfrX58w+2VVYOH/zvbQxJP6wZ9Pe7/iEsrTLTura7cr/KW5/XwBtac2C+r/ fDNZmCf/oMn2+awbO/d8vHDm1Nmeu23Niq8PerbrKLEUZyQaajEXFScCAN7gH6lJAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupkkeLIzCtJLcpLzFFi42I5/e/4Fd3s1+pRBl0/lS02zljPajHnfAuL RfcrGYv+x6+ZLc6f38Bucf/eTyaLy7vmsFnMOL+PyWLtkbvsFq17j7BbTD29lsWB2+PTlStM HltW3mTy2HZA1WPTqk42j74tqxg9Pm+SC2CL4rJJSc3JLEst0rdL4MpYOfU1S8EnzYqHB14x NjDOV+pi5OSQEDCReDvhEAuELSZx4d56ti5GLg4hgSWMElt6prCDJIQEvjFKLHiVBWKzCdhL 9B/ZB9YgIqApcf3vd1aQBmaBpcwSD56uZwJJCAu4Ssy6vJkZojlAovvDTrA4i4CqxIpF7WBx ToFgiZNfH4LFeQWMJWYeuwxmiwpYStzru8sGEReU+DH5HtgyZoFsiQsX37BMYOSfhSQ1C0lq FiMHkK0psX6XPkRYW2LZwtfMELatxLp171kWMLKuYhRJLS3OTc8tNtIrTswtLs1L10vOz93E CIyUbcd+btnB2PUu+BCjAAejEg/vhZfqUUKsiWXFlbmHGFWAxjzasPoCoxRLXn5eqpII7+XL QGnelMTKqtSi/Pii0pzU4kOM0hwsSuK8vXtWRwoJpCeWpGanphakFsFkmTg4pRoYk2SUnq3c cbX9w7RwwX/Lv95waXIul7ri01fOFKJtEX9+6RzWCzs57Fjqvb/uk1zv8evq6YBpke5zs/k2 qmbsP+uoe+H3/ZD5dsd+7T8sXK8//TrX0dPcURO2PMl4bpdj9ixVXuBHSJsiI9t8uzUJEr2T YuJaFa7/f6d6aPEDFqaN7Nu/Njw+o8RSnJFoqMVcVJwIAGKmHjKcAgAA X-CMS-MailID: 20171206130651eucas1p22b5d0799f2a128d3d9efcc799fc3cfdc X-Msg-Generator: CA CMS-TYPE: 201P X-CMS-RootMailID: 20171206130651eucas1p22b5d0799f2a128d3d9efcc799fc3cfdc X-RootMTR: 20171206130651eucas1p22b5d0799f2a128d3d9efcc799fc3cfdc References: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable It was <2017-12-06 =C5=9Bro 12:37>, when Krzysztof Kozlowski wrote: > On Wed, Dec 6, 2017 at 12:32 PM, =C5=81ukasz Stelmach wrote: >> It was <2017-12-05 wto 19:06>, when Krzysztof Kozlowski wrote: >>> On Tue, Dec 5, 2017 at 6:53 PM, Krzysztof Kozlowski w= rote: >>>> On Tue, Dec 05, 2017 at 05:43:10PM +0100, =C5=81ukasz Stelmach wrote: >>>>> It was <2017-12-05 wto 14:54>, when Stephan Mueller wrote: >>>>>> Am Dienstag, 5. Dezember 2017, 13:35:57 CET schrieb =C5=81ukasz Stel= mach: >>>>>>> Use memcpy_fromio() instead of custom exynos_rng_copy_random() func= tion >>>>>>> to retrieve generated numbers from the registers of PRNG. >>>>>>> >>>>>>> Remove unnecessary invocation of cpu_relax(). >>>>>>> >>>>>>> Signed-off-by: =C5=81ukasz Stelmach >>>>>>> --- >>>>>>> drivers/crypto/exynos-rng.c | 36 +++++----------------------------= --- >>>>>>> 1 file changed, 5 insertions(+), 31 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rn= g.c >>>>>>> index 894ef93ef5ec..002e9d2a83cc 100644 >>>>>>> --- a/drivers/crypto/exynos-rng.c >>>>>>> +++ b/drivers/crypto/exynos-rng.c >>>>> >>>>> [...] >>>>> >>>>>>> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct exynos_= rng_dev >>>>>>> *rng, { >>>>>>> int retry =3D EXYNOS_RNG_WAIT_RETRIES; >>>>>>> >>>>>>> + *read =3D min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE); >>>>>>> + >>>>>>> if (rng->type =3D=3D EXYNOS_PRNG_TYPE4) { >>>>>>> exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START, >>>>>>> EXYNOS_RNG_CONTROL); >>>>>>> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct exynos_= rng_dev >>>>>>> *rng, } >>>>>>> >>>>>>> while (!(exynos_rng_readl(rng, >>>>>>> - EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE)= && --retry) >>>>>>> - cpu_relax(); >>>>>>> + EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE)= && >>>>>>> + --retry); >> >> [...] >> >>>>> The busy loop is not very busy. Every time I checked the loop (w/o >>>>> cpu_relax()) was executed twice (retry was 98) and the operation was >>>>> reliable. I don't see why do we need a memory barrier here. On the ot= her >>>>> hand, I am not sure the whole exynos_rng_get_random() shouldn't be ran >>>>> under a mutex or a spinlock (I don't see anything like this in the up= per >>>>> layers of the crypto framework). >>>>> >>>>> The *_relaxed() I/O operations do not enforce memory >>>> >>>> The cpu_relax() is a common pattern for busy-loop. If you want to break >>>> this pattern - please explain why only this part of kernel should not >>>> follow it (and rest of kernel should). >>>> >>>> The other part - this code is already using relaxed versions which mig= ht >>>> get you into difficult to debug issues. You mentioned that loop works >>>> reliable after removing the cpu_relax... yeah, it might for 99.999% but >>>> that's not the argument. I remember few emails from Arnd Bergmann >>>> mentioning explicitly to avoid using relaxed versions "just because", >>>> unless it is necessary or really understood. >>>> >>>> The code first writes to control register, then checks for status so y= ou >>>> should have these operations strictly ordered. Therefore I think >>>> cpu_relax() should not be removed. >>> >>> ... or just convert it to readl_poll_timeout() because it makes code >>> more readable, takes care of timeout and you do not have care about >>> specific implementation (whether there should or should not be >>> cpu_relax). >> >> OK. This appears to perform reasonably. >> >> do { >> cpu_relax(); >> } while (!(exynos_rng_readl(rng, EXYNOS_RNG_STATUS) & >> EXYNOS_RNG_STATUS_RNG_DONE) && --retry); > > You mean that: > while (readl_relaxed()) cpu_relax(); > is slower than > do cpu_relax() while (readl_relaxed()) > ? > > Hmm... Interesting. I would be happy to learn more about it why it > behaves so differently. Maybe the execution of cpu_relax() before > readl_relaxed() reduces the amount of loops to actually one read? > > Indeed some parts of kernel code for ARM prefers this approach, > although still the most popular pattern is existing one (while() > cpu_relax). Without cpu_relax() retry is decremented twice. So there are three reads. It appears that a single call cpu_relax() gives the hardware enough time to be ready when the first read occurs (retry is 99 after the loop). =2D-=20 =C5=81ukasz Stelmach Samsung R&D Institute Poland Samsung Electronics --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBCAAGBQJaJ+tqAAoJELCuHpyYpYAQBeAH/34JmrtzI7k9+hgt5WK5pEo5 /mPUuXmiujDopjHiWp1C33fYrcGIaourU7+vJJFnOgmtew4S3SN/of6IiY+tTHs9 wveY6lg+u94YlzXdXxISdI1+Ac2796xeLKvQ0OzgXpJTQevBBsgBOltfWqjZngGq mjTQcpOWeWmMJ3MefxXcCwbuYTB/PEqVq9R9gLmUdbPpIKCUwVgHSHQim6gEM665 0i8g411FBrsnWdYo/fDawDe8KZ7jUgmalImxc+L+6YE+I18zNBduiJ77Z+LJBwdS f3BY1NRmqDtvQnGGiBJaNbO+uNnYyUZVzqksUghmvyn0pk4h293XhJ9VBnBSco0= =/tBU -----END PGP SIGNATURE----- --=-=-=--