* [PATCH] doc: arm64: document DPDK application profiling methods
From: Jerin Jacob @ 2016-10-04 10:40 UTC (permalink / raw)
To: dev; +Cc: thomas.monjalon, jianbo.liu, viktorin, Jerin Jacob
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
doc/guides/prog_guide/profile_app.rst | 58 +++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/doc/guides/prog_guide/profile_app.rst b/doc/guides/prog_guide/profile_app.rst
index 3226187..bb78623 100644
--- a/doc/guides/prog_guide/profile_app.rst
+++ b/doc/guides/prog_guide/profile_app.rst
@@ -31,6 +31,14 @@
Profile Your Application
========================
+Introduction
+------------
+
+The following sections describe the methods to profile DPDK applications on
+different architectures.
+
+x86
+~~~
Intel processors provide performance counters to monitor events.
Some tools provided by Intel can be used to profile and benchmark an application.
See the *VTune Performance Analyzer Essentials* publication from Intel Press for more information.
@@ -50,3 +58,53 @@ The main situations that should be monitored through event counters are:
Refer to the
`Intel Performance Analysis Guide <http://software.intel.com/sites/products/collateral/hpc/vtune/performance_analysis_guide.pdf>`_
for details about application profiling.
+
+ARM64
+~~~~~
+
+Perf
+^^^^
+ARM64 architecture provide performance counters to monitor events.
+The Linux perf tool can be used to profile and benchmark an application.
+In addition to the standard events, perf can be used to profile arm64 specific
+PMU events through raw events(-e -rXX)
+
+Refer to the
+`ARM64 specific PMU events enumeration <http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.100095_0002_04_en/way1382543438508.html>`_
+
+High-resolution cycle counter
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+The default cntvct_el0 based rte_rdtsc() provides portable means to get wall
+clock counter at user space. Typically it runs at <= 100MHz.
+
+The alternative method to enable rte_rdtsc() for high resolution
+wall clock counter is through armv8 PMU subsystem.
+The PMU cycle counter runs at CPU frequency, However, access to PMU cycle
+counter from user space is not enabled by default in the arm64 linux kernel.
+It is possible to enable cycle counter at user space access
+by configuring the PMU from the privileged mode (kernel space).
+
+by default rte_rdtsc() implementation uses portable cntvct_el0 scheme.
+Application can choose the PMU based implementation with
+CONFIG_RTE_ARM_EAL_RDTSC_USE_PMU
+
+The PMU based scheme useful for high accuracy performance profiling.
+Find below the example steps to configure the PMU based cycle counter on an
+armv8 machine.
+
+.. code-block:: console
+
+ git clone https://github.com/jerinjacobk/armv8_pmu_cycle_counter_el0
+ cd armv8_pmu_cycle_counter_el0
+ make
+ sudo insmod pmu_el0_cycle_counter.ko
+ cd $DPDK_DIR
+ make config T=arm64-armv8a-linuxapp-gcc
+ echo "CONFIG_RTE_ARM_EAL_RDTSC_USE_PMU=y" >> build/.config
+ make
+
+.. warning::
+
+ This method can not be used in production systems as this may alter PMU
+ state used by standard Linux user space tool like perf.
+
--
2.5.5
^ permalink raw reply related
* Proposal: enable redirection of DPDK logs from the user app
From: Montorsi, Francesco @ 2016-10-04 10:24 UTC (permalink / raw)
To: dev@dpdk.org
Hi all,
I've not been following closely latest DPDK activity but my company is using DPDK and we recently upgraded to 16.07.
We apply several patches to DPDK sources, to make it more similar to a "standard library" (currently it is quite intrusive: calls abort() at will, writes its own log, etc etc)... I think that it may be useful to give back to the community some of these (small) "enhancements".
One of them is about logging: as the application where we embed DPDK already has its log file, we want DPDK to log in our log facility framework.
My "fix" is simple: I just put a callback function in RTE logging system that, by default, points to the existing rte_vlog() function. If needed the library user can provide its own callback function to do what he likes.
The attached patch is what we use right now. I totally understand it needs some rework to put it in a better shape... but first of all: are you interested in such patch?
Thanks,
Francesco Montorsi
^ permalink raw reply
* Re: [PATCH v3] tools: fix issue with virtio interfaces
From: Thomas Monjalon @ 2016-10-04 9:59 UTC (permalink / raw)
To: souvikdey33; +Cc: dev
In-Reply-To: <20160826113558.11856-1-sodey@sonusnet.com>
2016-08-26 07:35, souvikdey33:
>
> This change is required to have the interface name for virtio interfaces.
> When we execute the status command the for virtio inerfaces we get
> Sample output without the change:
> 0000:00:04.0 'Virtio network device' if= drv=virtio-pci unused=virtio_pci,igb_uio
> Though for other drivers this works.
> Sample output with the change:
> 0000:00:04.0 'Virtio network device' if=eth0 drv=virtio-pci unused=virtio_pci,igb_uio
>
> souvikdey33 (1):
> Signed-off-by: Souvik Dey <sodey@sonusnet.com>
The patch from Gary - which do not use subprocess - has been preferred:
http://dpdk.org/patch/15595
^ permalink raw reply
* Re: [PATCH] tools: fix json output of pmdinfo
From: Thomas Monjalon @ 2016-10-04 9:52 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev, nhorman, stable
In-Reply-To: <1472217332-26077-1-git-send-email-olivier.matz@6wind.com>
2016-08-26 15:15, Olivier Matz:
> Using dpdk-pmdinfo with the '-r' flag does not produce a json output as
> documented. Instead, the python representation of the json object is
> shown, which is nearly the same, but cannot be properly parsed by a json
> parser.
>
> python repr (before):
> {u'pci_ids': [[5549, 1968, 65535, 65535]], u'name': u'vmxnet3'}
> json (after):
> {"pci_ids": [[5549, 1968, 65535, 65535]], "name": "vmxnet3"}
>
> Fixes: c67c9a5c646a ("tools: query binaries for HW and other support information")
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] pmdinfogen: fix clang build error
From: Thomas Monjalon @ 2016-10-04 9:48 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Neil Horman
In-Reply-To: <20160928100519.14885-1-ferruh.yigit@intel.com>
2016-09-28 11:05, Ferruh Yigit:
> Compile error:
> CC mlx5.o.pmd.o
> mlx5.o.pmd.c:1:227:
> error: no newline at end of file [-Werror,-Wnewline-eof]
> ...__attribute__((used)) = "PMD_INFO_STRING= {...}";
> ^
>
> Produced with clang 3.8.0 and MLX5_PMD and MLX5_DEBUG
> config options enabled.
>
> Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH v4 0/4] Cuckoo hash enhancements
From: Bruce Richardson @ 2016-10-04 9:47 UTC (permalink / raw)
To: De Lara Guarch, Pablo; +Cc: dev@dpdk.org
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8973CA03DCE@IRSMSX108.ger.corp.intel.com>
On Tue, Oct 04, 2016 at 08:17:28AM +0100, De Lara Guarch, Pablo wrote:
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of De Lara Guarch,
> > Pablo
> > Sent: Monday, October 03, 2016 11:51 PM
> > To: Richardson, Bruce
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v4 0/4] Cuckoo hash enhancements
> >
> > Hi Bruce,
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Monday, October 03, 2016 2:59 AM
> > > To: De Lara Guarch, Pablo
> > > Cc: dev@dpdk.org
> > > Subject: Re: [PATCH v4 0/4] Cuckoo hash enhancements
> > >
> > > On Fri, Sep 30, 2016 at 08:38:52AM +0100, Pablo de Lara wrote:
> > > > This patchset improves lookup performance on the current hash library
> > > > by changing the existing lookup bulk pipeline, with an improved pipeline,
> > > > based on a loop-and-jump model, instead of the current 4-stage 2-entry
> > > pipeline.
> > > > Also, x86 vectorized intrinsics are used to improve performance when
> > > comparing signatures.
> > > >
> > > > First patch reorganizes the order of the hash structure.
> > > > The structure takes more than one 64-byte cache line, but not all
> > > > the fields are used in the lookup operation (the most common operation).
> > > > Therefore, all these fields have been moved to the first part of the
> > structure,
> > > > so they all fit in one cache line, improving slightly the performance in
> > some
> > > > scenarios.
> > > >
> > > > Second patch modifies the order of the bucket structure.
> > > > Currently, the buckets store all the signatures together (current and
> > > alternative).
> > > > In order to be able to perform a vectorized signature comparison,
> > > > all current signatures have to be together, so the order of the bucket has
> > > been changed,
> > > > having separated all the current signatures from the alternative
> > signatures.
> > > >
> > > > Third patch introduces x86 vectorized intrinsics.
> > > > When performing a lookup bulk operation, all current signatures in a
> > bucket
> > > > are compared against the signature of the key being looked up.
> > > > Now that they all are together, a vectorized comparison can be
> > performed,
> > > > which takes less instructions to be carried out.
> > > > In case of having a machine with AVX2, number of entries per bucket are
> > > > increased from 4 to 8, as AVX2 allows comparing two 256-bit values, with
> > > 8x32-bit integers,
> > > > which are the 8 signatures on the bucket.
> > > >
> > > > Fourth (and last) patch modifies the current pipeline of the lookup bulk
> > > function.
> > > > The new pipeline is based on a loop-and-jump model. The two key
> > > improvements are:
> > > >
> > > > - Better prefetching: in this case, first 4 keys to be looked up are
> > prefetched,
> > > > and after that, the rest of the keys are prefetched at the time the
> > > calculation
> > > > of the signatures are being performed. This gives more time for the CPU
> > to
> > > > prefetch the data requesting before actually need it, which result in less
> > > > cache misses and therefore, higher throughput.
> > > >
> > > > - Lower performance penalty when using fallback: the lookup bulk
> > > algorithm
> > > > assumes that most times there will not be a collision in a bucket, but it
> > > might
> > > > happen that two or more signatures are equal, which means that more
> > > than one
> > > > key comparison might be necessary. In that case, only the key of the first
> > > hit is prefetched,
> > > > like in the current implementation. The difference now is that if this
> > > comparison
> > > > results in a miss, the information of the other keys to be compared has
> > > been stored,
> > > > unlike the current implementation, which needs to perform an entire
> > > simple lookup again.
> > > >
> > > > Changes in v4:
> > > > - Reordered hash structure, so alt signature is at the start
> > > > of the next cache line, and explain in the commit message
> > > > why it has been moved
> > > > - Reordered hash structure, so name field is on top of the structure,
> > > > leaving all the fields used in lookup in the next cache line
> > > > (instead of the first cache line)
> > > >
> > > > Changes in v3:
> > > > - Corrected the cover letter (wrong number of patches)
> > > >
> > > > Changes in v2:
> > > > - Increased entries per bucket from 4 to 8 for all cases,
> > > > so it is not architecture dependent any longer.
> > > > - Replaced compile-time signature comparison function election
> > > > with run-time election, so best optimization available
> > > > will be used from a single binary.
> > > > - Reordered the hash structure, so all the fields used by lookup
> > > > are in the same cache line (first).
> > > >
> > > > Byron Marohn (3):
> > > > hash: reorganize bucket structure
> > > > hash: add vectorized comparison
> > > > hash: modify lookup bulk pipeline
> > > >
> > >
> > > Hi,
> > >
> > > Firstly, checkpatches is reporting some style errors in these patches.
> > >
> > > Secondly, when I run the "hash_multiwriter_autotest" I get what I assume
> > to
> > > be
> > > an error after applying this patchset. Before this set is applied, running
> > > that test shows the cycles per insert with/without lock elision. Now, though
> > > I'm getting an error about a key being dropped or failing to insert in the lock
> > > elision case, e.g.
> > >
> > > Core #2 inserting 1572864: 0 - 1,572,864
> > > key 1497087 is lost
> > > 1 key lost
> > >
> > > I've run the test a number of times, and there is a single key lost each time.
> > > Please check on this, is it expected or is it a problem?
> >
> > I am seeing that error even without the patchset. I am still investigating it,
> > but using "git bisect" looks like the problem is in commit 5fc74c2e146d
> > ("hash: check if slot is empty with key index").
>
> I found the problem, and I submitted a patch for it (http://dpdk.org/dev/patchwork/patch/16361/).
> Could you check if it works for you?
>
That patch looks like a correct bugfix so I've acked it for you. However, I still
see the error appearing very occasionally. Since it also appeared before I applied
this set, I am ok to accept this set anyway. Please do a new version of the
set with checkpatch issues fixed and keep my ack.
Series Acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply
* Re: [PATCH] hash: fix incorrect free slot check
From: Thomas Monjalon @ 2016-10-04 9:41 UTC (permalink / raw)
To: Pablo de Lara; +Cc: dev, Bruce Richardson
In-Reply-To: <20161004093444.GA100068@bricha3-MOBL3>
2016-10-04 10:34, Bruce Richardson:
> On Tue, Oct 04, 2016 at 08:16:34AM +0100, Pablo de Lara wrote:
> > In function rte_hash_cuckoo_insert_mw_tm, while looking for
> > an empty slot, only the first entry in the bucket was being checked,
> > as key_idx array was not being iterated.
> >
> > Fixes: 5fc74c2e146d ("hash: check if slot is empty with key index")
> >
> > Reported-by: Bruce Richardson <bruce.richardson@intel.com>
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] ethdev: clarify API comment for imissed stats
From: Thomas Monjalon @ 2016-10-04 9:37 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev
In-Reply-To: <1473408921-13036-1-git-send-email-olivier.matz@6wind.com>
2016-09-09 10:15, Olivier Matz:
> The "imissed" stats represent RX packets dropped by the HW,
> so we should not talk about mbufs as the hardware is not aware
> of this structure. Buffer seems to be a better word.
>
> Fixes: 4eadb8ba11b7 ("ethdev: do not deprecate imissed counter")
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] ethdev: fix statistics description
From: Thomas Monjalon @ 2016-10-04 9:34 UTC (permalink / raw)
To: Wei Dai, john.mcnamara; +Cc: dev
In-Reply-To: <1472206104-22035-1-git-send-email-wei.dai@intel.com>
2016-08-26 18:08, Wei Dai:
> /**
> * A structure used to retrieve statistics for an Ethernet port.
> + * Not all statistics fields in struct rte_eth_stats are supported
> + * by any type of network interface card (NIC). If any statistics
> + * field is not supported, its value is 0 .
> */
> struct rte_eth_stats {
I'm missing the point of this patch.
Why do you think it is a fix?
John, any opinion?
^ permalink raw reply
* Re: [PATCH] hash: fix incorrect free slot check
From: Bruce Richardson @ 2016-10-04 9:34 UTC (permalink / raw)
To: Pablo de Lara; +Cc: dev
In-Reply-To: <1475565394-184188-1-git-send-email-pablo.de.lara.guarch@intel.com>
On Tue, Oct 04, 2016 at 08:16:34AM +0100, Pablo de Lara wrote:
> In function rte_hash_cuckoo_insert_mw_tm, while looking for
> an empty slot, only the first entry in the bucket was being checked,
> as key_idx array was not being iterated.
>
> Fixes: 5fc74c2e146d ("hash: check if slot is empty with key index")
>
> Reported-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply
* Re: [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
From: Thomas Monjalon @ 2016-10-04 9:31 UTC (permalink / raw)
To: jean.tourrilhes; +Cc: Sergio Gonzalez Monroy, dev
In-Reply-To: <417ffa55-a8bf-a769-d751-b023d72993d8@intel.com>
2016-10-04 09:03, Sergio Gonzalez Monroy:
> On 03/10/2016 21:46, Thomas Monjalon wrote:
> > 2016-10-03 08:55, Jean Tourrilhes:
> >> On Mon, Oct 03, 2016 at 02:25:40PM +0100, Sergio Gonzalez Monroy wrote:
> >>> Hi Jean,
> >>>
> >>> There are some format issues with the patch:
> >>>
> >>> You can run scripts/check-git-log.sh to check them:
> >>> Wrong headline format:
> >>> eal: Fix misleading error messages, errno can't be trusted.
> >>> Wrong headline uppercase:
> >>> eal: Fix misleading error messages, errno can't be trusted.
> >>> Missing 'Fixes' tag:
> >>> eal: Fix misleading error messages, errno can't be trusted.
> >>>
> >>> The script's output highlights the different issues.
> >> SOrry about that, I casually read the page on
> >> http://dpdk.org/dev, but obviously I need to look at it again.
> > No problem. This guide is more oriented towards regular contributors.
> > You come with a bug and its fix, we can make some effort to format
> > the patch :)
> >
> > The title could be "mem: fix hugepage mapping error messages"
[...]
> > Sergio, do you have more comments?
>
> Nop.
>
> > Should we wait another version or is it OK?
> > Maybe you'd prefer to rework it yourself?
>
> AFAICS it really is just the commit message formatting, so maybe you can
> fix that when applying it?
Yes but I've just noticed that the Signed-off-by line is missing.
Please Jean, could you add this required line with your explanations,
rebase on fresh head and resend please?
^ permalink raw reply
* Re: [PATCH v2 1/2] mempool: fix comments for mempool create functions
From: Thomas Monjalon @ 2016-10-04 9:18 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Olivier Matz
In-Reply-To: <b10104ea-8006-2556-5276-f733adf01220@6wind.com>
2016-10-03 17:06, Olivier Matz:
> On 09/28/2016 03:59 PM, Ferruh Yigit wrote:
> > Fixes: 85226f9c526b ("mempool: introduce a function to create an empty pool")
> > Fixes: d1d914ebbc25 ("mempool: allocate in several memory chunks by default")
> >
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> Series:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] eal/armv8: high-resolution cycle counter
From: Thomas Monjalon @ 2016-10-04 8:46 UTC (permalink / raw)
To: Jerin Jacob
Cc: Hemant Agrawal, dev, jianbo.liu@linaro.org,
viktorin@rehivetech.com
In-Reply-To: <DB5PR04MB16052F5FCA2D9C0F0CE7C08989EB0@DB5PR04MB1605.eurprd04.prod.outlook.com>
> > Existing cntvct_el0 based rte_rdtsc() provides portable means to get wall clock
> > counter at user space. Typically it runs at <= 100MHz.
> >
> > The alternative method to enable rte_rdtsc() for high resolution wall clock
> > counter is through armv8 PMU subsystem.
> > The PMU cycle counter runs at CPU frequency, However, access to PMU cycle
> > counter from user space is not enabled by default in the arm64 linux kernel.
> > It is possible to enable cycle counter at user space access by configuring the
> > PMU from the privileged mode (kernel space).
> >
> > by default rte_rdtsc() implementation uses portable
> > cntvct_el0 scheme. Application can choose the PMU based implementation with
> > CONFIG_RTE_ARM_EAL_RDTSC_USE_PMU
> >
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Applied, thanks
Please do not forget documentation and upstreaming efforts.
^ permalink raw reply
* Re: [PATCH] eal/armv8: high-resolution cycle counter
From: Thomas Monjalon @ 2016-10-04 8:42 UTC (permalink / raw)
To: Jerin Jacob, Jan Viktorin
Cc: Nipun Gupta, dev, jianbo.liu@linaro.org, Hemant Agrawal
In-Reply-To: <20160819125217.GA7169@localhost.localdomain>
2016-08-19 18:22, Jerin Jacob:
> On Fri, Aug 19, 2016 at 02:24:58PM +0200, Jan Viktorin wrote:
> > On Fri, 19 Aug 2016 17:16:12 +0530
> > Jerin Jacob <jerin.jacob@caviumnetworks.com> wrote:
> >
> >
> > I've got a private kernel driver enabling and disabling (hopefully) properly
> > this for ARMv7. If we'd like to merge it, I'd like to have a single module
> > or at least single module with 2 implementations...
> >
> > I can post it if it would be helpful.
>
> I don't think we can use this in production as this may alter PMU state used
> by 'perf' etc.I think let it be a debug interface for armv7 and armv8
> and disable it by default.
Please could you document the use of PMU for debug and how it alters
usage of kernel counters?
A patch in doc/guides/prog_guide/profile_app.rst would be welcome.
Ideally, it would be a lot better to have a sysfs entry to enable PMU
counter with an upstream kernel.
^ permalink raw reply
* Re: [PATCH] log: do not drop debug logs at compile time
From: David Marchand @ 2016-10-04 8:28 UTC (permalink / raw)
To: Wiles, Keith; +Cc: Olivier Matz, Thomas Monjalon, dev@dpdk.org
In-Reply-To: <036D8827-DFD7-421D-8133-3011D42CEAB4@intel.com>
On Mon, Oct 3, 2016 at 6:27 PM, Wiles, Keith <keith.wiles@intel.com> wrote:
>> On Oct 3, 2016, at 10:37 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
>> What makes you feel it's easier to add a log level instead of adding a
>> new RTE_LOG_DP() function?
>
> It seems to me the log levels are for displaying logs at different levels adding a new macro to not log is just a hack because we do not have a log level for data path. This is why I would like to see a log level added and not a new macro.
>
> It also appears the new RTE_LOG() will always be in the code as you moved the test to the RTE_LOG_DP() macro. This would mean all RTE_LOG() in the code will always call rte_log(), correct?
>
> If using a new DEBUG_DP (maybe DATAPATH is a better log level name) level we can use the same macro as before and modify the level only. This way we can remove via the compiler any log that is below the default RTE_LOG_LEVEL. I see keeping the rte_log() could be a performance problem or code blot when you really want to remove them all.
>
> The DATAPATH log level would be above (smaller number) then DEBUG in the enum list. To remove all debug logs just set the RTE_LOG_LEVEL to RTE_LOG_DATAPATH.
If I try to draw a parrallel to syslog (well, the log subsystem in eal
has always been bound to syslog ...), what you propose here is like
adding a new level in syslog while you have syslog facilities.
With the current log api, we have types and levels, can't we filter at
build time depending on the log "type" ?
Here we would strip PMD type logs > INFO.
--
David Marchand
^ permalink raw reply
* Re: [PATCH] log: do not drop debug logs at compile time
From: David Marchand @ 2016-10-04 8:26 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev@dpdk.org
In-Reply-To: <1474011832-29987-1-git-send-email-olivier.matz@6wind.com>
On Fri, Sep 16, 2016 at 9:43 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> Today, all logs whose level is lower than INFO are dropped at
> compile-time. This prevents from enabling debug logs at runtime using
> --log-level=8.
>
> The rationale was to remove debug logs from the data path at
> compile-time, avoiding a test at run-time.
>
> This patch changes the behavior of RTE_LOG() to avoid the compile-time
> optimization, and introduces the RTE_LOG_DP() macro that has the same
> behavior than the previous RTE_LOG(), for the rare cases where debug
> logs are in the data path.
>
> So it is now possible to enable debug logs at run-time by just
> specifying --log-level=8. Some drivers still have special compile-time
> options to enable more debug log. Maintainers may consider to
> remove/reduce them.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> config/common_base | 1 +
> doc/guides/faq/faq.rst | 2 +-
> drivers/net/bnxt/bnxt_txr.c | 2 +-
> drivers/net/nfp/nfp_net.c | 8 +++---
> examples/distributor/main.c | 4 +--
> examples/ipsec-secgw/esp.c | 2 +-
> examples/ipsec-secgw/ipsec.c | 4 +--
> examples/packet_ordering/main.c | 6 ++--
> examples/quota_watermark/qw/main.c | 2 +-
> examples/tep_termination/main.c | 4 +--
> examples/vhost/main.c | 14 +++++-----
> examples/vhost_xen/main.c | 20 +++++++-------
> lib/librte_eal/common/include/rte_log.h | 49 +++++++++++++++++++++------------
> 13 files changed, 67 insertions(+), 51 deletions(-)
>
> diff --git a/config/common_base b/config/common_base
> index 7830535..04b71e9 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -89,6 +89,7 @@ CONFIG_RTE_MAX_MEMSEG=256
> CONFIG_RTE_MAX_MEMZONE=2560
> CONFIG_RTE_MAX_TAILQ=32
> CONFIG_RTE_LOG_LEVEL=RTE_LOG_INFO
> +CONFIG_RTE_LOG_DP_LEVEL=RTE_LOG_INFO
> CONFIG_RTE_LOG_HISTORY=256
> CONFIG_RTE_LIBEAL_USE_HPET=n
> CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
[snip]
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index 919563c..76b198f 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
[snip]
> @@ -266,6 +257,30 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
> * - Negative on error.
> */
> #define RTE_LOG(l, t, ...) \
> + rte_log(RTE_LOG_ ## l, \
> + RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
> +
> +/**
> + * Generates a log message for data path.
> + *
> + * Similar to RTE_LOG(), except that it is removed at compilation time
> + * if the RTE_LOG_DP_LEVEL configuration option is lower than the log
> + * level argument.
> + *
> + * @param l
> + * Log level. A value between EMERG (1) and DEBUG (8). The short name is
> + * expanded by the macro, so it cannot be an integer value.
> + * @param t
> + * The log type, for example, EAL. The short name is expanded by the
> + * macro, so it cannot be an integer value.
> + * @param ...
> + * The fmt string, as in printf(3), followed by the variable arguments
> + * required by the format.
> + * @return
> + * - 0: Success.
> + * - Negative on error.
> + */
> +#define RTE_LOG_DP(l, t, ...) \
> (void)((RTE_LOG_ ## l <= RTE_LOG_LEVEL) ? \
> rte_log(RTE_LOG_ ## l, \
> RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) : \
> --
> 2.8.1
Hum, I suppose RTE_LOG_DP should look at RTE_LOG_DP_LEVEL.
--
David Marchand
^ permalink raw reply
* Re: [PATCH 0/5] i40e: vector poll-mode driver on ARM64
From: Thomas Monjalon @ 2016-10-04 8:24 UTC (permalink / raw)
To: dev; +Cc: Bruce Richardson, Jianbo Liu, helin.zhang, jingjing.wu,
jerin.jacob
In-Reply-To: <20160919162506.GL45548@bricha3-MOBL3>
2016-09-19 17:25, Bruce Richardson:
> On Wed, Aug 24, 2016 at 03:23:40PM +0530, Jianbo Liu wrote:
> > This patch set is to implement i40e vector PMD on ARM64.
> > For x86, vPMD is only reorganized, there should be no performance loss.
> >
> > Jianbo Liu (5):
> > i40e: extract non-x86 specific code from vector driver
> > i40e: implement vector PMD for ARM architecture
> > i40e: enable i40e vector PMD on ARMv8a platform
> > i40e: make vector driver filenames consistent
> > maintainers: claim i40e vector PMD on ARM
> >
> Ping for comment/ack - especially from i40e maintainers.
Why is there no review of these patches for acceleration on ARM?
^ permalink raw reply
* Re: [PATCH] ethdev: support PCI domains
From: Thomas Monjalon @ 2016-10-04 8:15 UTC (permalink / raw)
To: Sinan Kaya; +Cc: dev, Stephen Hemminger
In-Reply-To: <e3fc647d-32fe-6fb8-b957-dfc8b55c4c95@codeaurora.org>
2016-07-22 18:56, Sinan Kaya:
> On 7/22/2016 5:12 PM, Stephen Hemminger wrote:
> > On Fri, 22 Jul 2016 11:34:10 -0400
> > Sinan Kaya <okaya@codeaurora.org> wrote:
> >
> >> The current code is enumerating devices based on bus, device and function
> >> pairs. This does not work well for architectures with multiple PCI
> >> segments/domains. Multiple PCI devices will have the same BDF value but
> >> different segment numbers (01:01:01.0 and 02:01:01.0) for instance.
> >>
> >> Adding segment numbers to device naming so that we can uniquely identify
> >> devices.
> >>
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >
> > I ran into this yes. There is a small risk of breaking some application that
> > assumed something about names though.
> >
> > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> >
>
> Thanks, hopefully the change is minor and can be contained until next release.
It is part of the EAL rework.
The function has been moved in EAL and includes the PCI domain:
http://dpdk.org/commit/affe1cdc
^ permalink raw reply
* Re: [PATCH] pci:fix missing free
From: Thomas Monjalon @ 2016-10-04 8:07 UTC (permalink / raw)
To: Yangchao Zhou; +Cc: David Marchand, dev, stable
In-Reply-To: <CALwxeUsJpXdgW4Bo13WdkvfZyRqWu-5WwgcAnjyDa+oKyboOqA@mail.gmail.com>
2016-09-30 17:27, David Marchand:
> On Fri, Sep 30, 2016 at 5:19 PM, David Marchand
> <david.marchand@6wind.com> wrote:
> > Hello,
> >
> > On Thu, Sep 29, 2016 at 3:41 AM, Yangchao Zhou <zhouyates@gmail.com> wrote:
> >> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
> >
> > For the title, how about:
> > pci: fix memory leak when detaching devices
> >
> > Afaics the fixes tag would be :
> >
> > Fixes: dbe6b4b61b0e ("pci: probe or close device")
> >
> > + CC stable
>
> Thomas, can you apply this with my ack ?
> Thanks.
Applied, thanks
^ permalink raw reply
* Re: [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
From: Sergio Gonzalez Monroy @ 2016-10-04 8:03 UTC (permalink / raw)
To: Thomas Monjalon, jean.tourrilhes; +Cc: dev
In-Reply-To: <6928094.Vgjcapsp68@xps13>
On 03/10/2016 21:46, Thomas Monjalon wrote:
> 2016-10-03 08:55, Jean Tourrilhes:
>> On Mon, Oct 03, 2016 at 02:25:40PM +0100, Sergio Gonzalez Monroy wrote:
>>> Hi Jean,
>>>
>>> There are some format issues with the patch:
>>>
>>> You can run scripts/check-git-log.sh to check them:
>>> Wrong headline format:
>>> eal: Fix misleading error messages, errno can't be trusted.
>>> Wrong headline uppercase:
>>> eal: Fix misleading error messages, errno can't be trusted.
>>> Missing 'Fixes' tag:
>>> eal: Fix misleading error messages, errno can't be trusted.
>>>
>>> The script's output highlights the different issues.
>> SOrry about that, I casually read the page on
>> http://dpdk.org/dev, but obviously I need to look at it again.
> No problem. This guide is more oriented towards regular contributors.
> You come with a bug and its fix, we can make some effort to format
> the patch :)
>
> The title could be "mem: fix hugepage mapping error messages"
>
>>> On 21/09/2016 22:10, Jean Tourrilhes wrote:
>>>> @@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
>>>> mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
>>>> sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
>>>> mem_cfg_fd, 0);
>>>> + if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
>>>> + if (mem_config != MAP_FAILED)
>>>> + /* errno is stale, don't use */
>>>> + rte_panic("Cannot mmap memory for rte_config at [%p], got [%p] - please use '--base-virtaddr' option\n",
>>>> + rte_mem_cfg_addr, mem_config);
>>>> + else
>>>> + rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
>>>> + errno, strerror(errno));
>>>> + }
>>>> close(mem_cfg_fd);
>>>> - if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
>>>> - rte_panic("Cannot mmap memory for rte_config\n");
>>> NIT but any reason you moved the check before closing the file descriptor?
>>> (not that it matters with current code as we panic anyway)
>> "close()" may change "errno" according to its man page.
> Sergio, do you have more comments?
Nop.
> Should we wait another version or is it OK?
> Maybe you'd prefer to rework it yourself?
AFAICS it really is just the commit message formatting, so maybe you can
fix that when applying it?
Sergio
^ permalink raw reply
* Re: [PATCH v11 00/24] Introducing rte_driver/rte_device generalization
From: Thomas Monjalon @ 2016-10-04 7:42 UTC (permalink / raw)
To: Shreyansh Jain; +Cc: dev, viktorin, David Marchand, hemant.agrawal
In-Reply-To: <629b0e70-5bd1-d376-4fb9-e7c7a102d79f@nxp.com>
2016-10-04 12:21, Shreyansh Jain:
> Hi Thomas,
>
> On Monday 03 October 2016 07:58 PM, Thomas Monjalon wrote:
> > Applied, thanks everybody for the great (re)work!
>
> Thanks!
>
> >
> > 2016-09-20 18:11, Shreyansh Jain:
> >> Future Work/Pending:
> >> ===================
> >> - Presently eth_driver, rte_eth_dev are not aligned to the rte_driver/
> >> rte_device model. eth_driver still is a PCI specific entity. This
> >> has been highlighted by comments from Ferruh in [9].
> >> - Some variables, like drv_name (as highlighted by Ferruh), are getting
> >> duplicated across rte_xxx_driver/device and rte_driver/device.
>
> Both the above are already part of my todo list.
>
> > What about those pending work?
> >
> > I would add more remaining issues:
> > - probe/remove naming could be applied to vdev for consistency
>
> Is that for uniformity reasons? I still feel 'probe/remove' are not
> appropriate for a virtual device. init/deinit are more appropriate. As
> for PCI, probe/remove are standard parlance and hence suit it better
> than init/deinit.
PCI probe is "scan + checks + init".
The vdev requires "args parsing + checks + init".
The device will not be initialized if checks fail,
e.g. missing support or name conflict.
I think it could fit in "probe" rather than "init".
The remove word looks appropriate in both cases.
> Nevertheless, uniform naming convention can have its benefits - ease of
> code understanding being one.
Yes that's the other pro for "probe/remove".
> Change is simple once we come to a conclusion.
>
> > - rte_eal_device_insert must be called in vdev
>
> Ok.
>
> > - REGISTER macros should be prefixed with RTE_
>
> That would include:
> DRIVER_REGISTER_VDEV
> DRIVER_REGISTER_PCI_TABLE
> DRIVER_REGISTER_PCI
>
> I will publish a patch soon. This would be fairly straightforward change.
>
> > - Some functions in EAL does not need eal_ in their prefix:
> > rte_eal_pci_ -> rte_pci_
> > rte_eal_dev_ -> rte_dev_
> > rte_eal_vdev_ -> rte_vdev_
> > rte_eal_driver -> rte_drv_
> > rte_eal_vdrv -> rte_vdrv_
> >
> >
>
> It can be merged with changes for:
> - drv_name
> - EAL_ before _REGISTER_ macros
> - eth_driver => rte_driver naming
Good.
Could you make it this week, please?
^ permalink raw reply
* Re: [PATCH v4 0/4] Cuckoo hash enhancements
From: De Lara Guarch, Pablo @ 2016-10-04 7:17 UTC (permalink / raw)
To: De Lara Guarch, Pablo, Richardson, Bruce; +Cc: dev@dpdk.org
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8973CA03D86@IRSMSX108.ger.corp.intel.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of De Lara Guarch,
> Pablo
> Sent: Monday, October 03, 2016 11:51 PM
> To: Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 0/4] Cuckoo hash enhancements
>
> Hi Bruce,
>
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Monday, October 03, 2016 2:59 AM
> > To: De Lara Guarch, Pablo
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH v4 0/4] Cuckoo hash enhancements
> >
> > On Fri, Sep 30, 2016 at 08:38:52AM +0100, Pablo de Lara wrote:
> > > This patchset improves lookup performance on the current hash library
> > > by changing the existing lookup bulk pipeline, with an improved pipeline,
> > > based on a loop-and-jump model, instead of the current 4-stage 2-entry
> > pipeline.
> > > Also, x86 vectorized intrinsics are used to improve performance when
> > comparing signatures.
> > >
> > > First patch reorganizes the order of the hash structure.
> > > The structure takes more than one 64-byte cache line, but not all
> > > the fields are used in the lookup operation (the most common operation).
> > > Therefore, all these fields have been moved to the first part of the
> structure,
> > > so they all fit in one cache line, improving slightly the performance in
> some
> > > scenarios.
> > >
> > > Second patch modifies the order of the bucket structure.
> > > Currently, the buckets store all the signatures together (current and
> > alternative).
> > > In order to be able to perform a vectorized signature comparison,
> > > all current signatures have to be together, so the order of the bucket has
> > been changed,
> > > having separated all the current signatures from the alternative
> signatures.
> > >
> > > Third patch introduces x86 vectorized intrinsics.
> > > When performing a lookup bulk operation, all current signatures in a
> bucket
> > > are compared against the signature of the key being looked up.
> > > Now that they all are together, a vectorized comparison can be
> performed,
> > > which takes less instructions to be carried out.
> > > In case of having a machine with AVX2, number of entries per bucket are
> > > increased from 4 to 8, as AVX2 allows comparing two 256-bit values, with
> > 8x32-bit integers,
> > > which are the 8 signatures on the bucket.
> > >
> > > Fourth (and last) patch modifies the current pipeline of the lookup bulk
> > function.
> > > The new pipeline is based on a loop-and-jump model. The two key
> > improvements are:
> > >
> > > - Better prefetching: in this case, first 4 keys to be looked up are
> prefetched,
> > > and after that, the rest of the keys are prefetched at the time the
> > calculation
> > > of the signatures are being performed. This gives more time for the CPU
> to
> > > prefetch the data requesting before actually need it, which result in less
> > > cache misses and therefore, higher throughput.
> > >
> > > - Lower performance penalty when using fallback: the lookup bulk
> > algorithm
> > > assumes that most times there will not be a collision in a bucket, but it
> > might
> > > happen that two or more signatures are equal, which means that more
> > than one
> > > key comparison might be necessary. In that case, only the key of the first
> > hit is prefetched,
> > > like in the current implementation. The difference now is that if this
> > comparison
> > > results in a miss, the information of the other keys to be compared has
> > been stored,
> > > unlike the current implementation, which needs to perform an entire
> > simple lookup again.
> > >
> > > Changes in v4:
> > > - Reordered hash structure, so alt signature is at the start
> > > of the next cache line, and explain in the commit message
> > > why it has been moved
> > > - Reordered hash structure, so name field is on top of the structure,
> > > leaving all the fields used in lookup in the next cache line
> > > (instead of the first cache line)
> > >
> > > Changes in v3:
> > > - Corrected the cover letter (wrong number of patches)
> > >
> > > Changes in v2:
> > > - Increased entries per bucket from 4 to 8 for all cases,
> > > so it is not architecture dependent any longer.
> > > - Replaced compile-time signature comparison function election
> > > with run-time election, so best optimization available
> > > will be used from a single binary.
> > > - Reordered the hash structure, so all the fields used by lookup
> > > are in the same cache line (first).
> > >
> > > Byron Marohn (3):
> > > hash: reorganize bucket structure
> > > hash: add vectorized comparison
> > > hash: modify lookup bulk pipeline
> > >
> >
> > Hi,
> >
> > Firstly, checkpatches is reporting some style errors in these patches.
> >
> > Secondly, when I run the "hash_multiwriter_autotest" I get what I assume
> to
> > be
> > an error after applying this patchset. Before this set is applied, running
> > that test shows the cycles per insert with/without lock elision. Now, though
> > I'm getting an error about a key being dropped or failing to insert in the lock
> > elision case, e.g.
> >
> > Core #2 inserting 1572864: 0 - 1,572,864
> > key 1497087 is lost
> > 1 key lost
> >
> > I've run the test a number of times, and there is a single key lost each time.
> > Please check on this, is it expected or is it a problem?
>
> I am seeing that error even without the patchset. I am still investigating it,
> but using "git bisect" looks like the problem is in commit 5fc74c2e146d
> ("hash: check if slot is empty with key index").
I found the problem, and I submitted a patch for it (http://dpdk.org/dev/patchwork/patch/16361/).
Could you check if it works for you?
>
> Thanks,
> Pablo
>
> >
> > Thanks,
> > /Bruce
^ permalink raw reply
* [PATCH] hash: fix incorrect free slot check
From: Pablo de Lara @ 2016-10-04 7:16 UTC (permalink / raw)
To: dev; +Cc: bruce.richardson, Pablo de Lara
In function rte_hash_cuckoo_insert_mw_tm, while looking for
an empty slot, only the first entry in the bucket was being checked,
as key_idx array was not being iterated.
Fixes: 5fc74c2e146d ("hash: check if slot is empty with key index")
Reported-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
lib/librte_hash/rte_cuckoo_hash_x86.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_hash/rte_cuckoo_hash_x86.h b/lib/librte_hash/rte_cuckoo_hash_x86.h
index e16d69c..7ffa56f 100644
--- a/lib/librte_hash/rte_cuckoo_hash_x86.h
+++ b/lib/librte_hash/rte_cuckoo_hash_x86.h
@@ -53,7 +53,7 @@ rte_hash_cuckoo_insert_mw_tm(struct rte_hash_bucket *prim_bkt,
*/
for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
/* Check if slot is available */
- if (likely(prim_bkt->key_idx == EMPTY_SLOT)) {
+ if (likely(prim_bkt->key_idx[i] == EMPTY_SLOT)) {
prim_bkt->signatures[i].current = sig;
prim_bkt->signatures[i].alt = alt_hash;
prim_bkt->key_idx[i] = new_idx;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v11 00/24] Introducing rte_driver/rte_device generalization
From: Shreyansh Jain @ 2016-10-04 6:51 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, viktorin, David Marchand, hemant.agrawal
In-Reply-To: <3582057.BR181DtlIn@xps13>
Hi Thomas,
On Monday 03 October 2016 07:58 PM, Thomas Monjalon wrote:
> Applied, thanks everybody for the great (re)work!
Thanks!
>
> 2016-09-20 18:11, Shreyansh Jain:
>> Future Work/Pending:
>> ===================
>> - Presently eth_driver, rte_eth_dev are not aligned to the rte_driver/
>> rte_device model. eth_driver still is a PCI specific entity. This
>> has been highlighted by comments from Ferruh in [9].
>> - Some variables, like drv_name (as highlighted by Ferruh), are getting
>> duplicated across rte_xxx_driver/device and rte_driver/device.
Both the above are already part of my todo list.
>
> What about those pending work?
>
> I would add more remaining issues:
> - probe/remove naming could be applied to vdev for consistency
Is that for uniformity reasons? I still feel 'probe/remove' are not
appropriate for a virtual device. init/deinit are more appropriate. As
for PCI, probe/remove are standard parlance and hence suit it better
than init/deinit.
Nevertheless, uniform naming convention can have its benefits - ease of
code understanding being one.
Change is simple once we come to a conclusion.
> - rte_eal_device_insert must be called in vdev
Ok.
> - REGISTER macros should be prefixed with RTE_
That would include:
DRIVER_REGISTER_VDEV
DRIVER_REGISTER_PCI_TABLE
DRIVER_REGISTER_PCI
I will publish a patch soon. This would be fairly straightforward change.
> - Some functions in EAL does not need eal_ in their prefix:
> rte_eal_pci_ -> rte_pci_
> rte_eal_dev_ -> rte_dev_
> rte_eal_vdev_ -> rte_vdev_
> rte_eal_driver -> rte_drv_
> rte_eal_vdrv -> rte_vdrv_
>
>
It can be merged with changes for:
- drv_name
- EAL_ before _REGISTER_ macros
- eth_driver => rte_driver naming
--
-
Shreyansh
^ permalink raw reply
* Re: [PATCH v4 0/4] Cuckoo hash enhancements
From: De Lara Guarch, Pablo @ 2016-10-04 6:50 UTC (permalink / raw)
To: Richardson, Bruce; +Cc: dev@dpdk.org
In-Reply-To: <20161003095906.GA83136@bricha3-MOBL3>
Hi Bruce,
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, October 03, 2016 2:59 AM
> To: De Lara Guarch, Pablo
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v4 0/4] Cuckoo hash enhancements
>
> On Fri, Sep 30, 2016 at 08:38:52AM +0100, Pablo de Lara wrote:
> > This patchset improves lookup performance on the current hash library
> > by changing the existing lookup bulk pipeline, with an improved pipeline,
> > based on a loop-and-jump model, instead of the current 4-stage 2-entry
> pipeline.
> > Also, x86 vectorized intrinsics are used to improve performance when
> comparing signatures.
> >
> > First patch reorganizes the order of the hash structure.
> > The structure takes more than one 64-byte cache line, but not all
> > the fields are used in the lookup operation (the most common operation).
> > Therefore, all these fields have been moved to the first part of the structure,
> > so they all fit in one cache line, improving slightly the performance in some
> > scenarios.
> >
> > Second patch modifies the order of the bucket structure.
> > Currently, the buckets store all the signatures together (current and
> alternative).
> > In order to be able to perform a vectorized signature comparison,
> > all current signatures have to be together, so the order of the bucket has
> been changed,
> > having separated all the current signatures from the alternative signatures.
> >
> > Third patch introduces x86 vectorized intrinsics.
> > When performing a lookup bulk operation, all current signatures in a bucket
> > are compared against the signature of the key being looked up.
> > Now that they all are together, a vectorized comparison can be performed,
> > which takes less instructions to be carried out.
> > In case of having a machine with AVX2, number of entries per bucket are
> > increased from 4 to 8, as AVX2 allows comparing two 256-bit values, with
> 8x32-bit integers,
> > which are the 8 signatures on the bucket.
> >
> > Fourth (and last) patch modifies the current pipeline of the lookup bulk
> function.
> > The new pipeline is based on a loop-and-jump model. The two key
> improvements are:
> >
> > - Better prefetching: in this case, first 4 keys to be looked up are prefetched,
> > and after that, the rest of the keys are prefetched at the time the
> calculation
> > of the signatures are being performed. This gives more time for the CPU to
> > prefetch the data requesting before actually need it, which result in less
> > cache misses and therefore, higher throughput.
> >
> > - Lower performance penalty when using fallback: the lookup bulk
> algorithm
> > assumes that most times there will not be a collision in a bucket, but it
> might
> > happen that two or more signatures are equal, which means that more
> than one
> > key comparison might be necessary. In that case, only the key of the first
> hit is prefetched,
> > like in the current implementation. The difference now is that if this
> comparison
> > results in a miss, the information of the other keys to be compared has
> been stored,
> > unlike the current implementation, which needs to perform an entire
> simple lookup again.
> >
> > Changes in v4:
> > - Reordered hash structure, so alt signature is at the start
> > of the next cache line, and explain in the commit message
> > why it has been moved
> > - Reordered hash structure, so name field is on top of the structure,
> > leaving all the fields used in lookup in the next cache line
> > (instead of the first cache line)
> >
> > Changes in v3:
> > - Corrected the cover letter (wrong number of patches)
> >
> > Changes in v2:
> > - Increased entries per bucket from 4 to 8 for all cases,
> > so it is not architecture dependent any longer.
> > - Replaced compile-time signature comparison function election
> > with run-time election, so best optimization available
> > will be used from a single binary.
> > - Reordered the hash structure, so all the fields used by lookup
> > are in the same cache line (first).
> >
> > Byron Marohn (3):
> > hash: reorganize bucket structure
> > hash: add vectorized comparison
> > hash: modify lookup bulk pipeline
> >
>
> Hi,
>
> Firstly, checkpatches is reporting some style errors in these patches.
>
> Secondly, when I run the "hash_multiwriter_autotest" I get what I assume to
> be
> an error after applying this patchset. Before this set is applied, running
> that test shows the cycles per insert with/without lock elision. Now, though
> I'm getting an error about a key being dropped or failing to insert in the lock
> elision case, e.g.
>
> Core #2 inserting 1572864: 0 - 1,572,864
> key 1497087 is lost
> 1 key lost
>
> I've run the test a number of times, and there is a single key lost each time.
> Please check on this, is it expected or is it a problem?
I am seeing that error even without the patchset. I am still investigating it,
but using "git bisect" looks like the problem is in commit 5fc74c2e146d
("hash: check if slot is empty with key index").
Thanks,
Pablo
>
> Thanks,
> /Bruce
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox