Date:   Tue, 3 Apr 2018 13:16:25 +0200
From:   Mathieu Xhonneux <m.xhonneux@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org, David Lebrun <dlebrun@...gle.com>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH net-next RFC 0/5] ipv6: sr: introduce seg6local End.BPF action

2018-03-31 1:03 GMT+02:00 Alexei Starovoitov <alexei.starovoitov@...il.com>:
>
> On Fri, Mar 23, 2018 at 10:15:59AM +0000, Mathieu Xhonneux wrote:
> > As of Linux 4.14, it is possible to define advanced local processing for
> > IPv6 packets with a Segment Routing Header through the seg6local LWT
> > infrastructure. This LWT implements the network programming principles
> > defined in the IETF “SRv6 Network Programming” draft.
> >
> > The implemented operations are generic, and it would be very interesting to
> > be able to implement user-specific seg6local actions, without having to
> > modify the kernel directly. To do so, this patchset adds an End.BPF action
> > to seg6local, powered by some specific Segment Routing-related helpers,
> > which provide SR functionalities that can be applied on the packet. This
> > BPF hook would then allow to implement specific actions at native kernel
> > speed such as OAM features, advanced SR SDN policies, SRv6 actions like
> > Segment Routing Header (SRH) encapsulation depending on the content of
> > the packet, etc ...
> >
> > This patchset is divided in 5 patches, whose main features are :
> >
> > - A new seg6local action End.BPF with the corresponding new BPF program
> >   type BPF_PROG_TYPE_LWT_SEG6LOCAL. Such attached BPF program can be
> >   passed to the LWT seg6local through netlink, the same way as the LWT
> >   BPF hook operates.
> > - 3 new BPF helpers for the seg6local BPF hook, allowing to edit/grow/
> >   shrink a SRH and apply on a packet some of the generic SRv6 actions.
> > - 1 new BPF helper for the LWT BPF IN hook, allowing to add a SRH through
> >   encapsulation (via IPv6 encapsulation or inlining if the packet contains
> >   already an IPv6 header).
> >
> > As this patchset adds a new LWT BPF hook, I took into account the result of
> > the discussions when the LWT BPF infrastructure got merged. Hence, the
> > seg6local BPF hook doesn’t allow write access to skb->data directly, only
> > the SRH can be modified through specific helpers, which ensures that the
> > integrity of the packet is maintained.
> > More details are available in the related patches messages.
> >
> > The performances of this BPF hook have been assessed with the BPF JIT
> > enabled on a Intel Xeon X3440 processors with 4 cores and 8 threads
> > clocked at 2.53 GHz. No throughput losses are noted with the seg6local
> > BPF hook when the BPF program does nothing (440kpps). Adding a 8-bytes
> > TLV (1 call each to bpf_lwt_seg6_adjust_srh and bpf_lwt_seg6_store_bytes)
> > drops the throughput to 410kpps, and inlining a SRH via
> > bpf_lwt_seg6_action drops the throughput to 420kpps.
> > All throughputs are stable.
> >
> > Any comments on the patchset are welcome.
>
> I've looked through the patches and everything looks very good.
> Feel free to resubmit without RFC tag.
Thanks, I will do this as soon as net-next opens.

>
> In patch 2 I was a bit concerned that:
> +       struct seg6_bpf_srh_state *srh_state = (struct seg6_bpf_srh_state *)
> +                                              &skb->cb;
> would not collide with other users of skb->cb, but it seems the way
> the hook is placed such usage should always be valid.
> Would be good to add a comment describing the situation.
Yes, it's indeed a little hack, but this should be OK since the IPv6 layer does
not use the cb field. Another solution would be to create a new field in
__sk_buff but it's more cumbersome.
I will add a comment.

>
> Looks like somewhat odd 'End.BPF' name comes from similar names in SRv6 draft.
> Do you plan to disclose such End.BPF action in the draft as well?
This is something I've discussed with David Lebrun (the author of the Segment
Routing implementation). There's no plan to disclose an End.BPF action as-is
in the draft, since eBPF is really specific to Linux, and David doesn't mind not
having a 1:1 mapping between the actions of the draft and the implemented
ones. Writing "End.BPF" instead of just "bpf" is important to indicate that the
action will advance to the next segment by itself, like all other End actions.
One could imagine adding later a T.BPF action (a transit action), whose SID
wouldn't have to be a segment, but that could still e.g. add/edit/delete TLVs.

>
> Thanks
>

Powered by blists - more mailing lists