[PATCH] net/nfp: fix representor port release queue problem

[Chaoyong He]

From: Long Wu 

The PF representor port's queue is different from the VF/physical
representor port. So the release process in close port should
be different too.

Fixes: 39b3951 ("net/nfp: fix resource leak for exit of flower firmware")
Cc: chaoyong...@corigine.com
Cc: sta...@dpdk.org

Signed-off-by: Long Wu 
Reviewed-by: Chaoyong He 
Reviewed-by: Peng Zhang 
---
 .../net/nfp/flower/nfp_flower_representor.c   | 69 ++-
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c 
b/drivers/net/nfp/flower/nfp_flower_representor.c
index f26bf83edb..c4f33cbb2e 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -304,6 +304,54 @@ nfp_flower_repr_tx_burst(void *tx_queue,
return sent;
 }
 
+static void
+nfp_flower_repr_free_queue(struct nfp_flower_representor *repr)
+{
+   uint16_t i;
+   struct rte_eth_dev *eth_dev = repr->eth_dev;
+
+   for (i = 0; i < eth_dev->data->nb_tx_queues; i++)
+   rte_free(eth_dev->data->tx_queues[i]);
+
+   for (i = 0; i < eth_dev->data->nb_rx_queues; i++)
+   rte_free(eth_dev->data->rx_queues[i]);
+}
+
+static void
+nfp_flower_pf_repr_close_queue(struct nfp_flower_representor *repr)
+{
+   struct rte_eth_dev *eth_dev = repr->eth_dev;
+
+   /*
+* We assume that the DPDK application is stopping all the
+* threads/queues before calling the device close function.
+*/
+   nfp_net_disable_queues(eth_dev);
+
+   /* Clear queues */
+   nfp_net_close_tx_queue(eth_dev);
+   nfp_net_close_rx_queue(eth_dev);
+}
+
+static void
+nfp_flower_repr_close_queue(struct nfp_flower_representor *repr)
+{
+   switch (repr->repr_type) {
+   case NFP_REPR_TYPE_PHYS_PORT:
+   nfp_flower_repr_free_queue(repr);
+   break;
+   case NFP_REPR_TYPE_PF:
+   nfp_flower_pf_repr_close_queue(repr);
+   break;
+   case NFP_REPR_TYPE_VF:
+   nfp_flower_repr_free_queue(repr);
+   break;
+   default:
+   PMD_DRV_LOG(ERR, "Unsupported repr port type.");
+   break;
+   }
+}
+
 static int
 nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev)
 {
@@ -348,8 +396,6 @@ nfp_flower_repr_dev_close(struct rte_eth_dev *dev)
uint16_t i;
struct nfp_net_hw *hw;
struct nfp_pf_dev *pf_dev;
-   struct nfp_net_txq *this_tx_q;
-   struct nfp_net_rxq *this_rx_q;
struct nfp_flower_representor *repr;
struct nfp_app_fw_flower *app_fw_flower;
 
@@ -361,26 +407,11 @@ nfp_flower_repr_dev_close(struct rte_eth_dev *dev)
hw = app_fw_flower->pf_hw;
pf_dev = hw->pf_dev;
 
-   /*
-* We assume that the DPDK application is stopping all the
-* threads/queues before calling the device close function.
-*/
-   nfp_net_disable_queues(dev);
-
-   /* Clear queues */
-   for (i = 0; i < dev->data->nb_tx_queues; i++) {
-   this_tx_q = dev->data->tx_queues[i];
-   nfp_net_reset_tx_queue(this_tx_q);
-   }
-
-   for (i = 0; i < dev->data->nb_rx_queues; i++) {
-   this_rx_q = dev->data->rx_queues[i];
-   nfp_net_reset_rx_queue(this_rx_q);
-   }
-
if (pf_dev->app_fw_id != NFP_APP_FW_FLOWER_NIC)
return -EINVAL;
 
+   nfp_flower_repr_close_queue(repr);
+
nfp_flower_repr_free(repr, repr->repr_type);
 
for (i = 0; i < MAX_FLOWER_VFS; i++) {
-- 
2.39.1

RE: [PATCH v5 1/6] examples/l3fwd: fix lcore ID restriction

[Morten Brørup]

> From: Sivaprasad Tummala [mailto:sivaprasad.tumm...@amd.com]
> Sent: Monday, 18 March 2024 18.32
> 
> Currently the config option allows lcore IDs up to 255,
> irrespective of RTE_MAX_LCORES and needs to be fixed.
> 
> The patch allows config options based on DPDK config.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Sivaprasad Tummala 
> Acked-by: Konstantin Ananyev 
> ---

I suggest you update the description of the individual patches too, like you 
did for patch 0/6.

E.g. this patch not only fixes the lcore_id, but also the queue_id type size.


For the series,
Acked-by: Morten Brørup 

RE: [PATCH 01/13] net/mlx5/hws: move warn into debug level when needed

[Raslan Darawsheh]

Hi,

> -Original Message-
> From: Thomas Monjalon 
> Sent: Monday, March 18, 2024 4:49 PM
> To: Itamar Gozlan ; Erez Shitrit ;
> Hamdan Agbariya ; Yevgeny Kliteynik
> ; Alex Vesker ; Raslan Darawsheh
> 
> Cc: Slava Ovsiienko ; Dariusz Sosnowski
> ; Ori Kam ; Suanming Mou
> ; Matan Azrad ; Mark Bloch
> ; dev@dpdk.org; Maayan Kashani
> 
> Subject: Re: [PATCH 01/13] net/mlx5/hws: move warn into debug level when
> needed
> 
> 18/03/2024 13:56, Raslan Darawsheh:
> > From: Itamar Gozlan 
> > > From: Erez Shitrit 
> > >
> > > When the user tries to create a matcher and if failed  with specific
> > > errno
> > > (E2BIG) the message will be in debug level and not in warning.
> > > It is a part of a feature when the user re-try to insert a new
> > > matching depends on that errno, no need the annoying message.
> > >
> > > Fixes: c55c2bf3533 ("net/mlx5/hws: net/mlx5/hws: add definer layer")
> > >
> > > Signed-off-by: Erez Shitrit 
> > > Acked-by: Matan Azrad 
> > Fixed Cc stable on several patches on this series, and reworded the
> > commits Series applied to next-net-mlx,
> 
> There is no cover letter for this series, so we are not able to understand how
> critical it is, and what is the general intent.
> 
> Is it supposed to be integrated in the last week of 24.03 release cycle?
> 
No, it's not critical for RC4 It's my fault, I'll drop it for now and we'll 
merge it in the next release cycle only.

Kindest regards
Raslan Darawsheh

RE: [PATCH v9 5/5] eal: add option to put timestamp on console output

[Morten Brørup]

> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Monday, 18 March 2024 23.03
> 
> When debugging driver or startup issues, it is useful to have
> a timestamp on each message printed. The messages in syslog
> already have a timestamp, but often syslog is not available
> during testing. The timestamp format is chosen to look
> like the default Linux dmesg timestamp.
> 
> The first few lines are not timestamped because the flag is stored
> in internal configuration which is stored in shared memory
> which is not setup up until a little later in startup process.
> 
> This logging skips the unnecessary step of going through stdio,
> which makes it more robust against being called in interrupt
> handlers etc.
> 
> Example:
> $ dpdk-testpmd --log-timestamp -- -i
> EAL: Detected CPU lcores: 16
> EAL: Detected NUMA nodes: 1
> EAL: Detected static linkage of DPDK
> EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'VA'
> [   0.112264] testpmd: No probed ethernet devices
> Interactive-mode selected
> [   0.184573] testpmd: create a new mbuf pool : n=163456,
> size=2176, socket=0
> [   0.184612] testpmd: preferred mempool ops selected: ring_mp_mc
> 
> Signed-off-by: Stephen Hemminger 
> ---

[...]

>  static ssize_t
>  console_log_write(__rte_unused void *c, const char *buf, size_t size)
>  {
> + struct timespec ts;
>   ssize_t ret;
> 
> - /* write on stderr */
> - ret = fwrite(buf, 1, size, stderr);
> + if (timestamp_enabled) {
> + clock_gettime(CLOCK_MONOTONIC, &ts);
> + ts.tv_sec -= log_started.tv_sec;
> + ts.tv_nsec -= log_started.tv_nsec;

Please log the absolute CLOCK_MONOTONIC instead of subtracting log_started, so 
timestamps can be easily compared with timestamps from other processes.

> + if (ts.tv_nsec < 0) {
> + --ts.tv_sec;
> + ts.tv_nsec += 10ul;
> + }
> +
> + ret = fprintf(stderr, "[%8lu.%06lu] %.*s",
> +   ts.tv_sec, ts.tv_nsec / 1000u,
> +   (int) size, buf);

With the above change,
For the series,
Acked-by: Morten Brørup 

RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption

[Akhil Goyal]

> > > + if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > +  options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
> > > + !options->out_of_place) {
> > > + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > throughput decryption.\n");
> > > + return -EINVAL;
> > > + }
> >
> > This check is blocking cipher_only decryption which should pass 
> > irrespective of
> > inplace/oop and Data correct/incorrect.
> 
> Sorry, in that case I will remove "options->cipher_op ==
> RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?

I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
Instead of cipher_op.

Ciara, What do you suggest? You were also seeing some issues in this patch.

fib{,6}: questions and proposals

[Robin Jarry]

Hi Vladimir,

I have been using rte_fib for a while and stumbled upon a few quirks. 
I was wondering if you would answer some questions:


1) Is it OK/safe to share the same fib to perform route lookups from 
  multiple lcores in parallel? So far my observations seem to validate 
  that assumption but I would like your opinion :)


2) Is it OK/safe to modify a fib from a control thread (read/write) 
  while it is used by data path threads (read only)?


3) There is no public API to list/walk all configured routes in a fib. 
  Would that be possible/easy to implement?


4) In rte_fib, every IPv4 address (route *and* next hop) needs to be in 
  host order. This is not consistent with fib6 where addresses are 
  stored in network order. It took me quite a while to figure out what 
  was wrong with my code.


  I assume this is because DIR24 needs host order integers and not 
  TRIE. Why was this not hidden in the API?


  Could we add a flag to rte_fib_conf to change the behaviour? This 
  would avoid error prone ntohl/htonl juggling.


Thanks in advance for your replies :)

--
Robin

Re: Email based retest request process: proposal for new pull/re-apply feature

[zhoumin]

On Mon, Mar 18, 2024 at 3:59PM, Patrick Robb wrote:

On Thu, Mar 7, 2024 at 12:06 PM Adam Hassick  wrote:


I'm not opposed to having the contexts be a key-value pair argument
like the others, however that does break backwards compatibility with
our existing syntax. If we don't care very much about backwards
compatibility, then we could make this change.

Instead of having a boolean and a string parameter for whether to
rebase and the branch to rebase on, we could have a single argument
specifying a branch. Then, labs rebase on the given branch and then
rerun all tests if the "rebase=" argument is present. This
would look like:

Recheck-request: rebase=main, iol-sample-apps-testing,
iol-unit-amd64-testing, iol-broadcom-Performance

I agree with this approach because it preserves backward
compatibility, while still providing us with all the functionality we
need. We will also be able to accept key value arguments in the future
if further feature requests come in which require it.


I don't think the context should be required if the request includes
the rebase argument, because we do not want to mix valid and invalid
test results as Aaron said.
This would be a valid format if contexts are optional:

Recheck-request: rebase=main

Okay, I agree that contexts should not be considered by labs when we
use rebase - but of course we will still store the contexts (if they
are submitted) alongside the key value args. In the future there may
be an application for this.

Zhoumin, does this sound acceptable, or do you think there are any
flaws? If it works, we will implement the updates and try to upstream
this week. Thanks!


Thanks for your hard work.

I also agree with this approach. The meaning of the key value 
`rebase=main` is sufficient, and loongson lab can support it.


One more thing I want to confirm is whether we should apply the patch 
onto the branch commit which existed at the time when that patch was 
submitted or onto the latest tip of branch if users request doing 
rebase. Users probably request a recheck with `rebase` when the CI lab 
chose a wrong branch onto which apply the patch. I worry we may 
encounter conflicts when apply the patch onto the latest commit of the 
target branch if that branch is just updated before the request.

[PATCH] net/nfp: fix uninitialized variable

[Chaoyong He]

CI found in the logic of 'nfp_aesgcm_iv_update()', the variable
'cfg_iv' may used uninitialized in some case.

Coverity issue: 415808
Fixes: 36361ca7fea2 ("net/nfp: fix data endianness problem")
Cc: shihong.w...@corigine.com
Cc: sta...@dpdk.org

Signed-off-by: Chaoyong He 
Reviewed-by: Long Wu 
Reviewed-by: Peng Zhang 
---
 drivers/net/nfp/nfp_ipsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_ipsec.c b/drivers/net/nfp/nfp_ipsec.c
index 205d1d594c..647bc2bb6d 100644
--- a/drivers/net/nfp/nfp_ipsec.c
+++ b/drivers/net/nfp/nfp_ipsec.c
@@ -526,7 +526,7 @@ nfp_aesgcm_iv_update(struct ipsec_add_sa *cfg,
char *iv_b;
char *iv_str;
const rte_be32_t *iv_value;
-   uint8_t cfg_iv[NFP_ESP_IV_LENGTH];
+   uint8_t cfg_iv[NFP_ESP_IV_LENGTH] = {};
 
iv_str = strdup(iv_string);
if (iv_str == NULL) {
-- 
2.39.1

Re: [PATCH] doc: add dma perf feature details

[zhoumin]

On Mon, Mar 18, 2024 at 6:17PM, Amit Prakash Shukla wrote:

Update dma perf test document with below support features:
1. Memory-to-device and device-to-memory copy.
2. Skip support.
3. Scatter-gather support.

Signed-off-by: Amit Prakash Shukla 
---
  doc/guides/tools/dmaperf.rst | 89 ++--
  1 file changed, 64 insertions(+), 25 deletions(-)

diff --git a/doc/guides/tools/dmaperf.rst b/doc/guides/tools/dmaperf.rst
index 9e3e78a6b7..4a5702a628 100644
--- a/doc/guides/tools/dmaperf.rst
+++ b/doc/guides/tools/dmaperf.rst
@@ -5,27 +5,23 @@ dpdk-test-dma-perf Application
  ==
  
  The ``dpdk-test-dma-perf`` tool is a Data Plane Development Kit (DPDK) application

-that enables testing the performance of DMA (Direct Memory Access) devices 
available within DPDK.
-It provides a test framework to assess the performance of CPU and DMA devices
-under various scenarios, such as varying buffer lengths.
-Doing so provides insight into the potential performance
-when using these DMA devices for acceleration in DPDK applications.
+that evaluates the performance of DMA (Direct Memory Access) devices 
accessible in DPDK environment.
+It provides a benchmark framework to assess the performance of CPU and DMA 
devices
+under various combinations, such as varying buffer lengths, scatter-gather 
copy, copying in remote
+memory etc. It helps in evaluating performance of DMA device as hardware 
acceleration vehicle in
+DPDK application.
  
-It supports memory copy performance tests for now,

-comparing the performance of CPU and DMA automatically in various conditions
-with the help of a pre-set configuration file.
+In addition, this tool supports memory-to-memory, memory-to-device and 
device-to-memory copy tests,
+to compare the performance of CPU and DMA capabilities under various 
conditions with the help of a
+pre-set configuration file.
  
  
  Configuration

  -
  
-This application uses inherent DPDK EAL command-line options

-as well as custom command-line options in the application.
-An example configuration file for the application is provided
-and gives the meanings for each parameter.
-
-Here is an extracted sample from the configuration file
-(the complete sample can be found in the application source directory):
+Along with EAL command-line arguments, this application supports various 
parameters for the
+benchmarking through a configuration file. An example configuration file is 
provided below along
+with the application to demonstrate all the parameters.
  
  .. code-block:: ini
  
@@ -53,14 +49,35 @@ Here is an extracted sample from the configuration file

 lcore = 3, 4
 eal_args=--in-memory --no-pci
  
+   [case3]

+   skip=1
+   type=DMA_MEM_COPY
+   direction=mem2dev
+   vchan_dev=raddr=0x2,coreid=1,pfid=2,vfid=3
+   dma_src_sge=4
+   dma_dst_sge=1
+   mem_size=10
+   buf_size=64,8192,2,MUL
+   dma_ring_size=1024
+   kick_batch=32
+   src_numa_node=0
+   dst_numa_node=0
+   cache_flush=0
+   test_seconds=2
+   lcore_dma=lcore10@:00:04.2, lcore11@:00:04.3
+   eal_args=--in-memory --file-prefix=test
+
  The configuration file is divided into multiple sections, each section 
represents a test case.
-The four variables ``mem_size``, ``buf_size``, ``dma_ring_size``, and 
``kick_batch``
-can vary in each test case.
-The format for this is ``variable=first,last,increment,ADD|MUL``.
-This means that the first value of the variable is 'first',
-the last value is 'last',
-'increment' is the step size,
-and 'ADD|MUL' indicates whether the change is by addition or multiplication.
+The four mandatory variables ``mem_size``, ``buf_size``, ``dma_ring_size``, 
and ``kick_batch``
+can vary in each test case. The format for this is 
``variable=first,last,increment,ADD|MUL``.
+This means that the first value of the variable is 'first', the last value is 
'last',
+'increment' is the step size, and 'ADD|MUL' indicates whether the change is by 
addition or
+multiplication.
+
+The variables for mem2dev and dev2mem copy are ``direction``, ``vchan_dev`` 
and can vary in each
+test case. If the direction is not configured, the default is mem2mem copy.
+
+For scatter-gather copy test ``dma_src_sge``, ``dma_dst_sge`` must be 
configured.
  
  Each case can only have one variable change,

  and each change will generate a scenario, so each case can have multiple 
scenarios.
@@ -69,10 +86,32 @@ and each change will generate a scenario, so each case can 
have multiple scenari
  Configuration Parameters
  
  
+``skip``

+  To skip a test-case, must be configured as ``1``
+
  ``type``
The type of the test.
Currently supported types are ``DMA_MEM_COPY`` and ``CPU_MEM_COPY``.
  
+``direction``

+  The direction of data transfer.
+  Currently supported directions:
+
+* ``mem2mem`` - memory to memory copy
+
+* ``mem2dev`` - memory to device copy
+
+* ``dev2mem`` - device to memory copy
+
+``vchan_dev``

RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption

[Suanming Mou]

> -Original Message-
> From: Akhil Goyal 
> Sent: Tuesday, March 19, 2024 4:23 PM
> To: Suanming Mou ; Anoob Joseph
> ; ciara.po...@intel.com
> Cc: dev@dpdk.org
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
> 
> > > > +   if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > > +   (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > > +options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
> > > > +   !options->out_of_place) {
> > > > +   RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > > throughput decryption.\n");
> > > > +   return -EINVAL;
> > > > +   }
> > >
> > > This check is blocking cipher_only decryption which should pass
> > > irrespective of inplace/oop and Data correct/incorrect.
> >
> > Sorry, in that case I will remove "options->cipher_op ==
> > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?
> 
> I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
> Instead of cipher_op.

I'm not sure. Since in AEAD OP, auth_op will always be 
RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be rejected.
If the combination here is too complicated, what about just remove that limits 
and let user to decide? If the input is not correct, PMD will reject it as well.

> 
> Ciara, What do you suggest? You were also seeing some issues in this patch.

RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption

[Akhil Goyal]

> > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP
> decryption
> >
> > > > > + if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > > > + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > > > +  options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT)
> &&
> > > > > + !options->out_of_place) {
> > > > > + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > > > throughput decryption.\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > >
> > > > This check is blocking cipher_only decryption which should pass
> > > > irrespective of inplace/oop and Data correct/incorrect.
> > >
> > > Sorry, in that case I will remove "options->cipher_op ==
> > > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> > > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?
> >
> > I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
> > Instead of cipher_op.
> 
> I'm not sure. Since in AEAD OP, auth_op will always be
> RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be
> rejected.
> If the combination here is too complicated, what about just remove that 
> limits and
> let user to decide? If the input is not correct, PMD will reject it as well.

The problematic cases are where auth data (ICV) is not correct.
i.e. AEAD, AUTH_ONLY and CIPHER_AUTH.

Hence following check should be ok.
if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
(options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
!options->out_of_place) {

Yes PMD will report error if the input data is not correct,
but we cannot just fail in that case just because the app is intentionally not 
filling the data.
It should report unsupported case. 
> 
> >
> > Ciara, What do you suggest? You were also seeing some issues in this patch.

[PATCH] app/testpmd: fix releasing action handle flush memory

[Bing Zhao]

The memory of the indirect action handles should be freed after
being destroyed in the flush. The behavior needs to be consistent
with the single handle destroy.

Or else, there will be some unexpected error when the action handle
is destroyed for the 2nd time, for example, the port needs to be
closed again.

Fixes: f7352c176bbf ("app/testpmd: fix use of indirect action after port close")
Cc: dmitry.kozl...@gmail.com
Cc: sta...@dpdk.org

Signed-off-by: Bing Zhao 
Reviewed-by: Dariusz Sosnowski 
---
 app/test-pmd/config.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ba1007ace6..f62ba90c87 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1918,8 +1918,7 @@ port_action_handle_flush(portid_t port_id)
/* Poisoning to make sure PMDs update it in case of error. */
memset(&error, 0x44, sizeof(error));
if (pia->handle != NULL) {
-   ret = pia->type ==
- RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
+   ret = pia->type == RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
  rte_flow_action_list_handle_destroy
  (port_id, pia->list_handle, &error) :
  rte_flow_action_handle_destroy
@@ -1929,11 +1928,9 @@ port_action_handle_flush(portid_t port_id)
   pia->id);
ret = port_flow_complain(&error);
}
-   tmp = &pia->next;
-   } else {
-   *tmp = pia->next;
-   free(pia);
}
+   *tmp = pia->next;
+   free(pia);
}
return ret;
 }
-- 
2.34.1

RE: [PATCH 0/2] Tx path check mbuf sub-segment

[Li, HongboX]

> -Original Message-
> From: Mingjin Ye 
> Sent: Friday, March 15, 2024 6:24 PM
> To: dev@dpdk.org
> Cc: Ye, MingjinX 
> Subject: [PATCH 0/2] Tx path check mbuf sub-segment
> 
> Add check mbuf sub-segment to Tx diagnostic path.
> 
> Mingjin Ye (2):
>   net/i40e: Tx path check mbuf sub-segment
>   net/ice: Tx path check mbuf sub-segment
> 
>  drivers/net/i40e/i40e_rxtx.c | 2 +-
>  drivers/net/ice/ice_rxtx.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> --
> 2.25.1

Tested-by: Li, HongboX 

RE: [PATCH v2] dmadev: fix structure alignment

[Jiale, SongX]

> -Original Message-
> From: Ma, WenwuX 
> Sent: Friday, March 15, 2024 9:44 AM
> To: dev@dpdk.org; fengcheng...@huawei.com
> Cc: Jiale, SongX ; Ma, WenwuX
> ; sta...@dpdk.org
> Subject: [PATCH v2] dmadev: fix structure alignment
> 
> The structure rte_dma_dev needs only 8 byte alignment.
> This patch replaces __rte_cache_aligned of rte_dma_dev with
> __rte_aligned(8).
> 
> Fixes: b36970f2e13e ("dmadev: introduce DMA device library")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Wenwu Ma 
> ---
Tested-by: Jiale Song 

RE: [PATCH] net/iavf: fix fail to reset vf when using dcf

[Li, HongboX]

> -Original Message-
> From: Kaiwen Deng 
> Sent: Thursday, March 14, 2024 9:01 AM
> To: dev@dpdk.org
> Cc: sta...@dpdk.org; Zhou, YidingX ; Deng, KaiwenX
> ; Wu, Jingjing ; Zeng,
> ZhichaoX ; Zhang, Qi Z 
> Subject: [PATCH] net/iavf: fix fail to reset vf when using dcf
> 
> On the latest ice kernel driver, renegotiating VIRTCHNL_OP_GET_VF_RESOURCES
> will fail without hardware reset when using dcf.
> 
> This commit will send VIRTCHNL_OP_RESET_VF to pf before dpdk resets vf.
> 
> Fixes: 7a93cd3575eb ("net/iavf: add VF reset check")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Kaiwen Deng 

Tested-by: Li, HongboX 

RE: [PATCH v7 1/4] hash: pack the hitmask for hash in bulk lookup

[Konstantin Ananyev]

Hi,

