On 11/11/2019 15:59, Ilya Maximets wrote: > On 11.11.2019 16:55, Kevin Traynor wrote: >> On 10/11/2019 23:20, Ilya Maximets wrote: >>> On 29.10.2019 15:50, Sriram Vatala wrote:> @@ -2388,12 +2412,16 @@ >>> __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >>>> } >>>> } while (cnt && (retries++ < max_retries)); >>>> >>>> + tx_failure = cnt; >>>> rte_spinlock_unlock(&dev->tx_q[qid].tx_lock); >>>> >>>> rte_spinlock_lock(&dev->stats_lock); >>>> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, >>>> cnt + dropped); >>>> - dev->tx_retries += MIN(retries, max_retries); >>>> + sw_stats->tx_retries += MIN(retries, max_retries); >>>> + sw_stats->tx_failure_drops += tx_failure; >>>> + sw_stats->tx_mtu_exceeded_drops += mtu_drops; >>>> + sw_stats->tx_qos_drops += qos_drops; >>> >>> Kevin pointed to this part of code in hope that we can move this to >>> a separate function and reuse in his review for v9. This code catches >>> my eyes too. I don't think that we can reuse this part, at least this >>> will be not very efficient in current situation (clearing of the unused >>> fields in a stats structure will be probably needed before calling such >>> a common function, but ETH tx uses only half of the struct). >>> >>> But there is another thing here. We already have special functions >>> for vhost tx/rx counters. And it looks strange when we're not using >>> these functions to update tx/rx failure counters. >>> >>> Suggesting following incremental. >>> Kevin, Sriram, please, share your thoughts. >>> >> >> The incremental patch looks good, thanks. One additional thing is that >> OVS_REQUIRES(dev->stats_lock) annotations can be used for the vhost >> rx/tx update counter functions now (even if it seems unlikely someone >> would miss doing that). >> >> @Sriram, see below or you can check similar OVS_REQUIRES usage elsewhere >> in the file. > > I'm not sure if clang annotations will work with rte_spinlock. > DPDK doesn't have proper annotations for locking functions. >
Ah, good point, I didn't check the lock type. In that case nevermind, patch+incremental LGTM as is. Acked-by: Kevin Traynor <ktray...@redhat.com> thanks, Kevin. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev