| Message ID | 20200204212826.29508-1-david.marchand@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] netdev-dpdk: Fix port init when lacking Tx offloads for TSO. | expand |
On 04/02/2020 21:28, David Marchand wrote: > The check on TSO capability did not ensure ip checksum, tcp checksum and > TSO tx offloads were available which resulted in a port init failure > (example below with a ena device): > > *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx > offloads 0x2a doesn't match Tx offloads capabilities 0xe in > rte_eth_dev_configure()* > > Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") > > Reported-by: Ravi Kerur <rkerur@gmail.com> > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > lib/netdev-dpdk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index b108cbd6b..eb1a7af94 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1132,7 +1132,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) > dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; > } > > - if (info.tx_offload_capa & tx_tso_offload_capa) { > + if ((info.tx_offload_capa & tx_tso_offload_capa) == tx_tso_offload_capa) { > dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; > } else { > dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; > Acked-by: Kevin Traynor <ktraynor@redhat.com>
On Tue, Feb 04, 2020 at 10:28:26PM +0100, David Marchand wrote: > The check on TSO capability did not ensure ip checksum, tcp checksum and > TSO tx offloads were available which resulted in a port init failure > (example below with a ena device): > > *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx > offloads 0x2a doesn't match Tx offloads capabilities 0xe in > rte_eth_dev_configure()* > > Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") > > Reported-by: Ravi Kerur <rkerur@gmail.com> > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- Works for me. Acked-by: Flavio Leitner <fbl@sysclose.org> This needs to go to master and branch-2.13. Thanks! fbl
On Wed, Feb 5, 2020 at 12:42 PM Flavio Leitner <fbl@sysclose.org> wrote: > > On Tue, Feb 04, 2020 at 10:28:26PM +0100, David Marchand wrote: > > The check on TSO capability did not ensure ip checksum, tcp checksum and > > TSO tx offloads were available which resulted in a port init failure > > (example below with a ena device): > > > > *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx > > offloads 0x2a doesn't match Tx offloads capabilities 0xe in > > rte_eth_dev_configure()* > > > > Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") > > > > Reported-by: Ravi Kerur <rkerur@gmail.com> > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > --- > > Works for me. > Acked-by: Flavio Leitner <fbl@sysclose.org> > > This needs to go to master and branch-2.13. Thinking again about this, don't we have an issue in that we always ask drivers for offloads even if TSO is disabled? I would expect a performance impact, since dpdk pmds (eyeing ixgbe driver which checks txq->offloads == 0) love to select optimised tx function based on offloads configuration.
On 2/5/20 7:01 PM, David Marchand wrote: > On Wed, Feb 5, 2020 at 12:42 PM Flavio Leitner <fbl@sysclose.org> wrote: >> >> On Tue, Feb 04, 2020 at 10:28:26PM +0100, David Marchand wrote: >>> The check on TSO capability did not ensure ip checksum, tcp checksum and >>> TSO tx offloads were available which resulted in a port init failure >>> (example below with a ena device): >>> >>> *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx >>> offloads 0x2a doesn't match Tx offloads capabilities 0xe in >>> rte_eth_dev_configure()* >>> >>> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") >>> >>> Reported-by: Ravi Kerur <rkerur@gmail.com> >>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>> --- >> >> Works for me. >> Acked-by: Flavio Leitner <fbl@sysclose.org> >> >> This needs to go to master and branch-2.13. > > Thinking again about this, don't we have an issue in that we always > ask drivers for offloads even if TSO is disabled? > > I would expect a performance impact, since dpdk pmds (eyeing ixgbe > driver which checks txq->offloads == 0) love to select optimised tx > function based on offloads configuration. > > Yes. There will be significant performance impact. Last time I compared different i40e tx functions there was performance difference ~20-25% in phy-phy test. It was a long time ago, however we should not change the default behavior anyway. Suggesting following incremental: --- diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index eb1a7af94..6187129c0 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1132,12 +1132,15 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; } - if ((info.tx_offload_capa & tx_tso_offload_capa) == tx_tso_offload_capa) { - dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; - } else { - dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; - VLOG_WARN("Tx TSO offload is not supported on %s port " - DPDK_PORT_ID_FMT, netdev_get_name(&dev->up), dev->port_id); + dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; + if (userspace_tso_enabled()) { + if ((info.tx_offload_capa & tx_tso_offload_capa) + == tx_tso_offload_capa) { + dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; + } else { + VLOG_WARN("%s: Tx TSO offload is not supported.", + netdev_get_name(&dev->up)); + } } n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); --- Could anyone, please, test this? David, you may squash this into your patch with corresponding update for a commit message or I could apply current patch as is and send above diff as a separate change. What will you prefer? BTW, one more thing I'm concerned about is that we're ultimately enabling any checksum offloading for VMs, but for HW Tx path we're enabling only TCP in pair with IPv4. What about IPv6 and UDP cases? We're actually not disabling UFO. We're most likely sending UDP/IPv6 packets with incorrect checksums and even trying to send non-fragmented huge UDP packets on the wire. This is the matter for a separate fix. And I believe that other parts of TSO implementation in OVS doesn't handle UDP case correctly. Best regards, Ilya Maximets.
On Thu, Feb 06, 2020 at 11:48:58AM +0100, Ilya Maximets wrote: > On 2/5/20 7:01 PM, David Marchand wrote: > > On Wed, Feb 5, 2020 at 12:42 PM Flavio Leitner <fbl@sysclose.org> wrote: > >> > >> On Tue, Feb 04, 2020 at 10:28:26PM +0100, David Marchand wrote: > >>> The check on TSO capability did not ensure ip checksum, tcp checksum and > >>> TSO tx offloads were available which resulted in a port init failure > >>> (example below with a ena device): > >>> > >>> *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx > >>> offloads 0x2a doesn't match Tx offloads capabilities 0xe in > >>> rte_eth_dev_configure()* > >>> > >>> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") > >>> > >>> Reported-by: Ravi Kerur <rkerur@gmail.com> > >>> Signed-off-by: David Marchand <david.marchand@redhat.com> > >>> --- > >> > >> Works for me. > >> Acked-by: Flavio Leitner <fbl@sysclose.org> > >> > >> This needs to go to master and branch-2.13. > > > > Thinking again about this, don't we have an issue in that we always > > ask drivers for offloads even if TSO is disabled? > > > > I would expect a performance impact, since dpdk pmds (eyeing ixgbe > > driver which checks txq->offloads == 0) love to select optimised tx > > function based on offloads configuration. > > > > > > > > Yes. There will be significant performance impact. Last time I compared > > different i40e tx functions there was performance difference ~20-25% in > > phy-phy test. > It was a long time ago, however we should not change the > default behavior anyway. > > > Suggesting following incremental: > > > > --- > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index eb1a7af94..6187129c0 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1132,12 +1132,15 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) > dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; > } > > - if ((info.tx_offload_capa & tx_tso_offload_capa) == tx_tso_offload_capa) { > - dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; > - } else { > - dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; > - VLOG_WARN("Tx TSO offload is not supported on %s port " > - DPDK_PORT_ID_FMT, netdev_get_name(&dev->up), dev->port_id); > + dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; > + if (userspace_tso_enabled()) { > + if ((info.tx_offload_capa & tx_tso_offload_capa) > + == tx_tso_offload_capa) { > + dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; > + } else { > + VLOG_WARN("%s: Tx TSO offload is not supported.", > + netdev_get_name(&dev->up)); > + } > } > > n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); > --- > > Could anyone, please, test this? I had the same patch under testing during the night: today: 13252499.10 +-71996.09 patch: 13732079.60 +-97931.85 pps We should do it. Thanks,
On Thu, Feb 6, 2020 at 12:50 PM Flavio Leitner <fbl@sysclose.org> wrote: > > On Thu, Feb 06, 2020 at 11:48:58AM +0100, Ilya Maximets wrote: > > On 2/5/20 7:01 PM, David Marchand wrote: > > > On Wed, Feb 5, 2020 at 12:42 PM Flavio Leitner <fbl@sysclose.org> wrote: > > >> > > >> On Tue, Feb 04, 2020 at 10:28:26PM +0100, David Marchand wrote: > > >>> The check on TSO capability did not ensure ip checksum, tcp checksum and > > >>> TSO tx offloads were available which resulted in a port init failure > > >>> (example below with a ena device): > > >>> > > >>> *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx > > >>> offloads 0x2a doesn't match Tx offloads capabilities 0xe in > > >>> rte_eth_dev_configure()* > > >>> > > >>> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") > > >>> > > >>> Reported-by: Ravi Kerur <rkerur@gmail.com> > > >>> Signed-off-by: David Marchand <david.marchand@redhat.com> > > >>> --- > > >> > > >> Works for me. > > >> Acked-by: Flavio Leitner <fbl@sysclose.org> > > >> > > >> This needs to go to master and branch-2.13. > > > > > > Thinking again about this, don't we have an issue in that we always > > > ask drivers for offloads even if TSO is disabled? > > > > > > I would expect a performance impact, since dpdk pmds (eyeing ixgbe > > > driver which checks txq->offloads == 0) love to select optimised tx > > > function based on offloads configuration. > > > > > > > > > > > > > > Yes. There will be significant performance impact. Last time I compared > > > > different i40e tx functions there was performance difference ~20-25% in > > > > phy-phy test. > > It was a long time ago, however we should not change the > > default behavior anyway. > > > > > > Suggesting following incremental: > > > > > > > > --- > > > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index eb1a7af94..6187129c0 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -1132,12 +1132,15 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) > > dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; > > } > > > > - if ((info.tx_offload_capa & tx_tso_offload_capa) == tx_tso_offload_capa) { > > - dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; > > - } else { > > - dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; > > - VLOG_WARN("Tx TSO offload is not supported on %s port " > > - DPDK_PORT_ID_FMT, netdev_get_name(&dev->up), dev->port_id); > > + dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; > > + if (userspace_tso_enabled()) { > > + if ((info.tx_offload_capa & tx_tso_offload_capa) > > + == tx_tso_offload_capa) { > > + dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; > > + } else { > > + VLOG_WARN("%s: Tx TSO offload is not supported.", > > + netdev_get_name(&dev->up)); > > + } > > } > > > > n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); > > --- > > > > Could anyone, please, test this? > > I had the same patch under testing during the night: > today: 13252499.10 +-71996.09 > patch: 13732079.60 +-97931.85 pps I am fine with taking my original patch as is. Then either Ilya or you send a follow up patch. Thanks.
On 2/6/20 1:28 PM, David Marchand wrote: > On Thu, Feb 6, 2020 at 12:50 PM Flavio Leitner <fbl@sysclose.org> wrote: >> >> On Thu, Feb 06, 2020 at 11:48:58AM +0100, Ilya Maximets wrote: >>> On 2/5/20 7:01 PM, David Marchand wrote: >>>> On Wed, Feb 5, 2020 at 12:42 PM Flavio Leitner <fbl@sysclose.org> wrote: >>>>> >>>>> On Tue, Feb 04, 2020 at 10:28:26PM +0100, David Marchand wrote: >>>>>> The check on TSO capability did not ensure ip checksum, tcp checksum and >>>>>> TSO tx offloads were available which resulted in a port init failure >>>>>> (example below with a ena device): >>>>>> >>>>>> *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx >>>>>> offloads 0x2a doesn't match Tx offloads capabilities 0xe in >>>>>> rte_eth_dev_configure()* >>>>>> >>>>>> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") >>>>>> >>>>>> Reported-by: Ravi Kerur <rkerur@gmail.com> >>>>>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>>>>> --- >>>>> >>>>> Works for me. >>>>> Acked-by: Flavio Leitner <fbl@sysclose.org> >>>>> >>>>> This needs to go to master and branch-2.13. >>>> >>>> Thinking again about this, don't we have an issue in that we always >>>> ask drivers for offloads even if TSO is disabled? >>>> >>>> I would expect a performance impact, since dpdk pmds (eyeing ixgbe >>>> driver which checks txq->offloads == 0) love to select optimised tx >>>> function based on offloads configuration. >>>> >>>> >>> >>> >>> >>> Yes. There will be significant performance impact. Last time I compared >>> >>> different i40e tx functions there was performance difference ~20-25% in >>> >>> phy-phy test. >>> It was a long time ago, however we should not change the >>> default behavior anyway. >>> >>> >>> Suggesting following incremental: >>> >>> >>> >>> --- >>> >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index eb1a7af94..6187129c0 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -1132,12 +1132,15 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) >>> dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; >>> } >>> >>> - if ((info.tx_offload_capa & tx_tso_offload_capa) == tx_tso_offload_capa) { >>> - dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; >>> - } else { >>> - dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; >>> - VLOG_WARN("Tx TSO offload is not supported on %s port " >>> - DPDK_PORT_ID_FMT, netdev_get_name(&dev->up), dev->port_id); >>> + dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD; >>> + if (userspace_tso_enabled()) { >>> + if ((info.tx_offload_capa & tx_tso_offload_capa) >>> + == tx_tso_offload_capa) { >>> + dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; >>> + } else { >>> + VLOG_WARN("%s: Tx TSO offload is not supported.", >>> + netdev_get_name(&dev->up)); >>> + } >>> } >>> >>> n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); >>> --- >>> >>> Could anyone, please, test this? >> >> I had the same patch under testing during the night: >> today: 13252499.10 +-71996.09 >> patch: 13732079.60 +-97931.85 pps > > I am fine with taking my original patch as is. > Then either Ilya or you send a follow up patch. I applied this patch to master and branch-2.13 as is and sent above diff as a separate patch. Best regards, Ilya Maximets.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index b108cbd6b..eb1a7af94 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1132,7 +1132,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; } - if (info.tx_offload_capa & tx_tso_offload_capa) { + if ((info.tx_offload_capa & tx_tso_offload_capa) == tx_tso_offload_capa) { dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD; } else { dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD;
The check on TSO capability did not ensure ip checksum, tcp checksum and TSO tx offloads were available which resulted in a port init failure (example below with a ena device): *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx offloads 0x2a doesn't match Tx offloads capabilities 0xe in rte_eth_dev_configure()* Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support") Reported-by: Ravi Kerur <rkerur@gmail.com> Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/netdev-dpdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)