Re: [PATCH v6 03/10] benchtests: Add arc4random benchtest

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

> On 28 Jun 2022, at 09:01, Florian Weimer <fweimer@redhat.com> wrote:
> 
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/benchtests/bench-arc4random.c b/benchtests/bench-arc4random.c
>> new file mode 100644
>> index 0000000000..626f2ba48c
>> --- /dev/null
>> +++ b/benchtests/bench-arc4random.c
>> @@ -0,0 +1,224 @@
> 
>> +/* Prevent compiler to optimize away call.  */
>> +#define DO_NOT_OPTIMIZE(__value)		\
>> +  ({						\
>> +    __typeof (__value) __v = (__value);		\
>> +    asm volatile("" : : "r,m" (__v) : "memory");\
>> +  })
> 
> Missing space after “volatile”.

Ack.

> 
> Maybe this should be a generic building block for benchtests?

Indeed, I moved it to benchtests/bench-util.h.

> 
>> +static void
>> +run_bench (json_ctx_t *json_ctx, const char *name,
>> +	   char *const*fnames, size_t fnameslen,
>> +	   void (*bench)(json_ctx_t *ctx))
> 
> Missing space between “)(”.

Ack.

> 
> 
>> diff --git a/stdlib/arc4random.c b/stdlib/arc4random.c
>> index f6553dfd7d..74c917b9b9 100644
>> --- a/stdlib/arc4random.c
>> +++ b/stdlib/arc4random.c
>> @@ -25,9 +25,9 @@
>> #include <sys/param.h>
>> #include <sys/random.h>
>> 
>> -/* Besides the cipher state 'ctx', it keeps two counters: 'have' is the
>> -   current valid bytes not yet consumed in 'buf', while 'count' is the maximum
>> -   number of bytes until a reseed.
>> +/* arc4random keeps two counters: 'have' is the current valid bytes not yet
>> +   consumed in 'buf' while 'count' is the maximum number of bytes until a
>> +   reseed.
>> 
>>    Both the initial seed and reseed try to obtain entropy from the kernel
>>    and abort the process if none could be obtained.
>> @@ -81,10 +81,10 @@ arc4random_getrandom_failure (void)
>>   __libc_fatal ("Fatal glibc error: Cannot get entropy for arc4random\n");
>> }
>> 
>> -/* Fork detection is done by checking if MADV_WIPEONFORK supported.  If not
>> -   the fork callback will reset the state on the fork call.  It does not
>> -   handle direct clone calls, nor vfork or _Fork (arc4random is not
>> -   async-signal-safe due the internal lock usage).  */
>> +/* If the kernel supports MADV_WIPEONFORK it is used to provide fork
>> +   detection. Otherwise, the state is resetied with an atfork handler. The
>> +   latter does not handle direct clone calls, nor vfork or _Fork (arc4random
>> +   is not async-signal-safe due to the internal lock usage).  */
>> static void
>> arc4random_init (uint8_t *buf)
>> {
> 
> I think this belongs into some other patch.

Indeed, I will fix it.

> 
> Thanks,
> Florian
>