search for: xdp_drop

...(And we've > > > already had: VIRTIO_XDP_REDIR and VIRTIO_XDP_TX ...) > > > > No, these are the three states of buffer after XDP processing. > > > > * PASS: goto make skb > > XDP_PASS goes for this. > > > * DROP: we should release buffer > > XDP_DROP and error conditions go with this. > > > * CUNSUMED: xdp prog used the buffer, we do nothing > > XDP_TX/XDP_REDIRECTION goes for this. > > So t virtnet_xdp_handler() just maps XDP ACTION plus the error > conditions to the above three states. > > We can simply map error...

...XDP_REDIR and VIRTIO_XDP_TX ...) > > > > > > No, these are the three states of buffer after XDP processing. > > > > > > * PASS: goto make skb > > > > XDP_PASS goes for this. > > > > > * DROP: we should release buffer > > > > XDP_DROP and error conditions go with this. > > > > > * CUNSUMED: xdp prog used the buffer, we do nothing > > > > XDP_TX/XDP_REDIRECTION goes for this. > > > > So t virtnet_xdp_handler() just maps XDP ACTION plus the error > > conditions to the above three states...

...er, care to chime in where we ended up in that last thread > >> discussion this? > > > > IHMO packets RX and TX on a device need to be accounted, in standard > > counters, regardless of XDP. For XDP RX the packet is counted as RX, > > regardless if XDP choose to XDP_DROP. On XDP TX which is via > > XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to > > account the packet in a TX counter (this if often delayed to DMA TX > > completion handling). We cannot break the expectation that RX and TX > > counter are visible to users...

...three states of buffer after XDP processing. > > > > > > > > > > * PASS: goto make skb > > > > > > > > XDP_PASS goes for this. > > > > > > > > > * DROP: we should release buffer > > > > > > > > XDP_DROP and error conditions go with this. > > > > > > > > > * CUNSUMED: xdp prog used the buffer, we do nothing > > > > > > > > XDP_TX/XDP_REDIRECTION goes for this. > > > > > > > > So t virtnet_xdp_handler() just maps XDP ACTION p...

...d up in that last thread >>>>> discussion this? >>>> >>>> IHMO packets RX and TX on a device need to be accounted, in standard >>>> counters, regardless of XDP. For XDP RX the packet is counted as RX, >>>> regardless if XDP choose to XDP_DROP. On XDP TX which is via >>>> XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to >>>> account the packet in a TX counter (this if often delayed to DMA TX >>>> completion handling). We cannot break the expectation that RX and TX >>>>...

