Re: [PATCH net-next 1/2] ebpf: add prandom helper for packet sampling

[Alexei Starovoitov <ast@plumgrid.com>]

On 3/13/15 3:44 PM, Daniel Borkmann wrote:
> On 03/13/2015 06:03 PM, Daniel Borkmann wrote:
>> On 03/13/2015 05:58 PM, Alexei Starovoitov wrote:
> ...
>>>> +static u64 bpf_random(u64 dw, u64 r2, u64 r3, u64 r4, u64 r5)
>>>> +{
>>>> +    return prandom_u32() | (dw ? ((u64)prandom_u32() << 32) : 0);
>>>
>>> can you make first argument to be a flag instead of bool ?
>>> 0 to return u32, 1 to return u64 and >=2 to be reserved and
>>> return -EINVAL?
> ...
>>> This way we can extend it later. For example 2 might return
>>> 8 truly random bytes from get_random_bytes_arch()
>
> Hmm, just got back to this thinking about it a bit more. I don't
> like that we would return with a negative error here. An eBPF
> application would then need to check for errors as well to make
> sure, and the signature returns max u64. So _theoretically_,
> -EINVAL could also come as a result from prandom() albeit unlikely.
> That would make the interface a bit ugly, imho.
>
> I think the interface should be as simple as possible for
> obtaining a random number. I think that if we would want to
> support something like get_random_bytes_arch(), it would be
> better to do so in a new interface, where the name also more
> clearly indicates where the random data comes from.
>
> That said, I'd rather prefer to leave the code as is, but go
> with the rename, maybe even BPF_FUNC_get_prandom_val so it
> indicates it's pseudo-random as opposed to, say, something like
> BPF_FUNC_get_srandom_val if we need that one day.

fair point.

I was also thinking about it more and I believe that 'bool' flag
is redundant.
If program wants to have 64-bit value it should just call this
helper twice. It actually will be faster, since such 64-bit
value will be without branches whereas 'dw' flag introduces
if condition in critical path that will be executed for every packet.
So my vote is for BPF_FUNC_get_prandom_u32 that returns
prandom_u32() and nothing else.