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 <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Vasily Gorbik <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Cc: [email protected]
---
drivers/s390/scsi/zfcp_fc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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);
}
static void zfcp_fc_port_scan_time(struct zfcp_adapter *adapter)
--
2.26.0
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 <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Vasily Gorbik <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> Cc: [email protected]
> ---
> 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);
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.
> }
>
> 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.
--
Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen / Gesch?ftsf?hrung: Dirk Wittkopp
Sitz der Gesellschaft: B?blingen / Registergericht: AmtsG Stuttgart, HRB 243294
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 <[email protected]>
>> Cc: Heiko Carstens <[email protected]>
>> Cc: Vasily Gorbik <[email protected]>
>> Cc: Christian Borntraeger <[email protected]>
>> Cc: [email protected]
>> ---
>> 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 <[email protected]>
>
> 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
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.