Hi Ben, Thanks for the feedback.
On Mon, Apr 22, 2019 at 10:57 AM Ben Pfaff <[email protected]> wrote: > On Fri, Apr 19, 2019 at 01:22:14PM -0700, William Tu wrote: > > The patch introduces experimental AF_XDP support for OVS netdev. > > AF_XDP is a new address family working together with eBPF/XDP. > > A socket with AF_XDP family can receive and send raw packets > > from an eBPF/XDP program attached to the netdev. > > For details introduction and configuration, see > > Documentation/intro/install/afxdp.rst > > > > Signed-off-by: William Tu <[email protected]> > > Co-authored-by: Yi-Hung Wei <[email protected]> > > Cc: Tim Rozet <[email protected]> > > Cc: Eelco Chaudron <[email protected]> > > Thanks, William and Yi-Hung. > > Before I installed libbpf, "configure" failed, even without > --enable-afxdp: > > config.status: error: Something went wrong bootstrapping makefile > fragments > for automatic dependency tracking. Try re-running configure with > the > '--disable-dependency-tracking' option to at least be able to build > the package (albeit without support for automatic dependency > tracking). > See `config.log' for more details > > Thanks, I will take a look. > I'm attaching the full config.log in case it is helpful. > > I would recommend adding a sentence or two to afxdp.rst explaining why > someone would want to use AF_XDP. Currently it is kind of there > ("...much better performance than AF_PACKET.") but it is not called out > very well. Maybe something along the lines of saying that it aims to > have comparable performance to DPDK but cooperate better with existing > kernel mechanisms. Otherwise, someone who is new to OVS or to AF_XDP > will not have any idea of the context. > OK > > In afxdp.rst, I'd also recommend explaining why the recommended kernel > config options are recommend. I'd guess that the BPF_JIT ones are there > for performance and XDP_SOCKETS_DIAG is for debugging, but it'd be nice > to say so. > OK > > s/Fist/First/: > +Fist, clone a recent version of Linux bpf-next tree:: > > In the instructions for installing libbpf, I had to run plain "ldconfig" > by hand before the library showed up in "ldconfig -p" output. > > I'm not sure everyone knows the system include path, so you might add > something like "e.g. /usr/local/include/bpf/xsk.h" to this note: > +.. note:: > + Make sure xsk.h is install in system's library path > > OK Thanks > "make install_headers" failed to install libbpf_util.h for me, so the > build failed. I installed it by hand and it worked OK after that: > > blp@sigill:~/nicira/bpf-next/tools/lib/bpf(0)$ sudo cp libbpf_util.h > /usr/local/include/bpf/ > > I didn't test AF_XDP at runtime, but I did verify that the binary > linked and started up OK. > > I had to apply the following patch to suppress warnings from sparse and > GCC 8.x: > > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > index 56f313606190..17c99f389e63 100644 > --- a/lib/netdev-afxdp.c > +++ b/lib/netdev-afxdp.c > @@ -91,8 +91,9 @@ VLOG_DEFINE_THIS_MODULE(netdev_afxdp); > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); > > #define UMEM2DESC(elem, base) ((uint64_t)((char *)elem - (char *)base)) > -#define UMEM2XPKT(base, i) \ > - (struct dp_packet_afxdp *)((char *)base + i * sizeof(struct > dp_packet_afxdp)) > +#define UMEM2XPKT(base, i) \ > + ALIGNED_CAST(struct dp_packet_afxdp *, \ > + (char *)base + i * sizeof(struct dp_packet_afxdp)) > > static uint32_t opt_xdp_bind_flags = XDP_COPY; > static uint32_t opt_xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST | > XDP_FLAGS_SKB_MODE; > @@ -130,8 +131,8 @@ static struct xsk_umem_info *xsk_configure_umem(void > *buffer, uint64_t size) > for (i = NUM_FRAMES - 1; i >= 0; i--) { > struct umem_elem *elem; > > - elem = (struct umem_elem *)((char *)umem->buffer > - + i * FRAME_SIZE); > + elem = ALIGNED_CAST(struct umem_elem *, > + (char *)umem->buffer + i * FRAME_SIZE); > umem_elem_push(&umem->mpool, elem); > } > > @@ -527,7 +528,8 @@ retry: > > addr = *xsk_ring_cons__comp_addr(&xsk->umem->cq, idx_cq++); > > - elem = (struct umem_elem *)((char *)xsk->umem->buffer + addr); > + elem = ALIGNED_CAST(struct umem_elem *, > + (char *)xsk->umem->buffer + addr); > umem_elem_push(&xsk->umem->mpool, elem); > } > xsk_ring_cons__release(&xsk->umem->cq, tx_done); > > And the following to suppress additional warnings from Clang: > > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > index 56f313606190..c675cf2e51a8 100644 > --- a/lib/netdev-afxdp.c > +++ b/lib/netdev-afxdp.c > @@ -273,7 +274,7 @@ xsk_destroy(struct xsk_socket_info *xsk) > return; > } > > -static inline void > +static inline void OVS_UNUSED > print_xsk_stat(struct xsk_socket_info *xsk OVS_UNUSED) { > struct xdp_statistics stat; > socklen_t optlen; > diff --git a/lib/xdpsock.c b/lib/xdpsock.c > index 9bd574e61774..8d6b37d3b6c2 100644 > --- a/lib/xdpsock.c > +++ b/lib/xdpsock.c > @@ -70,7 +70,7 @@ static inline void ovs_spin_unlock(ovs_spinlock_t *sl) > __atomic_store_n(&sl->locked, 0, __ATOMIC_RELEASE); > } > > -static inline int ovs_spin_trylock(ovs_spinlock_t *sl) > +static inline int OVS_UNUSED ovs_spin_trylock(ovs_spinlock_t *sl) > { > int exp = 0; > return __atomic_compare_exchange_n(&sl->locked, &exp, 1, > > Thanks, I did not test compile with clang and sparse. I will make sure no error in next version Regards, William _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