> Current hitmask includes padding due to Intel's SIMD
> implementation detail. This patch allows non Intel SIMD
> implementations to benefit from a dense hitmask.
> In addition, the new dense hitmask interweave the primary
> and secondary matches which allow a better cache usage and
> enable future improvements for the SIMD implementations
> 
> Signed-off-by: Yoan Picchi 
> Reviewed-by: Ruifeng Wang 
> Reviewed-by: Nathan Brown 
> ---
>  .mailmap  |   2 +
>  lib/hash/arch/arm/compare_signatures.h|  61 +++
>  lib/hash/arch/common/compare_signatures.h |  38 +
>  lib/hash/arch/x86/compare_signatures.h|  53 ++
>  lib/hash/rte_cuckoo_hash.c| 192 --
>  5 files changed, 255 insertions(+), 91 deletions(-)
>  create mode 100644 lib/hash/arch/arm/compare_signatures.h
>  create mode 100644 lib/hash/arch/common/compare_signatures.h
>  create mode 100644 lib/hash/arch/x86/compare_signatures.h
> 
> diff --git a/.mailmap b/.mailmap
> index 66ebc20666..00b50414d3 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -494,6 +494,7 @@ Hari Kumar Vemula 
>  Harini Ramakrishnan 
>  Hariprasad Govindharajan 
>  Harish Patil  
> +Harjot Singh 
>  Harman Kalra 
>  Harneet Singh 
>  Harold Huang 
> @@ -1633,6 +1634,7 @@ Yixue Wang 
>  Yi Yang  
>  Yi Zhang 
>  Yoann Desmouceaux 
> +Yoan Picchi 
>  Yogesh Jangra 
>  Yogev Chaimovich 
>  Yongjie Gu 
> diff --git a/lib/hash/arch/arm/compare_signatures.h 
> b/lib/hash/arch/arm/compare_signatures.h
> new file mode 100644
> index 00..1af6ba8190
> --- /dev/null
> +++ b/lib/hash/arch/arm/compare_signatures.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2016 Intel Corporation
> + * Copyright(c) 2018-2024 Arm Limited
> + */
> +
> +/*
> + * Arm's version uses a densely packed hitmask buffer:
> + * Every bit is in use.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include "rte_cuckoo_hash.h"
> +
> +#define DENSE_HASH_BULK_LOOKUP 1
> +
> +static inline void
> +compare_signatures_dense(uint16_t *hitmask_buffer,
> + const uint16_t *prim_bucket_sigs,
> + const uint16_t *sec_bucket_sigs,
> + uint16_t sig,
> + enum rte_hash_sig_compare_function sig_cmp_fn)
> +{
> +
> + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8),
> + "The hitmask must be exactly wide enough to accept the whole hitmask if 
> it is dense");
> +
> + /* For match mask every bits indicates the match */
> + switch (sig_cmp_fn) {
> +#if RTE_HASH_BUCKET_ENTRIES <= 8
> + case RTE_HASH_COMPARE_NEON: {
> + uint16x8_t vmat, vsig, x;
> + int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7};
> + uint16_t low, high;
> +
> + vsig = vld1q_dup_u16((uint16_t const *)&sig);
> + /* Compare all signatures in the primary bucket */
> + vmat = vceqq_u16(vsig,
> + vld1q_u16((uint16_t const *)prim_bucket_sigs));
> + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
> + low = (uint16_t)(vaddvq_u16(x));
> + /* Compare all signatures in the secondary bucket */
> + vmat = vceqq_u16(vsig,
> + vld1q_u16((uint16_t const *)sec_bucket_sigs));
> + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
> + high = (uint16_t)(vaddvq_u16(x));
> + *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES;
> +
> + }
> + break;
> +#endif
> + default:
> + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
> + *hitmask_buffer |=
> + ((sig == prim_bucket_sigs[i]) << i);
> + *hitmask_buffer |=
> + ((sig == sec_bucket_sigs[i]) << i) << 
> RTE_HASH_BUCKET_ENTRIES;
> + }
> + }
> +}
> diff --git a/lib/hash/arch/common/compare_signatures.h 
> b/lib/hash/arch/common/compare_signatures.h
> new file mode 100644
> index 00..dcf9444032
> --- /dev/null
> +++ b/lib/hash/arch/common/compare_signatures.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2016 Intel Corporation
> + * Copyright(c) 2018-2024 Arm Limited
> + */
> +
> +/*
> + * The generic version could use either a dense or sparsely packed hitmask 
> buffer,
> + * but the dense one is slightly faster.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include "rte_cuckoo_hash.h"
> +
> +#define DENSE_HASH_BULK_LOOKUP 1
> +
> +static inline void
> +compare_signatures_dense(uint16_t *hitmask_buffer,
> + const uint16_t *prim_bucket_sigs,
> + const uint16_t *sec_bucket_sigs,
> + uint16_t sig,
> + enum rte_hash_sig_compare_function sig_cmp_fn)
> +{
> + (void) sig_cmp_fn;
> +
> + s

Re: [PATCH] net/nfp: fix representor port release queue problem

[Ferruh Yigit]

On 3/19/2024 7:07 AM, Chaoyong He wrote:
> From: Long Wu 
> 
> The PF representor port's queue is different from the VF/physical
> representor port. So the release process in close port should
> be different too.
> 
> Fixes: 39b3951 ("net/nfp: fix resource leak for exit of flower firmware")
> Cc: chaoyong...@corigine.com
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Long Wu 
> Reviewed-by: Chaoyong He 
> Reviewed-by: Peng Zhang 
>

Hi Chaoyong,

Can you please clarify the impact of the issue?
As we are post -rc3, is this something we can consider for next release?

RE: [PATCH] net/nfp: fix representor port release queue problem

[Chaoyong He]

> On 3/19/2024 7:07 AM, Chaoyong He wrote:
> > From: Long Wu 
> >
> > The PF representor port's queue is different from the VF/physical
> > representor port. So the release process in close port should be
> > different too.
> >
> > Fixes: 39b3951 ("net/nfp: fix resource leak for exit of flower
> > firmware")
> > Cc: chaoyong...@corigine.com
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Long Wu 
> > Reviewed-by: Chaoyong He 
> > Reviewed-by: Peng Zhang 
> >
> 
> Hi Chaoyong,
> 
> Can you please clarify the impact of the issue?
> As we are post -rc3, is this something we can consider for next release?

Okay, we can delay it for next release.
Thanks.

[PATCH v2] net/mlx5: fix sync meter action

[Gregory Etelson]

PMD implements sync METER flow action as async.
Queue selected for sync operations is `MLX5_HW_INV_QUEUE`.
That dummy queue value is translated into `CTRL_QUEUE_ID(priv)`.
Async job allocation converts INV queue into the real value, but
job release does not.

This patch fixes the queue value provided to `flow_hw_job_put()`.

This patch also removes dead code found in METER_MARK
destroy handler.

Coverity issue: 415806
Coverity issue: 415804

Fixes: 4359d9d1f76b ("net/mlx5: fix sync meter processing in HWS")

Signed-off-by: Gregory Etelson 
Acked-by: Dariusz Sosnowski 
---
v2: Fixed Coverity tag.
---
 drivers/net/mlx5/mlx5_flow_hw.c| 5 +
 drivers/net/mlx5/mlx5_flow_meter.c | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
index 35f1ed7a03..9ebbe664d1 100644
--- a/drivers/net/mlx5/mlx5_flow_hw.c
+++ b/drivers/net/mlx5/mlx5_flow_hw.c
@@ -11494,10 +11494,7 @@ flow_hw_action_handle_destroy(struct rte_eth_dev *dev, 
uint32_t queue,
NULL, "Unable to wait for ASO meter CQE");
break;
}
-   if (!job)
-   mlx5_ipool_free(pool->idx_pool, idx);
-   else
-   aso = true;
+   aso = true;
break;
case MLX5_INDIRECT_ACTION_TYPE_RSS:
ret = flow_dv_action_destroy(dev, handle, error);
diff --git a/drivers/net/mlx5/mlx5_flow_meter.c 
b/drivers/net/mlx5/mlx5_flow_meter.c
index 4045c4c249..ca361f7efa 100644
--- a/drivers/net/mlx5/mlx5_flow_meter.c
+++ b/drivers/net/mlx5/mlx5_flow_meter.c
@@ -2265,7 +2265,7 @@ mlx5_flow_meter_hws_create(struct rte_eth_dev *dev, 
uint32_t meter_id,
ret = mlx5_aso_meter_update_by_wqe(priv, MLX5_HW_INV_QUEUE, aso_mtr,
   &priv->mtr_bulk, job, true);
if (ret) {
-   flow_hw_job_put(priv, job, MLX5_HW_INV_QUEUE);
+   flow_hw_job_put(priv, job, CTRL_QUEUE_ID(priv));
return -rte_mtr_error_set(error, ENOTSUP,
  RTE_MTR_ERROR_TYPE_UNSPECIFIED,
  NULL, "Failed to create devx meter.");
-- 
2.39.2

RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption

[Suanming Mou]

> -Original Message-
> From: Akhil Goyal 
> Sent: Tuesday, March 19, 2024 5:32 PM
> To: Suanming Mou ; Anoob Joseph
> ; ciara.po...@intel.com
> Cc: dev@dpdk.org
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
> 
> > > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP
> > decryption
> > >
> > > > > > +   if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > > > > +   (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > > > > +options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT)
> > &&
> > > > > > +   !options->out_of_place) {
> > > > > > +   RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > > > > throughput decryption.\n");
> > > > > > +   return -EINVAL;
> > > > > > +   }
> > > > >
> > > > > This check is blocking cipher_only decryption which should pass
> > > > > irrespective of inplace/oop and Data correct/incorrect.
> > > >
> > > > Sorry, in that case I will remove "options->cipher_op ==
> > > > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> > > > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?
> > >
> > > I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
> > > Instead of cipher_op.
> >
> > I'm not sure. Since in AEAD OP, auth_op will always be
> > RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be
> > rejected.
> > If the combination here is too complicated, what about just remove
> > that limits and let user to decide? If the input is not correct, PMD will 
> > reject it as
> well.
> 
> The problematic cases are where auth data (ICV) is not correct.
> i.e. AEAD, AUTH_ONLY and CIPHER_AUTH.
> 
> Hence following check should be ok.
> if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
>   (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
>   options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
>   !options->out_of_place) {

OK, that make sense. Will update, thanks.

> 
> Yes PMD will report error if the input data is not correct, but we cannot 
> just fail in
> that case just because the app is intentionally not filling the data.
> It should report unsupported case.
> >
> > >
> > > Ciara, What do you suggest? You were also seeing some issues in this 
> > > patch.

[PATCH v2] app/test-crypto-perf: add throughput OOP decryption

[Suanming Mou]

During throughput running, re-filling the test data will
impact the performance test result. So for now, to run
decrypt throughput testing is not supported since the
test data is not filled.

But if user requires OOP(out-of-place) mode, the test
data from source mbuf will never be modified, and if
the test data can be prepared out of the running loop,
the decryption test should be fine.

This commit adds the support of out-of-place decryption
testing for throughput.

[1]:
http://mails.dpdk.org/archives/dev/2023-July/273328.html

Signed-off-by: Suanming Mou 
---
 app/test-crypto-perf/cperf_ops.c |  5 ++-
 app/test-crypto-perf/cperf_options_parsing.c |  8 +
 app/test-crypto-perf/cperf_test_throughput.c | 34 +---
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
index d3fd115bc0..714616c697 100644
--- a/app/test-crypto-perf/cperf_ops.c
+++ b/app/test-crypto-perf/cperf_ops.c
@@ -644,7 +644,10 @@ cperf_set_ops_aead(struct rte_crypto_op **ops,
}
 
if ((options->test == CPERF_TEST_TYPE_VERIFY) ||
-   (options->test == CPERF_TEST_TYPE_LATENCY)) {
+   (options->test == CPERF_TEST_TYPE_LATENCY) ||
+   (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
+(options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
+ options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT))) {
for (i = 0; i < nb_ops; i++) {
uint8_t *iv_ptr = rte_crypto_op_ctod_offset(ops[i],
uint8_t *, iv_offset);
diff --git a/app/test-crypto-perf/cperf_options_parsing.c 
b/app/test-crypto-perf/cperf_options_parsing.c
index 8c20974273..90526e676f 100644
--- a/app/test-crypto-perf/cperf_options_parsing.c
+++ b/app/test-crypto-perf/cperf_options_parsing.c
@@ -1341,6 +1341,14 @@ cperf_options_check(struct cperf_options *options)
}
}
 
+   if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
+   (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
+options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
+   !options->out_of_place) {
+   RTE_LOG(ERR, USER1, "Only out-of-place is allowed in throughput 
decryption.\n");
+   return -EINVAL;
+   }
+
if (options->op_type == CPERF_CIPHER_ONLY ||
options->op_type == CPERF_CIPHER_THEN_AUTH ||
options->op_type == CPERF_AUTH_THEN_CIPHER) {
diff --git a/app/test-crypto-perf/cperf_test_throughput.c 
b/app/test-crypto-perf/cperf_test_throughput.c
index e3d266d7a4..b347baa913 100644
--- a/app/test-crypto-perf/cperf_test_throughput.c
+++ b/app/test-crypto-perf/cperf_test_throughput.c
@@ -99,6 +99,26 @@ cperf_throughput_test_constructor(struct rte_mempool 
*sess_mp,
return NULL;
 }
 
+static void
+cperf_verify_init_ops(struct rte_mempool *mp __rte_unused,
+ void *opaque_arg,
+ void *obj,
+ __rte_unused unsigned int i)
+{
+   uint16_t iv_offset = sizeof(struct rte_crypto_op) +
+   sizeof(struct rte_crypto_sym_op);
+   uint32_t imix_idx = 0;
+   struct cperf_throughput_ctx *ctx = opaque_arg;
+   struct rte_crypto_op *op = obj;
+
+   (ctx->populate_ops)(&op, ctx->src_buf_offset,
+   ctx->dst_buf_offset,
+   1, ctx->sess, ctx->options,
+   ctx->test_vector, iv_offset, &imix_idx, NULL);
+
+   cperf_mbuf_set(op->sym->m_src, ctx->options, ctx->test_vector);
+}
+
 int
 cperf_throughput_test_runner(void *test_ctx)
 {
@@ -144,6 +164,9 @@ cperf_throughput_test_runner(void *test_ctx)
uint16_t iv_offset = sizeof(struct rte_crypto_op) +
sizeof(struct rte_crypto_sym_op);
 
+   if (ctx->options->out_of_place)
+   rte_mempool_obj_iter(ctx->pool, cperf_verify_init_ops, (void 
*)ctx);
+
while (test_burst_size <= ctx->options->max_burst_size) {
uint64_t ops_enqd = 0, ops_enqd_total = 0, ops_enqd_failed = 0;
uint64_t ops_deqd = 0, ops_deqd_total = 0, ops_deqd_failed = 0;
@@ -176,11 +199,12 @@ cperf_throughput_test_runner(void *test_ctx)
}
 
/* Setup crypto op, attach mbuf etc */
-   (ctx->populate_ops)(ops, ctx->src_buf_offset,
-   ctx->dst_buf_offset,
-   ops_needed, ctx->sess,
-   ctx->options, ctx->test_vector,
-   iv_offset, &imix_idx, &tsc_start);
+   if (!ctx->options->out_of_place)
+   (ctx->populate_ops)(ops, ctx->src_buf_offset,
+   ctx->dst_buf_offset,
+   ops_needed, ctx->sess

Re: [dpdk-dev] [v6] doc: define qualification criteria for external library

[Ferruh Yigit]

On 1/9/2024 2:10 PM, jer...@marvell.com wrote:
> From: Jerin Jacob 
> 
> Define qualification criteria for external library
> based on a techboard meeting minutes [1] and past
> learnings from mailing list discussion.
> 
> [1]
> http://mails.dpdk.org/archives/dev/2019-June/135847.html
> https://mails.dpdk.org/archives/dev/2024-January/284849.html
> 
> Signed-off-by: Jerin Jacob 
> Acked-by: Thomas Monjalon 
> ---
> 
> v6:
> - Address Morten's comments at 
> https://mails.dpdk.org/archives/dev/2024-January/285029.html
> 
> v5:
> - Added "Dependency nature" section based on Stephen's input
> 
> v4:
> - Address Thomas comments from 
> https://patches.dpdk.org/project/dpdk/patch/20240105121215.3950532-1-jer...@marvell.com/
> 
> v3:
> - Updated the content based on TB discussion which is documented at
> https://mails.dpdk.org/archives/dev/2024-January/284849.html
> 
> v2:
> - Added "Meson build integration" and "Code readability" sections.
> 
>  doc/guides/contributing/index.rst |  1 +
>  .../contributing/library_dependency.rst   | 53 +++
>  2 files changed, 54 insertions(+)
>  create mode 100644 doc/guides/contributing/library_dependency.rst
> 
> diff --git a/doc/guides/contributing/index.rst 
> b/doc/guides/contributing/index.rst
> index dcb9b1fbf0..e5a8c2b0a3 100644
> --- a/doc/guides/contributing/index.rst
> +++ b/doc/guides/contributing/index.rst
> @@ -15,6 +15,7 @@ Contributor's Guidelines
>  documentation
>  unit_test
>  new_library
> +library_dependency
>  patches
>  vulnerability
>  stable
> diff --git a/doc/guides/contributing/library_dependency.rst 
> b/doc/guides/contributing/library_dependency.rst
> new file mode 100644
> index 00..3b275f1c52
> --- /dev/null
> +++ b/doc/guides/contributing/library_dependency.rst
> @@ -0,0 +1,53 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright(c) 2024 Marvell.
> +
> +External Library dependency
> +===
> +
> +This document defines the qualification criteria for external libraries that 
> may be
> +used as dependencies in DPDK drivers or libraries.
> +The final decision to accept or reject is at the discretion of the DPDK 
> Project's Technical Board.
> +
> +#. **Documentation:**
> +
> +   - Must have adequate documentation for the steps to build it.
> +   - Must have clear license documentation on distribution and usage aspects 
> of external library.
> +
> +#. **Free availability:**
> +
> +   - The library must be freely available to build in either source or 
> binary form.
>

As binary form can't be built, just for language can we drop "to build":
"The library must be freely available in either source or binary form."

> +   - It shall be downloadable from a direct link. There shall not be any 
> requirement to explicitly
> + login or sign a user agreement.
> +
> +#. **Usage License:**
> +
> +   - Both permissive (e.g., BSD-3 or Apache) and non-permissive (e.g., 
> GPLv3) licenses are acceptable.
>

Both above sample licenses are open source licenses, but as far as I can
see proprietary licenses are accepted.
Does it make sense to clarify it, like:
"Both open-source and proprietary licenses are acceptable."


I believe it is OK to have binary or proprietary dependencies for the
device support (drivers) code, but this may have consequences for
libraries, if specially a core library ends up having this kind of
dependency.
We don't have a guarantee that a proprietary licensed dependency won't
be stopped distributing or changing its license conditions, right?

Does it make sense to make this distinction, as driver and library code,
for binary or proprietary dependencies?
Or are we freely open to any kind of binary or proprietary dependency?


> +   - In the case of a permissive license, automatic inclusion in the build 
> process is assumed.
> + For non-permissive licenses, an additional build configuration option 
> is required.
> +
>

As this is about external dependency, what is about "inclusion in the
build process", in build system we just detect the availability of the
library, right? How it changes for different license type?
What kind of 'additional build configuration option' mentioned, can it
be possible to elaborate?

> +#. **Distribution License:**
> +
> +   - No specific constraints, but clear documentation on distribution usage 
> aspects is required.
> +
> +#. **Compiler compatibility:**
> +
> +   - The library must be able to compile with a DPDK supported compiler for 
> the given target
> + environment.
>

Item says 'must', but as there is an option to deliver as binary, this
requirement is only for source distribution, although this is kind of
obvious does it worth to mention it?

> + For example, for Linux, the library must be able to compile with GCC 
> and/or clang.
> +   - Library may be limited to a specific OS and/or specific hardware.
> +
> +#. **Meson build integration:**
> +
> +   - The library must have standard method l

Re: [PATCH] net/nfp: fix uninitialized variable

[Ferruh Yigit]

On 3/19/2024 8:55 AM, Chaoyong He wrote:
> CI found in the logic of 'nfp_aesgcm_iv_update()', the variable
> 'cfg_iv' may used uninitialized in some case.
> 
> Coverity issue: 415808
> Fixes: 36361ca7fea2 ("net/nfp: fix data endianness problem")
> Cc: shihong.w...@corigine.com
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Chaoyong He 
> Reviewed-by: Long Wu 
> Reviewed-by: Peng Zhang 
>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH v1 1/1] iavf: document limitation on MTU

[Bruce Richardson]

On Wed, Mar 13, 2024 at 03:43:35PM +, Anatoly Burakov wrote:
> When configuring a port, the configured MTU will
> not include VLAN tag size, but the physical
> function driver will add it automatically if the
> port has VLAN filtering configured, which may
> result in seemingly valid MTU to be rejected by
> the PF.
> 
> Document the limitation.
> 
> Signed-off-by: Anatoly Burakov 

Acked-by: Bruce Richardson 

RE: [RFC v5 1/6] eal: add static per-lcore memory allocation facility

[Konstantin Ananyev]

Hi Mattias,
> Introduce DPDK per-lcore id variables, or lcore variables for short.
> 
> An lcore variable has one value for every current and future lcore
> id-equipped thread.
> 
> The primary  use case is for statically allocating
> small chunks of often-used data, which is related logically, but where
> there are performance benefits to reap from having updates being local
> to an lcore.
> 
> Lcore variables are similar to thread-local storage (TLS, e.g., C11
> _Thread_local), but decoupling the values' life time with that of the
> threads.
> 
> Lcore variables are also similar in terms of functionality provided by
> FreeBSD kernel's DPCPU_*() family of macros and the associated
> build-time machinery. DPCPU uses linker scripts, which effectively
> prevents the reuse of its, otherwise seemingly viable, approach.
> 
> The currently-prevailing way to solve the same problem as lcore
> variables is to keep a module's per-lcore data as RTE_MAX_LCORE-sized
> array of cache-aligned, RTE_CACHE_GUARDed structs. The benefit of
> lcore variables over this approach is that data related to the same
> lcore now is close (spatially, in memory), rather than data used by
> the same module, which in turn avoid excessive use of padding,
> polluting caches with unused data.

Thanks for the RFC, very interesting one.
Few comments/questions below. 

 
> RFC v5:
>  * In Doxygen, consistenly use @ (and not \).
>  * The RTE_LCORE_VAR_GET() and SET() convience access macros
>covered an uncommon use case, where the lcore value is of a
>primitive type, rather than a struct, and is thus eliminated
>from the API. (Morten Brørup)
>  * In the wake up GET()/SET() removeal, rename RTE_LCORE_VAR_PTR()
>RTE_LCORE_VAR_VALUE().
>  * The underscores are removed from __rte_lcore_var_lcore_ptr() to
>signal that this function is a part of the public API.
>  * Macro arguments are documented.
> 
> RFV v4:
>  * Replace large static array with libc heap-allocated memory. One
>implication of this change is there no longer exists a fixed upper
>bound for the total amount of memory used by lcore variables.
>RTE_MAX_LCORE_VAR has changed meaning, and now represent the
>maximum size of any individual lcore variable value.
>  * Fix issues in example. (Morten Brørup)
>  * Improve access macro type checking. (Morten Brørup)
>  * Refer to the lcore variable handle as "handle" and not "name" in
>various macros.
>  * Document lack of thread safety in rte_lcore_var_alloc().
>  * Provide API-level assurance the lcore variable handle is
>always non-NULL, to all applications to use NULL to mean
>"not yet allocated".
>  * Note zero-sized allocations are not allowed.
>  * Give API-level guarantee the lcore variable values are zeroed.
> 
> RFC v3:
>  * Replace use of GCC-specific alignof() with alignof().
>  * Update example to reflect FOREACH macro name change (in RFC v2).
> 
> RFC v2:
>  * Use alignof to derive alignment requirements. (Morten Brørup)
>  * Change name of FOREACH to make it distinct from 's
>*per-EAL-thread* RTE_LCORE_FOREACH(). (Morten Brørup)
>  * Allow user-specified alignment, but limit max to cache line size.
> 
> Signed-off-by: Mattias Rönnblom 
> Acked-by: Morten Brørup 
> ---
>  config/rte_config.h   |   1 +
>  doc/api/doxy-api-index.md |   1 +
>  lib/eal/common/eal_common_lcore_var.c |  68 +
>  lib/eal/common/meson.build|   1 +
>  lib/eal/include/meson.build   |   1 +
>  lib/eal/include/rte_lcore_var.h   | 368 ++
>  lib/eal/version.map   |   4 +
>  7 files changed, 444 insertions(+)
>  create mode 100644 lib/eal/common/eal_common_lcore_var.c
>  create mode 100644 lib/eal/include/rte_lcore_var.h
> 
> diff --git a/config/rte_config.h b/config/rte_config.h
> index d743a5c3d3..0dac33d3b9 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -41,6 +41,7 @@
>  /* EAL defines */
>  #define RTE_CACHE_GUARD_LINES 1
>  #define RTE_MAX_HEAPS 32
> +#define RTE_MAX_LCORE_VAR 1048576
>  #define RTE_MAX_MEMSEG_LISTS 128
>  #define RTE_MAX_MEMSEG_PER_LIST 8192
>  #define RTE_MAX_MEM_MB_PER_LIST 32768
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index 8c1eb8fafa..a3b8391570 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -99,6 +99,7 @@ The public API headers are grouped by topics:
>[interrupts](@ref rte_interrupts.h),
>[launch](@ref rte_launch.h),
>[lcore](@ref rte_lcore.h),
> +  [lcore-varible](@ref rte_lcore_var.h),
>[per-lcore](@ref rte_per_lcore.h),
>[service cores](@ref rte_service.h),
>[keepalive](@ref rte_keepalive.h),
> diff --git a/lib/eal/common/eal_common_lcore_var.c 
> b/lib/eal/common/eal_common_lcore_var.c
> new file mode 100644
> index 00..5c353ebd46
> --- /dev/null
> +++ b/lib/eal/common/eal_common_lcore_var.c
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 20

