Date:   Thu, 11 Apr 2019 22:49:22 +0100
From:   Jiong Wang <jiong.wang@...ronome.com>
To:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     Jakub Kicinski <jakub.kicinski@...ronome.com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, oss-drivers@...ronome.com
Subject: Re: [PATCH/RFC v2 bpf-next 11/19] libbpf: new global variable
 "libbpf_test_mode"


> On 11 Apr 2019, at 15:32, Jiong Wang <jiong.wang@...ronome.com> wrote:
> 
> 
>> On 11 Apr 2019, at 04:19, Jakub Kicinski <jakub.kicinski@...ronome.com> wrote:
>> 
>> On Wed, 10 Apr 2019 20:50:25 +0100, Jiong Wang wrote:
>>> Enable BPF_F_TEST_RND_HI32 for all existing bpf selftests or other
>>> independent tests could involve quite a few changes to make sure all bpf
>>> prog load places has BPF_F_TEST_RND_HI32 set.
>>> 
>>> Given most of the tests are using libbpf, this patch introduces a new
>>> global variable "libbpf_test_mode" into libbpf, once which is set, all bpf
>>> prog load issued through libbpf will have BPF_F_TEST_RND_HI32 set
>>> automatically, this could minimize changes required from testsuite.
>>> 
>>> The other way might be introducing new load function like
>>> "bpf_prog_test_load", which will set BPF_F_TEST_RND_HI32. But there are
>>> several prog load APIs, and we need minor changes on some parameters.
>>> 
>>> The global variable approach seems to be a proper first step for easy
>>> testsuite porting.
>>> 
>>> Signed-off-by: Jiong Wang <jiong.wang@...ronome.com>
>> 
>> Can we perhaps make it per-object by setting it after
>> bpf_object__open() but before bpf_object__load(), or add 
>> it to struct bpf_object_open_attr?
> 
> Not sure I followed the meaning correctly. My read is you mean it would
> better if there is more accurate and fine control on this feature, especially
> if prog_flags could become an API parameter during file/obj/prog load?
> 
> As mentioned in the cover letter, I tried to implement bpf_prog_test_load
> In parallel with existing bpf_prog_test_run, but then found there are
> also similar APIs for bpf_object__open, and raw insn load “bpf_load_progams”
> etc and they may involve extending bpf_prog_load_attr/bpf_load_program_attr
> etc. I thought the change is a little bit heavy. I could go further if it
> is thought to be the correct way or there is any other suggestion on nicely
> passing “prog_flags” to low level bpf syscall in libbpf.
> 

Would really appreciate more feedbacks on how to do the libbpf change for       
the next patch set re-spin.

Whether this global variable approach is acceptable or we must find another     
way which enable high 32-bit randomization for all tests under                  
test_verifier/test_progs with reasonable testsuite and libbpf change.           

I feel the global variable approach taken in this patch has done its job        
with minimum change, that is to offer a interface to enable high 32-bit         
randomization for all tests under test_verifier/test_progs to deliver           
equal stressfull tests on x86_64 platform and to expose bugs in the              
optimization, now given there is no regression, we could have confidence        
on the correctness of the optimization. If we want a more clean change in       
libbpf, it is more about how to offer clean API to pass "prog_flags" when       
load file/obj/raw_insn, it looks to me could be a separate patch set.           
However, for this set, we also need to make sure high 32-bit randomization       
*always enabled* for all test_verifier + test_progs so the new opt is
tested in daily bpf selftest.

Regards,
Jiong

Powered by blists - more mailing lists