Date:   Tue, 16 Apr 2019 12:11:22 +0530
From:   "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:     Jiong Wang <jiong.wang@...ronome.com>
Cc:     alexei.starovoitov@...il.com, bpf@...r.kernel.org,
        daniel@...earbox.net, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH v3 bpf-next 08/19] bpf: insert explicit zero extension
 insn when hardware doesn't do it implicitly

Jiong Wang wrote:
> 
>> On 15 Apr 2019, at 19:21, Naveen N. Rao <naveen.n.rao@...ux.vnet.ibm.com> wrote:
>> 
>> Jiong Wang wrote:
>>> It will be great if you could test the latest set on PowerPC to see if
>>> there is any regression for example for those under test_progs and
>>> test_verifier.
>> 
>> With test_bpf, I am seeing a few failures with this patchset.
>> 
>>> And it will be even greater if you also use latest llvm snapshot for the
>>> testing, which then will enable test_progs_32 etc.
>> 
>> Is a newer llvm a dependency? Or, is this also expected to work with older llvm levels?
> 
> There is no newer LLVM dependency. This set should work with older llvm.        
>                                                                                 
> It is just newer LLVM has better sub-register code-gen support that could       
> the generate bpf program contains more elimination opportunities for verifier.

Ok, I will try and get to that by next week (busy with other things 
right now).

> 
>> 
>> The set of tests that are failing are listed further below. I looked into MUL_X2 and it looks like zero extension for the two initial ALU32 loads (-1) are being removed, resulting in the failure.
>> 
>> I didn't get to look into this in detail -- am I missing something?
> 
> Hmm, I guess the issue is:
>                                                                                 
>   1. test_bpf.c is a testsuite running inside kernel space, it is calling some
>      kernel eBPF jit interface directly without calling verifier first, so this
>      set actually hasn’t been triggered.

Ah, indeed.

> 
>   2. However, the elimination information at the moment is passed from verifier
>      to JIT backend through
> 
>        fp->aux->no_verifier_zext
> 
>      “no_verifier_zext” is initially false, and once verifier inserted zero
>      extension, it will be set to true.
> 
>      Now, for test_bpf, because it doesn’t go through verifier at all, so
>      “no_verifier_zext” is left at default value which is false, meaning
>      verifier has inserted zero-extension, so PPC backend then thinks it is
>      safe to eliminate zero-extension by himself.
> 
>      Perhaps should change “no_verifier_zext” to “verifier_zext”, then default
>      is false and will only be true when verifier really has inserted zext.

Yes, that's probably better.

>       
>      Was thinking, this will cause JIT backend writing the check like
>         if (no_verifier_zext)
>           insert_zext_by_JIT
>      
>      is better than:
>         
>         if (!verifier_zext)
>           insert_zext_by_JIT
> 
> BTW, does test_progs and test_verifier has a full pass on PowerPC?
> On arch without hardware zext like PowerPC, verifier will insert zext and test
> mode will still randomisation high 32-bit for those sub-registers not zext,
> this is very stressful test.

test_verfier is throwing up one failure with this patchset:
#569/p ld_abs: vlan + abs, test 1 FAIL
Failed to load prog 'Success'!
insn 2463 cannot be patched due to 16-bit range
verification time 172602 usec
stack depth 0
processed 30728 insns (limit 1000000) max_states_per_insn 1 total_states 1022 peak_states 1022 mark_read 1

This test passes with bpf-next/master. Btw, I tried with your v4 patches 
though I am replying here...

test_progs has no regression, but has 15 failures even without these 
patches that I need to look into.


- Naveen

Powered by blists - more mailing lists