Re: [net-next, PATCH 3/3, v2] net: netsec: add XDP support

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Sun, Jun 30, 2019 at 07:41:28PM +0300, Ivan Khoronzhuk wrote:
> On Sun, Jun 30, 2019 at 07:32:14PM +0300, Ilias Apalodimas wrote:
> >On Sun, Jun 30, 2019 at 07:25:53PM +0300, Ivan Khoronzhuk wrote:
> >>On Sat, Jun 29, 2019 at 08:23:25AM +0300, Ilias Apalodimas wrote:
> >>>The interface only supports 1 Tx queue so locking is introduced on
> >>>the Tx queue if XDP is enabled to make sure .ndo_start_xmit and
> >>>.ndo_xdp_xmit won't corrupt Tx ring
> >>>
> >>>- Performance (SMMU off)
> >>>
> >>>Benchmark   XDP_SKB     XDP_DRV
> >>>xdp1        291kpps     344kpps
> >>>rxdrop      282kpps     342kpps
> >>>
> >>>- Performance (SMMU on)
> >>>Benchmark   XDP_SKB     XDP_DRV
> >>>xdp1        167kpps     324kpps
> >>>rxdrop      164kpps     323kpps
> >>>
> >>>Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>---
> >>>drivers/net/ethernet/socionext/netsec.c | 361 ++++++++++++++++++++++--
> >>>1 file changed, 334 insertions(+), 27 deletions(-)
> >>>
> >>
> >>[...]
> >>
> >>>+
> >>>+static int netsec_xdp_setup(struct netsec_priv *priv, struct bpf_prog *prog,
> >>>+			    struct netlink_ext_ack *extack)
> >>>+{
> >>>+	struct net_device *dev = priv->ndev;
> >>>+	struct bpf_prog *old_prog;
> >>>+
> >>>+	/* For now just support only the usual MTU sized frames */
> >>>+	if (prog && dev->mtu > 1500) {
> >>>+		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported on XDP");
> >>>+		return -EOPNOTSUPP;
> >>>+	}
> >>>+
> >>>+	if (netif_running(dev))
> >>>+		netsec_netdev_stop(dev);
> >>And why to stop the interface. XDP allows to update prog in runtime.
> >>
> >Adding the support is not limited to  adding a prog only in this driver.
> >It also rebuilts the queues which changes the dma mapping of buffers.
> >Since i don't want to map BIDIRECTIONAL buffers if XDP is not in place,
> >i am resetting the device and forcing the buffer re-allocation
> >
> >Thanks
> >/Ilias
> I don't know the internals, probably it has some dependencies, but here you
> just update the prog and can at least do it when exchange is happening.
> I mean not in case of prog is attached/removed first time.
> In case of prog -> prog it seems doable...
> 
> It ups to you ofc, but I can run smth like:
> ip -force link set dev eth0 xdp obj xdp-example-pass.o sec .text
> and expect it's updated w/o interface reset I mean on new prog.
> 
> I'm not sure, but maintainers can help, conceptually it's supposed to be in
> runtime the prog be update uder rcu as a part of API usage...
It's doable but it means i'd have to change the buffer allocation again. I'd
also prefer mapping FOR_DEVICE only if XDP is not loaded. Most of the drivers do
restart so i'll stick with this for the current version. 
Most of the drivers do restart now so i'll stick to that for now.

Thanks
/Ilias