Regards _Sugesh
> -----Original Message----- > From: Pravin Shelar [mailto:[email protected]] > Sent: Friday, December 16, 2016 7:50 AM > To: Chandran, Sugesh <[email protected]> > Cc: ovs dev <[email protected]>; Jesse Gross <[email protected]>; Ben > Pfaff <[email protected]> > Subject: Re: [ovs-dev] [PATCH v7] netdev-dpdk: Enable Rx checksum > offloading feature on DPDK physical ports. > > On Thu, Dec 15, 2016 at 4:07 AM, Chandran, Sugesh > <[email protected]> wrote: > > > > > > Regards > > _Sugesh > > > >> -----Original Message----- > >> From: Pravin Shelar [mailto:[email protected]] > >> Sent: Wednesday, December 14, 2016 9:42 PM > >> To: Chandran, Sugesh <[email protected]> > >> Cc: ovs dev <[email protected]>; Jesse Gross <[email protected]>; > >> Ben Pfaff <[email protected]> > >> Subject: Re: [ovs-dev] [PATCH v7] netdev-dpdk: Enable Rx checksum > >> offloading feature on DPDK physical ports. > >> > >> Thanks for the patch. I have couple of questions. > >> > >> On Wed, Dec 14, 2016 at 7:30 AM, Sugesh Chandran > >> <[email protected]> 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 <[email protected]> > >> > Acked-by: Jesse Gross <[email protected]> > >> > > >> ... > >> ... > >> > 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. > > [Sugesh] Reconfigure is called here to enable/disable the checksum at > > run time. Do you think the checksum will be updated on a user request > > without having this call? > > I do not see need to reconfigure netdev here. [Sugesh] Sorry Pravin, Looks like I am missing something here. Can you please elaborate why you feel the reconfigure is not needed 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. > > [Sugesh] This is also a good place where we can think of using the Rx > checksum offload. > > As I mentioned in the commit message, the checksum offload disables > > vectorization which impacts the performance. > > We must need to test the checksum offload in different conn-track use > > cases to make sure the checksum offload really offers the performance > improvement before refactoring the code. > > > > I just had a glance on conntrack lib, it seems only L4 checksum is validated > when compared to the tunneling case where both IP and L4 checksum > validated for every packets. > > > > I can modify the conntrack lib and test to see if any performance > > improvement can be achieved with Rx checksum offload. If it offers > > acceptable performance boost, I will send out another patch to enable Rx > checksum offload in conntrack module + refactoring the checksum offload > around checksum API. > > What do you think? > > > > I think it should improve performance even in this case, But I am fine with > working on this later as separate patch series. [Sugesh] Great, Thanks Pravin, I will work on the follow up patches once this get upstreamed. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
