On Fri, Feb 16, 2018 at 04:41:26PM +0100, Jesper Dangaard Brouer wrote: > On Fri, 16 Feb 2018 13:31:37 +0800 > Jason Wang <jasow...@redhat.com> wrote: > > > On 2018年02月16日 06:43, Jesper Dangaard Brouer wrote: > > > The virtio_net code have three different RX code-paths in receive_buf(). > > > Two of these code paths can handle XDP, but one of them is broken for > > > at least XDP_REDIRECT. > > > > > > Function(1): receive_big() does not support XDP. > > > Function(2): receive_small() support XDP fully and uses build_skb(). > > > Function(3): receive_mergeable() broken XDP_REDIRECT uses > > > napi_alloc_skb(). > > > > > > The simple explanation is that receive_mergeable() is broken because > > > it uses napi_alloc_skb(), which violates XDP given XDP assumes packet > > > header+data in single page and enough tail room for skb_shared_info. > > > > > > The longer explaination is that receive_mergeable() tries to > > > work-around and satisfy these XDP requiresments e.g. by having a > > > function xdp_linearize_page() that allocates and memcpy RX buffers > > > around (in case packet is scattered across multiple rx buffers). This > > > does currently satisfy XDP_PASS, XDP_DROP and XDP_TX (but only because > > > we have not implemented bpf_xdp_adjust_tail yet). > > > > > > The XDP_REDIRECT action combined with cpumap is broken, and cause hard > > > to debug crashes. The main issue is that the RX packet does not have > > > the needed tail-room (SKB_DATA_ALIGN(skb_shared_info)), causing > > > skb_shared_info to overlap the next packets head-room (in which cpumap > > > stores info). > > > > > > Reproducing depend on the packet payload length and if RX-buffer size > > > happened to have tail-room for skb_shared_info or not. But to make > > > this even harder to troubleshoot, the RX-buffer size is runtime > > > dynamically change based on an Exponentially Weighted Moving Average > > > (EWMA) over the packet length, when refilling RX rings. > > > > > > This patch only disable XDP_REDIRECT support in receive_mergeable() > > > case, because it can cause a real crash. > > > > > > But IMHO we should NOT support XDP in receive_mergeable() at all, > > > because the principles behind XDP are to gain speed by (1) code > > > simplicity, (2) sacrificing memory and (3) where possible moving > > > runtime checks to setup time. These principles are clearly being > > > violated in receive_mergeable(), that e.g. runtime track average > > > buffer size to save memory consumption. > > > > I agree to disable it for -net now. > > Okay... I'll send an official patch later. > > > For net-next, we probably can do: > > > > - drop xdp_linearize_page() and do XDP through generic XDP helper > > after skb was built > > I disagree strongly here - it makes no sense. > > Why do you want to explicit fallback to Generic-XDP? > (... then all the performance gain is gone!) > And besides, a couple of function calls later, the generic XDP code > will/can get invoked anyhow... > > > Take a step back: > What is the reason/use-case for implementing XDP inside virtio_net?
Firewall within guest, controllable by guest admin. > From a DDoS/performance perspective XDP in virtio_net happens on the > "wrong-side" as it is activated _inside_ the guest OS, which is too > late for a DDoS filter, as the guest kick/switch overhead have already > occurred. > > I do use XDP_DROP inside the guest (driver virtio_net), but just to > perform what I can zoom-in benchmarking, for perf-record isolating the > early RX code path in the guest. (Using iptables "raw" table drop is > almost as useful for that purpose). > > > > The XDP ndo_xdp_xmit in tuntap/tun.c (that you also implemented) is > significantly more interesting. As it allow us to skip large parts of > the network stack and redirect from a physical device (ixgbe) into a > guest device. Ran a benchmark: > - 0.5 Mpps with normal code path into device with driver tun > - 3.7 Mpps with XDP_REDIRECT from ixgbe into same device > > Plus, there are indications that 3.7Mpps is not the real limit, as > guest CPU doing XDP_DROP is 75% idle... thus this is a likely a > scheduling + queue size issue. > > > > - disable EWMA when XDP is set and reserve enough tailroom. > > > > > > > > Besides the described bug: > > > > > > Update(1): There is also a OOM leak in the XDP_REDIRECT code, which > > > receive_small() is likely also affected by. > > > > > > Update(2): Also observed a guest crash when redirecting out an > > > another virtio_net device, when device is down. > > > > Will have a look at these issues. (Holiday in china now, so will do it > > after). > > > > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer
