[ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.
Ilya Maximets
i.maximets at samsung.com
Tue Apr 3 15:25:13 UTC 2018
On 22.03.2018 11:05, Weglicki, MichalX wrote:
>
>
>> -----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.
IMHO, this should not happen. Only reconfiguration could change the set of
available counters, so, we should not care about some magical cases where
everything messed up without config changes inside OVS. We could just print
the rate-limited warning and wait for reconfiguration to reconfigure stats.
Anyway, I'll check the behaviour and maybe will keep the current stats'
reconfiguration on failure.
Let me prepare v2. At least, we'll have subject to discuss after that.
Key point was to simplify the code itself, without significant functional changes.
> 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