From: Bert van Leeuwen <bert.vanleeuwen@netronome.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: Alejandro Lucero <alejandro.lucero@netronome.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] ethdev: check number of queues less than RTE_ETHDEV_QUEUE_STAT_CNTRS
Date: Fri, 11 Nov 2016 11:48:15 +0200 [thread overview]
Message-ID: <4D79411C-0FB4-45DF-9CDB-06570DDB2818@netronome.com> (raw)
In-Reply-To: <1630834.YGmmObtqnA@xps13>
> On 11 Nov 2016, at 11:29 AM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
>
> 2016-11-11 09:16, Alejandro Lucero:
>> Thomas,
>>
>> We are wondering if you realize this patch fixes a bug with current ethdev
>> code as a device can have more than RTE_ETHDEV_QUEUE_STAT_CNTRS.
>>
>> Maybe the commit message is giving the wrong impression and as you
>> commented, it should just focus on the bug it fixes and to leave for
>> another email thread the discussion of how to solve the
>> RTE_ETHDEV_QUEUE_STAT_CNTRS
>> problem.
>>
>> Should we remove this from patchwork and to send another patch that way?
>
> Yes please. It was my first comment, we don't understand the exact issue
> you are fixing.
In a nutshell, the rte_eth_xstats_get function was reading memory beyond what was stored in the internal arrays (and thus returning junk values). The patch simply prevents it from going out of bounds, it does not address the issue that the rest of the counters are lost though. This issue is not specific to our device, in fact we found it while using a multiqueue virtio device (32 queues), and traced the issue into this core ethdev functionality.
> And I have a bad feeling it could break something else (really just a feeling).
> It is not the kind of patch we can apply the last day of a release.
> That's why I think it should wait 17.02.
>
> Of course you can try to convince me and others to apply it as a last minute
> patch. But why are you sending a patch on the generic API in the last days?
>
> Last argument: it is not fixing a regression of 16.11, so it is not so urgent.
Yes, it looks like this bug was present in DPDK 2.2 even, and possibly before that too.
prev parent reply other threads:[~2016-11-11 9:48 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-10 14:00 Alejandro Lucero
2016-11-10 14:42 ` Thomas Monjalon
2016-11-10 15:43 ` Alejandro Lucero
2016-11-10 16:01 ` Thomas Monjalon
2016-11-10 16:04 ` Alejandro Lucero
2016-11-11 9:16 ` Alejandro Lucero
2016-11-11 9:29 ` Thomas Monjalon
2016-11-11 9:32 ` Alejandro Lucero
2016-11-11 9:48 ` Bert van Leeuwen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D79411C-0FB4-45DF-9CDB-06570DDB2818@netronome.com \
--to=bert.vanleeuwen@netronome.com \
--cc=alejandro.lucero@netronome.com \
--cc=dev@dpdk.org \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).