Re: [PATCH v6 1/3] KVM: selftests: implement random number generation for guest code

From: Sean Christopherson <seanjc@google.com>
To: Colton Lewis <coltonlewis@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, maz@kernel.org,
	dmatlack@google.com, oupton@google.com, ricarkol@google.com,
	andrew.jones@linux.dev
Subject: Re: [PATCH v6 1/3] KVM: selftests: implement random number generation for guest code
Date: Tue, 11 Oct 2022 18:39:48 +0000	[thread overview]
Message-ID: <Y0W4dImhloev7Iaq@google.com> (raw)
In-Reply-To: <gsntfsfu2pt1.fsf@coltonlewis-kvm.c.googlers.com>

On Tue, Oct 11, 2022, Colton Lewis wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Mon, Sep 12, 2022, Colton Lewis wrote:
> > > diff --git a/tools/testing/selftests/kvm/include/test_util.h
> > > b/tools/testing/selftests/kvm/include/test_util.h
> > > index 99e0dcdc923f..2dd286bcf46f 100644
> > > --- a/tools/testing/selftests/kvm/include/test_util.h
> > > +++ b/tools/testing/selftests/kvm/include/test_util.h
> > > @@ -143,4 +143,6 @@ static inline void *align_ptr_up(void *x, size_t
> > > size)
> > >   	return (void *)align_up((unsigned long)x, size);
> > >   }
> 
> > > +void guest_random(uint32_t *seed);
> 
> > This is a weird and unintuitive API.  The in/out param exposes the gory
> > details of the pseudo-RNG to the caller, and makes it cumbersome to use,
> > e.g. to create a 64-bit number or to consume the result in a conditional.
> 
> To explain my reasoning:
> 
> It's simple because there is exactly one way to use it and it's short so
> anyone can understand how the function works at a glance. It's similar
> to the API used by other thread-safe RNGs like rand_r and random_r. They
> also use in/out parameters. That's the only way to buy thread
> safety. Callers would also have to manage their own state in your
> example with an in/out parameter if they want thread safety.
> 
> I disagree the details are gory. You put in a number and get a new
> number.

Regardless of whether or not the details are gory, having to be aware of those
details unnecessarily impedes understanding the code.  The vast, vast majority of
folks that read this code won't care about how PRNGs work.  Even if the reader is
familiar with PRNGs, those details aren't at all relevant to understanding what
the guest code does.  The reader only needs to know "oh, this is randomizing the
address".  How the randomization works is completely irrelevant for that level of
understanding.

> It's common knowledge PRNGs work this way.

For people that are familiar with PRNGs, yes, but there will undoubtedly be people
that read this code and have practically zero experience with PRNGs.

> I understand you are thinking about ease of future extensions, but this
> strikes me as premature abstraction. Additional APIs can always be added
> later for the fancy stuff without modifying the callers that don't need it.
> 
> I agree returning the value could make it easier to use as part of
> expressions, but is it that important?

Yes, because in pretty much every use case, the random number is going to be
immediately consumed.  Readability and robustness aside, returning the value cuts
the amount of code need to generate and consume a random number in half.

> > It's also not inherently guest-specific, or even KVM specific.  We
> > should consider
> > landing this in common selftests code so that others can use it and even
> > expand on
> > it.  E.g. in a previous life, I worked with a tool that implemented all
> > sorts of
> > random number magic that provided APIs to get random bools with 1->99
> > probabilty,
> > random numbers along Guassian curves, bounded numbers, etc.
> 
> 
> People who need random numbers outside the guest should use stdlib.h. No
> need to reimplement a full random library. The point of this
> implementation was to do the simplest thing that could provide random
> numbers to the guest.

Ah, right.