Re: [PATCH v7 1/4] hash: pack the hitmask for hash in bulk lookup

[Yoan Picchi]

On 3/19/24 10:41, Konstantin Ananyev wrote:


Hi,


Current hitmask includes padding due to Intel's SIMD
implementation detail. This patch allows non Intel SIMD
implementations to benefit from a dense hitmask.
In addition, the new dense hitmask interweave the primary
and secondary matches which allow a better cache usage and
enable future improvements for the SIMD implementations

Signed-off-by: Yoan Picchi 
Reviewed-by: Ruifeng Wang 
Reviewed-by: Nathan Brown 
---
  .mailmap  |   2 +
  lib/hash/arch/arm/compare_signatures.h|  61 +++
  lib/hash/arch/common/compare_signatures.h |  38 +
  lib/hash/arch/x86/compare_signatures.h|  53 ++
  lib/hash/rte_cuckoo_hash.c| 192 --
  5 files changed, 255 insertions(+), 91 deletions(-)
  create mode 100644 lib/hash/arch/arm/compare_signatures.h
  create mode 100644 lib/hash/arch/common/compare_signatures.h
  create mode 100644 lib/hash/arch/x86/compare_signatures.h

diff --git a/.mailmap b/.mailmap
index 66ebc20666..00b50414d3 100644
--- a/.mailmap
+++ b/.mailmap
@@ -494,6 +494,7 @@ Hari Kumar Vemula 
  Harini Ramakrishnan 
  Hariprasad Govindharajan 
  Harish Patil  
+Harjot Singh 
  Harman Kalra 
  Harneet Singh 
  Harold Huang 
@@ -1633,6 +1634,7 @@ Yixue Wang 
  Yi Yang  
  Yi Zhang 
  Yoann Desmouceaux 
+Yoan Picchi 
  Yogesh Jangra 
  Yogev Chaimovich 
  Yongjie Gu 
diff --git a/lib/hash/arch/arm/compare_signatures.h 
b/lib/hash/arch/arm/compare_signatures.h
new file mode 100644
index 00..1af6ba8190
--- /dev/null
+++ b/lib/hash/arch/arm/compare_signatures.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2016 Intel Corporation
+ * Copyright(c) 2018-2024 Arm Limited
+ */
+
+/*
+ * Arm's version uses a densely packed hitmask buffer:
+ * Every bit is in use.
+ */
+
+#include 
+#include 
+#include 
+#include "rte_cuckoo_hash.h"
+
+#define DENSE_HASH_BULK_LOOKUP 1
+
+static inline void
+compare_signatures_dense(uint16_t *hitmask_buffer,
+   const uint16_t *prim_bucket_sigs,
+   const uint16_t *sec_bucket_sigs,
+   uint16_t sig,
+   enum rte_hash_sig_compare_function sig_cmp_fn)
+{
+
+   static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8),
+   "The hitmask must be exactly wide enough to accept the whole hitmask if it 
is dense");
+
+   /* For match mask every bits indicates the match */
+   switch (sig_cmp_fn) {
+#if RTE_HASH_BUCKET_ENTRIES <= 8
+   case RTE_HASH_COMPARE_NEON: {
+   uint16x8_t vmat, vsig, x;
+   int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7};
+   uint16_t low, high;
+
+   vsig = vld1q_dup_u16((uint16_t const *)&sig);
+   /* Compare all signatures in the primary bucket */
+   vmat = vceqq_u16(vsig,
+   vld1q_u16((uint16_t const *)prim_bucket_sigs));
+   x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
+   low = (uint16_t)(vaddvq_u16(x));
+   /* Compare all signatures in the secondary bucket */
+   vmat = vceqq_u16(vsig,
+   vld1q_u16((uint16_t const *)sec_bucket_sigs));
+   x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
+   high = (uint16_t)(vaddvq_u16(x));
+   *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES;
+
+   }
+   break;
+#endif
+   default:
+   for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
+   *hitmask_buffer |=
+   ((sig == prim_bucket_sigs[i]) << i);
+   *hitmask_buffer |=
+   ((sig == sec_bucket_sigs[i]) << i) << 
RTE_HASH_BUCKET_ENTRIES;
+   }
+   }
+}
diff --git a/lib/hash/arch/common/compare_signatures.h 
b/lib/hash/arch/common/compare_signatures.h
new file mode 100644
index 00..dcf9444032
--- /dev/null
+++ b/lib/hash/arch/common/compare_signatures.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2016 Intel Corporation
+ * Copyright(c) 2018-2024 Arm Limited
+ */
+
+/*
+ * The generic version could use either a dense or sparsely packed hitmask 
buffer,
+ * but the dense one is slightly faster.
+ */
+
+#include 
+#include 
+#include 
+#include "rte_cuckoo_hash.h"
+
+#define DENSE_HASH_BULK_LOOKUP 1
+
+static inline void
+compare_signatures_dense(uint16_t *hitmask_buffer,
+   const uint16_t *prim_bucket_sigs,
+   const uint16_t *sec_bucket_sigs,
+   uint16_t sig,
+   enum rte_hash_sig_compare_function sig_cmp_fn)
+{
+   (void) sig_cmp_fn;
+
+   static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8),
+   "The hitmask must be exactly wide enough to accept the whole hitma

RE: [PATCH v7 1/4] hash: pack the hitmask for hash in bulk lookup

[Konstantin Ananyev]

> >
> > Hi,
> >
> >> Current hitmask includes padding due to Intel's SIMD
> >> implementation detail. This patch allows non Intel SIMD
> >> implementations to benefit from a dense hitmask.
> >> In addition, the new dense hitmask interweave the primary
> >> and secondary matches which allow a better cache usage and
> >> enable future improvements for the SIMD implementations
> >>
> >> Signed-off-by: Yoan Picchi 
> >> Reviewed-by: Ruifeng Wang 
> >> Reviewed-by: Nathan Brown 
> >> ---
> >>   .mailmap  |   2 +
> >>   lib/hash/arch/arm/compare_signatures.h|  61 +++
> >>   lib/hash/arch/common/compare_signatures.h |  38 +
> >>   lib/hash/arch/x86/compare_signatures.h|  53 ++
> >>   lib/hash/rte_cuckoo_hash.c| 192 --
> >>   5 files changed, 255 insertions(+), 91 deletions(-)
> >>   create mode 100644 lib/hash/arch/arm/compare_signatures.h
> >>   create mode 100644 lib/hash/arch/common/compare_signatures.h
> >>   create mode 100644 lib/hash/arch/x86/compare_signatures.h
> >>
> >> diff --git a/.mailmap b/.mailmap
> >> index 66ebc20666..00b50414d3 100644
> >> --- a/.mailmap
> >> +++ b/.mailmap
> >> @@ -494,6 +494,7 @@ Hari Kumar Vemula 
> >>   Harini Ramakrishnan 
> >>   Hariprasad Govindharajan 
> >>   Harish Patil  
> >> +Harjot Singh 
> >>   Harman Kalra 
> >>   Harneet Singh 
> >>   Harold Huang 
> >> @@ -1633,6 +1634,7 @@ Yixue Wang 
> >>   Yi Yang  
> >>   Yi Zhang 
> >>   Yoann Desmouceaux 
> >> +Yoan Picchi 
> >>   Yogesh Jangra 
> >>   Yogev Chaimovich 
> >>   Yongjie Gu 
> >> diff --git a/lib/hash/arch/arm/compare_signatures.h 
> >> b/lib/hash/arch/arm/compare_signatures.h
> >> new file mode 100644
> >> index 00..1af6ba8190
> >> --- /dev/null
> >> +++ b/lib/hash/arch/arm/compare_signatures.h
> >> @@ -0,0 +1,61 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2010-2016 Intel Corporation
> >> + * Copyright(c) 2018-2024 Arm Limited
> >> + */
> >> +
> >> +/*
> >> + * Arm's version uses a densely packed hitmask buffer:
> >> + * Every bit is in use.
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include "rte_cuckoo_hash.h"
> >> +
> >> +#define DENSE_HASH_BULK_LOOKUP 1
> >> +
> >> +static inline void
> >> +compare_signatures_dense(uint16_t *hitmask_buffer,
> >> +  const uint16_t *prim_bucket_sigs,
> >> +  const uint16_t *sec_bucket_sigs,
> >> +  uint16_t sig,
> >> +  enum rte_hash_sig_compare_function sig_cmp_fn)
> >> +{
> >> +
> >> +  static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8),
> >> +  "The hitmask must be exactly wide enough to accept the whole hitmask if 
> >> it is dense");
> >> +
> >> +  /* For match mask every bits indicates the match */
> >> +  switch (sig_cmp_fn) {
> >> +#if RTE_HASH_BUCKET_ENTRIES <= 8
> >> +  case RTE_HASH_COMPARE_NEON: {
> >> +  uint16x8_t vmat, vsig, x;
> >> +  int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7};
> >> +  uint16_t low, high;
> >> +
> >> +  vsig = vld1q_dup_u16((uint16_t const *)&sig);
> >> +  /* Compare all signatures in the primary bucket */
> >> +  vmat = vceqq_u16(vsig,
> >> +  vld1q_u16((uint16_t const *)prim_bucket_sigs));
> >> +  x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
> >> +  low = (uint16_t)(vaddvq_u16(x));
> >> +  /* Compare all signatures in the secondary bucket */
> >> +  vmat = vceqq_u16(vsig,
> >> +  vld1q_u16((uint16_t const *)sec_bucket_sigs));
> >> +  x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
> >> +  high = (uint16_t)(vaddvq_u16(x));
> >> +  *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES;
> >> +
> >> +  }
> >> +  break;
> >> +#endif
> >> +  default:
> >> +  for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
> >> +  *hitmask_buffer |=
> >> +  ((sig == prim_bucket_sigs[i]) << i);
> >> +  *hitmask_buffer |=
> >> +  ((sig == sec_bucket_sigs[i]) << i) << 
> >> RTE_HASH_BUCKET_ENTRIES;
> >> +  }
> >> +  }
> >> +}
> >> diff --git a/lib/hash/arch/common/compare_signatures.h 
> >> b/lib/hash/arch/common/compare_signatures.h
> >> new file mode 100644
> >> index 00..dcf9444032
> >> --- /dev/null
> >> +++ b/lib/hash/arch/common/compare_signatures.h
> >> @@ -0,0 +1,38 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2010-2016 Intel Corporation
> >> + * Copyright(c) 2018-2024 Arm Limited
> >> + */
> >> +
> >> +/*
> >> + * The generic version could use either a dense or sparsely packed 
> >> hitmask buffer,
> >> + * but the dense one is slightly faster.
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include "rte_cuckoo_hash.h"
> >> +
> >> +#define DENSE_HASH_BULK_LOOKUP 1
> >> +
> >> +sta

[PATCH v3] net/netvsc: fix number Tx queues > Rx queues

[Alan Elder]

The previous code allowed the number of Tx queues to be set higher than
the number of Rx queues.  If a packet was sent on a Tx queue with index
>= number Rx queues there was a segfault.

This commit fixes the issue by creating an Rx queue for every Tx queue
meaning that an event buffer is allocated to handle receiving Tx
completion messages.

mbuf pool and Rx ring are not allocated for these additional Rx queues
and RSS configuration ensures that no packets are received on them.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthem...@microsoft.com
Cc: sta...@dpdk.org

Signed-off-by: Alan Elder 
---
v3:
* Handle case of Rx queue creation failure in hn_dev_tx_queue_setup.
* Re-use rx queue if it has already been allocated.
* Don't allocate an mbuf if pool is NULL.  This avoids segfault if RSS
  configuration is incorrect.

v2:
* Remove function declaration for static non-member function

---
 drivers/net/netvsc/hn_ethdev.c |  9 +
 drivers/net/netvsc/hn_rxtx.c   | 70 +-
 2 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/net/netvsc/hn_ethdev.c b/drivers/net/netvsc/hn_ethdev.c
index b8a32832d7..d7e3f12346 100644
--- a/drivers/net/netvsc/hn_ethdev.c
+++ b/drivers/net/netvsc/hn_ethdev.c
@@ -313,6 +313,15 @@ static int hn_rss_reta_update(struct rte_eth_dev *dev,
 
if (reta_conf[idx].mask & mask)
hv->rss_ind[i] = reta_conf[idx].reta[shift];
+
+   /*
+* Ensure we don't allow config that directs traffic to an Rx
+* queue that we aren't going to poll
+*/
+   if (hv->rss_ind[i] >=  dev->data->nb_rx_queues) {
+   PMD_DRV_LOG(ERR, "RSS distributing traffic to invalid 
Rx queue");
+   return -EINVAL;
+   }
}
 
err = hn_rndis_conf_rss(hv, NDIS_RSS_FLAG_DISABLE);
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index 9bf1ec5509..e23880c176 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -234,6 +234,17 @@ static void hn_reset_txagg(struct hn_tx_queue *txq)
txq->agg_prevpkt = NULL;
 }
 
