Re: [PATCH bpf-next v1] ARC: Add eBPF JIT support

["Björn Töpel" <bjorn@kernel.org>]

Shahab Vahedi <list+bpf@vahedi.org> writes:

> Hi Björn,
>
> Thank you very much for your inputs. Please find my remarks below.
>
> Björn Töpel <bjorn@kernel.org> writes:
>
>> Shahab Vahedi <list+bpf@vahedi.org> writes:
>> 
>> What's the easiest way to test test this w/o ARC HW? Is there a qemu
>> port avaiable?
>
> Yes, there is a (downstream) port available on GitHub [1]. If one is
> interested, there are also guides about building QEMU for ARC targets [2]
> and how to run eBPF tests for ARC Linux [3].
>
> [1] ARC QEMU port
> https://github.com/foss-for-synopsys-dwc-arc-processors/qemu
>
> [2] Building ARC QEMU
> https://foss-for-synopsys-dwc-arc-processors.github.io/experimental-documentation/2023.09/simulators/qemu/
>
> [3] Runing eBPF tests for ARC Linux
> https://foss-for-synopsys-dwc-arc-processors.github.io/experimental-documentation/2023.09/linux/ebpf/build/

Cool, TY.

>> I don't know much about ARC -- Is v2 compatible with v3?
>
> No, they're not. For what it's worth, ARCv3 comes in {32,64}-bit
> flavours which are not compatible with each other either.
>
>> I'm curious about the missing support; tailcall/atomic/division/extable
>> support. Would it require a lot of work to add that support in the
>> initial change set?
>
> If you're asking whether it is possible that I add those features now,
> my answer unfortunately would be "no". However, the way that things
> are implemented, it will be a straightforward addition.

Ok! Did you try building the kselftest/bpf suite? Would be interesting
to see the pass/fail rate of test_progs.

>> There are a lot of checkpatch/kernel style issues. Run, e.g.,
>> "checkpatch --strict -g HEAD" and you'll get a bunch of issues. Most of
>> them are just basic style issues. Please try to fix most of them for the
>> next rev.
>
> I did run the "checkpatch" before submitting. I've fixed all the "errors"
> and most of the "warnings". But now that you brought it up, I will try to
> fix as many "warnings"/"checks" as make sense.

Ok. I noticed a lot non-kernel style in your patch (checking against
NULL e.g.)

>> You should add yourself to the MAINTAINERS file.
>
> I will. Thanks!
>
>> Please try to avoid static inline in the C-files. The compiler usually
>> knows better.
>
> I will replace them with "static" then.
>
>> > +/* Sane initial values for the globals */
>> > +bool emit = true;
>> > +bool zext_thyself = true;
>> 
>> Hmm, this is racy. Can we move this into the jit context? Also, is
>> zext_thyself even used?
>
> I will get rid of those. For the record, "zext_thyself" is used by
> calling "zext()" after handling "BPF_ALU" operations.

Ah, indeed!

>> > +#define CHECK_RET(cmd)			\
>> > +	do {				\
>> > +		ret = (cmd);		\
>> > +		if (ret < 0)		\
>> > +			return ret;	\
>> > +	} while (0)
>> > +
>> 
>> Nit/personal taste, but I prefer not having these kind of macros. I
>> think it makes it harder to read the code.
>
> At some point, I found myself distracted from seeing the bigger picture
> while the code was interspersed by the menial "return checking"s. If
> you don't mind, I'd rather keep it as is, unless you feel strong about
> it or Vineet also agrees with you.
>
>> Care to elaborate a bit more on ARC_BPF_JIT_DEBUG. This smells of
>> duplicated funtionality with bpf_jit_dump(), and the BUG()s are scary.
>
> ARC_BPF_JIT_DEBUG is supposed to be enabled for development purposes.
> It enables:
>
> 1. A set of assert-like condition checking which makes the code
> slow and can lead to ungraceful terminations.
>
> 2. Use of a custom version of hex dumps. The most important difference
> with bpf_jit_dump() is that bpf_jit_dump() cannot be used for dumping
> the input BPF byte stream. Rest, I can live with. An example follows:
>
> Using only "bpf_jit_dump" (ARC_BPF_JIT_DEBUG is not defined)
>
>   flen=2 proglen=20 pass=1 image=2e8c6fb9 from=hello pid=127
>   JIT code: 00000000: 8a 20 00 10 8a 21 00 10 0a 20 00 02 0a 21 40 02
>   JIT code: 00000010: e0 20 c0 07
>
> vs.
>
> Using the custom version (ARC_BPF_JIT_DEBUG is defined)
>   -----------------[  VM   ]-----------------
>   0xb7, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>   0x95, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>   -----------------[ JIT:1 ]-----------------
>   0x8a, 0x20, 0x00, 0x10, 0x8a, 0x21, 0x00, 0x10
>   0x0a, 0x20, 0x00, 0x02, 0x0a, 0x21, 0x40, 0x02
>   0xe0, 0x20, 0xc0, 0x07
>
>> > +static int jit_ctx_init(struct jit_context *ctx, struct bpf_prog *prog)
>> > +{
>> > +   ...
>> 
>> I'd just make sure that ctx is zeroed, and init the non-zero members here.
>
> Very good point! I will implement it that way.
>
> If you have read this far, I'd like to thank you again for spending time
> on reviewing this patch. It is much appreciated.

Looking forward for the next revision!


Björn

_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc