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.
>>
>> > +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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
