* Re: [RFC PATCH v1 27/50] drivers/s390/scsi/zcsp_fc.c: Use prandom_u32_max() for backoff
2020-03-31 16:13 ` [RFC PATCH v1 27/50] drivers/s390/scsi/zcsp_fc.c: Use prandom_u32_max() for backoff Benjamin Block
@ 2020-03-31 16:23 ` Steffen Maier
2020-03-31 19:04 ` George Spelvin
1 sibling, 0 replies; 3+ messages in thread
From: Steffen Maier @ 2020-03-31 16:23 UTC (permalink / raw)
To: Benjamin Block, George Spelvin
Cc: linux-kernel, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, linux-s390, linux-scsi
On 3/31/20 6:13 PM, Benjamin Block wrote:
> On Fri, Nov 29, 2019 at 03:39:41PM -0500, George Spelvin wrote:
>> We don't need crypto-grade random numbers for randomized backoffs.
>>
>> (We could skip the if() if we wanted to rely on the undocumented fact
>> that prandom_u32_max(0) always returns 0. That would be a net time
>> saving it port_scan_backoff == 0 is rare; if it's common, the if()
>> is false often enough to pay for itself. Not sure which applies here.)
>>
>> Signed-off-by: George Spelvin <lkml@sdf.org>
>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Cc: linux-s390@vger.kernel.org
>> ---
>> drivers/s390/scsi/zfcp_fc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Hello George,
>
> it would be nice, if you could address the mails to the
> driver-maintainers (`scripts/get_maintainer.pl drivers/s390/scsi/zfcp_fc.c`
> will tell you that this is me and Steffen); I'd certainly have noticed
> it earlier then :-).
>
>>
>> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
>> index b018b61bd168e..d24cafe02708f 100644
>> --- a/drivers/s390/scsi/zfcp_fc.c
>> +++ b/drivers/s390/scsi/zfcp_fc.c
>> @@ -48,7 +48,7 @@ unsigned int zfcp_fc_port_scan_backoff(void)
>> {
>> if (!port_scan_backoff)
>> return 0;
>> - return get_random_int() % port_scan_backoff;
>> + return prandom_u32_max(port_scan_backoff);
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
>
> I think the change is fine. You are right, we don't need a crypto nonce
> here.
>
> I think I'd let the zero-check stand as is, because the internal
> behaviour of prandom_u32_max() is, as you say, undocumented. This is not
> a performance critical code-path for us anyway.
yes, let's keep the extra check as it's intentional and documented user
interface for zfcp, so better be explicit
>
>> }
>>
>> static void zfcp_fc_port_scan_time(struct zfcp_adapter *adapter)
>> --
>> 2.26.0
>>
>
> Steffen, do you have any objections? Otherwise I can queue this up -
> minus the somewhat mangled subject - for when we send something next time.
>
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z Development
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [RFC PATCH v1 27/50] drivers/s390/scsi/zcsp_fc.c: Use prandom_u32_max() for backoff
2020-03-31 16:13 ` [RFC PATCH v1 27/50] drivers/s390/scsi/zcsp_fc.c: Use prandom_u32_max() for backoff Benjamin Block
2020-03-31 16:23 ` Steffen Maier
@ 2020-03-31 19:04 ` George Spelvin
1 sibling, 0 replies; 3+ messages in thread
From: George Spelvin @ 2020-03-31 19:04 UTC (permalink / raw)
To: Benjamin Block, Steffen Maier
Cc: linux-kernel, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, linux-s390, linux-scsi, lkml
On Tue, Mar 31, 2020 at 06:13:21PM +0200, Benjamin Block wrote:
> it would be nice, if you could address the mails to the
> driver-maintainers (`scripts/get_maintainer.pl drivers/s390/scsi/zfcp_fc.c`
> will tell you that this is me and Steffen); I'd certainly have noticed
> it earlier then :-).
How the $%$# did I mess that up? I know choosing recipients for
this series was mostly manual, becase it didn't fit the usual
pattern of the entire series going to everyone affectrd by any part
of it.
And then there waqs a whole lot of shuffling things into a logical
order and grouping.
But I checked MAINTAINERS originally, I really did. :-(
> On Fri, Nov 29, 2019 at 03:39:41PM -0500, George Spelvin wrote:
>> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
>> index b018b61bd168e..d24cafe02708f 100644
>> --- a/drivers/s390/scsi/zfcp_fc.c
>> +++ b/drivers/s390/scsi/zfcp_fc.c
>> @@ -48,7 +48,7 @@ unsigned int zfcp_fc_port_scan_backoff(void)
>> {
>> if (!port_scan_backoff)
>> return 0;
>> - return get_random_int() % port_scan_backoff;
>> + return prandom_u32_max(port_scan_backoff);
>
> I think the change is fine. You are right, we don't need a crypto nonce
> here.
>
> I think I'd let the zero-check stand as is, because the internal
> behaviour of prandom_u32_max() is, as you say, undocumented. This is not
> a performance critical code-path for us anyway.
I agree. Sorry, that comment in the commit message was a bit of
a "not to self" that I didn't clean up. Feel free to rm it when
queueing if you like.
> Steffen, do you have any objections? Otherwise I can queue this up -
> minus the somewhat mangled subject - for when we send something next time.
Thank you, I'll put it in my "accepted" pile.
^ permalink raw reply [flat|nested] 3+ messages in thread