[ovs-dev] [PATCH v7] netdev-dpdk: Enable Rx checksum offloading feature on DPDK physical ports.

[Pravin Shelar]

Thanks for the patch. I have couple of questions.

On Wed, Dec 14, 2016 at 7:30 AM, Sugesh Chandran
<sugesh.chandran at intel.com> wrote:
> Add Rx checksum offloading feature support on DPDK physical ports. By default,
> the Rx checksum offloading is enabled if NIC supports. However,
> the checksum offloading can be turned OFF either while adding a new DPDK
> physical port to OVS or at runtime.
>
> The rx checksum offloading can be turned off by setting the parameter to
> 'false'. For eg: To disable the rx checksum offloading when adding a port,
>
>      'ovs-vsctl add-port br0 dpdk0 -- \
>       set Interface dpdk0 type=dpdk options:rx-checksum-offload=false'
>
> OR (to disable at run time after port is being added to OVS)
>
>     'ovs-vsctl set Interface dpdk0 options:rx-checksum-offload=false'
>
> Similarly to turn ON rx checksum offloading at run time,
>     'ovs-vsctl set Interface dpdk0 options:rx-checksum-offload=true'
>
> The Tx checksum offloading support is not implemented due to the following
> reasons.
>
> 1) Checksum offloading and vectorization are mutually exclusive in DPDK poll
> mode driver. Vector packet processing is turned OFF when checksum offloading
> is enabled which causes significant performance drop at Tx side.
>
> 2) Normally, OVS generates checksum for tunnel packets in software at the
> 'tunnel push' operation, where the tunnel headers are created. However
> enabling Tx checksum offloading involves,
>
> *) Mark every packets for tx checksum offloading at 'tunnel_push' and
> recirculate.
> *) At the time of xmit, validate the same flag and instruct the NIC to do the
> checksum calculation.  In case NIC doesnt support Tx checksum offloading,
> the checksum calculation has to be done in software before sending out the
> packets.
>
> No significant performance improvement noticed with Tx checksum offloading
> due to the e overhead of additional validations + non vector packet processing.
> In some test scenarios, it introduces performance drop too.
>
> Rx checksum offloading still offers 8-9% of improvement on VxLAN tunneling
> decapsulation even though the SSE vector Rx function is disabled in DPDK poll
> mode driver.
>
> Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>
> Acked-by: Jesse Gross <jesse at kernel.org>
>
...
...
>  static void
> +dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)
> +    OVS_REQUIRES(dev->mutex)
> +{
> +    struct rte_eth_dev_info info;
> +    bool rx_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;
> +    rte_eth_dev_info_get(dev->port_id, &info);
> +    rx_csum_ol_flag = (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD) != 0;
> +
> +    if (rx_csum_ol_flag &&
> +        (info.rx_offload_capa & rx_chksm_offload_capa) !=
> +         rx_chksm_offload_capa) {
> +        VLOG_WARN_ONCE("Failed to enable Rx checksum offload on device %d",
> +                   dev->port_id);
> +        dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
> +        return;
> +    }
> +    netdev_request_reconfigure(&dev->up);
> +}
> +
I am not sure about need for netdev reconfigure here.

> +static void
>  dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)
>  {
>      if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
> @@ -851,6 +884,9 @@ netdev_dpdk_init(struct netdev *netdev, unsigned int port_no,
>
>      /* Initialize the flow control to NULL */
>      memset(&dev->fc_conf, 0, sizeof dev->fc_conf);
> +
> +    /* Initilize the hardware offload flags to 0 */
> +    dev->hw_ol_features = 0;
>      if (type == DPDK_DEV_ETH) {
>          err = dpdk_eth_dev_init(dev);
>          if (err) {
> @@ -1118,6 +1154,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
>          {RTE_FC_NONE,     RTE_FC_TX_PAUSE},
>          {RTE_FC_RX_PAUSE, RTE_FC_FULL    }
>      };
> +    bool rx_chksm_ofld;
> +    bool temp_flag;
>
>      ovs_mutex_lock(&dev->mutex);
>
> @@ -1141,6 +1179,15 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
>          dpdk_eth_flow_ctrl_setup(dev);
>      }
>
> +    /* Rx checksum offload configuration */
> +    /* By default the Rx checksum offload is ON */
> +    rx_chksm_ofld = smap_get_bool(args, "rx-checksum-offload", true);
> +    temp_flag = (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD)
> +                        != 0;
> +    if (temp_flag != rx_chksm_ofld) {
> +        dev->hw_ol_features ^= NETDEV_RX_CHECKSUM_OFFLOAD;
> +        dpdk_eth_checksum_offload_configure(dev);
> +    }
>      ovs_mutex_unlock(&dev->mutex);
>
Can you also reflect this feature back to user via get-status.

>      return 0;
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index ce2582f..c730e72 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -85,9 +85,11 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>
>          ovs_be32 ip_src, ip_dst;
>
> -        if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
> -            VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
> -            return NULL;
> +        if (OVS_UNLIKELY(!dp_packet_ip_checksum_valid(packet))) {
> +            if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
> +                VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
> +                return NULL;
> +            }
>          }
>
>          if (ntohs(ip->ip_tot_len) > l3_size) {
> @@ -179,18 +181,21 @@ udp_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>      }
>
>      if (udp->udp_csum) {
> -        uint32_t csum;
> -        if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> -            csum = packet_csum_pseudoheader6(dp_packet_l3(packet));
> -        } else {
> -            csum = packet_csum_pseudoheader(dp_packet_l3(packet));
> -        }
> -
> -        csum = csum_continue(csum, udp, dp_packet_size(packet) -
> -                             ((const unsigned char *)udp -
> -                              (const unsigned char *)dp_packet_l2(packet)));
> -        if (csum_finish(csum)) {
> -            return NULL;
> +        if (OVS_UNLIKELY(!dp_packet_l4_checksum_valid(packet))) {
> +            uint32_t csum;
> +            if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> +                csum = packet_csum_pseudoheader6(dp_packet_l3(packet));
> +            } else {
> +                csum = packet_csum_pseudoheader(dp_packet_l3(packet));
> +            }
> +
> +            csum = csum_continue(csum, udp, dp_packet_size(packet) -
> +                                 ((const unsigned char *)udp -
> +                                  (const unsigned char *)dp_packet_l2(packet)
> +                                 ));
> +            if (csum_finish(csum)) {
> +                return NULL;
> +            }

Can we make use of RX checksum offload for userspace conntrack lib? I
think there is already code duplication in checksum verification, we
could refactor code around checksum API with hardware offload to avoid
it and likely improve performance.