[ovs-dev] [PATCHv14 2/2] netdev-afxdp: add new netdev type for AF_XDP.

William Tu u9012063 at gmail.com
Mon Jul 8 20:56:07 UTC 2019


On Fri, Jul 5, 2019 at 10:48 AM Ilya Maximets <i.maximets at samsung.com> wrote:
>
> Few more comments inline.
>
> On 01.07.2019 19:08, William Tu wrote:
> > The patch introduces experimental AF_XDP support for OVS netdev.
> > AF_XDP, the Address Family of the eXpress Data Path, is a new Linux socket
> > type built upon the eBPF and XDP technology.  It is aims to have comparable
> > performance to DPDK but cooperate better with existing kernel's networking
> > stack.  An AF_XDP socket receives and sends packets from an eBPF/XDP program
> > attached to the netdev, by-passing a couple of Linux kernel's subsystems
> > As a result, AF_XDP socket shows much better performance than AF_PACKET
> > For more details about AF_XDP, please see linux kernel's
> > Documentation/networking/af_xdp.rst. Note that by default, this feature is
> > not compiled in.
> >
> > Signed-off-by: William Tu <u9012063 at gmail.com>
> > ---
>
> <snip>
>
> > diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> > index 0976a35e758b..cc746b70af89 100644
> > --- a/lib/dp-packet.c
> > +++ b/lib/dp-packet.c
> > @@ -19,6 +19,7 @@
> >  #include <string.h>
> >
> >  #include "dp-packet.h"
> > +#include "netdev-afxdp.h"
> >  #include "netdev-dpdk.h"
> >  #include "openvswitch/dynamic-string.h"
> >  #include "util.h"
> > @@ -59,6 +60,28 @@ dp_packet_use(struct dp_packet *b, void *base, size_t allocated)
> >      dp_packet_use__(b, base, allocated, DPBUF_MALLOC);
> >  }
> >
> > +#if HAVE_AF_XDP
> > +/* Initialize 'b' as an empty dp_packet that contains
> > + * memory starting at AF_XDP umem base.
> > + */
> > +void
> > +dp_packet_use_afxdp(struct dp_packet *b, void *data, size_t allocated,
> > +                    size_t headroom)
> > +{
> > +    dp_packet_set_base(b, (char *)data - headroom);
> > +    dp_packet_set_data(b, data);
> > +    dp_packet_set_size(b, 0);
> > +
> > +    dp_packet_set_allocated(b, allocated);
> > +    b->source = DPBUF_AFXDP;
> > +    dp_packet_reset_offsets(b);
> > +    pkt_metadata_init(&b->md, 0);
> > +    dp_packet_reset_cutlen(b);
> > +    dp_packet_reset_offload(b);
> > +    b->packet_type = htonl(PT_ETH);
>
> Above block could be replaced with:
>
>     dp_packet_init__(b, allocated, DPBUF_AFXDP);
>
> At least it misses init_specific().
>

Yes, thank you.

> > +}
> > +#endif
> > +
> >  /* Initializes 'b' as an empty dp_packet that contains the 'allocated' bytes of
> >   * memory starting at 'base'.  'base' should point to a buffer on the stack.
> >   * (Nothing actually relies on 'base' being allocated on the stack.  It could
> > @@ -122,6 +145,8 @@ dp_packet_uninit(struct dp_packet *b)
> >               * created as a dp_packet */
> >              free_dpdk_buf((struct dp_packet*) b);
> >  #endif
> > +        } else if (b->source == DPBUF_AFXDP) {
> > +            free_afxdp_buf(b);
> >          }
> >      }
> >  }
>
> <snip>
>
> > +int
> > +netdev_afxdp_get_stats(const struct netdev *netdev,
> > +                       struct netdev_stats *stats)
> > +{
> > +    struct netdev_linux *dev = netdev_linux_cast(netdev);
> > +    struct xsk_socket_info *xsk_info;
> > +    struct netdev_stats dev_stats;
> > +    int error, i;
> > +
> > +    ovs_mutex_lock(&dev->mutex);
> > +
> > +    error = get_stats_via_netlink(netdev, &dev_stats);
> > +    if (error) {
> > +        VLOG_WARN_RL(&rl, "Error getting AF_XDP statistics");
> > +    } else {
> > +        /* Use kernel netdev's packet and byte counts */
> > +        stats->rx_packets = dev_stats.rx_packets;
> > +        stats->rx_bytes = dev_stats.rx_bytes;
> > +        stats->tx_packets = dev_stats.tx_packets;
> > +        stats->tx_bytes = dev_stats.tx_bytes;
> > +
> > +        stats->rx_errors           += dev_stats.rx_errors;
> > +        stats->tx_errors           += dev_stats.tx_errors;
> > +        stats->rx_dropped          += dev_stats.rx_dropped;
> > +        stats->tx_dropped          += dev_stats.tx_dropped;
> > +        stats->multicast           += dev_stats.multicast;
> > +        stats->collisions          += dev_stats.collisions;
> > +        stats->rx_length_errors    += dev_stats.rx_length_errors;
> > +        stats->rx_over_errors      += dev_stats.rx_over_errors;
> > +        stats->rx_crc_errors       += dev_stats.rx_crc_errors;
> > +        stats->rx_frame_errors     += dev_stats.rx_frame_errors;
> > +        stats->rx_fifo_errors      += dev_stats.rx_fifo_errors;
> > +        stats->rx_missed_errors    += dev_stats.rx_missed_errors;
> > +        stats->tx_aborted_errors   += dev_stats.tx_aborted_errors;
> > +        stats->tx_carrier_errors   += dev_stats.tx_carrier_errors;
> > +        stats->tx_fifo_errors      += dev_stats.tx_fifo_errors;
> > +        stats->tx_heartbeat_errors += dev_stats.tx_heartbeat_errors;
> > +        stats->tx_window_errors    += dev_stats.tx_window_errors;
> > +
> > +        /* Account the dropped in each xsk */
> > +        for (i = 0; i < netdev_n_rxq(netdev); i++) {
> > +            xsk_info = dev->xsks[i];
> > +            if (xsk_info) {
> > +                stats->rx_dropped += xsk_info->rx_dropped;
> > +                stats->tx_dropped += xsk_info->tx_dropped;
>
> 'xsk_info->rx_dropped' is never increased. I'm not sure if we'll ever
> need this field. Maybe it's better to just drop it?
yes I will drop it.

>
> Also, 'xsk_info->tx_dropped' updated concurrently. It should be under
> special 'stats_lock' like we do in netdev-dpdk. Another option is to
> make it atomic, since this is the only stat variable, this might be
> a better solution.

OK, I will make it atomic,

Regards,
William


More information about the dev mailing list