Re: [oss-drivers] Re: [PATCH v3 bpf-next 08/19] bpf: insert explicit zero extension insn when hardware doesn't do it implicitly

[Jiong Wang <jiong.wang@netronome.com>]

Naveen N. Rao writes:

> Jiong Wang wrote:
>>
>>> On 15 Apr 2019, at 19:21, Naveen N. Rao <naveen.n.rao@linux.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).

Great, thanks!

>>>
>>> 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...

ld_abs: vlan + abs is a special test which calls a helper
"bpf_fill_ld_abs_vlan_push_pop" to fill (1 << 15) insns which it the jump
distance maximum. Extra code insertion may overflow some jump inside the
test. The selftest patch in this set changed the one place to ALU64 to
avoid high 32-bit randomization sequence insertion. Now for PowerPC,
zero-extension for low 32-bit could be inserted, so this testcase needs
further adjustment.

I will try to emulate and fix this issue on my x86_64 env.

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

That's a good news to hear no regression on test_progs.

Thanks.

Regards,
Jiong