[ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.
Weglicki, MichalX
michalx.weglicki at intel.com
Tue Mar 20 09:35:17 UTC 2018
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> Sent: Tuesday, March 20, 2018 9:50 AM
> To: Weglicki, MichalX <michalx.weglicki at intel.com>; 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>
> Subject: Re: [PATCH] netdev-dpdk: Refactor custom stats.
>
> 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.
For me it is straightforward error checking, so I don't really see an advantage.
Not everything is implemented using "goto" in netdev-dpdk. I assume this is
preference thing so maybe we could ask someone to make a call, Ben/Ian?
>
> >
> > 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?
-
- dev->rte_xstats_names = NULL;
- dev->rte_xstats_names_size = 0;
-
- dev->rte_xstats_ids = NULL;
- dev->rte_xstats_ids_size = 0;
I know you are checking it in runtime against other variables, but still I believe that such initialization should remain nevertheless.
>
> > 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.
I'm not sure if what you are stating is true to be honest - "'netdev_dpdk_reconfigure' is the only function that
could make influence on the stats counters in HW" - also I'm not sure if it can be called just once .
And I'm not sure what gain do you really have in mind. Currently If counters configuration will change, all IDs
and Names will be queried again - as I said this is what I've encountered, however
It was 3 months ago so I don't recall all the details. Anyhow, I still think that we can't assume that counters will
remain the same, And we have to guarantee that cached data is up to date.
>
> We also could refactor 'netdev_dpdk_get_stats' in a same way and reuse
> already configured xstats.
> What do you think?
Yes Ilya, to be honest I even had it in my backlog, but didn't manage to do that yet. However definitely it should be done.
>
> 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