+static void
+hn_rx_queue_free_common(struct hn_rx_queue *rxq)
+{
+   if (!rxq)
+   return;
+
+   rte_free(rxq->rxbuf_info);
+   rte_free(rxq->event_buf);
+   rte_free(rxq);
+}
+
 int
 hn_dev_tx_queue_setup(struct rte_eth_dev *dev,
  uint16_t queue_idx, uint16_t nb_desc,
@@ -243,6 +254,7 @@ hn_dev_tx_queue_setup(struct rte_eth_dev *dev,
 {
struct hn_data *hv = dev->data->dev_private;
struct hn_tx_queue *txq;
+   struct hn_rx_queue *rxq = NULL;
char name[RTE_MEMPOOL_NAMESIZE];
uint32_t tx_free_thresh;
int err = -ENOMEM;
@@ -301,6 +313,27 @@ hn_dev_tx_queue_setup(struct rte_eth_dev *dev,
goto error;
}
 
+   /*
+* If there are more Tx queues than Rx queues, allocate rx_queues
+* with event buffer so that Tx completion messages can still be
+* received
+*/
+   if (queue_idx >= dev->data->nb_rx_queues) {
+   rxq = hn_rx_queue_alloc(hv, queue_idx, socket_id);
+
+   if (!rxq) {
+   err = -ENOMEM;
+   goto error;
+   }
+
+   /*
+* Don't allocate mbuf pool or rx ring.  RSS is always 
configured
+* to ensure packets aren't received by this Rx queue.
+*/
+   rxq->mb_pool = NULL;
+   rxq->rx_ring = NULL;
+   }
+
txq->agg_szmax  = RTE_MIN(hv->chim_szmax, hv->rndis_agg_size);
txq->agg_pktmax = hv->rndis_agg_pkts;
txq->agg_align  = hv->rndis_agg_align;
@@ -311,12 +344,15 @@ hn_dev_tx_queue_setup(struct rte_eth_dev *dev,
 socket_id, tx_conf);
if (err == 0) {
dev->data->tx_queues[queue_idx] = txq;
+   if (rxq != NULL)
+   dev->data->rx_queues[queue_idx] = rxq;
return 0;
}
 
 error:
rte_mempool_free(txq->txdesc_pool);
rte_memzone_free(txq->tx_rndis_mz);
+   hn_rx_queue_free_common(rxq);
rte_free(txq);
return err;
 }
@@ -364,6 +400,13 @@ hn_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t 
qid)
if (!txq)
return;
 
+   /*
+* Free any Rx queues allocated for a Tx queue without a corresponding
+* Rx queue
+*/
+   if (qid >= dev->data->nb_rx_queues)
+   hn_rx_queue_free_common(dev->data->rx_queues[qid]);
+
rte_mempool_free(txq->txdesc_pool);
 
rte_memzone_free(txq->tx_rndis_mz);
@@ -552,10 +595,12 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct 
hn_rx_bufinfo *rxb,
 const struct hn_rxinfo *info)
 {
struct hn_data *hv = rxq->hv;
-   str

RE: [PATCH v2] net/netvsc: fix number Tx queues > Rx queues

[Alan Elder]

Thanks for the feedback Long.

I've made both changes you suggested, plus one additional one to not try and 
allocate an mbuf if the pool is null.

This means if a packet is received on a Rx queue that isn't being polled we 
will see it appear as "mbuf allocation failed" rather than causing a segfault.

Cheers,
Alan

> -Original Message-
> From: Long Li 
> Sent: Tuesday, March 12, 2024 7:09 PM
> To: Alan Elder ; Ferruh Yigit
> ; Andrew Rybchenko
> 
> Cc: dev@dpdk.org; stephen 
> Subject: RE: [PATCH v2] net/netvsc: fix number Tx queues > Rx queues
> 
> > a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c index
> > 9bf1ec5509..c0aaeaa972 100644
> > --- a/drivers/net/netvsc/hn_rxtx.c
> > +++ b/drivers/net/netvsc/hn_rxtx.c
> > @@ -243,6 +243,7 @@ hn_dev_tx_queue_setup(struct rte_eth_dev *dev,  {
> > struct hn_data *hv = dev->data->dev_private;
> > struct hn_tx_queue *txq;
> > +   struct hn_rx_queue *rxq;
> > char name[RTE_MEMPOOL_NAMESIZE];
> > uint32_t tx_free_thresh;
> > int err = -ENOMEM;
> > @@ -301,6 +302,22 @@ hn_dev_tx_queue_setup(struct rte_eth_dev *dev,
> > goto error;
> > }
> >
> > +   /*
> > +* If there are more Tx queues than Rx queues, allocate rx_queues
> > +* with event buffer so that Tx completion messages can still be
> > +* received
> > +*/
> > +   if (queue_idx >= dev->data->nb_rx_queues) {
> > +   rxq = hn_rx_queue_alloc(hv, queue_idx, socket_id);
> 
> Need to check if rxq is NULL.
> 
> > +   /*
> > +* Don't allocate mbuf pool or rx ring.  RSS is always 
> > configured
> > +* to ensure packets aren't received by this Rx queue.
> > +*/
> > +   rxq->mb_pool = NULL;
> > +   rxq->rx_ring = NULL;
> > +   dev->data->rx_queues[queue_idx] = rxq;
> > +   }
> > +
> > txq->agg_szmax  = RTE_MIN(hv->chim_szmax, hv->rndis_agg_size);
> > txq->agg_pktmax = hv->rndis_agg_pkts;
> > txq->agg_align  = hv->rndis_agg_align; @@ -354,6 +371,17 @@ static
> > void hn_txd_put(struct hn_tx_queue *txq, struct hn_txdesc *txd)
> > rte_mempool_put(txq->txdesc_pool, txd);  }
> >
> > +static void
> > +hn_rx_queue_free_common(struct hn_rx_queue *rxq) {
> > +   if (!rxq)
> > +   return;
> > +
> > +   rte_free(rxq->rxbuf_info);
> > +   rte_free(rxq->event_buf);
> > +   rte_free(rxq);
> > +}
> > +
> >  void
> >  hn_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t qid)  { @@
> > -364,6
> > +392,13 @@ hn_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t
> > +qid)
> > if (!txq)
> > return;
> >
> > +   /*
> > +* Free any Rx queues allocated for a Tx queue without a
> corresponding
> > +* Rx queue
> > +*/
> > +   if (qid >= dev->data->nb_rx_queues)
> > +   hn_rx_queue_free_common(dev->data->rx_queues[qid]);
> > +
> > rte_mempool_free(txq->txdesc_pool);
> >
> > rte_memzone_free(txq->tx_rndis_mz);
> > @@ -942,6 +977,13 @@ hn_dev_rx_queue_setup(struct rte_eth_dev *dev,
> > if (queue_idx == 0) {
> > rxq = hv->primary;
> > } else {
> > +   /*
> > +* If the number of Tx queues was previously greater than
> > +* the number of Rx queues, we may already have allocated
> > +* an rxq. If so, free it now before allocating a new one.
> > +*/
> > +   hn_rx_queue_free_common(dev->data-
> > >rx_queues[queue_idx]);
> 
> This logic seems strange. How about check if rxq is already allocated. If not,
> allocate it.
> 
> Something like:
> 
> if (!dev->data->rx_queues[queue_idx])
>   rxq = hn_rx_queue_alloc(hv, queue_idx, socket_id);
> 
> 
> 
> Thanks,
> 
> Long

Re: [PATCH] app/testpmd: fix releasing action handle flush memory

[Ferruh Yigit]

On 3/19/2024 9:32 AM, Bing Zhao wrote:
> The memory of the indirect action handles should be freed after
> being destroyed in the flush. The behavior needs to be consistent
> with the single handle destroy.
> 
> Or else, there will be some unexpected error when the action handle
> is destroyed for the 2nd time, for example, the port needs to be
> closed again.
> 

Ports can be closed only once, so above reasoning is not valid, but I
assume the purpose of this patch is to prevent memory leak, can you
please clarify the problem and impact of the patch in commit log?


Also what is "single handle destroy" mentioned above?

The fixed commit is from a few release ago, so this is not something
introduced in this release, do you think can this wait next release
instead of merging for -rc4 which is more risky?


> Fixes: f7352c176bbf ("app/testpmd: fix use of indirect action after port 
> close")
> Cc: dmitry.kozl...@gmail.com
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Bing Zhao 
> Reviewed-by: Dariusz Sosnowski 
> ---
>  app/test-pmd/config.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index ba1007ace6..f62ba90c87 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1918,8 +1918,7 @@ port_action_handle_flush(portid_t port_id)
>   /* Poisoning to make sure PMDs update it in case of error. */
>   memset(&error, 0x44, sizeof(error));
>   if (pia->handle != NULL) {
> - ret = pia->type ==
> -   RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
> + ret = pia->type == RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
> rte_flow_action_list_handle_destroy
> (port_id, pia->list_handle, &error) :
> rte_flow_action_handle_destroy
> @@ -1929,11 +1928,9 @@ port_action_handle_flush(portid_t port_id)
>  pia->id);
>   ret = port_flow_complain(&error);
>   }
> - tmp = &pia->next;
> - } else {
> - *tmp = pia->next;
> - free(pia);
>   }
> + *tmp = pia->next;
> + free(pia);
>   }
>   return ret;
>  }

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Ferruh Yigit]

On 3/18/2024 9:21 AM, Shani Peretz wrote:
> In the process of auto completion of a command in testpmd,
> the parser splits the command into tokens, where each token
> represents an argument and defines a parsing function.
> The parsing function of the indirect_list action argument was returning
> before having the opportunity to handle the argument.
> 

Hi Shani,

I can see a few other handles follows the updated logic, but to
understand more, was the problematic part following:
```
if (!action)
return -1;
```

If so why 'action' can be NULL and why need to continue for this case,
can you please help me understand?

Also even if 'action' is NULL, function will return output of
'parse_int()', is this expected?


Thanks,
ferruh

> The fix ensures that the function appropriately handles
> the argument before finishing.
> 
> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
> 
> Signed-off-by: Shani Peretz 
> ---
>  app/test-pmd/cmdline_flow.c | 46 -
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index fd6c51f72d..60ee9337cf 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -7839,11 +7839,13 @@ static const struct token token_list[] = {
>   .type = "UNSIGNED",
>   .help = "unsigned integer value",
>   .call = parse_indlst_id2ptr,
> + .comp = comp_none,
>   },
>   [INDIRECT_LIST_ACTION_ID2PTR_CONF] = {
>   .type = "UNSIGNED",
>   .help = "unsigned integer value",
>   .call = parse_indlst_id2ptr,
> + .comp = comp_none,
>   },
>   [ACTION_SHARED_INDIRECT] = {
>   .name = "shared_indirect",
> @@ -11912,34 +11914,36 @@ parse_indlst_id2ptr(struct context *ctx, const 
> struct token *token,
>   uint32_t id;
>   int ret;
>  
> - if (!action)
> - return -1;
>   ctx->objdata = 0;
>   ctx->object = &id;
>   ctx->objmask = NULL;
>   ret = parse_int(ctx, token, str, len, ctx->object, sizeof(id));
> + ctx->object = action;
>   if (ret != (int)len)
>   return ret;
> - ctx->object = action;
> - action_conf = (void *)(uintptr_t)action->conf;
> - action_conf->conf = NULL;
> - switch (ctx->curr) {
> - case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
> - action_conf->handle = (typeof(action_conf->handle))
> - port_action_handle_get_by_id(ctx->port, id);
> - if (!action_conf->handle) {
> - printf("no indirect list handle for id %u\n", id);
> - return -1;
> +
> + /* set handle and conf */
> + if (action) {
> + action_conf = (void *)(uintptr_t)action->conf;
> + action_conf->conf = NULL;
> + switch (ctx->curr) {
> + case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
> + action_conf->handle = (typeof(action_conf->handle))
> + port_action_handle_get_by_id(ctx->port, 
> id);
> + if (!action_conf->handle) {
> + printf("no indirect list handle for id %u\n", 
> id);
> + return -1;
> + }
> + break;
> + case INDIRECT_LIST_ACTION_ID2PTR_CONF:
> + indlst_conf = indirect_action_list_conf_get(id);
> + if (!indlst_conf)
> + return -1;
> + action_conf->conf = (const void **)indlst_conf->conf;
> + break;
> + default:
> + break;
>   }
> - break;
> - case INDIRECT_LIST_ACTION_ID2PTR_CONF:
> - indlst_conf = indirect_action_list_conf_get(id);
> - if (!indlst_conf)
> - return -1;
> - action_conf->conf = (const void **)indlst_conf->conf;
> - break;
> - default:
> - break;
>   }
>   return ret;
>  }

Re: [PATCH] net/igc: fix disabling timesync

[Bruce Richardson]

On Mon, Mar 18, 2024 at 04:11:14AM +, Liao, TingtingX wrote:
>Tested-by: Tingting Liao 
>  __
> 
>> -Original Message-
> 
>> From: Ma, WenwuX 
> 
>> Sent: Friday, March 15, 2024 09:06
> 
>> To: dev@dpdk.org ; Guo, Junfeng
>; Su, Simei 
> 
>> Cc: Liao, TingtingX ; Ma, WenwuX
>; sta...@dpdk.org 
> 
>> Subject: [PATCH] net/igc: fix disabling timesync
> 
>>
> 
>> When disabling timesync, we should clear the IGC_RXPBS_CFG_TS_EN bit
> 
>> of IGC_RXPBS, the patch fixes this.
> 
>>
> 
>> Fixes: 4f6fbbf6f17d ("net/igc: support IEEE 1588 PTP")
> 
>> Cc: sta...@dpdk.org
> 
>>
> 
>> Signed-off-by: Wenwu Ma 
>

This seems a small enough, low-risk fix, so applying to
dpdk-next-net-intel.

Thanks,
/Bruce

RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption

[Power, Ciara]

> -Original Message-
> From: Suanming Mou 
> Sent: Tuesday, March 19, 2024 11:46 AM
> To: gak...@marvell.com; Power, Ciara 
> Cc: dev@dpdk.org
> Subject: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
> 
> During throughput running, re-filling the test data will impact the 
> performance
> test result. So for now, to run decrypt throughput testing is not supported 
> since
> the test data is not filled.
> 
> But if user requires OOP(out-of-place) mode, the test data from source mbuf 
> will
> never be modified, and if the test data can be prepared out of the running 
> loop,
> the decryption test should be fine.
> 
> This commit adds the support of out-of-place decryption testing for 
> throughput.
> 
> [1]:
> http://mails.dpdk.org/archives/dev/2023-July/273328.html
> 
> Signed-off-by: Suanming Mou 
> ---
>  app/test-crypto-perf/cperf_ops.c |  5 ++-
>  app/test-crypto-perf/cperf_options_parsing.c |  8 +  app/test-crypto-
> perf/cperf_test_throughput.c | 34 +---
>  3 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-crypto-perf/cperf_ops.c 
> b/app/test-crypto-perf/cperf_ops.c
> index d3fd115bc0..714616c697 100644
> --- a/app/test-crypto-perf/cperf_ops.c
> +++ b/app/test-crypto-perf/cperf_ops.c
> @@ -644,7 +644,10 @@ cperf_set_ops_aead(struct rte_crypto_op **ops,
>   }
> 
>   if ((options->test == CPERF_TEST_TYPE_VERIFY) ||
> - (options->test == CPERF_TEST_TYPE_LATENCY)) {
> + (options->test == CPERF_TEST_TYPE_LATENCY) ||
> + (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> +  (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> +   options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT))) {
>   for (i = 0; i < nb_ops; i++) {
>   uint8_t *iv_ptr = rte_crypto_op_ctod_offset(ops[i],
>   uint8_t *, iv_offset);
> diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-
> perf/cperf_options_parsing.c
> index 8c20974273..90526e676f 100644
> --- a/app/test-crypto-perf/cperf_options_parsing.c
> +++ b/app/test-crypto-perf/cperf_options_parsing.c
> @@ -1341,6 +1341,14 @@ cperf_options_check(struct cperf_options
> *options)
>   }
>   }
> 
> + if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> +  options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
> + !options->out_of_place) {
> + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> throughput decryption.\n");
> + return -EINVAL;
> + }

Not totally following some of this, why do we only want to add this for OOP 
mode?

For example an inplace command I can use before this patch but not after:
./build/app/dpdk-test-crypto-perf -l 2,3 -- --ptest throughput --optype aead 
--aead-algo aes-gcm --aead-op decrypt --devtype crypto_qat --aead-key-sz 16

I get an error;
USER1: Only out-of-place is allowed in throughput decryption.
USER1: Checking one or more user options failed

Do we want to always force the user to use OOP + test vector file for these 
throughput decryption tests?
Or should we just add a warning that the throughput may not be reflecting the 
"success" verify path in PMD if using inplace and the dummy data.

I am not sure.
If we do want to add the limitation on the throughput tests, these changes I 
think are ok for that.

Thanks,
Ciara

> +
>   if (options->op_type == CPERF_CIPHER_ONLY ||
>   options->op_type == CPERF_CIPHER_THEN_AUTH ||
>   options->op_type == CPERF_AUTH_THEN_CIPHER) { diff
> --git a/app/test-crypto-perf/cperf_test_throughput.c b/app/test-crypto-
> perf/cperf_test_throughput.c
> index e3d266d7a4..b347baa913 100644
> --- a/app/test-crypto-perf/cperf_test_throughput.c
> +++ b/app/test-crypto-perf/cperf_test_throughput.c
> @@ -99,6 +99,26 @@ cperf_throughput_test_constructor(struct rte_mempool
> *sess_mp,
>   return NULL;
>  }
> 
> +static void
> +cperf_verify_init_ops(struct rte_mempool *mp __rte_unused,
> +   void *opaque_arg,
> +   void *obj,
> +   __rte_unused unsigned int i)
> +{
> + uint16_t iv_offset = sizeof(struct rte_crypto_op) +
> + sizeof(struct rte_crypto_sym_op);
> + uint32_t imix_idx = 0;
> + struct cperf_throughput_ctx *ctx = opaque_arg;
> + struct rte_crypto_op *op = obj;
> +
> + (ctx->populate_ops)(&op, ctx->src_buf_offset,
> + ctx->dst_buf_offset,
> + 1, ctx->sess, ctx->options,
> + ctx->test_vector, iv_offset, &imix_idx, NULL);
> +
> + cperf_mbuf_set(op->sym->m_src, ctx->options, ctx->test_vector); }
> +
>  int
>  cperf_throughput_test_runner(void *test_ctx)  { @@ -144,6 +164,9 @@
> cperf_throughput_test_runner(void *test_ctx)
>   uint16_t iv_offset = sizeof(struct rte_crypto_op) +
>

Re: [PATCH v1 1/1] iavf: document limitation on MTU

[Bruce Richardson]

On Tue, Mar 19, 2024 at 12:14:53PM +, Bruce Richardson wrote:
> On Wed, Mar 13, 2024 at 03:43:35PM +, Anatoly Burakov wrote:
> > When configuring a port, the configured MTU will
> > not include VLAN tag size, but the physical
> > function driver will add it automatically if the
> > port has VLAN filtering configured, which may
> > result in seemingly valid MTU to be rejected by
> > the PF.
> > 
> > Document the limitation.
> > 
> > Signed-off-by: Anatoly Burakov 
> 
> Acked-by: Bruce Richardson 

Applied to dpdk-next-net-intel

Thanks,
/Bruce

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Ferruh Yigit]

On 3/19/2024 2:51 PM, Ferruh Yigit wrote:
> On 3/18/2024 9:21 AM, Shani Peretz wrote:
>> In the process of auto completion of a command in testpmd,
>> the parser splits the command into tokens, where each token
>> represents an argument and defines a parsing function.
>> The parsing function of the indirect_list action argument was returning
>> before having the opportunity to handle the argument.
>>
> Hi Shani,
> 
> I can see a few other handles follows the updated logic, but to
> understand more, was the problematic part following:
> ```
>   if (!action)
>   return -1;
> ```
> 
> If so why 'action' can be NULL and why need to continue for this case,
> can you please help me understand?
> 
> Also even if 'action' is NULL, function will return output of
> 'parse_int()', is this expected?
> 

I can verify the fix via debugging,

it seems missing ".comp = comp_none" cause calling handler
(parse_indlst_id2ptr), and 'parse_indlst_id2ptr()' needs to be fixed to
parse correctly.

I will proceed with patch since it is local to a specific flow command,


BUT overall how can we catch issues like this in the feature, we don't
have a good way to test testpmd flow commands.
@Ori, @Gregory, do you have any idea?
cc'ed CI mail list too.

Re: [PATCH] doc: deprecate graph data structures

[Stephen Hemminger]

On Tue, 19 Mar 2024 08:52:13 +0530
Jerin Jacob  wrote:

> On Wed, Feb 21, 2024 at 9:43 PM  wrote:
> >
> > From: Pavan Nikhilesh 
> >
> > Deprecate rte_node, rte_node_register and rte_graph_cluster_node_stats
> > structures as will be extended to include node specific error counters
> > and error description.
> >
> > Signed-off-by: Pavan Nikhilesh   
> 
> Implementation patches for 24.11 at
> https://patches.dpdk.org/project/dpdk/list/?series=31181.
> The deprecation notice looks good to me.
> 
> Acked-by: Jerin Jacob 

Can more of the internals be private to the library to avoid future changes.

Re: [PATCH v9 5/5] eal: add option to put timestamp on console output

[Stephen Hemminger]

On Tue, 19 Mar 2024 08:37:30 +0100
Morten Brørup  wrote:

> >  static ssize_t
> >  console_log_write(__rte_unused void *c, const char *buf, size_t size)
> >  {
> > +   struct timespec ts;
> > ssize_t ret;
> > 
> > -   /* write on stderr */
> > -   ret = fwrite(buf, 1, size, stderr);
> > +   if (timestamp_enabled) {
> > +   clock_gettime(CLOCK_MONOTONIC, &ts);
> > +   ts.tv_sec -= log_started.tv_sec;
> > +   ts.tv_nsec -= log_started.tv_nsec;  
> 
> Please log the absolute CLOCK_MONOTONIC instead of subtracting log_started, 
> so timestamps can be easily compared with timestamps from other processes.


No, was trying to do what kernel dmesg does.

Re: [PATCH v7 1/4] hash: pack the hitmask for hash in bulk lookup

[Stephen Hemminger]

On Tue, 12 Mar 2024 15:42:12 +
Yoan Picchi  wrote:

> + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8),

Space around math operations please.

RE: [PATCH v9 5/5] eal: add option to put timestamp on console output

[Morten Brørup]

> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Tuesday, 19 March 2024 16.52
> 
> On Tue, 19 Mar 2024 08:37:30 +0100
> Morten Brørup  wrote:
> 
> > >  static ssize_t
> > >  console_log_write(__rte_unused void *c, const char *buf, size_t
> size)
> > >  {
> > > + struct timespec ts;
> > >   ssize_t ret;
> > >
> > > - /* write on stderr */
> > > - ret = fwrite(buf, 1, size, stderr);
> > > + if (timestamp_enabled) {
> > > + clock_gettime(CLOCK_MONOTONIC, &ts);
> > > + ts.tv_sec -= log_started.tv_sec;
> > > + ts.tv_nsec -= log_started.tv_nsec;
> >
> > Please log the absolute CLOCK_MONOTONIC instead of subtracting
> log_started, so timestamps can be easily compared with timestamps from
> other processes.
> 
> 
> No, was trying to do what kernel dmesg does.

What do you mean? Doesn't the kernel output CLOCK_MONOTONIC timestamps (without 
offset)?

And by "timestamps from other processes" I also mean timestamps in log messages 
from the kernel itself.

[PATCH] net/ixgbe: using dpdk-dumpcap capture packet coredump

[Jun Wang]

Signed-off-by: Jun Wang 
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 73 
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index c61c52b..0e624f5 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4313,49 +4313,50 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused 
struct rte_eth_dev *dev,
 #ifdef RTE_EXEC_ENV_FREEBSD
wait = 1;
 #endif
+   if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+   if (vf)
+   diag = ixgbevf_check_link(hw, &link_speed, &link_up, 
wait);
+   else
+   diag = ixgbe_check_link(hw, &link_speed, &link_up, 
wait);
 
-   if (vf)
-   diag = ixgbevf_check_link(hw, &link_speed, &link_up, wait);
-   else
-   diag = ixgbe_check_link(hw, &link_speed, &link_up, wait);
+   if (diag != 0) {
+   link.link_speed = RTE_ETH_SPEED_NUM_100M;
+   link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
+   return rte_eth_linkstatus_set(dev, &link);
+   }
 
-   if (diag != 0) {
-   link.link_speed = RTE_ETH_SPEED_NUM_100M;
-   link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
-   return rte_eth_linkstatus_set(dev, &link);
-   }
+   if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber &&
+   !ad->sdp3_no_tx_disable) {
+   esdp_reg = IXGBE_READ_REG(hw, IXGBE_ESDP);
+   if ((esdp_reg & IXGBE_ESDP_SDP3))
+   link_up = 0;
+   }
 
-   if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber &&
-   !ad->sdp3_no_tx_disable) {
-   esdp_reg = IXGBE_READ_REG(hw, IXGBE_ESDP);
-   if ((esdp_reg & IXGBE_ESDP_SDP3))
-   link_up = 0;
-   }
-
-   if (link_up == 0) {
-   if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) {
-   ixgbe_dev_wait_setup_link_complete(dev, 0);
-   /* NOTE: review for potential ordering optimization */
-   if (!__atomic_test_and_set(&ad->link_thread_running, 
__ATOMIC_SEQ_CST)) {
-   /* To avoid race condition between threads, set
-* the IXGBE_FLAG_NEED_LINK_CONFIG flag only
-* when there is no link thread running.
-*/
-   intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
-   if 
(rte_thread_create_internal_control(&ad->link_thread_tid,
-   "ixgbe-link",
-   
ixgbe_dev_setup_link_thread_handler, dev) < 0) {
+   if (link_up == 0) {
+   if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) 
{
+   ixgbe_dev_wait_setup_link_complete(dev, 0);
+   /* NOTE: review for potential ordering 
optimization */
+   if 
(!__atomic_test_and_set(&ad->link_thread_running, __ATOMIC_SEQ_CST)) {
+   /* To avoid race condition between 
threads, set
+   * the IXGBE_FLAG_NEED_LINK_CONFIG flag 
only
+   * when there is no link thread running.
+   */
+   intr->flags |= 
IXGBE_FLAG_NEED_LINK_CONFIG;
+   if 
(rte_thread_create_internal_control(&ad->link_thread_tid,
+   "ixgbe-link",
+   
ixgbe_dev_setup_link_thread_handler, dev) < 0) {
+   PMD_DRV_LOG(ERR,
+   "Create link thread 
failed!");
+   /* NOTE: review for potential 
ordering optimization */
+   
__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
+   }
+   } else {
PMD_DRV_LOG(ERR,
-   "Create link thread failed!");
-   /* NOTE: review for potential ordering 
optimization */
-   
__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
+   "Other link thread is running 
now!");
}
-   } else {
-   PMD_DRV_LOG(ERR,
- 

Re: fib{,6}: questions and proposals

[Medvedkin, Vladimir]

Hi Robin,

On 19/03/2024 08:30, Robin Jarry wrote:

Hi Vladimir,

I have been using rte_fib for a while and stumbled upon a few quirks. 
I was wondering if you would answer some questions:


1) Is it OK/safe to share the same fib to perform route lookups from   
multiple lcores in parallel? So far my observations seem to validate   
that assumption but I would like your opinion :)

Yes, 100% :)


2) Is it OK/safe to modify a fib from a control thread (read/write)   
while it is used by data path threads (read only)?


This part is a bit more complicated. In practice, I would say yes, 
however, there is a possibility that if the lookup thread is preempted 
in the middle of the lookup process, and at the same time the control 
thread deletes the corresponding route, then the lookup result may 
return outdated data. This problem is solved in LPM with RCU enabled. I 
have plans to implement it in the near future in the FIB.




3) There is no public API to list/walk all configured routes in a fib. 
  Would that be possible/easy to implement?


Yes, it already there. FIB under the hood uses rte_rib to hold existing 
routes. So walking through can be implemented like:


struct rte_fib fib;



struct rte_rib rib = rte_fib_get_rib(fib);

struct rte_rib_node *cur = NULL;

do {

cur = rte_rib_get_nxt(rib, RTE_IPV4(0,0,0,0) /*this is supernet where 
you'd like to iterate*/, 0 /*and this is depth*/, cur, RTE_RIB_GET_NXT_ALL);


if (cur)

    printf...

} while (cur)




4) In rte_fib, every IPv4 address (route *and* next hop) needs to be 
in   host order. This is not consistent with fib6 where addresses are 
  stored in network order. It took me quite a while to figure out what 
  was wrong with my code.


  I assume this is because DIR24 needs host order integers and not   
TRIE. Why was this not hidden in the API?


  Could we add a flag to rte_fib_conf to change the behaviour? This   
would avoid error prone ntohl/htonl juggling.


This API behavior was created in such a way that it is the same as LPM.

As for LPM, I think it was done this way for performance reasons because 
in some scenarios you only working with the host order ipv4 addresses.




Thanks in advance for your replies :)


--
Regards,
Vladimir

Re: Email based retest request process: proposal for new pull/re-apply feature

[Patrick Robb]

On Tue, Mar 19, 2024 at 4:37 AM zhoumin  wrote:
>
>
> One more thing I want to confirm is whether we should apply the patch
> onto the branch commit which existed at the time when that patch was
> submitted or onto the latest tip of branch if users request doing
> rebase. Users probably request a recheck with `rebase` when the CI lab
> chose a wrong branch onto which apply the patch. I worry we may
> encounter conflicts when apply the patch onto the latest commit of the
> target branch if that branch is just updated before the request.
>
>

That's a good edge case to think about...  but I also think if the
patch no longer applies cleanly on tip of intended branch, then we
would be correct to report an apply failure there. And then the
submitter should refactor their patch so it applies, and submit again.

So I think the process is like

A) If retest is requested without rebase key, then retest "original"
dpdk artifact (either by re-using the existing tarball (unh lab) or
tracking the commit from submit time and re-applying onto dpdk at that
commit (loongson)).

B) If rebase key is included, apply to tip of the indicated branch.
If, because the branch has changed, the patch no longer applies, then
we can report an apply failure. Then, submitter has to refactor their
patch and resubmit.

In either case, report the new results with an updated test result in
the email (i.e. report "_Testing PASS RETEST #1" instead of "_Testing
PASS" in the email body).

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Ferruh Yigit]

On 3/19/2024 3:29 PM, Ferruh Yigit wrote:
> On 3/19/2024 2:51 PM, Ferruh Yigit wrote:
>> On 3/18/2024 9:21 AM, Shani Peretz wrote:
>>> In the process of auto completion of a command in testpmd,
>>> the parser splits the command into tokens, where each token
>>> represents an argument and defines a parsing function.
>>> The parsing function of the indirect_list action argument was returning
>>> before having the opportunity to handle the argument.
>>>
>> Hi Shani,
>>
>> I can see a few other handles follows the updated logic, but to
>> understand more, was the problematic part following:
>> ```
>>  if (!action)
>>  return -1;
>> ```
>>
>> If so why 'action' can be NULL and why need to continue for this case,
>> can you please help me understand?
>>
>> Also even if 'action' is NULL, function will return output of
>> 'parse_int()', is this expected?
>>
> 
> I can verify the fix via debugging,
> 
> it seems missing ".comp = comp_none" cause calling handler
> (parse_indlst_id2ptr), and 'parse_indlst_id2ptr()' needs to be fixed to
> parse correctly.
> 
> I will proceed with patch since it is local to a specific flow command,
> 

Tested-by: Ferruh Yigit 

Applied to dpdk-next-net/main, thanks.

Re: Email based retest request process: proposal for new pull/re-apply feature

[Aaron Conole]

Patrick Robb  writes:

> On Tue, Mar 19, 2024 at 4:37 AM zhoumin  wrote:
>>
>>
>> One more thing I want to confirm is whether we should apply the patch
>> onto the branch commit which existed at the time when that patch was
>> submitted or onto the latest tip of branch if users request doing
>> rebase. Users probably request a recheck with `rebase` when the CI lab
>> chose a wrong branch onto which apply the patch. I worry we may
>> encounter conflicts when apply the patch onto the latest commit of the
>> target branch if that branch is just updated before the request.
>>
>>
>
> That's a good edge case to think about...  but I also think if the
> patch no longer applies cleanly on tip of intended branch, then we
> would be correct to report an apply failure there. And then the
> submitter should refactor their patch so it applies, and submit again.

+1

> So I think the process is like
>
> A) If retest is requested without rebase key, then retest "original"
> dpdk artifact (either by re-using the existing tarball (unh lab) or
> tracking the commit from submit time and re-applying onto dpdk at that
> commit (loongson)).
>
> B) If rebase key is included, apply to tip of the indicated branch.
> If, because the branch has changed, the patch no longer applies, then
> we can report an apply failure. Then, submitter has to refactor their
> patch and resubmit.

That makes sense to me.

