Re: [net-next PATCH 06/10] bpf: sockmap with sk redirect support

[John Fastabend <john.fastabend@gmail.com>]

On 08/18/2017 09:50 PM, Alexei Starovoitov wrote:
> 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 :)

I'll work up the patches Monday/Tuesday and we should have plenty of time to
work out any kinks. The bit I did Friday makes me think the changes to support
this should be straight forward.

Thanks,
John