From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.99]:37448 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751721AbeBZUQJ (ORCPT ); Mon, 26 Feb 2018 15:16:09 -0500 Date: Mon, 26 Feb 2018 22:16:05 +0200 From: "Michael S. Tsirkin" To: Jesper Dangaard Brouer Cc: Jason Wang , netdev@vger.kernel.org, John Fastabend , Alexei Starovoitov , Daniel Borkmann , "David S. Miller" Subject: Re: [RFC net PATCH] virtio_net: disable XDP_REDIRECT in receive_mergeable() case Message-ID: <20180226221518-mutt-send-email-mst@kernel.org> References: <151873460188.31003.8350168978875601093.stgit@firesoul> <25a9b6f0-e3f4-30b7-054a-d8e58d80cfcd@redhat.com> <20180216164126.4bbf05e5@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180216164126.4bbf05e5@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 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