> In either case, report the new results with an updated test result in
> the email (i.e. report "_Testing PASS RETEST #1" instead of "_Testing
> PASS" in the email body).

Ack - makes sense here too.

RE: [PATCH v3] net/netvsc: fix number Tx queues > Rx queues

[Long Li]

> Subject: [PATCH v3] net/netvsc: fix number Tx queues > Rx queues
> 
> The previous code allowed the number of Tx queues to be set higher than the
> number of Rx queues.  If a packet was sent on a Tx queue with index
> >= number Rx queues there was a segfault.
> 
> This commit fixes the issue by creating an Rx queue for every Tx queue meaning
> that an event buffer is allocated to handle receiving Tx completion messages.
> 
> mbuf pool and Rx ring are not allocated for these additional Rx queues and RSS
> configuration ensures that no packets are received on them.
> 
> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> Cc: sthem...@microsoft.com
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Alan Elder 

Reviewed-by: Long Li 

Re: [PATCH v5 0/6] fix lcore ID restriction

[Ferruh Yigit]

On 3/18/2024 5:31 PM, Sivaprasad Tummala wrote:
> With modern CPUs, it is possible to have higher
> CPU count thus we can have higher RTE_MAX_LCORES.
> In DPDK sample applications, the current config
> lcore options are hard limited to 255.
>   
> The patchset fixes these constraints by allowing
> all lcore IDs up to RTE_MAX_LCORES. Also the queue
> IDs are increased to support up to 65535.
>   
> v5: 
>  - updated lcore_id type to uint32_t 
> 
> v4:
>  - fixed build errors with queue_id type
>in ipsec-secgw 
>  
> v3: 
>  - updated queue_id type to uint16_t
>  
> v2:
>  - fixed typo with lcore_id type in l3fwd  
> 
> Sivaprasad Tummala (6):
>   examples/l3fwd: fix lcore ID restriction
>   examples/l3fwd-power: fix lcore ID restriction
>   examples/l3fwd-graph: fix lcore ID restriction
>   examples/ipsec-secgw: fix lcore ID restriction
>   examples/qos_sched: fix lcore ID restriction
>   examples/vm_power_manager: fix lcore ID restriction
>

For series,
Acked-by: Ferruh Yigit 

[DPDK/DTS Bug 1404] Ensure primary applications such as testpmd are properly cleaned up

[bugzilla]

https://bugs.dpdk.org/show_bug.cgi?id=1404

Bug ID: 1404
   Summary: Ensure primary applications such as testpmd are
properly cleaned up
   Product: DPDK
   Version: unspecified
  Hardware: All
OS: All
Status: UNCONFIRMED
  Severity: critical
  Priority: Normal
 Component: DTS
  Assignee: dev@dpdk.org
  Reporter: jspew...@iol.unh.edu
CC: juraj.lin...@pantheon.tech, pr...@iol.unh.edu
  Target Milestone: ---

Right now we are relying on the garbage collector to clean up interactive
shells after they fall out of scope, and this is fine for applications like a
python shell where it doesn't really matter if we run multiple at once.
However, some applications (such as primary applications in DPDK) cannot run if
there is an instance of another specific application running. Because of this,
we need to be able to guarantee that these critical applications close at
certain times but the python garbage collector makes no guarantees for when it
cleans up these objects. 

One of the ways to make this more explicit which is being looked into right now
is a context manager. A context manager will help guarantee that when the
application is needed in its context block, it will be running, but as soon as
we exit the block (whether through exceptions being raised, reaching the end of
the block, or anything that would take the code out of that scope) the
application is properly cleaned up. This allows many more guarantees for when
these critical applications are running and exactly when they close.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[DPDK/DTS Bug 1383] DTS: clean up old tarball before copying a new one over

[bugzilla]

https://bugs.dpdk.org/show_bug.cgi?id=1383

Patrick Robb (pr...@iol.unh.edu) changed:

   What|Removed |Added

 Resolution|--- |DUPLICATE
 Status|UNCONFIRMED |RESOLVED

--- Comment #1 from Patrick Robb (pr...@iol.unh.edu) ---
Won't do

*** This bug has been marked as a duplicate of bug 1362 ***

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: fib{,6}: questions and proposals

[Robin Jarry]

Hi Vladimir,

Medvedkin, Vladimir, Mar 19, 2024 at 18:16:
> 2) Is it OK/safe to modify a fib from a control thread (read/write) 
>while it is used by data path threads (read only)?


This part is a bit more complicated. In practice, I would say yes, 
however, there is a possibility that if the lookup thread is preempted 
in the middle of the lookup process, and at the same time the control 
thread deletes the corresponding route, then the lookup result may 
return outdated data. This problem is solved in LPM with RCU enabled. 
I have plans to implement it in the near future in the FIB.


OK that's good to know, thanks.

> 3) There is no public API to list/walk all configured routes in 
>a fib. Would that be possible/easy to implement?


Yes, it already there. FIB under the hood uses rte_rib to hold 
existing routes. So walking through can be implemented like:


I had tried it and got confusing results out of this. This must have 
been before I had realized that all addresses needed to be in host 
order...


I tried again and it works as advertised with a small missing detail: 
after configuring a default route, e.g.:


   rte_fib_add(fib, RTE_IPV4(2, 2, 0, 0), 16, RTE_IPV4(1, 2, 3, 4));
   rte_fib_add(fib, RTE_IPV4(3, 3, 3, 0), 24, RTE_IPV4(4, 3, 2, 1));
   rte_fib_add(fib, RTE_IPV4(0, 0, 0, 0), 0, RTE_IPV4(9, 9, 9, 9));

It is not returned by rte_rib_get_nxt() successive calls. I only see the 
other two routes:


   2.2.0.0/16 via 1.2.3.4
   3.3.3.0/24 via 4.3.2.1

Is this expected?

> 4) In rte_fib, every IPv4 address (route *and* next hop) needs to be 
>in host order. This is not consistent with fib6 where addresses 
>are stored in network order. It took me quite a while to figure 
>out what was wrong with my code. 

This API behavior was created in such a way that it is the same as 
LPM.


As for LPM, I think it was done this way for performance reasons 
because in some scenarios you only working with the host order ipv4 
addresses.


This should really be advertised in strong capital letters in the API 
docs. Or (preferably) hidden to the user. I don't see any valid scenario 
where you would work with host order IPv4 addresses.


Do you think we could change that API or at least add a flag at FIB/RIB 
creation to make it transparent to the user and consistent between IPv4 
and IPv6?


Thanks!

RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption

[Suanming Mou]

> -Original Message-
> From: Power, Ciara 
> Sent: Tuesday, March 19, 2024 11:15 PM
> To: Suanming Mou ; gak...@marvell.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
> 
> 
> 
> > -Original Message-
> > From: Suanming Mou 
> > Sent: Tuesday, March 19, 2024 11:46 AM
> > To: gak...@marvell.com; Power, Ciara 
> > Cc: dev@dpdk.org
> > Subject: [PATCH v2] app/test-crypto-perf: add throughput OOP
> > decryption
> >
> > During throughput running, re-filling the test data will impact the
> > performance test result. So for now, to run decrypt throughput testing
> > is not supported since the test data is not filled.
> >
> > But if user requires OOP(out-of-place) mode, the test data from source
> > mbuf will never be modified, and if the test data can be prepared out
> > of the running loop, the decryption test should be fine.
> >
> > This commit adds the support of out-of-place decryption testing for 
> > throughput.
> >
> > [1]:
> > http://mails.dpdk.org/archives/dev/2023-July/273328.html
> >
> > Signed-off-by: Suanming Mou 
> > ---
> >  app/test-crypto-perf/cperf_ops.c |  5 ++-
> >  app/test-crypto-perf/cperf_options_parsing.c |  8 +
> > app/test-crypto- perf/cperf_test_throughput.c | 34
> > +---
> >  3 files changed, 41 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/test-crypto-perf/cperf_ops.c
> > b/app/test-crypto-perf/cperf_ops.c
> > index d3fd115bc0..714616c697 100644
> > --- a/app/test-crypto-perf/cperf_ops.c
> > +++ b/app/test-crypto-perf/cperf_ops.c
> > @@ -644,7 +644,10 @@ cperf_set_ops_aead(struct rte_crypto_op **ops,
> > }
> >
> > if ((options->test == CPERF_TEST_TYPE_VERIFY) ||
> > -   (options->test == CPERF_TEST_TYPE_LATENCY)) {
> > +   (options->test == CPERF_TEST_TYPE_LATENCY) ||
> > +   (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > +(options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > + options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT))) {
> > for (i = 0; i < nb_ops; i++) {
> > uint8_t *iv_ptr = rte_crypto_op_ctod_offset(ops[i],
> > uint8_t *, iv_offset);
> > diff --git a/app/test-crypto-perf/cperf_options_parsing.c
> > b/app/test-crypto- perf/cperf_options_parsing.c index
> > 8c20974273..90526e676f 100644
> > --- a/app/test-crypto-perf/cperf_options_parsing.c
> > +++ b/app/test-crypto-perf/cperf_options_parsing.c
> > @@ -1341,6 +1341,14 @@ cperf_options_check(struct cperf_options
> > *options)
> > }
> > }
> >
> > +   if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > +   (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > +options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
> > +   !options->out_of_place) {
> > +   RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > throughput decryption.\n");
> > +   return -EINVAL;
> > +   }
> 
> Not totally following some of this, why do we only want to add this for OOP
> mode?
> 
> For example an inplace command I can use before this patch but not after:
> ./build/app/dpdk-test-crypto-perf -l 2,3 -- --ptest throughput --optype aead 
> --
> aead-algo aes-gcm --aead-op decrypt --devtype crypto_qat --aead-key-sz 16
> 
> I get an error;
> USER1: Only out-of-place is allowed in throughput decryption.
> USER1: Checking one or more user options failed
> 
> Do we want to always force the user to use OOP + test vector file for these
> throughput decryption tests?
> Or should we just add a warning that the throughput may not be reflecting the
> "success" verify path in PMD if using inplace and the dummy data.
> 
> I am not sure.
> If we do want to add the limitation on the throughput tests, these changes I 
> think
> are ok for that.

Yes, think about that, in throughput mode, we will not fill the test data time 
to time, otherwise the testing is useless.
So that means the test data should not be overwritten, otherwise decryption 
will be with invalid data after the first round of decryption. Since the 1st 
round decryption overwritten the data to the original buf. In that case, test 
decryption throughput in non-oop mode is meaningless. 
That's the reason we add that limit to avoid the invalid data issue.

> 
> Thanks,
> Ciara
> 
> > +
> > if (options->op_type == CPERF_CIPHER_ONLY ||
> > options->op_type == CPERF_CIPHER_THEN_AUTH ||
> > options->op_type == CPERF_AUTH_THEN_CIPHER) { diff
> --git
> > a/app/test-crypto-perf/cperf_test_throughput.c b/app/test-crypto-
> > perf/cperf_test_throughput.c index e3d266d7a4..b347baa913 100644
> > --- a/app/test-crypto-perf/cperf_test_throughput.c
> > +++ b/app/test-crypto-perf/cperf_test_throughput.c
> > @@ -99,6 +99,26 @@ cperf_throughput_test_constructor(struct
> > rte_mempool *sess_mp,
> > return NULL;
> >  }
> >
> > +static void
> > +cperf_verify_init_ops(struct rte_mempool *mp __rte_u

Re: [PATCH v2] app/dma-perf: calrify incorrect NUMA config

[Varghese, Vipin]

Thank you Konstantin for the reply, Adding back the comments as it is 
not reflected the mail thread





diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index 9b1f58c78c..b6d0dbe4c0 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -311,9 +311,14 @@ setup_memory_env(struct test_configure *cfg, struct 
rte_mbuf ***srcs,
   uint32_t nr_buf = cfg->nr_buf;

   nr_sockets = rte_socket_count();
- if (cfg->src_numa_node >= nr_sockets ||
- cfg->dst_numa_node >= nr_sockets) {
- printf("Error: Source or destination numa exceeds the acture numa 
nodes.\n");
+
+ bool isSrcNumaIncorrect = (cfg->src_numa_node >= nr_sockets);
+ bool isDstNumaIncorrect = (cfg->dst_numa_node >= nr_sockets);

The naming style needs to be adjusted, how about
bool is_src_numa_exceed, is_dst_numa_exceed;


Ok, the naming convention used by me is `CamelCase`. One suggested 
from your end is `snake_case`.


Does DPDK has a constrain it can not use CamelCase.


[KA]

Please refer to:

https://doc.dpdk.org/guides/contributing/coding_style.html

In particular:
1.5.4. Variable Declarations
In declarations, do not put any whitespace between asterisks and adjacent 
tokens, except for tokens that are identifiers related to types. (These 
identifiers are the names of basic types, type qualifiers, and typedef-names 
other than the one being declared.) Separate these identifiers from asterisks 
using a single space.
For example:
int *x; /* no space after asterisk */
int * const x;  /* space after asterisk when using a type qualifier */
· All externally-visible variables should have an rte_ prefix in the 
name to avoid namespace collisions.
· Do not use uppercase letters - either in the form of ALL_UPPERCASE, 
or CamelCase - in variable names. Lower-case letters and underscores only.

[VV] Thank you for the clarification, I mistook this is applicable only to `All 
externally-visible variables`and not for `static` functions.

Re: [PATCH 1/3] ethdev: support setting lanes

[huangdengdui]

On 2024/3/19 11:02, Stephen Hemminger wrote:
> On Tue, 12 Mar 2024 15:52:36 +0800
> Dengdui Huang  wrote:
> 
>> -ret = snprintf(str, len, "Link up at %s %s %s",
>> +ret = snprintf(str, len, "Link up at %s %ulanes %s %s",
> 
> Don't you want a space after %u?
> 
> Could you make it so that lanes is only part of the message if non-default 
> value
> is used?
Ok, I'll do it in the next version.

Re: Email based retest request process: proposal for new pull/re-apply feature

[zhoumin]

On Tue, Mar 19, 2024 at 5:30PM, Patrick Robb wrote:

On Tue, Mar 19, 2024 at 4:37 AM zhoumin  wrote:


One more thing I want to confirm is whether we should apply the patch
onto the branch commit which existed at the time when that patch was
submitted or onto the latest tip of branch if users request doing
rebase. Users probably request a recheck with `rebase` when the CI lab
chose a wrong branch onto which apply the patch. I worry we may
encounter conflicts when apply the patch onto the latest commit of the
target branch if that branch is just updated before the request.



That's a good edge case to think about...  but I also think if the
patch no longer applies cleanly on tip of intended branch, then we
would be correct to report an apply failure there. And then the
submitter should refactor their patch so it applies, and submit again.

Yes, it is more reasonable for submitter.

So I think the process is like

A) If retest is requested without rebase key, then retest "original"
dpdk artifact (either by re-using the existing tarball (unh lab) or
tracking the commit from submit time and re-applying onto dpdk at that
commit (loongson)).

B) If rebase key is included, apply to tip of the indicated branch.
If, because the branch has changed, the patch no longer applies, then
we can report an apply failure. Then, submitter has to refactor their
patch and resubmit.

Thanks for making the applying process more clear.

In either case, report the new results with an updated test result in
the email (i.e. report "_Testing PASS RETEST #1" instead of "_Testing
PASS" in the email body).
Yes, I agree with this approach and reporting a new title for the retest 
result is necessary.

[PATCH v3] app/dma-perf: calrify incorrect NUMA config

[Vipin Varghese]

In case incorrect NUMA configuration, the current commit shares
 1) either `source or destination numa is greater`
 2) instead of `actual NUMA` it is `acture NUMA`
 3) uses `printf` instead of PRINT_ERR

current patch changes the above to
 1) identify if source or|and destination is incorrect
 2) fix wording to incorrect
 3) use PRINT_ERR macro

Signed-off-by: Vipin Varghese 
---

V3 changes:
 - use snake-case instead of camel case for static scope functions.
 - convert console words to lower case.

V2 changes:
 - inform incorrect numa
 - fix spelling from acture to actual
 - use PRINT_ERR instead of printf
