[ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.
Ilya Maximets
i.maximets at samsung.com
Tue Mar 20 08:49:35 UTC 2018
On 19.03.2018 13:22, Weglicki, MichalX wrote:
> Hello,
Hello.
>
> I've went through the patch quite carefully.
Thanks for reviewing this.
> Main change refactors creation cached IDs and Names from IF-like code block to "Goto" code block.
Current code is over-nested. It has nesting level of 6 (7 including function definition).
It's like:
netdev_dpdk_configure_xstats
{
if () {
if () {
...
} else {
...
if () {
...
if () {
} else if {
}
...
if () {
for () {
if () { <-- level #7 !
}
}
} else {
}
}
}
} else {
}
if () {
}
}
IMHO, This is not readable. Especially, combining with constant line wraps because
of limited line lengths. I wanted to make plain execution sequence to simplify
understanding of the code. Most of the 'if' statements are just error checking.
And the single exit point from such conditions usually implemented by 'goto's.
It's a common practice for system programming. It doesn't worth to keep so deep
nesting level for error checking conditions. This also matches the style of all
the other code in netdev-dpdk and not only here.
>
> There are also some initializations removal, which I don't personally agree with - even if those seems to be not needed, code may always evolve in the future.
Could you, please, specify?
> There is one xcalloc pointless check, true.
>
> The last important thing is that counters configuration can change during runtime, and I was facing a problem, where amount of counters was different during some configuration stages. That is why my initial implementation was looking for counter name based on given ID, not returned index in array => That was a reason to keep whole list of names, but only limited IDs(filtered out). Also I do think that returned array should be checked against IDs, as implementation can always change in the future as well.
Hmm. OK. But I think, in this case we could even more simplify the code.
As I understand, the 'netdev_dpdk_reconfigure' is the only function that
could make influence on the stats counters in HW. Correct me, if I'm wrong.
In this case we could call 'netdev_dpdk_configure_xstats()' only once at the
end of reconfiguration. After that all the inconsistency with returned
from DPDK values should be treated as HW or DPDK error and we should not
handle this case in OVS and just do not return any stats.
This will simplify 'netdev_dpdk_get_custom_stats()' too.
We also could refactor 'netdev_dpdk_get_stats' in a same way and reuse
already configured xstats.
What do you think?
Best regards, Ilya Maximets.
>
> Br,
> Michal.
>
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets at samsung.com]
>> Sent: Tuesday, January 23, 2018 2:14 PM
>> To: ovs-dev at openvswitch.org
>> Cc: Heetae Ahn <heetae82.ahn at samsung.com>; Ben Pfaff <blp at ovn.org>; Stokes, Ian <ian.stokes at intel.com>; Weglicki, MichalX
>> <michalx.weglicki at intel.com>; Ilya Maximets <i.maximets at samsung.com>
>> Subject: [PATCH] netdev-dpdk: Refactor custom stats.
>>
>> This code is overcomplicated and completely unreadable. And a
>> bunch of recently fixed memory leaks confirms that statement.
>>
>> Main concerns that were fixed:
>> * Too big nesting level.
>> * Useless checks like pointer checking after xmalloc.
>> * Misleading comments.
>> * Bad names of the variables.
>>
>> As a bonus, size of the code significantly reduced.
>>
>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>> ---
>>
>> This patch made on top of memory leaks' fixes:
>> * https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343537.html
>> * https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343538.html
>>
>> lib/netdev-dpdk.c | 215 ++++++++++++++++++++----------------------------------
>> 1 file changed, 80 insertions(+), 135 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 50a94d1..e834c7e 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -437,10 +437,9 @@ struct netdev_dpdk {
>>
>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>> /* Names of all XSTATS counters */
>> - struct rte_eth_xstat_name *rte_xstats_names;
>> - int rte_xstats_names_size;
>> - int rte_xstats_ids_size;
>> - uint64_t *rte_xstats_ids;
>> + struct rte_eth_xstat_name *xstats_names;
>> + uint64_t *xstats_ids;
>> + int xstats_count;
>> );
>> };
>>
>> @@ -901,6 +900,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
>> dev->vhost_reconfigured = false;
>> dev->attached = false;
>>
>> + dev->xstats_count = 0;
>> +
>> ovsrcu_init(&dev->qos_conf, NULL);
>>
>> ovsrcu_init(&dev->ingress_policer, NULL);
>> @@ -925,13 +926,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
>> ovs_list_push_back(&dpdk_list, &dev->list_node);
>>
>> netdev_request_reconfigure(netdev);
>> -
>> - dev->rte_xstats_names = NULL;
>> - dev->rte_xstats_names_size = 0;
>> -
>> - dev->rte_xstats_ids = NULL;
>> - dev->rte_xstats_ids_size = 0;
>> -
>> return 0;
>> }
>>
>> @@ -1174,123 +1168,83 @@ netdev_dpdk_dealloc(struct netdev *netdev)
>> static void
>> netdev_dpdk_clear_xstats(struct netdev_dpdk *dev)
>> {
>> - /* If statistics are already allocated, we have to
>> - * reconfigure, as port_id could have been changed. */
>> - if (dev->rte_xstats_names) {
>> - free(dev->rte_xstats_names);
>> - dev->rte_xstats_names = NULL;
>> - dev->rte_xstats_names_size = 0;
>> - }
>> - if (dev->rte_xstats_ids) {
>> - free(dev->rte_xstats_ids);
>> - dev->rte_xstats_ids = NULL;
>> - dev->rte_xstats_ids_size = 0;
>> - }
>> -}
>> -
>> -static const char*
>> -netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id)
>> -{
>> - if (id >= dev->rte_xstats_names_size) {
>> - return "UNKNOWN";
>> + if (dev->xstats_count) {
>> + free(dev->xstats_ids);
>> + free(dev->xstats_names);
>> + dev->xstats_count = 0;
>> }
>> - return dev->rte_xstats_names[id].name;
>> }
>>
>> static bool
>> netdev_dpdk_configure_xstats(struct netdev_dpdk *dev)
>> OVS_REQUIRES(dev->mutex)
>> {
>> - int rte_xstats_len;
>> - bool ret;
>> - struct rte_eth_xstat *rte_xstats;
>> - uint64_t id;
>> - int xstats_no;
>> - const char *name;
>> -
>> - /* Retrieving all XSTATS names. If something will go wrong
>> - * or amount of counters will be equal 0, rte_xstats_names
>> - * buffer will be marked as NULL, and any further xstats
>> - * query won't be performed (e.g. during netdev_dpdk_get_stats
>> - * execution). */
>> -
>> - ret = false;
>> - rte_xstats = NULL;
>> -
>> - if (dev->rte_xstats_names == NULL || dev->rte_xstats_ids == NULL) {
>> - dev->rte_xstats_names_size =
>> - rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>> -
>> - if (dev->rte_xstats_names_size < 0) {
>> - VLOG_WARN("Cannot get XSTATS for port: %"PRIu8, dev->port_id);
>> - dev->rte_xstats_names_size = 0;
>> - } else {
>> - /* Reserve memory for xstats names and values */
>> - dev->rte_xstats_names = xcalloc(dev->rte_xstats_names_size,
>> - sizeof *dev->rte_xstats_names);
>> -
>> - if (dev->rte_xstats_names) {
>> - /* Retreive xstats names */
>> - rte_xstats_len =
>> - rte_eth_xstats_get_names(dev->port_id,
>> - dev->rte_xstats_names,
>> - dev->rte_xstats_names_size);
>> -
>> - if (rte_xstats_len < 0) {
>> - VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,
>> - dev->port_id);
>> - goto out;
>> - } else if (rte_xstats_len != dev->rte_xstats_names_size) {
>> - VLOG_WARN("XSTATS size doesn't match for port: %"PRIu8,
>> - dev->port_id);
>> - goto out;
>> - }
>> -
>> - dev->rte_xstats_ids = xcalloc(dev->rte_xstats_names_size,
>> - sizeof(uint64_t));
>> -
>> - /* We have to calculate number of counters */
>> - rte_xstats = xmalloc(rte_xstats_len * sizeof *rte_xstats);
>> - memset(rte_xstats, 0xff, sizeof *rte_xstats * rte_xstats_len);
>> -
>> - /* Retreive xstats values */
>> - if (rte_eth_xstats_get(dev->port_id, rte_xstats,
>> - rte_xstats_len) > 0) {
>> - dev->rte_xstats_ids_size = 0;
>> - xstats_no = 0;
>> - for (uint32_t i = 0; i < rte_xstats_len; i++) {
>> - id = rte_xstats[i].id;
>> - name = netdev_dpdk_get_xstat_name(dev, id);
>> - /* We need to filter out everything except
>> - * dropped, error and management counters */
>> - if (string_ends_with(name, "_errors") ||
>> - strstr(name, "_management_") ||
>> - string_ends_with(name, "_dropped")) {
>> -
>> - dev->rte_xstats_ids[xstats_no] = id;
>> - xstats_no++;
>> - }
>> - }
>> - dev->rte_xstats_ids_size = xstats_no;
>> - ret = true;
>> - } else {
>> - VLOG_WARN("Can't get XSTATS IDs for port: %"PRIu8,
>> - dev->port_id);
>> - }
>> + int i, ret, count;
>> + struct rte_eth_xstat *xstats;
>> + struct rte_eth_xstat_name *names;
>> + uint64_t *ids;
>>
>> - free(rte_xstats);
>> - }
>> - }
>> - } else {
>> + if (dev->xstats_count) {
>> /* Already configured */
>> - ret = true;
>> + return true;
>> }
>>
>> -out:
>> - if (!ret) {
>> - netdev_dpdk_clear_xstats(dev);
>> + /* Get number of counters. */
>> + count = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>> + if (count < 0) {
>> + goto fail;
>> }
>> - return ret;
>> +
>> + /* Get list of available ids. */
>> + xstats = xmalloc(count * sizeof *xstats);
>> + ret = rte_eth_xstats_get(dev->port_id, xstats, count);
>> + if (ret != count) {
>> + free(xstats);
>> + goto fail;
>> + }
>> +
>> + ids = xmalloc(count * sizeof *ids);
>> + for (i = 0; i < count; i++) {
>> + ids[i] = xstats[i].id;
>> + }
>> + free(xstats);
>> +
>> + /* Get names for ids. */
>> + names = xmalloc(count * sizeof *names);
>> + ret = rte_eth_xstats_get_names_by_id(dev->port_id, names, count, ids);
>> + if (ret != count) {
>> + goto fail_free;
>> + }
>> +
>> + /* Filter out uninteresting counters. */
>> + dev->xstats_count = 0;
>> + for (i = 0; i < count; i++) {
>> + if (string_ends_with(names[i].name, "_errors")
>> + || strstr(names[i].name, "_management_")
>> + || string_ends_with(names[i].name, "_dropped")) {
>> +
>> + ids[dev->xstats_count] = i;
>> + ovs_strlcpy(names[dev->xstats_count].name, names[i].name,
>> + NETDEV_CUSTOM_STATS_NAME_SIZE);
>> + dev->xstats_count++;
>> + }
>> + }
>> + if (!dev->xstats_count) {
>> + /* No counters left. */
>> + goto fail_free;
>> + }
>> +
>> + dev->xstats_ids = ids;
>> + dev->xstats_names = names;
>> +
>> + return true;
>> +
>> +fail_free:
>> + free(ids);
>> + free(names);
>> +fail:
>> + VLOG_WARN("Cannot get XSTATS for port: %"PRIu8, dev->port_id);
>> + return false;
>> }
>>
>> static int
>> @@ -2326,40 +2280,31 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
>> struct netdev_custom_stats *custom_stats)
>> {
>>
>> - uint32_t i;
>> + int i, ret;
>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>> - int rte_xstats_ret;
>>
>> ovs_mutex_lock(&dev->mutex);
>>
>> if (netdev_dpdk_configure_xstats(dev)) {
>> - uint64_t *values = xcalloc(dev->rte_xstats_ids_size,
>> - sizeof(uint64_t));
>> -
>> - rte_xstats_ret =
>> - rte_eth_xstats_get_by_id(dev->port_id, dev->rte_xstats_ids,
>> - values, dev->rte_xstats_ids_size);
>> + uint64_t *values = xcalloc(dev->xstats_count, sizeof *values);
>>
>> - if (rte_xstats_ret > 0 &&
>> - rte_xstats_ret <= dev->rte_xstats_ids_size) {
>> + ret = rte_eth_xstats_get_by_id(dev->port_id, dev->xstats_ids,
>> + values, dev->xstats_count);
>>
>> - custom_stats->size = rte_xstats_ret;
>> - custom_stats->counters =
>> - (struct netdev_custom_counter *) xcalloc(rte_xstats_ret,
>> - sizeof(struct netdev_custom_counter));
>> + if (ret == dev->xstats_count) {
>> + custom_stats->size = dev->xstats_count;
>> + custom_stats->counters = xcalloc(dev->xstats_count,
>> + sizeof *custom_stats->counters);
>>
>> - for (i = 0; i < rte_xstats_ret; i++) {
>> + for (i = 0; i < dev->xstats_count; i++) {
>> ovs_strlcpy(custom_stats->counters[i].name,
>> - netdev_dpdk_get_xstat_name(dev,
>> - dev->rte_xstats_ids[i]),
>> + dev->xstats_names[i].name,
>> NETDEV_CUSTOM_STATS_NAME_SIZE);
>> custom_stats->counters[i].value = values[i];
>> }
>> } else {
>> VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
>> dev->port_id);
>> - custom_stats->counters = NULL;
>> - custom_stats->size = 0;
>> /* Let's clear statistics cache, so it will be
>> * reconfigured */
>> netdev_dpdk_clear_xstats(dev);
>> --
>> 2.7.4
>
>
>
>
More information about the dev
mailing list