Re: [RFC PATCH] bpf: s390: add JIT support for multi-function programs

[Yauheni Kaliuta <yauheni.kaliuta@redhat.com>]

Hi, Ilya!

>>>>> On Tue, 27 Aug 2019 16:37:04 +0200, Ilya Leoshkevich  wrote:

 >> Am 27.08.2019 um 16:21 schrieb Yauheni Kaliuta <yauheni.kaliuta@redhat.com>:
 >> 
 >> Hi, Ilya!
 >> 
 >>>>>>> On Tue, 27 Aug 2019 15:46:43 +0200, Ilya Leoshkevich  wrote:
 >> 
 >>>> Am 27.08.2019 um 15:21 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
 >>>> 
 >>>>> Am 26.08.2019 um 20:20 schrieb Yauheni Kaliuta <yauheni.kaliuta@redhat.com>:
 >>>>> 
 >>>>> test_verifier (5.3-rc6):
 >>>>> 
 >>>>> without patch:
 >>>>> Summary: 1501 PASSED, 0 SKIPPED, 47 FAILED
 >>>>> 
 >>>>> with patch:
 >>>>> Summary: 1540 PASSED, 0 SKIPPED, 8 FAILED
 >>>> 
 >>>> Are you per chance running with a testsuite patch like this one?
 >>>> 
 >>>> --- a/tools/testing/selftests/bpf/test_verifier.c
 >>>> +++ b/tools/testing/selftests/bpf/test_verifier.c
 >>>> @@ -846,7 +846,7 @@ static int do_prog_test_run(int fd_prog, bool
 >>>> unpriv, uint32_t expected_val,
 >>>> tmp, &size_tmp, &retval, NULL);
 >>>> if (unpriv)
 >>>> set_admin(false);
 >>>> -	if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
 >>>> +	if (err && errno != EPERM) {
 >>>> printf("Unexpected bpf_prog_test_run error ");
 >>>> return err;
 >>>> }
 >>>> 
 >>>> Without it, all the failures appear to be masked for me.
 >> 
 >>> Hmm, I'm sorry, I thought about it a bit more, and the patch I
 >>> posted above doesn't make any sense, because the failures you
 >>> fixed are during load, and not run time.
 >> 
 >>> Now I think you are using CONFIG_BPF_JIT_ALWAYS_ON for your
 >>> testing, is that right? If yes, it would be nice to mention
 >> 
 >> Right.
 >> 
 >>> this in the commit message.
 >> 
 >> Sure. Should I post non-RFC v2 or wait for some more comments?

 > So far I only spotted a minor issue:

 > +		if (ret < 0)
 > +			return ret;

 > Right now bpf_jit_insn returns 0 or -1, but
 > bpf_jit_get_func_addr returns 0 or -errno. This does not
 > affect anything in the end, but just to be uniform, maybe
 > return -1 here or -EINVAL in the default: branch?

ok. I choose "return -1" since changing default to -EINVAL sounds
as unrelated change to the patch.

 > I don't see any other obvious problems with the patch, but I'd
 > like to take some time to understand how exactly some parts of
 > it work before acking it. So I think it's fine to post a
 > non-RFC version.

Good, thanks!

-- 
WBR,
Yauheni Kaliuta