Re: [RFC net PATCH] virtio_net: disable XDP_REDIRECT in receive_mergeable() case

["Michael S. Tsirkin" <mst@redhat.com>]

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 <jasowang@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