On 9/30/22 5:05 PM, Daniel Rosenberg wrote:
On Tue, Sep 27, 2022 at 11:41 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
Interesting idea.
Some comments on review logistics:
- The set is too long and some of the individual patches are way too long for
one single patch to review. Keep in mind that not all of us here are experts in
both fuse and bpf. Making it easier to review first will help at the beginning.
Some ideas:
- Only implement a few ops in the initial revision. From quickly browsing the
set, it is implementing the 'struct file_operations fuse_file_operations'?
Maybe the first few revisions can start with a few of the ops first.
I've split it up a fair bit already, do you mean just sending a subset
of them at a time? I think the current splitting roughly allows for
that. Patch 1-4 and 5 deal with bpf/verifier code which isn't used
until patch 24. I can reorder/split up the opcodes arbitrarily.
Putting the op codes that implement file passthrough first makes
sense. The code is much easier to test when all/most are present,
since then I can just use patch 23 to mount without a daemon and run
xfs tests on them. At least initially I felt the whole stack was
useful to give the full picture.
I don't mind to have all op codes in each re-spin as long as it can apply
cleanly to bpf-next where the bpf implementation part will eventually land.
Patch 26 has to split up though. It is a few thousand lines in one patch.
I was just thinking to only do a few op codes, eg. the few android use cases you
have mentioned. My feeling is other op codes should not be very different in
term of the bpf side implementation (or it is not true?). When the patch set
getting enough traction, then start adding more op codes in the later revisions.
That will likely help to re-spin faster and save you time also.
- iiuc, the idea is to allow bpf prog to optionally handle the 'struct
file_operations' without going back to the user daemon? Have you looked at
struct_ops which seems to be a better fit here? If the bpf prog does not know
how to handle an operation (or file?), it can call fuse_file_llseek (for
example) as a kfunc to handle the request.
I wasn't aware of struct_ops. It looks like that may work for us
instead of making a new prog type. I'll definitely look into that.
I'll likely sign up for the bpf office hours next week.
You can take a look at the tools/testing/selftests/bpf/progs/bpf_cubic.c.
It implements the whole tcp congestion in bpf. In particular, the bpf prog is
implementing the kernel 'struct tcp_congestion_ops'. That selftest example is
pretty much a direct copy from the kernel net/ipv4/tcp_cubic.c. Also, in
BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, ...), it is directly calling the kfunc's
tcp_reno_undo_cwnd() when the bpf prog does not need to do anything different
from the kernel's tcp_reno_undo_cwnd(). Look at how it is marked as __ksym in
bpf_cubic.c
However, echoing Alexei's earlier reply, struct_ops is good when it needs to
implement a well defined 'struct xyz_operations' that has all function pointer
in it. Taking another skim at the set, it seems like it is mostly trying to
intercept the fuse_simple_request() call?
