Hi Zhenyu, Thank you for working on this, I have couple of questions in this patch.
Regards _Sugesh > -----Original Message----- > From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- > boun...@openvswitch.org] On Behalf Of Zhenyu Gao > Sent: Friday, June 16, 2017 1:54 PM > To: b...@ovn.org; u9012...@gmail.com; ktray...@redhat.com; Kavanagh, > Mark B <mark.b.kavan...@intel.com>; d...@openvswitch.org > Subject: [ovs-dev] [RFC PATCH v1] net-dpdk: Introducing TX tcp HW > checksum offload support for DPDK pnic > > This patch introduce TX tcp-checksum offload support for DPDK pnic. > The feature is disabled by default and can be enabled by setting tx- > checksum-offload, which like: > ovs-vsctl set Interface dpdk-eth3 \ > options:tx-checksum-offload=true > --- > lib/netdev-dpdk.c | 112 > +++++++++++++++++++++++++++++++++++++++++++++++---- > vswitchd/vswitch.xml | 13 ++++-- > 2 files changed, 115 insertions(+), 10 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index bba4de3..5a68a48 > 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -32,6 +32,7 @@ > #include <rte_mbuf.h> > #include <rte_meter.h> > #include <rte_virtio_net.h> > +#include <rte_ip.h> > > #include "dirs.h" > #include "dp-packet.h" > @@ -328,6 +329,7 @@ struct ingress_policer { > > enum dpdk_hw_ol_features { > NETDEV_RX_CHECKSUM_OFFLOAD = 1 << 0, > + NETDEV_TX_CHECKSUM_OFFLOAD = 1 << 1, > }; > > struct netdev_dpdk { > @@ -649,6 +651,8 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk > *dev, int n_rxq, int n_txq) > int diag = 0; > int i; > struct rte_eth_conf conf = port_conf; > + struct rte_eth_txconf *txconf; > + struct rte_eth_dev_info dev_info; > > if (dev->mtu > ETHER_MTU) { > conf.rxmode.jumbo_frame = 1; > @@ -676,9 +680,16 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk > *dev, int n_rxq, int n_txq) > break; > } > > + rte_eth_dev_info_get(dev->port_id, &dev_info); > + txconf = &dev_info.default_txconf; > + if (dev->hw_ol_features & NETDEV_TX_CHECKSUM_OFFLOAD) { > + /*Enable tx offload feature on pnic*/ > + txconf->txq_flags = 0; > + } > + > for (i = 0; i < n_txq; i++) { > diag = rte_eth_tx_queue_setup(dev->port_id, i, dev->txq_size, > - dev->socket_id, NULL); > + dev->socket_id, txconf); > if (diag) { > VLOG_INFO("Interface %s txq(%d) setup error: %s", > dev->up.name, i, rte_strerror(-diag)); @@ -724,11 > +735,15 @@ > dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev) { > struct rte_eth_dev_info info; > bool rx_csum_ol_flag = false; > + bool tx_csum_ol_flag = false; > uint32_t rx_chksm_offload_capa = DEV_RX_OFFLOAD_UDP_CKSUM | > DEV_RX_OFFLOAD_TCP_CKSUM | > DEV_RX_OFFLOAD_IPV4_CKSUM; > + uint32_t tx_chksm_offload_capa = DEV_TX_OFFLOAD_TCP_CKSUM; [Sugesh] Any reason, why this patch does only the TCP checksum offload?? The command line option says tx_checksum offload (it could be mistakenly considered for full checksum offload). > + > rte_eth_dev_info_get(dev->port_id, &info); > rx_csum_ol_flag = (dev->hw_ol_features & > NETDEV_RX_CHECKSUM_OFFLOAD) != 0; > + tx_csum_ol_flag = (dev->hw_ol_features & > + NETDEV_TX_CHECKSUM_OFFLOAD) != 0; > > if (rx_csum_ol_flag && > (info.rx_offload_capa & rx_chksm_offload_capa) != @@ -736,9 +751,15 > @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev) > VLOG_WARN_ONCE("Rx checksum offload is not supported on device > %"PRIu8, > dev->port_id); > dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD; > - return; > + } else if (tx_csum_ol_flag && > + (info.tx_offload_capa & tx_chksm_offload_capa) != > + tx_chksm_offload_capa) { > + VLOG_WARN_ONCE("Tx checksum offload is not supported on device > %"PRIu8, > + dev->port_id); > + dev->hw_ol_features &= ~NETDEV_TX_CHECKSUM_OFFLOAD; > + } else { > + netdev_request_reconfigure(&dev->up); > } > - netdev_request_reconfigure(&dev->up); > } > > -- [Sugesh] What is the performance improvement offered with this feature? Do you have any numbers to share? I think DPDK uses non-vector functions when Tx checksum offload is enabled. Will it give enough performance improvement to mitigate that cost? Finally Rx checksum offload is going to be a default option (there wont be any configuration option to enable/disable, Kevin's patch for the support is already acked and waiting to merge). Similarly can't we enable it by default when it is supported? > 1.8.3.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev