On 11.11.2019 17:06, Kevin Traynor wrote: > 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. In this case, I think, there is no need to re-spin the series. I'll just squash the incremental with this patch and give it another try. If it'll work fine, I'll apply the series. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
