On 8/18/17 8:30 PM, John Fastabend wrote:
So this is really close to what I proposed above. For a TX_SOCKMAP
simply do not attach any programs,
bpf_create_map(BPF_MAP_TYPE_SOCKMAP, .... )
[...]
For an RX_SOCKMAP,
bpf_create_map(BPF_MAP_TYPE_SOCKMAP, .... )
bpf_prog_attach(verdict_prog, map_fd, BPF_SMAP_STREAM_VERDICT, 0);
bpf_prog_attach(parse_prog, map_fd, BPF_SMAP_STREAM_PARSER, 0);
With the new attach type (compared to the fd2 thing before) we can easily
extend maps to contain other program types as needed. So in the future
we might have TX_SOCKMAP, RX_SOCKMAP, FOO_SOCKMAP, ...
agree. that sounds as good generalization.
I don't see the need to have the API enforce the map type via update
specifiers bpf_{rx|tx}_sock_map_update. The programmer should "know"
the type by virtue of the programs attached. This is more flexible
as well because it allows a map to be TX only, RX only or TX/RX.
makes sense. good point.
With this proposal we can relax the restriction where a sock can only
be in a single map and even allow a sock to be in the same map multiple
times. The limitation we do have to enforce is allowing a sock in the
a map with different BPF_SMAP_STREAM_* programs. But I think this
should be clear to the programmer (with good tracing functions and
error codes).
Slight aside: but by creating map size of 1 we have an object that
contains programs and later we can attach a sock to it, looks like
the following,
create_map(BPF_MAP_TYPE_SOCKMAP,...)
bpf_prog_attach(...)
[...]
bpf_update_map_elem(fd, map, key, flags)
I think this is very close to your first approach where you suggested
a program container object.
yep.
Or you have cases when two RX sockets need to redirect into each
other and in both cases strparser+verdict need to run?
If we don't do rx, tx restrictions and use my suggestion here we
don't have this limitation. OR because we allow socks in multiple
maps now the user can simply put the sockets in different maps.
agree. good point as well.
In such case we need to allow bpf_sk_redirect_map() to use on
RX_SOCKMAP map as well,
but looking at current implementation you only allow one psock per map,
so two sockets forwarding to each other cannot work due to only one queue.
Am I missing anything from what you want to achieve?
I don't think so. But lets get rid of the one psock per map, I took a shot
at relaxing that today and was able to get it with a refcount on the psock
which seems to work OK.
+1
Also reorganizing the psock structure into clear sections tx_psock, rx_psock,
general_psock will probably help readers.
nice. thanks!
Thoughts?
What do you think of my counter proposal I started coding it up and it
actually (other than pushing code snippets around) seems to work out
nicely with the existing code base. I think it is really a nice improvement.
ok. I think we're mostly on the same page and patches will
either bring us to the full agreement or show where we disagree :)
To clarify, I think the current code base is pretty good.
I'm only arguing to fix up the rough spots of the uapi to make
sure we don't corner ourselves with future extensions that I feel
inevitably will follow.
The feature itself is quite important and I feel a bit sad that it
landed without enough due diligence. The RFC patches didn't get
much attentions and I didn't have time until now to look into them
in depth.