---
 app/test-dma-perf/benchmark.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index d167adc4d2..a437b715bd 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -442,11 +442,16 @@ setup_memory_env(struct test_configure *cfg,
unsigned int nr_sockets;
uint32_t nr_buf = cfg->nr_buf;
uint32_t i;
+   bool is_src_numa_incorrect, is_dst_numa_incorrect;
 
nr_sockets = rte_socket_count();
-   if (cfg->src_numa_node >= nr_sockets ||
-   cfg->dst_numa_node >= nr_sockets) {
-   printf("Error: Source or destination numa exceeds the acture 
numa nodes.\n");
+   is_src_numa_incorrect = (cfg->src_numa_node >= nr_sockets);
+   is_dst_numa_incorrect = (cfg->dst_numa_node >= nr_sockets);
+
+   if (is_src_numa_incorrect || is_dst_numa_incorrect) {
+   PRINT_ERR("Error: Incorrect NUMA config for %s.\n",
+   (is_src_numa_incorrect && is_dst_numa_incorrect) ? 
"source & destination" :
+   (is_src_numa_incorrect) ? "source" : 
"destination");
return -1;
}
 
-- 
2.39.3

RE: [PATCH] doc: deprecate graph data structures

[Yan, Zhirun]

Acked-by: Zhirun Yan 

> -Original Message-
> From: Nithin Dabilpuram 
> Sent: Tuesday, March 19, 2024 1:21 PM
> To: pbhagavat...@marvell.com
> Cc: jer...@marvell.com; ndabilpu...@marvell.com; kirankum...@marvell.com;
> Yan, Zhirun ; dev@dpdk.org
> Subject: Re: [PATCH] doc: deprecate graph data structures
> 
> Acked-by: Nithin Dabilpuram 
> 
> On Wed, Feb 21, 2024 at 9:50 PM  wrote:
> >
> > From: Pavan Nikhilesh 
> >
> > Deprecate rte_node, rte_node_register and rte_graph_cluster_node_stats
> > structures as will be extended to include node specific error counters
> > and error description.
> >
> > Signed-off-by: Pavan Nikhilesh 
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index 10630ba255..b3dfd06ed6 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -142,3 +142,8 @@ Deprecation Notices
> >will be deprecated and subsequently removed in DPDK 24.11 release.
> >Before this, the new port library API (functions rte_swx_port_*)
> >will gradually transition from experimental to stable status.
> > +
> > +* graph: The graph library data structures will be modified to
> > +  support node specific errors, the structures ``rte_node``,
> > +  ``rte_node_register`` and ``rte_graph_cluster_node_stats`` will be
> > +  extended to include node error counters and error description.
> > --
> > 2.25.1
> >

Tech Board Meeting Tomorrow Wed. 3/20 - 7am pacific/10am Eastern/1400h utc

[Nathan Southern]

Agenda (read only)

https://annuel.framapad.org/p/r.0c3cc4d1e011214183872a98f6b5c7db

1. Join from PC, Mac, iPad, or Android

*https://zoom-lfx.platform.linuxfoundation.org/meeting/91865709518?password=df302f34-a942-4d14-a95b-a6c46ae98fae*


2. Join via audio

One tap mobile:
US: +12532158782,,91865709518# or +13462487799,,91865709518

Or dial:
US: +1 253 215 8782 or +1 346 248 7799 or +1 669 900 6833 or +1 301 715
8592 or +1 312 626 6799 or +1 646 374 8656 or 877 369 0926 (Toll Free) or
855 880 1246 (Toll Free)
Canada: +1 647 374 4685 or +1 647 558 0588 or +1 778 907 2071 or +1 204 272
7920 or +1 438 809 7799 or +1 587 328 1099 or 855 703 8985 (Toll Free)

Meeting ID: 91865709518

Meeting Passcode: 610444


International numbers: *https://zoom.us/u/alwnPIaVT*

RE: [PATCH] graph: fix head move when graph walk in mcore dispatch

[Yan, Zhirun]

> -Original Message-
> From: Wu, Jingjing 
> Sent: Tuesday, March 19, 2024 10:15 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing ; jer...@marvell.com;
> pbhagavat...@marvell.com; Yan, Zhirun ;
> sta...@dpdk.org
> Subject: [PATCH] graph: fix head move when graph walk in mcore dispatch
> 
> Head move should happen after the core id check, otherwise source node will be
> missed.
> 
> Fixes: 35dfd9b9fd85 ("graph: introduce graph walk by cross-core dispatch")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Jingjing Wu 
> ---
>  lib/graph/rte_graph_model_mcore_dispatch.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/graph/rte_graph_model_mcore_dispatch.h
> b/lib/graph/rte_graph_model_mcore_dispatch.h
> index 75ec388cad..b96469296e 100644
> --- a/lib/graph/rte_graph_model_mcore_dispatch.h
> +++ b/lib/graph/rte_graph_model_mcore_dispatch.h
> @@ -97,12 +97,12 @@ rte_graph_walk_mcore_dispatch(struct rte_graph
> *graph)
>   __rte_graph_mcore_dispatch_sched_wq_process(graph);
> 
>   while (likely(head != graph->tail)) {
> - node = (struct rte_node *)RTE_PTR_ADD(graph,
> cir_start[(int32_t)head++]);
> + node = (struct rte_node *)RTE_PTR_ADD(graph,
> +cir_start[(int32_t)head]);
> 
>   /* skip the src nodes which not bind with current worker */
>   if ((int32_t)head < 0 && node->dispatch.lcore_id != graph-
> >dispatch.lcore_id)
>   continue;
> -
> + head++;
If current src node not bind with current core, It will go into infinite loop.
This line would have no chance to run.

>   /* Schedule the node until all task/objs are done */
>   if (node->dispatch.lcore_id != RTE_MAX_LCORE &&
>   graph->dispatch.lcore_id != node->dispatch.lcore_id &&
> --
> 2.34.1

Re: Tech Board Meeting Tomorrow Wed. 3/20 - 7am pacific/10am Eastern/1400h utc

[Nathan Southern]

Dear DPDK Community,

i apologize - this was in error. Ignore the information from that previous
message. For some reason, I had two lfx meetings for the tech board on my
calendar at different times tomorrow.

*The correct time should be 8am Pacific/11am Eastern/1500h UTC.*

*And the correct log in and dial in information are below. Sorry for the
misstatement.*

*Nathan*

You have been invited to a recurring meeting for Data Plane Development Kit
(DPDK)

Agenda (read-only):
https://annuel.framapad.org/p/r.0c3cc4d1e011214183872a98f6b5c7db

 Minutes:
http://core.dpdk.org/techboard/minutes


Ways to join meeting:

1. Join from PC, Mac, iPad, or Android

https://zoom-lfx.platform.linuxfoundation.org/meeting/96459488340?password=d808f1f6-0a28-4165-929e-5a5bcae7efeb


2. Join via audio

One tap mobile:
US: +12532158782,,96459488340# or +13462487799,,96459488340

Or dial:
US: +1 253 215 8782 or +1 346 248 7799 or +1 669 900 6833 or +1 301 715
8592 or +1 312 626 6799 or +1 646 374 8656 or 877 369 0926 (Toll Free) or
855 880 1246 (Toll Free)
Canada: +1 647 374 4685 or +1 647 558 0588 or +1 778 907 2071 or +1 204 272
7920 or +1 438 809 7799 or +1 587 328 1099 or 855 703 8985 (Toll Free)

Meeting ID: 96459488340

Meeting Passcode: 699526


International numbers: https://zoom.us/u/alwnPIaVT


On Tue, Mar 19, 2024 at 10:24 PM Nathan Southern <
nsouth...@linuxfoundation.org> wrote:

> Agenda (read only)
>
> https://annuel.framapad.org/p/r.0c3cc4d1e011214183872a98f6b5c7db
>
> 1. Join from PC, Mac, iPad, or Android
>
>
> *https://zoom-lfx.platform.linuxfoundation.org/meeting/91865709518?password=df302f34-a942-4d14-a95b-a6c46ae98fae*
> 
>
> 2. Join via audio
>
> One tap mobile:
> US: +12532158782,,91865709518# or +13462487799,,91865709518
>
> Or dial:
> US: +1 253 215 8782 or +1 346 248 7799 or +1 669 900 6833 or +1 301 715
> 8592 or +1 312 626 6799 or +1 646 374 8656 or 877 369 0926 (Toll Free) or
> 855 880 1246 (Toll Free)
> Canada: +1 647 374 4685 or +1 647 558 0588 or +1 778 907 2071 or +1 204
> 272 7920 or +1 438 809 7799 or +1 587 328 1099 or 855 703 8985 (Toll Free)
>
> Meeting ID: 91865709518
>
> Meeting Passcode: 610444
>
>
> International numbers: *https://zoom.us/u/alwnPIaVT*
> 
>

Re: [PATCH v9 5/5] eal: add option to put timestamp on console output

[Stephen Hemminger]

On Tue, 19 Mar 2024 17:13:35 +0100
Morten Brørup  wrote:

> > From: Stephen Hemminger [mailto:step...@networkplumber.org]
> > Sent: Tuesday, 19 March 2024 16.52
> > 
> > On Tue, 19 Mar 2024 08:37:30 +0100
> > Morten Brørup  wrote:
> >   
> > > >  static ssize_t
> > > >  console_log_write(__rte_unused void *c, const char *buf, size_t  
> > size)  
> > > >  {
> > > > +   struct timespec ts;
> > > > ssize_t ret;
> > > >
> > > > -   /* write on stderr */
> > > > -   ret = fwrite(buf, 1, size, stderr);
> > > > +   if (timestamp_enabled) {
> > > > +   clock_gettime(CLOCK_MONOTONIC, &ts);
> > > > +   ts.tv_sec -= log_started.tv_sec;
> > > > +   ts.tv_nsec -= log_started.tv_nsec;  
> > >
> > > Please log the absolute CLOCK_MONOTONIC instead of subtracting  
> > log_started, so timestamps can be easily compared with timestamps from
> > other processes.
> > 
> > 
> > No, was trying to do what kernel dmesg does.  
> 
> What do you mean? Doesn't the kernel output CLOCK_MONOTONIC timestamps 
> (without offset)?
> 
> And by "timestamps from other processes" I also mean timestamps in log 
> messages from the kernel itself.
> 

If you look at dmesg command that formats the messages, it has lots of 
timestamp options.
Next version will support more of these.

   --time-format format
   Print timestamps using the given format, which can be ctime,
   reltime, delta or iso. The first three formats are aliases of
   the time-format-specific options. The iso format is a dmesg
   implementation of the ISO-8601 timestamp format. The purpose
   of this format is to make the comparing of timestamps between
   two systems, and any other parsing, easy. The definition of
   the iso timestamp is:
   -MM-DDHH:MM:SS,←+>.

Re: [PATCH v2] dmadev: fix structure alignment

[fengchengwen]

Hi Wenwu,

On 2024/3/15 17:27, Ma, WenwuX wrote:
> Hi Chengwen
> 
>> -Original Message-
>> From: fengchengwen 
>> Sent: Friday, March 15, 2024 4:32 PM
>> To: Ma, WenwuX ; dev@dpdk.org
>> Cc: Jiale, SongX ; sta...@dpdk.org
>> Subject: Re: [PATCH v2] dmadev: fix structure alignment
>>
>> Hi Wenwu,
>>
>> On 2024/3/15 15:44, Ma, WenwuX wrote:
>>> Hi Chengwen,
>>>
 -Original Message-
 From: Ma, WenwuX
 Sent: Friday, March 15, 2024 2:26 PM
 To: fengchengwen ; dev@dpdk.org
 Cc: Jiale, SongX ; sta...@dpdk.org
 Subject: RE: [PATCH v2] dmadev: fix structure alignment

 Hi Chengwen,

> -Original Message-
> From: fengchengwen 
> Sent: Friday, March 15, 2024 2:06 PM
> To: Ma, WenwuX ; dev@dpdk.org
> Cc: Jiale, SongX ; sta...@dpdk.org
> Subject: Re: [PATCH v2] dmadev: fix structure alignment
>
> Hi Wenwu,
>
> On 2024/3/15 9:43, Wenwu Ma wrote:
>> The structure rte_dma_dev needs only 8 byte alignment.
>> This patch replaces __rte_cache_aligned of rte_dma_dev with
>> __rte_aligned(8).
>>
>> Fixes: b36970f2e13e ("dmadev: introduce DMA device library")
>> Cc: sta...@dpdk.org
>>
>> Signed-off-by: Wenwu Ma 
>> ---
>> v2:
>>  - Because of performance drop, adjust the code to
>>no longer demand cache line alignment
>
> Which two versions observed performance drop? And which benchmark
> observed drop?
> Could you provide more information?
>
>>
 V1 patch:

>> https://patches.dpdk.org/project/dpdk/patch/20240308053711.1260154-
 1-wenwux...@intel.com/

 To view detailed results, visit:
 https://lab.dpdk.org/results/dashboard/patchsets/29472/

>> ---
>>  lib/dmadev/rte_dmadev_pmd.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/dmadev/rte_dmadev_pmd.h
> b/lib/dmadev/rte_dmadev_pmd.h
>> index 58729088ff..b569bb3502 100644
>> --- a/lib/dmadev/rte_dmadev_pmd.h
>> +++ b/lib/dmadev/rte_dmadev_pmd.h
>> @@ -122,7 +122,7 @@ enum rte_dma_dev_state {
>>   * @internal
>>   * The generic data structure associated with each DMA device.
>>   */
>> -struct __rte_cache_aligned rte_dma_dev {
>> +struct __rte_aligned(8) rte_dma_dev {
>
> The DMA fast-path was implemented by struct rte_dma_fp_objs, which
> is not rte_dma_dev? So why is it a problem here?
>
> Thanks
>
 The DMA device object is expected to align cache line, so clang will
 use “vmovaps” assembly instruction,

 And the instruction demands 16 bytes alignment or will cause segment
 fault in some environments.

>>> Test case:
>>> 1. compile dpdk
>>> rm -rf x86_64-native-linuxapp-clang
>>> CC=clang meson -Denable_kmods=True -Dlibdir=lib
>>> --default-library=static x86_64-native-linuxapp-clang ninja -C
>>> x86_64-native-linuxapp-clang -j 72 2. start dpdk-test
>>> /root/dpdk/x86_64-native-linuxapp-clang/app/dpdk-test -l 0-39
>>> --vdev=dma_skeleton -a 31:00.0 -a 31:00.1 -a 31:00.2 -a 31:00.3 （Note:
>>> If it cannot be reproduced, please try using a different core）
>>> 3. exit dpdk-test
>>> RTE>>quit
>>> Segmentation fault (core dumped)

I reproduce it just with --vdev=dma_skeleton.
When execute quit command, it will invoke rte_dma_close->dma_release, pls see 
my annotations (//) below:

void
dma_release(struct rte_dma_dev *dev)
{
if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
rte_free(dev->data->dev_private);
memset(dev->data, 0, sizeof(struct rte_dma_dev_data));
}

dma_fp_object_dummy(dev->fp_obj);
memset(dev, 0, sizeof(struct rte_dma_dev));   // this memset was 
compiles using vmovaps, its
//  8c24da:   c5 f8 57 c0   
  vxorps %xmm0,%xmm0,%xmm0
//  8c24de:   c5 fc 29 43 
20  vmovaps %ymm0,0x20(%rbx)
//  8c24e3:   c5 fc 29 03   
  vmovaps %ymm0,(%rbx)
// but the dev is not align 16B 
(in my env the rte_dma_devices addr is 0x15d39950)
}

>>
>> I will try to reproduce, but still a question: does above test has already 
>> merged
>> your patch [1] or the current main branch code has this problem?
>>
>> [1]
>> https://patches.dpdk.org/project/dpdk/patch/20240308053711.1260154-
>> 1-wenwux...@intel.com/
>>
>> Thanks
>>
> the current main branch code has this problem.
> 
> Both patch v1 and v2 are able to solve this problem, but v1 has a performance 
> issue.

The performance issue is ethdev benchmark, it will not invoke any dmadev API, I 
don't think these two has any relations.

So I prefer v1, Plus Pavan also submit a commit [1] to align the struct, but it 
was not a fix for clang-x86-platform.

[1] 
https://lore.kernel.org/all/20240210062758.1510-1-pbhagavat...@marve

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Gregory Etelson]

Hello Ferruh,

>BUT overall how can we catch issues like this in the feature, we don't
>have a good way to test testpmd flow commands.
>@Ori, @Gregory, do you have any idea?
>cc'ed CI mail list too.

We have a tool for unit tests based on the testpmd.
The tool details are here:  
https://drive.google.com/drive/folders/1cHrPwx4fUJ6ibUCtHd4kNKsrmmvQvvOj?usp=drive_link.
There's also a short description here: 
https://inbox.dpdk.org/ci/2a287ee7-cda4-f2ab-a4e6-a47021f85...@nvidia.com/

Consider an option when a code patch is accompanied with a short test script 
that validates that patch functionality.
DPDK CI can run the script to verify that the patch functions correctly.

Regards,
Gregory

RE: [PATCH] graph: fix head move when graph walk in mcore dispatch

[Wu, Jingjing]

> > /* skip the src nodes which not bind with current worker */
> > if ((int32_t)head < 0 && node->dispatch.lcore_id != graph-
> > >dispatch.lcore_id)
> > continue;
> > -
> > +   head++;
> If current src node not bind with current core, It will go into infinite loop.
> This line would have no chance to run.

Seems reasonable, it might be OK to change "head<0" to "head <1" the condition 
check?