Performance Analyses for DPDK Applications Using the Ethdev Library #83

adel-belkhiri · 2024-09-26T17:15:09Z

This PR introduces 5 performance analyses for DPDK applications implemented using the ethdev library. These analyses provide insights into packet distribution, rate, throughput, and PMD thread busyness, thus helping monitor and optimize application performance.

New Features:

Packet Rate Analysis:
Computes the packet rate (in pps) for transmitted and received Ethernet traffic on a per-queue basis. It helps monitor the efficiency of traffic processing by calculating the number of packets received or transmitted in a given period.
Packet Throughput Analysis:
Analyzes packet throughput (in bps) for transmitted and received Ethernet traffic. This is done on a per-queue basis to evaluate bandwidth usage.
Busyness Analysis for PMD Threads:
Estimates how busy PMD threads are by comparing the times they successfully retrieve packets from the NIC queue versus when they are merely spinning without processing any packets. This can be used to evaluate the efficiency of polling operations.
Packet Distribution Analysis:
Analyzes the distribution of packets retrieved in a single rte_eth_rx_burst() call, on a per-thread and per-queue basis. This analysis provides insights into packet retrieval patterns, including how evenly or unevenly the workload is distributed across queues.
Packet Distribution Statistics:
Computes various statistics related to packet retrieval distribution, including the minimum, maximum, average, and standard deviation for packets retrieved in a single rte_eth_rx_burst() call, which offers more granular visibility into traffic distribution patterns.

MatthewKhouzam

minor changes.

MatthewKhouzam · 2024-09-26T19:13:57Z

...c/org/eclipse/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/Attributes.java

