[ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.

Weglicki, MichalX michalx.weglicki at intel.com
Thu Mar 22 08:05:08 UTC 2018



> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> Sent: Wednesday, March 21, 2018 3:07 PM
> 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 20.03.2018 12:35, Weglicki, MichalX wrote:
> >> -----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?
> 
> 
> Would like to hear some opinions too.
> 
> >
> >>
> >>>
> >>> 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.
> 
> If the stats could change without touching the device, we can't work with that at all,
> because we can not guarantee atomicity of 2 dpdk calls (rte_eth_stats_get and
> rte_eth_xstats_get_names). I.e. sequential calls to these functions could return
> stats for different ids and we will never be sure that returned stats and the names
> are really connected. IMHO, that is not how device or driver should work.
> Without changing configuration, stats (ids and names) should remain the same.
> (We could check this additionally with DPDK community.)
> 
> This means that we could call 'netdev_dpdk_configure_xstats()' at the end of
> 'netdev_dpdk_reconfigure()' and use cached stats ids and names until next
> reconfiguration.
> 
> In this case 'netdev_dpdk_get_custom_stats()' could be simplified by removing
> 'if (netdev_dpdk_configure_xstats(dev))' checking. In case of error while getting
> xstats by id, we could only print rate limited warning.

Don't really follow your atomicity explanation - I didn't say that dpdk calls aren't atomic 
(there is mutex to guarantee that),  I just said that counters may change, or rather that we 
should be prepared for it to be changed.  As far as I recall mutex guards getting the stats. 
In general it works exactly how your wrote, changing configuration of the device 
trigger cache refresh. The trigger is in set_config, maybe it should be in reconfigure. 
However if something will not match,  for any reason, stats will also will be reconfigured. 
I don't really understand what you  would like to simplify, it is already very simple. 

> 
> 
> About you concern about looking for names by ids, I think we could just create
> a 'namemap' for this purpose.

But it works like that already. 

> 
> >
> >>
> >> 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