Re: [PATCH v1 1/2] random-bits: Factor out entropy generating function

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

On 04/04/2022 16:48, Noah Goldstein wrote:
> On Mon, Apr 4, 2022 at 2:20 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 04/04/2022 15:52, Noah Goldstein wrote:
>>> On Mon, Apr 4, 2022 at 1:38 PM Adhemerval Zanella
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 04/04/2022 15:23, Noah Goldstein wrote:
>>>>> On Mon, Apr 4, 2022 at 12:42 PM Adhemerval Zanella
>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 01/04/2022 15:01, Cristian Rodríguez wrote:
>>>>>>> On Thu, Mar 31, 2022 at 8:05 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>>>>>>
>>>>>>>> AFAIK our goal is entropy more so than security. For example
>>>>>>>> if this is used to generate jiffies to stagger threads its not a security
>>>>>>>> issue in any sense, it's just not ideal for performance.
>>>>>>>
>>>>>>> In any case this should be more than fast enough for the other use
>>>>>>> cases of random_bits() .. maybe one new random_bits_fast() function
>>>>>>> foe edge cases where even this is too slow?
>>>>>>
>>>>>> I think we are bike-shedding in the same issue OpenBSD guys stumbled
>>>>>> and which they have solved 10 years ago [1].  Essentially, we need to
>>>>>> come  up with a internal PRNG interface that can be used internally and
>>>>>> externally, instead of reinventing cleaver ways to use the timer as
>>>>>> entropy source.
>>>>>>
>>>>>> The issue is not really the cypher used, ideally it could be replace if
>>>>>> we find out that it does not fit.  The main issue is to glue together
>>>>>> all the requirements to have a concise internal interface, taking in
>>>>>> consideration the glibc constrains to work with multiple kernel version
>>>>>> and environments (where we can't assume we have access to a source or
>>>>>> reliable entropy like getrandom syscall).
>>>>>>
>>>>>> My plan to rehearse Florian arc4random proposal to have some simpler
>>>>>> to where we might improve upon (a simpler fork detection for kernels
>>>>>> without MADV_WIPEONFORK that just issue a atfork handle, maybe using
>>>>>> ChaCha20 as virtually all other systems do, no per-thread state).
>>>>>
>>>>> Why no per-thread / per-cpu? It seems otherwise there will need to be
>>>>> some explicit synchronization on the stream.
>>>>
>>>> Mainly because it simplifies a lot the *initial* implementation. I would
>>>> prefer to incremental add per-thread optimization than dump a large patch
>>>> so we can review in integrate the code more easier.
>>>>
>>>>>
>>>>>
>>>>> Regarding this patch, do we want to skip it and just wait on arc4random
>>>>> interface in kernel/glibc or should I go forward with it and some arch
>>>>> specific entropy functions in the mean-time?
>>>>
>>>> I don't have a strong opinion on this patch, it does improve x86_64
>>>> latency on random_bits although currently internal usage are far from
>>>> latency sensitive.  It is really a microoptimization without much real
>>>> work gain for current code.
>>>
>>> That's fair. The motivation is so random_bits can be used for lightweight
>>> jitter i.e in cases like:
>>>
>>> [v2] nptl: Add backoff mechanism to spinlock loop
>>>
>>> The x86 backend with `rdtsc` is just because it's a simple improvement,
>>> other arch will hopefully be able to get off syscall if they don't have vdso
>>> gettime.
>>> But agree the improvement gains a minimal so if people don't feel its worth
>>> the added complexity we can wait on a strong arch-random interface.
>>
>> I think for mutex optimization it would be better to just add a arch-specific
>> jitter code and use the backoff optimization iff the arch-specific code is
>> used. And maybe not tied to random_bits(), since I am not sure if an interface
>> like arc4random would be good to use in such scenario (since a possible state
>> reschedule might call getentropy with my add unexpected latency).
> 
> So you propose drop this patch in favor of some new internal interface like
> get_fast_jitter() and a define such as 'HAS_FAST_JITTER'?

Yeah, this would be better than try to improve random_bits.  Also, recently
we are trying to avoid internal HAS/HAVE defines, it is better to add a
generic interface that does either nothing or return a default value
(so default get_fast_jitter() would return 1 or 0, so the mutex code will
se it just do the current spinning without backoff).