+ * @author Adel Belkhiri
+ */
+@SuppressWarnings({ "nls" })
+public interface Attributes {


I recommend renaming to EthAttributes or something more descriptive

Thanks for the comments Matthew. I renamed it as suggested in the new version.

MatthewKhouzam · 2024-09-26T19:14:36Z

...c/org/eclipse/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/Attributes.java

+ *
+ * @author Adel Belkhiri
+ */
+@SuppressWarnings({ "nls" })


@NonNullByDefault

MatthewKhouzam · 2024-09-26T19:17:23Z

...e/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/DpdkEthdevEventHandler.java

+        try {
+            StateSystemBuilderUtils.incrementAttributeLong(ssb, ts, queueQuark, nbPkts);
+        } catch (StateValueTypeException e) {
+            Activator.getInstance().logWarning(getClass().getName() + ": problem accessing the state of a NIC queue (Quark =" + String.valueOf(queueQuark) + ")"); //$NON-NLS-1$ //$NON-NLS-2$


I really like this... can you put it in the activator to have it everywhere?

MatthewKhouzam · 2024-09-26T19:18:46Z

...e/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/DpdkEthdevEventHandler.java

+     * @param ts
+     *            time to use for state change
+     */
+    static private void createNicQueue(ITmfStateSystemBuilder ssb, int queueSetQuark, int nbQueues, long ts) {


createNicQueues

MatthewKhouzam · 2024-09-26T19:19:25Z

...e/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/DpdkEthdevEventHandler.java

+            updateCounts(ssb, rxQueueQark, Objects.requireNonNull(nbRxPkts), ts);
+
+        } else if (eventName.equals(DpdkEthdevEventLayout.eventEthdevTxqBurst())) {
+            Integer queueId = event.getContent().getFieldValue(Integer.class, DpdkEthdevEventLayout.fieldQueueId());


put in a common helper function, you have it for 100-103 and 108-110

MatthewKhouzam · 2024-09-26T19:19:37Z

...se/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/DpdkEthdevEventLayout.java

@@ -0,0 +1,109 @@
+package org.eclipse.tracecompass.incubator.internal.dpdk.core.ethdev.rate.analysis;


copyright header

MatthewKhouzam · 2024-09-26T19:19:53Z

...se/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/DpdkEthdevEventLayout.java

+    // Event field names
+    // ------------------------------------------------------------------------
+
+    /**


needs a description too

MatthewKhouzam · 2024-09-26T19:21:59Z

...racecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/DpdkEtherRateDataProvider.java

+                i++;
+            }
+        } catch (IndexOutOfBoundsException e) {
+            Activator.getInstance().logWarning("A DPDK event (" + DpdkEthdevEventLayout.eventEthdevConfigure() + ") is missing"); //$NON-NLS-1$ //$NON-NLS-2$


write the index? or just log the exception.

MatthewKhouzam · 2024-09-26T19:22:38Z

...rg/eclipse/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/IODataPalette.java

+ *
+ * @author Geneviève Bastien
+ */
+public class IODataPalette {


reuse for the win!

arfio

I think all the analyses look very interesting but there is a lot of duplicated that can easily be reused throughout. In particular the throughput and rate analysis that are very similar, it could be merged into one analysis with multiple views.

arfio · 2024-09-30T16:01:01Z

...ss/incubator/internal/dpdk/core/ethdev/poll/distribution/analysis/DpdkEthdevEventLayout.java

This file is duplicated throughout the different packages you created. You should only have one version of this file. Also it is missing a copyright header

Thanks, Arnauld for the feedback. I have a question: should I just change the name of the file/class for each analysis (example: DpdkEthdevThroughputEventLayout.java), or combine all these files into one single layout class used by all the analyses for Ethdev?

arfio · 2024-09-30T16:06:18Z

...mpass/incubator/internal/dpdk/core/ethdev/poll/distribution/analysis/TmfEtherPollAspect.java

Why not adding this file to the trace package, and adding it to the DpdkTrace aspects as well ?

I changed all the aspects in the ethdev poll analyses into regular classes. It turns out that aspects are not required for this type of analysis.

arfio · 2024-09-30T16:06:50Z

...cecompass/incubator/internal/dpdk/core/ethdev/poll/stats/analysis/DpdkEthdevEventLayout.java

Same comment as above

arfio · 2024-09-30T16:21:58Z

...e/tracecompass/incubator/internal/dpdk/core/ethdev/rate/analysis/DpdkEthdevEventHandler.java

Why not merging this analysis and the throughput one ? Both uses 99% the same code, you could merge both and just use 2 different data providers.

Thanks for the comment Arnaud. Indeed, I merged the throughput and rate analyses, thus, reducing much the duplicated code.

arfio · 2024-09-30T16:22:46Z

...ecompass/incubator/internal/dpdk/core/ethdev/spin/analysis/DpdkEthdevSpinAnalysisModule.java

+
+            }
+        } catch (TimeRangeException | AttributeNotFoundException | StateSystemDisposedException e) {
+        }


This needs at least a warning or something.

arfio · 2024-09-30T17:45:20Z

...racecompass/incubator/internal/dpdk/core/ethdev/spin/analysis/DpdkEtherSpinDataProvider.java

Maybe you could have a parent class for this to reuse most of the code.

MatthewKhouzam

Small changes needed

MatthewKhouzam · 2024-11-04T21:07:45Z

.../internal/dpdk/core/ethdev/throughput/analysis/AbstractDpdkEthdevThroughputDataProvider.java

+        return nodes;
+    }
+
+    @Override


Do we still need this?

Thank you @MatthewKhouzam for this comment. Omitting the "@OverRide" annotation results in an error, so I believe we still need it.

MatthewKhouzam · 2024-11-04T21:08:26Z

.../internal/dpdk/core/ethdev/throughput/analysis/AbstractDpdkEthdevThroughputDataProvider.java

+    /**
+     * Abstract class for building data series illustrating DPDK NICs throughput.
+     */
+    protected abstract class AbstractNicQueueBuilder {


Separate file?

Yes! I put this class in a separate file and changed its visibility to "package".

MatthewKhouzam · 2024-11-04T21:09:16Z

...bator/internal/dpdk/core/ethdev/throughput/analysis/DpdkEthdevThroughputBpsDataProvider.java

+            return stateValue instanceof Number ? ((Number) stateValue).doubleValue() : 0.0;
+        } catch (AttributeNotFoundException | IndexOutOfBoundsException e) {
+            Activator.getInstance().logError("Error accessing packets size on state system", e); //$NON-NLS-1$
+            return 0.0;


Should this be NaN or 0.0? asking

@MatthewKhouzam: You are right! thanks.

MatthewKhouzam · 2024-11-04T21:09:52Z

...bator/internal/dpdk/core/ethdev/throughput/analysis/DpdkEthdevThroughputPpsDataProvider.java

+            return stateValue instanceof Number ? ((Number) stateValue).doubleValue() : 0.0;
+        } catch (AttributeNotFoundException | IndexOutOfBoundsException e) {
+            Activator.getInstance().logError("Error accessing packets count on state system", e); //$NON-NLS-1$
+            return 0.0;


same question.

MatthewKhouzam · 2024-11-04T21:10:28Z

...g/eclipse/tracecompass/incubator/internal/dpdk/core/ethdev/throughput/analysis/Messages.java

+        NLS.initializeMessages(BUNDLE_NAME, Messages.class);
+    }
+
+    private Messages() {


Add a //does nothing here

MatthewKhouzam · 2024-11-04T21:10:40Z

.../src/org/eclipse/tracecompass/incubator/internal/dpdk/ui/ethdev/throughput/bps/Messages.java

+        NLS.initializeMessages(BUNDLE_NAME, Messages.class);
+    }
+
+    private Messages() {


MatthewKhouzam · 2024-11-04T21:10:56Z

...tracecompass/incubator/internal/dpdk/ui/ethdev/throughput/bps/NicQueueThroughputBpsView.java

+            @Override
+            protected ITmfTreeColumnDataProvider getColumnDataProvider() {
+                return () -> {
+                    return ImmutableList.of(


Can we get away with List.of?

@MatthewKhouzam: Yes, we can replace ImmutableList.of() with List.of(). I checked and found that List.of() was introduced in Java 9, making it a suitable replacement here. Thank you for the comment!

MatthewKhouzam · 2024-11-04T21:11:08Z

.../src/org/eclipse/tracecompass/incubator/internal/dpdk/ui/ethdev/throughput/pps/Messages.java

+        NLS.initializeMessages(BUNDLE_NAME, Messages.class);
+    }
+
+    private Messages() {


//do nothing

arfio

A lot of small comments, once they are fixed, I will be happy to approve !

arfio · 2024-10-23T14:41:00Z

analyses/org.eclipse.tracecompass.incubator.dpdk.core/META-INF/MANIFEST.MF

@@ -15,8 +15,12 @@ Require-Bundle: org.eclipse.ui,
 org.eclipse.tracecompass.tmf.core,
 org.eclipse.tracecompass.tmf.ctf.core,
 org.eclipse.tracecompass.analysis.os.linux.core,
- org.eclipse.jdt.annotation;bundle-version="2.2.400"
+ org.eclipse.jdt.annotation;bundle-version="2.2.400",
+ com.google.guava,


This should be set in Imported packages.

arfio · 2024-10-23T14:43:09Z

...bator/internal/dpdk/core/ethdev/poll/distribution/analysis/DpdkPollDistributionAnalysis.java

+    @Override
+    public boolean canExecute(ITmfTrace trace) {
+        if (trace instanceof DpdkTrace) {
+            return ((DpdkTrace) trace).validate(null, trace.getPath()).isOK() ? true : false;


Why use a ternary operator there, you can just return the result from isOk().

arfio · 2024-11-28T15:40:53Z

tracetypes/org.eclipse.tracecompass.incubator.dpdk.core/META-INF/MANIFEST.MF

@@ -15,8 +15,12 @@ Require-Bundle: org.eclipse.ui,
 org.eclipse.tracecompass.tmf.core,
 org.eclipse.tracecompass.tmf.ctf.core,
 org.eclipse.tracecompass.analysis.os.linux.core,
- org.eclipse.jdt.annotation;bundle-version="2.2.400"
+ com.google.guava,


Should be in the imported packages section.

arfio · 2024-11-28T15:41:53Z

tracetypes/org.eclipse.tracecompass.incubator.dpdk.core/META-INF/MANIFEST.MF

- org.eclipse.jdt.annotation;bundle-version="2.2.400"
+ com.google.guava,
+ org.eclipse.jdt.annotation;bundle-version="2.2.400",
+ org.eclipse.tracecompass.analysis.lami.core


Should be above the org.eclipse.jdt.annotation;bundle-version="2.2.400" line to make a clearer diff.

arfio · 2024-11-28T15:54:06Z

...rc/org/eclipse/tracecompass/incubator/internal/dpdk/core/analysis/DpdkEthdevEventLayout.java

Should maybe be in org.eclipse.tracecompass.incubator.internal.dpdk.core.ethdev

arfio · 2024-11-28T16:57:51Z

...bator/internal/dpdk/core/ethdev/throughput/analysis/DpdkEthdevThroughputPpsDataProvider.java

+            prevTime = time;
+        }
+
+        return ImmutableList.copyOf(Iterables.transform(builders, builder -> builder.build(Y_AXIS_DESCRIPTION)));


unchecked conversion warning

arfio · 2024-11-28T16:59:23Z

...bator/internal/dpdk/core/ethdev/throughput/analysis/DpdkEthdevThroughputPpsDataProvider.java

+
+        for (Entry<Long, Integer> entry : getSelectedEntries(filter).entrySet()) {
+            long id = entry.getKey();
+            int quark = entry.getValue();


two unsafe interpretation, the getKey() and getValue() should be put in a Objects.requireNonNull().

arfio · 2024-11-28T17:04:18Z

...rg/eclipse/tracecompass/incubator/internal/dpdk/ui/ethdev/spin/ThreadSpinStatisticsView.java

+        return new AbstractSelectTreeViewer2(parent, 1, DpdkEthdevSpinDataProvider.ID) {
+            @Override
+            protected ITmfTreeColumnDataProvider getColumnDataProvider() {
+                return () -> {


curly braces should be removed, List.of should be changed to ImmutableList to fix the warning

arfio · 2024-11-28T17:04:58Z

...tracecompass/incubator/internal/dpdk/ui/ethdev/throughput/bps/NicQueueThroughputBpsView.java

+        return new AbstractSelectTreeViewer2(parent, 1, DpdkEthdevThroughputBpsDataProvider.ID) {
+            @Override
+            protected ITmfTreeColumnDataProvider getColumnDataProvider() {
+                return () -> {


Same comment.

arfio · 2024-11-28T17:05:15Z

...tracecompass/incubator/internal/dpdk/ui/ethdev/throughput/pps/NicQueueThroughputPpsView.java

+        return new AbstractSelectTreeViewer2(parent, 1, DpdkEthdevThroughputPpsDataProvider.ID) {
+            @Override
+            protected ITmfTreeColumnDataProvider getColumnDataProvider() {
+                return () -> {


Same comment

Introduces packet throughput analysis for DPDK applications using the ethdev library. The analysis calculates packet throughput in both bits per second (bps) and packets per second (pps) for transmitted and received Ethernet traffic on a per-queue basis. Note that the calculation of throughput in bps requires the existence of custom profiling events in the trace Signed-off-by: Adel Belkhiri <[email protected]>

PMD threads continuously poll NIC queues, leading to constant 100% CPU usage. This analysis provides a rough estimation of how busy PMD threads are with actual work, by comparing the times they successfully retrieve packets from the NIC queue versus the times they are merely spinning without processing any packets. Signed-off-by: Adel Belkhiri <[email protected]>

Introduce packet distribution analysis for PMD threads based on ethdev library. This analysis computes the distribution of packets retrieved in a single rte_eth_rx_burst() call, on a per-queue basis. Signed-off-by: Adel Belkhiri <[email protected]>

Introduce packet distribution statistics analysis for PMD threads in the ethdev library. This analysis calculates various statistics related to the distribution of packets retrieved in a single rte_eth_rx_burst() call, on a per-thread and per-queue basis. The computed statistics include the minimum, maximum, average number of packets retrieved, as well as the standard deviation. Signed-off-by: Adel Belkhiri <[email protected]>

MatthewKhouzam requested changes Sep 26, 2024

View reviewed changes

arfio self-requested a review September 27, 2024 19:04

arfio requested changes Sep 30, 2024

View reviewed changes

adel-belkhiri force-pushed the dpdk-ethdev branch 2 times, most recently from 0b9fd6f to d2f9125 Compare October 16, 2024 15:54

MatthewKhouzam requested changes Nov 4, 2024

View reviewed changes

adel-belkhiri force-pushed the dpdk-ethdev branch from d2f9125 to 12cfb9a Compare November 12, 2024 17:25

adel-belkhiri mentioned this pull request Nov 12, 2024

Move the initial dpdk analysis to the 'tracetypes' directory #117

Merged

adel-belkhiri force-pushed the dpdk-ethdev branch from 12cfb9a to 7a8e39d Compare November 22, 2024 20:03

arfio requested changes Nov 28, 2024

View reviewed changes

adel-belkhiri force-pushed the dpdk-ethdev branch from 7a8e39d to 451df36 Compare December 16, 2024 19:49

adel-belkhiri added 3 commits December 16, 2024 15:08

adel-belkhiri force-pushed the dpdk-ethdev branch from 451df36 to d21e82b Compare December 16, 2024 20:08

		@@ -0,0 +1,109 @@
		package org.eclipse.tracecompass.incubator.internal.dpdk.core.ethdev.rate.analysis;

Performance Analyses for DPDK Applications Using the Ethdev Library #83

Are you sure you want to change the base?

Performance Analyses for DPDK Applications Using the Ethdev Library #83

Conversation

adel-belkhiri commented Sep 26, 2024

MatthewKhouzam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arfio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adel-belkhiri Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewKhouzam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adel-belkhiri Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adel-belkhiri Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arfio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adel-belkhiri Sep 30, 2024 •

edited

Loading

adel-belkhiri Nov 5, 2024 •

edited

Loading

adel-belkhiri Nov 5, 2024 •

edited

Loading