...NET_XDP_RES_CONSUMED; > + > + default: > + bpf_warn_invalid_xdp_action(dev, xdp_prog, act); > + fallthrough; > + case XDP_ABORTED: > + trace_xdp_exception(dev, xdp_prog, act); > + fallthrough; > + case XDP_DROP: > + return VIRTNET_XDP_RES_DROP; > + } > +} > + > static unsigned int virtnet_get_headroom(struct virtnet_info *vi) > { > return vi->xdp_enabled ? VIRTIO_XDP_HEADROOM : 0; > @@ -876,7 +938,6 @@ static struct sk_buff *receive_small(struct net...

...e > similar behavior between XDP and non XDP cases. I don't think "skb allocation fails" should increase rx packets counter. The difference is that these events are outside sysadm/users control, and is an error detected inside the driver. The XDP program takes a policy choice to XDP_DROP a packet, which can be accounted inside the XDP prog (as the samples show) or as we also discuss via a more generic XDP-action counters. That said, I took at quick look at driver code, and it seems this behavior differs per driver... ixgbe and mlx5 does not count "skb allocation fails" a...

...on by maintaining a reference bias which is initially USHRT_MAX. Each time a page is got, instead of calling get_page() we decrease the bias and when we find it's time to use a new page we will decrease the bias at one time through __page_cache_drain_cache(). Testpmd(virtio_user + vhost_net) + XDP_DROP on TAP shows about 1.6% improvement. Before: 4.63Mpps After: 4.71Mpps Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/vhost/net.c | 54 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 3 deletions(-) diff --git a/drivers/vhost/net.c b/drivers...

...+ goto out; > + case XDP_PASS: > + goto out; Do we need goto? why not just return? > + default: > + bpf_warn_invalid_xdp_action(act); > + /* fall through */ > + case XDP_ABORTED: > + trace_xdp_exception(tun->dev, xdp_prog, act); > + /* fall through */ > + case XDP_DROP: > + break; > + } > + > + put_page(virt_to_head_page(xdp->data_hard_start)); put here because caller does get_page :( Not pretty. I'd move this out to the caller. > +out: > + return act; How about combining err and act? err is < 0 XDP_PASS is > 0. No need for poin...

...+ goto out; > + case XDP_PASS: > + goto out; Do we need goto? why not just return? > + default: > + bpf_warn_invalid_xdp_action(act); > + /* fall through */ > + case XDP_ABORTED: > + trace_xdp_exception(tun->dev, xdp_prog, act); > + /* fall through */ > + case XDP_DROP: > + break; > + } > + > + put_page(virt_to_head_page(xdp->data_hard_start)); put here because caller does get_page :( Not pretty. I'd move this out to the caller. > +out: > + return act; How about combining err and act? err is < 0 XDP_PASS is > 0. No need for poin...

...); > unsigned int delta = 0; > @@ -1668,6 +1668,9 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, > if (copied != len) > return ERR_PTR(-EFAULT); > > + get_page(alloc_frag->page); > + alloc_frag->offset += buflen; > + This adds an atomic op on XDP_DROP which is a data path operation for some workloads. > /* There's a small window that XDP may be set after the check > * of xdp_prog above, this should be rare and for simplicity > * we do XDP on skb in case the headroom is not enough. > @@ -1695,23 +1698,15 @@ static struct...

...); > unsigned int delta = 0; > @@ -1668,6 +1668,9 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, > if (copied != len) > return ERR_PTR(-EFAULT); > > + get_page(alloc_frag->page); > + alloc_frag->offset += buflen; > + This adds an atomic op on XDP_DROP which is a data path operation for some workloads. > /* There's a small window that XDP may be set after the check > * of xdp_prog above, this should be rare and for simplicity > * we do XDP on skb in case the headroom is not enough. > @@ -1695,23 +1698,15 @@ static struct...

...8,9 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, > > > if (copied != len) > > > return ERR_PTR(-EFAULT); > > > + get_page(alloc_frag->page); > > > + alloc_frag->offset += buflen; > > > + > > This adds an atomic op on XDP_DROP which is a data path > > operation for some workloads. > > Yes, I have patch on top to amortize this, the idea is to have a very big > refcount once after the frag was allocated and maintain a bias and decrease > them all when allocating new frags.' Why bother with refcounti...

...: + *err = tun_xdp_tx(tun->dev, xdp); + if (*err < 0) + break; + *err = 0; + goto out; + case XDP_PASS: + goto out; + default: + bpf_warn_invalid_xdp_action(act); + /* fall through */ + case XDP_ABORTED: + trace_xdp_exception(tun->dev, xdp_prog, act); + /* fall through */ + case XDP_DROP: + break; + } + + put_page(virt_to_head_page(xdp->data_hard_start)); +out: + return act; +} + static struct sk_buff *tun_build_skb(struct tun_struct *tun, struct tun_file *tfile, struct iov_iter *from, @@ -1645,10 +1683,10 @@ static struct sk_buff *tun_build_skb(struct tun_...

...nation 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). """ So I consider the tailroom is a must for the (future) tail adjustment. > > You could re-enable native redirect, and push the responsibility to > cpumap for detecting this too-small fram...

...nation 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). """ So I consider the tailroom is a must for the (future) tail adjustment. > > You could re-enable native redirect, and push the responsibility to > cpumap for detecting this too-small fram...

...goto? why not just return? I don't see any difference. > >> + default: >> + bpf_warn_invalid_xdp_action(act); >> + /* fall through */ >> + case XDP_ABORTED: >> + trace_xdp_exception(tun->dev, xdp_prog, act); >> + /* fall through */ >> + case XDP_DROP: >> + break; >> + } >> + >> + put_page(virt_to_head_page(xdp->data_hard_start)); > put here because caller does get_page :( Not pretty. > I'd move this out to the caller. Then we need a switch in the caller, not sure it's better. > >> +out: >&...

...ps, from pktgen and testpmd. This way is easy to plot them. > > Maybe is easier as tables, if mail readers/gmail does not misalign them. > > > > # Tx > > > # === > > Base: With the previous code, not integrating any patch. testpmd is txonly mode, tap interface is XDP_DROP everything. > We vary VHOST_NET_BATCH (1, 16, 32, ...). As Jason put in a previous mail: > > TX: testpmd(txonly) -> virtio-user -> vhost_net -> XDP_DROP on TAP > > > 1 | 16 | 32 | 64 | 128 | 256 | 512 | > 2293304.308...

...if (err) - return VIRTNET_XDP_RES_DROP; + goto drop; *xdp_xmit |= VIRTIO_XDP_REDIR; return VIRTNET_XDP_RES_CONSUMED; @@ -862,8 +862,12 @@ static int virtnet_xdp_handler(struct bpf_prog *xdp_prog, struct xdp_buff *xdp, trace_xdp_exception(dev, xdp_prog, act); fallthrough; case XDP_DROP: - return VIRTNET_XDP_RES_DROP; + goto drop; } + +drop: + put_xdp_frags(xdp); + return VIRTNET_XDP_RES_DROP; } static unsigned int virtnet_get_headroom(struct virtnet_info *vi) @@ -1199,7 +1203,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, dev->name, *num_buf,...

...tained, in pps, from pktgen and testpmd. This way is easy to plot them. > > Maybe is easier as tables, if mail readers/gmail does not misalign them. > >>> # Tx >>> # === > Base: With the previous code, not integrating any patch. testpmd is txonly mode, tap interface is XDP_DROP everything. > We vary VHOST_NET_BATCH (1, 16, 32, ...). As Jason put in a previous mail: > > TX: testpmd(txonly) -> virtio-user -> vhost_net -> XDP_DROP on TAP > > > 1 | 16 | 32 | 64 | 128 | 256 | 512 | > 2293304.308|...