| Message ID | 20210105171921.8022-1-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: mvpp2: increase MTU limit when XDP enabled | expand |
On Tue, Jan 05, 2021 at 06:19:21PM +0100, Marek Behún wrote: > Currently mvpp2_xdp_setup won't allow attaching XDP program if > mtu > ETH_DATA_LEN (1500). > > The mvpp2_change_mtu on the other hand checks whether > MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE. > > These two checks are semantically different. > > Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in > mvpp2_rx we have > xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM; > xdp.frame_sz = PAGE_SIZE; > > Change the checks to check whether > mtu > MVPP2_MAX_RX_BUF_SIZE Hello Marek, in general, XDP is based on the model, that packets are not bigger than 1500. I am not sure if that has changed, I don't believe Jumbo Frames are upstreamed yet. You are correct that the MVPP2 driver can handle bigger packets without a problem but if you do XDP redirect that won't work with other drivers and your packets will disappear. Best Sven > > Signed-off-by: Marek Behún <kabel@kernel.org> > Cc: Sven Auhagen <sven.auhagen@voleatech.de> > Cc: Matteo Croce <mcroce@microsoft.com> > Cc: Lorenzo Bianconi <lorenzo@kernel.org> > --- > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index afdd22827223..65490a0eb657 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -4623,11 +4623,12 @@ static int mvpp2_change_mtu(struct net_device *dev, int mtu) > mtu = ALIGN(MVPP2_RX_PKT_SIZE(mtu), 8); > } > > + if (port->xdp_prog && mtu > MVPP2_MAX_RX_BUF_SIZE) { > + netdev_err(dev, "Illegal MTU value %d for XDP mode\n", mtu); > + return -EINVAL; > + } > + > if (MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE) { > - if (port->xdp_prog) { > - netdev_err(dev, "Jumbo frames are not supported with XDP\n"); > - return -EINVAL; > - } > if (priv->percpu_pools) { > netdev_warn(dev, "mtu %d too high, switching to shared buffers", mtu); > mvpp2_bm_switch_buffers(priv, false); > @@ -4913,8 +4914,8 @@ static int mvpp2_xdp_setup(struct mvpp2_port *port, struct netdev_bpf *bpf) > bool running = netif_running(port->dev); > bool reset = !prog != !port->xdp_prog; > > - if (port->dev->mtu > ETH_DATA_LEN) { > - NL_SET_ERR_MSG_MOD(bpf->extack, "XDP is not supported with jumbo frames enabled"); > + if (port->dev->mtu > MVPP2_MAX_RX_BUF_SIZE) { > + NL_SET_ERR_MSG_MOD(bpf->extack, "MTU too large for XDP"); > return -EOPNOTSUPP; > } > > -- > 2.26.2 >
On Tue, 5 Jan 2021 18:24:37 +0100 Sven Auhagen <sven.auhagen@voleatech.de> wrote: > On Tue, Jan 05, 2021 at 06:19:21PM +0100, Marek Behún wrote: > > Currently mvpp2_xdp_setup won't allow attaching XDP program if > > mtu > ETH_DATA_LEN (1500). > > > > The mvpp2_change_mtu on the other hand checks whether > > MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE. > > > > These two checks are semantically different. > > > > Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in > > mvpp2_rx we have > > xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM; > > xdp.frame_sz = PAGE_SIZE; > > > > Change the checks to check whether > > mtu > MVPP2_MAX_RX_BUF_SIZE > > Hello Marek, > > in general, XDP is based on the model, that packets are not bigger than 1500. > I am not sure if that has changed, I don't believe Jumbo Frames are upstreamed yet. > You are correct that the MVPP2 driver can handle bigger packets without a problem but > if you do XDP redirect that won't work with other drivers and your packets will disappear. At least 1508 is required when I want to use XDP with a Marvell DSA switch: the DSA header is 4 or 8 bytes long there. The DSA driver increases MTU on CPU switch interface by this length (on my switches to 1504). So without this I cannot use XDP with mvpp2 with a Marvell switch with default settings, which I think is not OK. Since with the mvneta driver it works (mvneta checks for MVNETA_MAX_RX_BUF_SIZE rather than ETH_DATA_LEN), I think it should also work with mvpp2. Marek
On Tue, Jan 05, 2021 at 06:43:08PM +0100, Marek Behún wrote: > On Tue, 5 Jan 2021 18:24:37 +0100 > Sven Auhagen <sven.auhagen@voleatech.de> wrote: > > > On Tue, Jan 05, 2021 at 06:19:21PM +0100, Marek Behún wrote: > > > Currently mvpp2_xdp_setup won't allow attaching XDP program if > > > mtu > ETH_DATA_LEN (1500). > > > > > > The mvpp2_change_mtu on the other hand checks whether > > > MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE. > > > > > > These two checks are semantically different. > > > > > > Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in > > > mvpp2_rx we have > > > xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM; > > > xdp.frame_sz = PAGE_SIZE; > > > > > > Change the checks to check whether > > > mtu > MVPP2_MAX_RX_BUF_SIZE > > > > Hello Marek, > > > > in general, XDP is based on the model, that packets are not bigger than 1500. > > I am not sure if that has changed, I don't believe Jumbo Frames are upstreamed yet. > > You are correct that the MVPP2 driver can handle bigger packets without a problem but > > if you do XDP redirect that won't work with other drivers and your packets will disappear. > > At least 1508 is required when I want to use XDP with a Marvell DSA > switch: the DSA header is 4 or 8 bytes long there. > > The DSA driver increases MTU on CPU switch interface by this length > (on my switches to 1504). > > So without this I cannot use XDP with mvpp2 with a Marvell switch with > default settings, which I think is not OK. Hi Marek You are running XDP programs on the master interface? So you still have the DSA tag? What sort of programs are you running? I guess DOS protection could work, once the program understands the DSA tag. To forward the frame out another interface you would have to remove the tag. Or i suppose you could modify the tag and send it back to the switch? Does XDP support that sort of hairpin operation? Andrew
> @@ -4913,8 +4914,8 @@ static int mvpp2_xdp_setup(struct mvpp2_port *port, struct netdev_bpf *bpf) > bool running = netif_running(port->dev); > bool reset = !prog != !port->xdp_prog; > > - if (port->dev->mtu > ETH_DATA_LEN) { > - NL_SET_ERR_MSG_MOD(bpf->extack, "XDP is not supported with jumbo frames enabled"); > + if (port->dev->mtu > MVPP2_MAX_RX_BUF_SIZE) { > + NL_SET_ERR_MSG_MOD(bpf->extack, "MTU too large for XDP"); Hi Marek Since MVPP2_MAX_RX_BUF_SIZE is a constant, you can probably do some CPP magic and have it part of the extack message, to give the user a clue what the maximum is. Andrew
On Tue, 5 Jan 2021 18:43:08 +0100 Marek Behún <kabel@kernel.org> wrote: > On Tue, 5 Jan 2021 18:24:37 +0100 > Sven Auhagen <sven.auhagen@voleatech.de> wrote: > > > On Tue, Jan 05, 2021 at 06:19:21PM +0100, Marek Behún wrote: > > > Currently mvpp2_xdp_setup won't allow attaching XDP program if > > > mtu > ETH_DATA_LEN (1500). > > > > > > The mvpp2_change_mtu on the other hand checks whether > > > MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE. > > > > > > These two checks are semantically different. > > > > > > Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in > > > mvpp2_rx we have > > > xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM; > > > xdp.frame_sz = PAGE_SIZE; > > > > > > Change the checks to check whether > > > mtu > MVPP2_MAX_RX_BUF_SIZE > > > > Hello Marek, > > > > in general, XDP is based on the model, that packets are not bigger > > than 1500. This is WRONG. The XDP design/model (with PAGE_SIZE 4096) allows MTU to be 3506 bytes. This comes from: * 4096 = frame_sz = PAGE_SIZE * -256 = reserved XDP_PACKET_HEADROOM * -320 = reserved tailroom with sizeof(skb_shared_info) * - 14 = Ethernet header size as MTU value is L3 4096-256-320-14 = 3506 bytes Depending on driver memory layout choices this can (of-cause) be lower. > > I am not sure if that has changed, I don't believe Jumbo Frames are > > upstreamed yet. This is unrelated to this patch, but Lorenzo and Eelco is assigned to work on this. > > You are correct that the MVPP2 driver can handle bigger packets > > without a problem but if you do XDP redirect that won't work with > > other drivers and your packets will disappear. > This statement is too harsh. The XDP layer will not do (IP-level) fragmentation for you. Thus, if you redirect/transmit frames out another interface with lower MTU than the frame packet size then the packet will of-cause be dropped (the drop counter is unfortunately not well defined). This is pretty standard behavior. This is why I'm proposing the BPF-helper bpf_check_mtu(). To allow the BPF-prog to check the MTU before doing the redirect. > At least 1508 is required when I want to use XDP with a Marvell DSA > switch: the DSA header is 4 or 8 bytes long there. > > The DSA driver increases MTU on CPU switch interface by this length > (on my switches to 1504). > > So without this I cannot use XDP with mvpp2 with a Marvell switch with > default settings, which I think is not OK. > > Since with the mvneta driver it works (mvneta checks for > MVNETA_MAX_RX_BUF_SIZE rather than ETH_DATA_LEN), I think it should also work > with mvpp2. I think you patch makes perfect sense.
On Wed, Jan 06, 2021 at 11:33:50AM +0100, Jesper Dangaard Brouer wrote: > On Tue, 5 Jan 2021 18:43:08 +0100 > Marek Behún <kabel@kernel.org> wrote: > > > On Tue, 5 Jan 2021 18:24:37 +0100 > > Sven Auhagen <sven.auhagen@voleatech.de> wrote: > > > > > On Tue, Jan 05, 2021 at 06:19:21PM +0100, Marek Behún wrote: > > > > Currently mvpp2_xdp_setup won't allow attaching XDP program if > > > > mtu > ETH_DATA_LEN (1500). > > > > > > > > The mvpp2_change_mtu on the other hand checks whether > > > > MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE. > > > > > > > > These two checks are semantically different. > > > > > > > > Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in > > > > mvpp2_rx we have > > > > xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM; > > > > xdp.frame_sz = PAGE_SIZE; > > > > > > > > Change the checks to check whether > > > > mtu > MVPP2_MAX_RX_BUF_SIZE > > > > > > Hello Marek, > > > > > > in general, XDP is based on the model, that packets are not bigger > > > than 1500. > > This is WRONG. > > The XDP design/model (with PAGE_SIZE 4096) allows MTU to be 3506 bytes. > > This comes from: > * 4096 = frame_sz = PAGE_SIZE > * -256 = reserved XDP_PACKET_HEADROOM > * -320 = reserved tailroom with sizeof(skb_shared_info) > * - 14 = Ethernet header size as MTU value is L3 > > 4096-256-320-14 = 3506 bytes > > Depending on driver memory layout choices this can (of-cause) be lower. Got it, thanks. > > > > I am not sure if that has changed, I don't believe Jumbo Frames are > > > upstreamed yet. > > This is unrelated to this patch, but Lorenzo and Eelco is assigned to > work on this. > > > > You are correct that the MVPP2 driver can handle bigger packets > > > without a problem but if you do XDP redirect that won't work with > > > other drivers and your packets will disappear. > > > > This statement is too harsh. The XDP layer will not do (IP-level) > fragmentation for you. Thus, if you redirect/transmit frames out > another interface with lower MTU than the frame packet size then the > packet will of-cause be dropped (the drop counter is unfortunately not > well defined). This is pretty standard behavior. Some drivers do not have a XDP drop counter and from own testing it is very difficult to find out what happened to the packet when it is dropped like that. > > This is why I'm proposing the BPF-helper bpf_check_mtu(). To allow the > BPF-prog to check the MTU before doing the redirect. > > > > At least 1508 is required when I want to use XDP with a Marvell DSA > > switch: the DSA header is 4 or 8 bytes long there. > > > > The DSA driver increases MTU on CPU switch interface by this length > > (on my switches to 1504). > > > > So without this I cannot use XDP with mvpp2 with a Marvell switch with > > default settings, which I think is not OK. > > > > Since with the mvneta driver it works (mvneta checks for > > MVNETA_MAX_RX_BUF_SIZE rather than ETH_DATA_LEN), I think it should also work > > with mvpp2. > > I think you patch makes perfect sense. > > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.linkedin.com%2Fin%2Fbrouer&data=04%7C01%7Csven.auhagen%40voleatech.de%7C2450996aa72245f4a6da08d8b22e971a%7Cb82a99f679814a7295344d35298f847b%7C0%7C0%7C637455260465184088%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LDmq8nFGgKuzG3rbqmaILTw6W4Qsc04MULSQvwmoVLw%3D&reserved=0 >
On Tue, 5 Jan 2021 21:21:10 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Jan 05, 2021 at 06:43:08PM +0100, Marek Behún wrote: > > On Tue, 5 Jan 2021 18:24:37 +0100 > > Sven Auhagen <sven.auhagen@voleatech.de> wrote: > > > > > On Tue, Jan 05, 2021 at 06:19:21PM +0100, Marek Behún wrote: > > > > Currently mvpp2_xdp_setup won't allow attaching XDP program if > > > > mtu > ETH_DATA_LEN (1500). > > > > > > > > The mvpp2_change_mtu on the other hand checks whether > > > > MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE. > > > > > > > > These two checks are semantically different. > > > > > > > > Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in > > > > mvpp2_rx we have > > > > xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM; > > > > xdp.frame_sz = PAGE_SIZE; > > > > > > > > Change the checks to check whether > > > > mtu > MVPP2_MAX_RX_BUF_SIZE > > > > > > Hello Marek, > > > > > > in general, XDP is based on the model, that packets are not bigger than 1500. > > > I am not sure if that has changed, I don't believe Jumbo Frames are upstreamed yet. > > > You are correct that the MVPP2 driver can handle bigger packets without a problem but > > > if you do XDP redirect that won't work with other drivers and your packets will disappear. > > > > At least 1508 is required when I want to use XDP with a Marvell DSA > > switch: the DSA header is 4 or 8 bytes long there. > > > > The DSA driver increases MTU on CPU switch interface by this length > > (on my switches to 1504). > > > > So without this I cannot use XDP with mvpp2 with a Marvell switch with > > default settings, which I think is not OK. > > Hi Marek > > You are running XDP programs on the master interface? So you still > have the DSA tag? What sort of programs are you running? I guess DOS > protection could work, once the program understands the DSA tag. To > forward the frame out another interface you would have to remove the > tag. Or i suppose you could modify the tag and send it back to the > switch? Does XDP support that sort of hairpin operation? > > Andrew Andrew, I am using bpf_fib_lookup to route the packets between switch interfaces. Here's the program for Marvell CN9130 CRB https://blackhole.sk/~kabel/src/xdp_mvswitch_route.c (This is just to experiment with XDP, so please excuse code quality.) I found out that on Turris MOX I am able to route 2.5gbps (at MTU 1500) with XDP. But when not using XDP, when the packets go via kernel's stack, MOX is able to route less than 1gbps (cca 800mbps, at MTU 1500, and the CPU is at 100%). I also to write a simple NAT masquerading program. I think XDP can increase NAT throughput to 2.5gbps as well. > Or i suppose you could modify the tag and send it back to the > switch? Does XDP support that sort of hairpin operation? You can modify the packet, prepend it with another headers (up to 256 bytes in some drivers, in mvpp2 only 224), append trailers... Look at my program above, I am popping vlan tag, for example. Marek
On Tue, 5 Jan 2021 21:23:12 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > @@ -4913,8 +4914,8 @@ static int mvpp2_xdp_setup(struct mvpp2_port *port, struct netdev_bpf *bpf) > > bool running = netif_running(port->dev); > > bool reset = !prog != !port->xdp_prog; > > > > - if (port->dev->mtu > ETH_DATA_LEN) { > > - NL_SET_ERR_MSG_MOD(bpf->extack, "XDP is not supported with jumbo frames enabled"); > > + if (port->dev->mtu > MVPP2_MAX_RX_BUF_SIZE) { > > + NL_SET_ERR_MSG_MOD(bpf->extack, "MTU too large for XDP"); > > Hi Marek > > Since MVPP2_MAX_RX_BUF_SIZE is a constant, you can probably do some > CPP magic and have it part of the extack message, to give the user a > clue what the maximum is. > > Andrew It is a constant that is computed using sizeof of some structs. I don't know if that can be converted at compile-time to string (it should be, theoretically, but I don't know if compiler allows it).
On Wed, 6 Jan 2021 12:56:08 +0100 Marek Behún <kabel@kernel.org> wrote: > I also to write a simple NAT masquerading program. I think XDP can > increase NAT throughput to 2.5gbps as well. BTW currently if XDP modifies the packet, it has to modify the checksums accordingly. There is a helper for that even, bpf_csum_diff. But many drivers can offload csum computation, mvneta and mvpp2 for example. But for this, somehow the XDP program has to let the driver know what kind of csum it needs to be computed (L3, L4 TCP/UDP). This could theoretically be communicated with the driver via metadata prepended to the packet. But a abstraction is needed, so that every driver does it in the same way. Maybe someone is already working on this, I don't know... Marek
On Wed, 6 Jan 2021 13:32:05 +0100 Marek Behún <kabel@kernel.org> wrote: > On Wed, 6 Jan 2021 12:56:08 +0100 > Marek Behún <kabel@kernel.org> wrote: > > > I also to write a simple NAT masquerading program. I think XDP can > > increase NAT throughput to 2.5gbps as well. > > BTW currently if XDP modifies the packet, it has to modify the > checksums accordingly. There is a helper for that even, bpf_csum_diff. (from L3 forwards, if you modify ethhdr, you don't have to modify L2 checksum. You can't even see it...)
On Wed, Jan 06, 2021 at 12:56:08PM +0100, Marek Behún wrote: > I found out that on Turris MOX I am able to route 2.5gbps (at MTU 1500) > with XDP. But when not using XDP, when the packets go via kernel's > stack, MOX is able to route less than 1gbps (cca 800mbps, at MTU > 1500, and the CPU is at 100%). Read this: https://patchwork.ozlabs.org/project/netdev/patch/73223229-6bc0-2647-6952-975961811866@gmail.com/ Disable GRO on the DSA interfaces and you'll be just fine even with IP forwarding done by the network stack. You don't _need_ XDP for this.
On Wed, Jan 06, 2021 at 01:32:05PM +0100, Marek Behún wrote: > On Wed, 6 Jan 2021 12:56:08 +0100 > Marek Behún <kabel@kernel.org> wrote: > > > I also to write a simple NAT masquerading program. I think XDP can > > increase NAT throughput to 2.5gbps as well. > > BTW currently if XDP modifies the packet, it has to modify the > checksums accordingly. There is a helper for that even, bpf_csum_diff. > > But many drivers can offload csum computation, mvneta and mvpp2 for > example. Hi Marek It does require that the MAC is DSA aware. The Freescale FEC for example cannot do such csum, because the DSA header confuses it, and it cannot find the IP header, etc. So if you do look at a generic way to implement this, you need the MAC driver to indicate it has the needed support. Andrew
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index afdd22827223..65490a0eb657 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -4623,11 +4623,12 @@ static int mvpp2_change_mtu(struct net_device *dev, int mtu) mtu = ALIGN(MVPP2_RX_PKT_SIZE(mtu), 8); } + if (port->xdp_prog && mtu > MVPP2_MAX_RX_BUF_SIZE) { + netdev_err(dev, "Illegal MTU value %d for XDP mode\n", mtu); + return -EINVAL; + } + if (MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE) { - if (port->xdp_prog) { - netdev_err(dev, "Jumbo frames are not supported with XDP\n"); - return -EINVAL; - } if (priv->percpu_pools) { netdev_warn(dev, "mtu %d too high, switching to shared buffers", mtu); mvpp2_bm_switch_buffers(priv, false); @@ -4913,8 +4914,8 @@ static int mvpp2_xdp_setup(struct mvpp2_port *port, struct netdev_bpf *bpf) bool running = netif_running(port->dev); bool reset = !prog != !port->xdp_prog; - if (port->dev->mtu > ETH_DATA_LEN) { - NL_SET_ERR_MSG_MOD(bpf->extack, "XDP is not supported with jumbo frames enabled"); + if (port->dev->mtu > MVPP2_MAX_RX_BUF_SIZE) { + NL_SET_ERR_MSG_MOD(bpf->extack, "MTU too large for XDP"); return -EOPNOTSUPP; }
Currently mvpp2_xdp_setup won't allow attaching XDP program if mtu > ETH_DATA_LEN (1500). The mvpp2_change_mtu on the other hand checks whether MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE. These two checks are semantically different. Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in mvpp2_rx we have xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM; xdp.frame_sz = PAGE_SIZE; Change the checks to check whether mtu > MVPP2_MAX_RX_BUF_SIZE Signed-off-by: Marek Behún <kabel@kernel.org> Cc: Sven Auhagen <sven.auhagen@voleatech.de> Cc: Matteo Croce <mcroce@microsoft.com> Cc: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)