Re: [dpdk-dev] [RESEND v2 3/7] net/dpaa2: change into dynamic logging

[Shreyansh Jain]

On Wed, Mar 21, 2018 at 12:01 PM, Hemant Agrawal
 wrote:
> Hi Shreyansh,
>
> On 3/13/2018 11:14 AM, Shreyansh Jain wrote:
>>
>> Signed-off-by: Shreyansh Jain 
>> ---
>>   config/common_base|   5 -
>>   config/defconfig_arm64-dpaa2-linuxapp-gcc |   9 -
>>   doc/guides/nics/dpaa2.rst |  44 ++---
>>   drivers/net/dpaa2/Makefile|   6 -
>>   drivers/net/dpaa2/base/dpaa2_hw_dpni.c|  30 ++--
>>   drivers/net/dpaa2/dpaa2_ethdev.c  | 290
>> +++---
>>   drivers/net/dpaa2/dpaa2_pmd_logs.h|  41 +
>>   drivers/net/dpaa2/dpaa2_rxtx.c|  59 +++---
>>   8 files changed, 258 insertions(+), 226 deletions(-)
>>   create mode 100644 drivers/net/dpaa2/dpaa2_pmd_logs.h
>>
>> diff --git a/config/common_base b/config/common_base
>> index ad03cf433..64bdfbb73 100644
>> --- a/config/common_base
>> +++ b/config/common_base
>> @@ -188,11 +188,6 @@ CONFIG_RTE_LIBRTE_DPAA2_USE_PHYS_IOVA=y
>>   # Compile burst-oriented NXP DPAA2 PMD driver
>>   #
>>   CONFIG_RTE_LIBRTE_DPAA2_PMD=n
>> -CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT=n
>> -CONFIG_RTE_LIBRTE_DPAA2_DEBUG_DRIVER=n
>
> See the comment in PATCH 2/7 w.r.t usages of DEBUG_DRIVER macro.
>>
>> @@ -322,9 +322,9 @@ dpaa2_attach_bp_list(struct dpaa2_dev_priv *priv,
>> retcode = dpni_set_pools(dpni, CMD_PRI_LOW, priv->token,
>> &bpool_cfg);
>> if (retcode != 0) {
>> -   PMD_INIT_LOG(ERR, "Error in attaching the buffer pool
>> list"
>> -   " bpid = %d Error code = %d\n",
>> -   bpool_cfg.pools[0].dpbp_id, retcode);
>> +   DPAA2_PMD_ERR("Error configuring buffer pool on
>> interface."
>> + " bpid = %d error code = %d",
>> + bpool_cfg.pools[0].dpbp_id, retcode);
>
> Can you try to convert this and others into a single line message?

Actually, the main debug string is a single line itself. It is the
variables which are being printed that have been shifted to new line.
That way, a developer can easy search through the code using the error
context (ignoring the values printed).
Unfortunately, at this point the indentation is too much to have a
long string with 80 character limit restriction.

I will push a v3 with all other comments fixed. If you still have
second thoughts about this, I will quickly spin-up a v4.

-
Shreyansh

Re: [dpdk-dev] [PATCH] net/mrvl: fix build error with gcc

[Tomasz Duszynski]

On Thu, Mar 22, 2018 at 06:18:47PM +, Ferruh Yigit wrote:
> gcc version:
> aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11) 7.2.1 20171011
>
> build error:
>   CC mrvl_qos.o
> .../drivers/net/mrvl/mrvl_qos.c: In function ‘mrvl_configure_rxqs’:
> .../drivers/net/mrvl/mrvl_qos.c:679:17:
>   error: ‘sprintf’ may write a terminating nul past the end of the
>  destination [-Werror=format-overflow=]
>   sprintf(match, "policer-%d:%d\n", priv->pp_id, priv->ppio_id);
>  ^
> .../drivers/net/mrvl/mrvl_qos.c:679:2:
>   note: ‘sprintf’ output between 13 and 17 bytes into a destination
> of size 16
>   sprintf(match, "policer-%d:%d\n", priv->pp_id, priv->ppio_id);
>   ^
>
> Fixed by replacing sprintf to snprintf.
>
> Fixes: 8860fd7b70f0 ("net/mrvl: add ingress policer support")
> Cc: t...@semihalf.com
>
> Signed-off-by: Ferruh Yigit 
> ---
> ---
>  drivers/net/mrvl/mrvl_qos.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/mrvl/mrvl_qos.c b/drivers/net/mrvl/mrvl_qos.c
> index e9c4531fd..741d3da7a 100644
> --- a/drivers/net/mrvl/mrvl_qos.c
> +++ b/drivers/net/mrvl/mrvl_qos.c
> @@ -676,7 +676,8 @@ setup_policer(struct mrvl_priv *priv, struct 
> pp2_cls_plcr_params *params)
>   char match[16];
>   int ret;
>
> - sprintf(match, "policer-%d:%d\n", priv->pp_id, priv->ppio_id);
> + snprintf(match, sizeof(match), "policer-%d:%d\n",
> + priv->pp_id, priv->ppio_id);
>   params->match = match;
>
>   ret = pp2_cls_plcr_init(params, &priv->policer);
> --
> 2.13.6
>

Acked-by: Tomasz Duszynski 

--
- Tomasz Duszyński

Re: [dpdk-dev] [PATCH v1] bus/fslmc: fix find device start condition

[Shreyansh Jain]

On Thu, Mar 22, 2018 at 3:58 PM, Gaetan Rivet  wrote:
> If start is set and a device before it matches the data,
> this device is returned.
>
> Fixes: c7fe1eea8a74 ("bus: simplify finding starting point")
> Cc: sta...@dpdk.org
>
> Cc: Hemant Agrawal 
> Cc: Shreyansh Jain 
> Signed-off-by: Gaetan Rivet 
> ---
>
> Hi Shreyansh, Hemant,
>
> Sorry, I did not test this.
> I found this issue while working on vdev and PCI.
>
> There is a better way to iterate on devices [1], but the gain
> is really minimal and the implementation slightly more complex.
> I preferred to avoid complex, as I could not test this patch.
>
> Regards,
>
> [1]: http://dpdk.org/ml/archives/dev/2018-March/092906.html
>

I think this change should suffice here. Thanks.

(On a side note, it seems compilation using clang is failing for this
patch [1] though it is not related to this change. I will look into
that)

[1] http://dpdk.org/ml/archives/test-report/2018-March/044684.html

Acked-by: Shreyansh Jain 

Re: [dpdk-dev] [PATCH v2 04/18] eal: add lightweight kvarg parsing utility

[Gaëtan Rivet]

On Thu, Mar 22, 2018 at 08:53:49PM -0400, Neil Horman wrote:
> On Thu, Mar 22, 2018 at 05:27:51PM +0100, Gaëtan Rivet wrote:
> > On Thu, Mar 22, 2018 at 10:10:37AM -0400, Neil Horman wrote:
> > > On Wed, Mar 21, 2018 at 05:32:24PM +, Wiles, Keith wrote:
> > > > 
> > > > 
> > > > > On Mar 21, 2018, at 12:15 PM, Gaetan Rivet  
> > > > > wrote:
> > > > > 
> > > > > This library offers a quick way to parse parameters passed with a
> > > > > key=value syntax.
> > > > > 
> > > > > A single function is needed and finds the relevant element within the
> > > > > text. No dynamic allocation is performed. It is possible to chain the
> > > > > parsing of each pairs for quickly scanning a list.
> > > > > 
> > > > > This utility is private to the EAL and should allow avoiding having to
> > > > > move around the more complete librte_kvargs.
> > > > 
> > > > What is the big advantage with this code and the librte_kvargs code. Is 
> > > > it just no allocation, rte_kvargs needs to be build before parts of EAL 
> > > > or what?
> > > > 
> > > > My concern is we have now two flavors one in EAL and one in 
> > > > librte_kvargs, would it not be more reasonable to improve rte_kvargs to 
> > > > remove your objections? I am all for fast, better, stronger code :-)
> > > > 
> > > +1, this really doesn't make much sense to me.  Two parsing routines 
> > > seems like
> > > its just asking for us to have to fix parsing bugs in two places.  If 
> > > allocation
> > > is a concern, I don't see why you can't just change the malloc in
> > > rte_kvargs_parse to an automatic allocation on the stack, or a 
> > > preallocation set
> > > of kvargs that can be shared from init time.
> > 
> > I think the existing allocation scheme is fine for other usages (in
> > drivers and so on). Not for what I wanted to do.
> > 
> Ok, but thats an adressable issue.  you can bifurcate the parse function to an
> internal function that accepts any preallocated kvargs struct, and export two
> wrapper functions, one which allocates the struct from the heap, another which
> allocated automatically on the stack.
> 

Sure, everything is possible.

> > >   librte_kvargs isn't 
> > > necessecarily
> > > the best parsing library ever, but its not bad, and it just seems wrong 
> > > to go
> > > re-inventing the wheel.
> > > 
> > 
> > It serves a different purpose than the one I'm pursuing.
> > 
> > This helper is lightweight and private. If I wanted to integrate my
> > needs with librte_kvargs, I would be adding new functionalities, making
> > it more complex, and for a use-case that is useless for the vast
> > majority of users of the lib.
> > 
> Ok, to that end:
> 
> 1) Privacy is not an issue (at least from my understanding of what your 
> doing).
> If we start with the assumption that librte_kvargs is capable of satisfying 
> your
> needs (even if its not done in an optimal way), the fact that your version of
> the function is internal to the library doesn't seem overly relevant, unless
> theres something critical to that privacy that I'm missing.
> 

Privacy is only a point I brought up to say that the impact of this
function is minimal. People looking to parse their kvargs should not
have any ambiguity regarding how they should do so. Only librte_kvargs
is available.

> 2) Lightweight function  seems like something that can be integrated with
> librte_kvargs.  Looking at it, what may I ask in librte_kvargs is 
> insufficiently
> non-performant for your needs, specifically?  We talked about the heap
> allocation above, is there something else? The string duplication perhaps?
> 
> 

Mostly the way to use it.
The filter strings are
bus=value,.../class=value,...

where either bus= list or class= list can be omitted, but at least one
must appear.

I want to read a single kvarg. I do not want to parse the whole string.
the '/' signifies the end of the current layer.

librte_kvargs does not care about those points. I cannot ask it to only
read either bus or class, as it would then throw an error for all the
other keys (which the EAL has necessarily no knowledge of).

So I would need to:

  * Add a custom storage scheme
  * Add a custom parsing mode stopping at the first kvarg
  * Add an edge-case to ignore the '/', so as not to throw off the rest
of the parsing (least it be considered part of the previous kvarg
value field).

Seeing this, does adding those really specifics functionality help
librte_kvargs to be more useful and usable? I do not think so.

It would only serve to disrupt the library for a marginal use-case, with
the introduction of edge-cases that will blur the specs of the lib's
API, making it harder to avoid subtle bugs.

Only way to do so sanely would be to add rte_parse_kv as part of
librte_kvargs, as is. But then the whole thing does not make sense IMO:
no one would care to use it, the maintainance effort is the same, the
likelyhood of bugs as well (but in the process we would disrupt the
distri

Re: [dpdk-dev] [PATCH 0/3] add ifcvf driver

[Wang, Xiao W]

Hi Maxime,

> -Original Message-
> From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
> Sent: Thursday, March 22, 2018 4:48 AM
> To: Wang, Xiao W ; dev@dpdk.org
> Cc: Wang, Zhihong ; y...@fridaylinux.org; Liang,
> Cunming ; Xu, Rosen ; Chen,
> Junjie J ; Daly, Dan 
> Subject: Re: [PATCH 0/3] add ifcvf driver
> 
> Hi Xiao,
> 
> On 03/15/2018 05:49 PM, Wang, Xiao W wrote:
> > Hi Maxime,
> >
> >> -Original Message-
> >> From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
> >> Sent: Sunday, March 11, 2018 2:24 AM
> >> To: Wang, Xiao W ; dev@dpdk.org
> >> Cc: Wang, Zhihong ; y...@fridaylinux.org; Liang,
> >> Cunming ; Xu, Rosen ;
> Chen,
> >> Junjie J ; Daly, Dan 
> >> Subject: Re: [PATCH 0/3] add ifcvf driver
> >>
> >> Hi Xiao,
> >>
> >> On 03/10/2018 12:08 AM, Xiao Wang wrote:
> >>> This patch set has dependency on
> >> http://dpdk.org/dev/patchwork/patch/35635/
> >>> (vhost: support selective datapath);
> >>>
> >>> ifc VF is compatible with virtio vring operations, this driver implements
> >>> vDPA driver ops which configures ifc VF to be a vhost data path 
> >>> accelerator.
> >>>
> >>> ifcvf driver uses vdev as a control domain to manage ifc VFs that belong
> >>> to it. It registers vDPA device ops to vhost lib to enable these VFs to be
> >>> used as vhost data path accelerator.
> >>>
> >>> Live migration feature is supported by ifc VF and this driver enables
> >>> it based on vhost lib.
> >>>
> >>> vDPA needs to create different containers for different devices, thus this
> >>> patch set adds APIs in eal/vfio to support multiple container.
> >> Thanks for this! That will avoind having to duplicate these functions
> >> for every new offload driver.
> >>
> >>
> >>>
> >>> Junjie Chen (1):
> >>> eal/vfio: add support for multiple container
> >>>
> >>> Xiao Wang (2):
> >>> bus/pci: expose sysfs parsing API
> >>
> >> Still, I'm not convinced the offload device should be a virtual device.
> >> It is a real PCI device, why not having a new device type for offload
> >> devices, and let the device to be probed automatically by the existing
> >> device model?
> >
> > IFC VFs are generated from SRIOV, with the PF driven by kernel driver.
> > In DPDK we need to have something to represent PF, to register itself as
> > a vDPA engine, so a virtual device is used for this purpose.
> I went through the code, and something is not clear to me.
> 
> Why do we need to have a representation of the PF in DPDK?
> Why cannot we just bind at VF level?

1. With the vdev representation we could use it to talk to PF kernel driver to 
do flow configuration, we can implement
flow API on the vdev in future for this purpose. Using a vdev allows 
introducing this kind of control plane thing.

2. When port representor is ready, we would integrate it into ifcvf driver, 
then each VF will have a
Representor port. For now we don’t have port representor, so this patch set 
manages VF resource internally.

BRs,
Xiao

Re: [dpdk-dev] [PATCH v3 3/5] vhost: add apis for datapath configuration

[Wang, Zhihong]

Hi Maxime,

> -Original Message-
> From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
> Sent: Thursday, March 22, 2018 10:19 PM
> To: Wang, Zhihong ; dev@dpdk.org
> Cc: Tan, Jianfeng ; Bie, Tiwei
> ; y...@fridaylinux.org; Liang, Cunming
> ; Wang, Xiao W ; Daly,
> Dan 
> Subject: Re: [PATCH v3 3/5] vhost: add apis for datapath configuration
> 
> Hi,
> 
> On 03/22/2018 09:22 AM, Wang, Zhihong wrote:
> >
> >
> >> -Original Message-
> >> From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
> >> Sent: Thursday, March 22, 2018 5:08 AM
> >> To: Wang, Zhihong ; dev@dpdk.org
> >> Cc: Tan, Jianfeng ; Bie, Tiwei
> >> ; y...@fridaylinux.org; Liang, Cunming
> >> ; Wang, Xiao W ;
> Daly,
> >> Dan 
> >> Subject: Re: [PATCH v3 3/5] vhost: add apis for datapath configuration
> >>
> >>
> >>
> >> On 02/27/2018 11:13 AM, Zhihong Wang wrote:
> >>> This patch adds APIs for datapath configuration. The eid and did of the
> >>> vhost-user socket can be configured to identify the actual device.
> >>>
> >>> When the default software datapath is used, eid and did are set to -1.
> >>> When alternative datapath is used, eid and did are set by app to specify
> >>> which device to use. Each vhost-user socket can have only 1 connection
> in
> >>> this case.
> >>>
> >>> Signed-off-by: Zhihong Wang 
> >>> ---
> >>>lib/librte_vhost/rte_vhost.h   | 70
> >> ++
> >>>lib/librte_vhost/rte_vhost_version.map |  6 +++
> >>>lib/librte_vhost/socket.c  | 65
> >> +++
> >>>lib/librte_vhost/vhost.c   | 50 
> >>>lib/librte_vhost/vhost.h   | 10 +
> >>>5 files changed, 201 insertions(+)
> >>>
> >>
> >> Isn't the notion of EID & DID Intel specifics?
> >> At vhost API level, shouldn't we only care of the offload device ID?
> >
> > It's not vendor specific: Engine id refers to an engine which is a device
> > on a bus, the engine could have multiple queue pairs or virtual functions.
> > The driver can manage them to present multiple vhost ports with vDPA to
> > application, so logically the concept of device id exists.
> >
> > In a lot of acceleration cases, application needs to be able to choose the
> > exact port to use instead of letting the driver to decide (because it does
> > make a difference), therefore it's necessary to expose the device id here.
> 
> Yes, but if I understood correctly with the IFCVF driver, we could pass
> directly the virtual function to the vhost-user lib, no need to specify
> the engine. We would just need to register one device per VF, but that
> looks like the right think to do looking at how IFCVF manages MAC
> addresses and link status for example.

The lib is for generic designs. An engine could also be an AFU device [1]
which has multiple virtio ring compatible queue pairs that can serve
different VMs independently, instead of multiple virtual functions. In
this case, we need eid to index the AFU device, and did to index the queue
pair(s).

[1] http://dpdk.org/ml/archives/dev/2018-March/093343.html (struct 
rte_afu_device)

Thanks
-Zhihong

> 
> Thanks,
> Maxime

Re: [dpdk-dev] [PATCH v2 3/3] net/ifcvf: add ifcvf driver

[Wang, Xiao W]

Hi Maxime,

> -Original Message-
> From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
> Sent: Thursday, March 22, 2018 4:58 AM
> To: Wang, Xiao W ; y...@fridaylinux.org
> Cc: dev@dpdk.org; Wang, Zhihong ; Bie, Tiwei
> ; Chen, Junjie J ; Xu, Rosen
> ; Daly, Dan ; Liang, Cunming
> ; Burakov, Anatoly ;
> gaetan.ri...@6wind.com
> Subject: Re: [PATCH v2 3/3] net/ifcvf: add ifcvf driver
> 
> 
> 
> On 03/21/2018 02:21 PM, Xiao Wang wrote:
> > ifcvf driver uses vdev as a control domain to manage ifc VFs that belong
> > to it. It registers vDPA device ops to vhost lib to enable these VFs to be
> > used as vhost data path accelerator.
> >
> > Live migration feature is supported by ifc VF and this driver enables
> > it based on vhost lib.
> >
> > Because vDPA driver needs to set up MSI-X vector to interrupt the guest,
> > only vfio-pci is supported currently.
> >
> > Signed-off-by: Xiao Wang 
> > Signed-off-by: Rosen Xu 
> > ---
> > v2:
> > - Rebase on Zhihong's vDPA v3 patch set.
> > ---
> >   config/common_base  |6 +
> >   config/common_linuxapp  |1 +
> >   drivers/net/Makefile|1 +
> >   drivers/net/ifcvf/Makefile  |   40 +
> >   drivers/net/ifcvf/base/ifcvf.c  |  329 
> >   drivers/net/ifcvf/base/ifcvf.h  |  156 
> >   drivers/net/ifcvf/base/ifcvf_osdep.h|   52 ++
> >   drivers/net/ifcvf/ifcvf_ethdev.c| 1240
> +++
> >   drivers/net/ifcvf/rte_ifcvf_version.map |4 +
> >   mk/rte.app.mk   |1 +
> >   10 files changed, 1830 insertions(+)
> >   create mode 100644 drivers/net/ifcvf/Makefile
> >   create mode 100644 drivers/net/ifcvf/base/ifcvf.c
> >   create mode 100644 drivers/net/ifcvf/base/ifcvf.h
> >   create mode 100644 drivers/net/ifcvf/base/ifcvf_osdep.h
> >   create mode 100644 drivers/net/ifcvf/ifcvf_ethdev.c
> >   create mode 100644 drivers/net/ifcvf/rte_ifcvf_version.map
> >
> 
> ...
> 
> > +static int
> > +eth_dev_ifcvf_create(struct rte_vdev_device *dev,
> > +   struct rte_pci_addr *pci_addr, int devices)
> > +{
> > +   const char *name = rte_vdev_device_name(dev);
> > +   struct rte_eth_dev *eth_dev = NULL;
> > +   struct ether_addr *eth_addr = NULL;
> > +   struct ifcvf_internal *internal = NULL;
> > +   struct internal_list *list = NULL;
> > +   struct rte_eth_dev_data *data = NULL;
> > +   struct rte_pci_addr pf_addr = *pci_addr;
> > +   int i;
> > +
> > +   list = rte_zmalloc_socket(name, sizeof(*list), 0,
> > +   dev->device.numa_node);
> > +   if (list == NULL)
> > +   goto error;
> > +
> > +   /* reserve an ethdev entry */
> > +   eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal));
> > +   if (eth_dev == NULL)
> > +   goto error;
> > +
> > +   eth_addr = rte_zmalloc_socket(name, sizeof(*eth_addr), 0,
> > +   dev->device.numa_node);
> > +   if (eth_addr == NULL)
> > +   goto error;
> > +
> > +   *eth_addr = base_eth_addr;
> > +   eth_addr->addr_bytes[5] = eth_dev->data->port_id;
> > +
> > +   internal = eth_dev->data->dev_private;
> > +   internal->dev_name = strdup(name);
> > +   if (internal->dev_name == NULL)
> > +   goto error;
> > +
> > +   internal->eng_addr.pci_addr = *pci_addr;
> > +   for (i = 0; i < devices; i++) {
> > +   pf_addr.domain = pci_addr->domain;
> > +   pf_addr.bus = pci_addr->bus;
> > +   pf_addr.devid = pci_addr->devid + (i + 1) / 8;
> > +   pf_addr.function = pci_addr->function + (i + 1) % 8;
> > +   internal->vf_info[i].pdev.addr = pf_addr;
> > +   rte_spinlock_init(&internal->vf_info[i].lock);
> > +   }
> > +   internal->max_devices = devices;
> > +
> > +   list->eth_dev = eth_dev;
> > +   pthread_mutex_lock(&internal_list_lock);
> > +   TAILQ_INSERT_TAIL(&internal_list, list, next);
> > +   pthread_mutex_unlock(&internal_list_lock);
> > +
> > +   data = eth_dev->data;
> > +   data->nb_rx_queues = IFCVF_MAX_QUEUES;
> > +   data->nb_tx_queues = IFCVF_MAX_QUEUES;
> > +   data->dev_link = vdpa_link;
> > +   data->mac_addrs = eth_addr;
> 
> We might want one ethernet device per VF, as for example you set
> dev_link.link_status to UP as soon as a VF is configured, and DOWN
> as when a single VF is removed.

Ideally it will be one representor port per VF, each representor port
has a link_status. Will integrate port representor when it's ready.
I will remove the vdev's ethdev registering for now, and add it back when we
need to implement flow APIs on the vdev.

> 
> > +   data->dev_flags = RTE_ETH_DEV_INTR_LSC;
> > +   eth_dev->dev_ops = &ops;
> > +
> > +   /* assign rx and tx ops, could be used as vDPA fallback */
> > +   eth_dev->rx_pkt_burst = eth_ifcvf_rx;
> > +   eth_dev->tx_pkt_burst = eth_ifcvf_tx;
> > +
> > +   if (rte_vdpa_register_engine(vdpa_ifcvf_driver.name,
> > +   &internal->eng_addr) < 0)
> > +   goto error;
> > +
> > + 

Re: [dpdk-dev] [PATCH v2 3/3] net/ifcvf: add ifcvf driver

[Wang, Xiao W]

Hi Thomas,

> -Original Message-
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Thursday, March 22, 2018 4:52 AM
> To: Wang, Xiao W ; Xu, Rosen 
> Cc: dev@dpdk.org; maxime.coque...@redhat.com; y...@fridaylinux.org; Wang,
> Zhihong ; Bie, Tiwei ; Chen,
> Junjie J ; Daly, Dan ; Liang,
> Cunming ; Burakov, Anatoly
> ; gaetan.ri...@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2 3/3] net/ifcvf: add ifcvf driver
> 
> 21/03/2018 14:21, Xiao Wang:
> > ifcvf driver uses vdev as a control domain to manage ifc VFs that belong
> > to it. It registers vDPA device ops to vhost lib to enable these VFs to be
> > used as vhost data path accelerator.
> 
> Not everybody work at Intel.
> Please explain what means ifcvf and what is a control domain.

OK, and I will add a document.
> 
> > Live migration feature is supported by ifc VF and this driver enables
> > it based on vhost lib.
> >
> > Because vDPA driver needs to set up MSI-X vector to interrupt the guest,
> > only vfio-pci is supported currently.
> >
> > Signed-off-by: Xiao Wang 
> > Signed-off-by: Rosen Xu 
> > ---
> > v2:
> > - Rebase on Zhihong's vDPA v3 patch set.
> > ---
> >  config/common_base  |6 +
> >  config/common_linuxapp  |1 +
> >  drivers/net/Makefile|1 +
> >  drivers/net/ifcvf/Makefile  |   40 +
> >  drivers/net/ifcvf/base/ifcvf.c  |  329 
> >  drivers/net/ifcvf/base/ifcvf.h  |  156 
> >  drivers/net/ifcvf/base/ifcvf_osdep.h|   52 ++
> >  drivers/net/ifcvf/ifcvf_ethdev.c| 1240
> +++
> >  drivers/net/ifcvf/rte_ifcvf_version.map |4 +
> >  mk/rte.app.mk   |1 +
> 
> This feature needs to be explained and documented.
> It will be helpful to understand the mechanism and to have a good review.
> Please do not merge it until there is a good documentation.
> 

Will add a doc with more details.

BRs,
Xiao

Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Ferruh Yigit]

On 3/23/2018 3:34 AM, Somnath Kotur wrote:
> Hi Ferruh,
> 
> 
> On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit  > wrote:
> 
> On 2/22/2018 2:58 AM, Somnath Kotur wrote:
> 
> Please start with lowercase after "net/bnxt: fix"
> 
> > When user reissues same flow director cmd with a different queue
> > update the existing filter to redirect flow to the new desired
> > queue as destination just like the other filters like 5 tuple and
> > generic flow.
> 
> Can you please add a fixes line?
> 
> Actually, I'm not sure if this qualifies as a 'fix for a regression', it
> prescribes a new behavior for this scenario (of same flow-director cmd ,
> different queue)  we never handled this before at all , Do you still think it
> needs it ?

Patch title starts with "fix bug with ..." :)

Previously it was returning error when filter exist, now it is updating the
existing filter with new destination. If this is fix or not depends on expected
behavior [1].

Practical reasons of having fixes tag:
1- Stable trees checks for Fixes tag and gets patch to stable trees, if you want
you patch backported the tag is required.

2- To help other developers that wants to work on your code, these tags helps to
trace easier and understand code easier.

3- Helps reviewers understand the scope and help on prioritization.


If this is not fix please drop "fix" from patch title and be aware that this
won't be included into stable trees. If this is a fix please add the fixes line.


[1]
technically a code requires update:
- To add a new feature (justify a new expectation)
- To refactor (improve readability, improve re-usability etc.. but same
functionality)
- To fix the functionality to make it behave as expected.

It is hard to claim a fix without clear expectations / requirements. And I think
code updates triggered because of expectation changes fits into first category.

> 
> 
> Also ./devtools/check-git-log.sh complains about long title, what about
> something like (just a sample):
> "net/bnxt: fix flow director with same cmd different queue"
> 
> >
> > Signed-off-by: Somnath Kotur  >
> 
> <...>
> 
> > @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
> >                       goto free_filter;
> >               filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
> >
> > -             match = bnxt_match_fdir(bp, filter);
> > +             if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
> > +                     vnic = STAILQ_FIRST(&bp->ff_pool[0]);
> > +             else
> > +                     vnic =
> > +                     STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
> 
> Is this done because of column limit? If so I would prefer a few extra 
> chars
> instead of this assignment.
> 
> Yes, please thank you , it was going over by 1 character :) 
> 
> 
> btw, not related to this patch, but in this switch there are a few "/*
> FALLTHROUGH */" comments but they may not be required (or wrong), can you 
> please
> check.
> 
> Sure , will do. 
> 
> <...>
> 
> 
> Thanks
> Som

Re: [dpdk-dev] [PATCH v2 23/41] mempool: add support for the new allocation methods

[Burakov, Anatoly]

On 20-Mar-18 11:35 AM, Shreyansh Jain wrote:

Hello Anatoly,

On Wed, Mar 7, 2018 at 10:26 PM, Anatoly Burakov
 wrote:

If a user has specified that the zone should have contiguous memory,
use the new _contig allocation API's instead of normal ones.
Otherwise, account for the fact that unless we're in IOVA_AS_VA
mode, we cannot guarantee that the pages would be physically
contiguous, so we calculate the memzone size and alignments as if
we were getting the smallest page size available.

Signed-off-by: Anatoly Burakov 
---


[...]


  static void
  mempool_add_elem(struct rte_mempool *mp, void *obj, rte_iova_t iova)
  {
@@ -549,6 +570,7 @@ rte_mempool_populate_default(struct rte_mempool *mp)
 unsigned mz_id, n;
 unsigned int mp_flags;
 int ret;
+   bool force_contig, no_contig;

 /* mempool must not be populated */
 if (mp->nb_mem_chunks != 0)
@@ -563,10 +585,46 @@ rte_mempool_populate_default(struct rte_mempool *mp)
 /* update mempool capabilities */
 mp->flags |= mp_flags;

-   if (rte_eal_has_hugepages()) {
-   pg_shift = 0; /* not needed, zone is physically contiguous */
+   no_contig = mp->flags & MEMPOOL_F_NO_PHYS_CONTIG;
+   force_contig = mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG;
+
+   /*
+* there are several considerations for page size and page shift here.
+*
+* if we don't need our mempools to have physically contiguous objects,
+* then just set page shift and page size to 0, because the user has
+* indicated that there's no need to care about anything.


I think the above case is not handled properly here.
reason below...


+*
+* if we do need contiguous objects, there is also an option to reserve
+* the entire mempool memory as one contiguous block of memory, in
+* which case the page shift and alignment wouldn't matter as well.
+*
+* if we require contiguous objects, but not necessarily the entire
+* mempool reserved space to be contiguous, then there are two options.
+*
+* if our IO addresses are virtual, not actual physical (IOVA as VA
+* case), then no page shift needed - our memory allocation will give us
+* contiguous physical memory as far as the hardware is concerned, so
+* act as if we're getting contiguous memory.
+*
+* if our IO addresses are physical, we may get memory from bigger
+* pages, or we might get memory from smaller pages, and how much of it
+* we require depends on whether we want bigger or smaller pages.
+* However, requesting each and every memory size is too much work, so
+* what we'll do instead is walk through the page sizes available, pick
+* the smallest one and set up page shift to match that one. We will be
+* wasting some space this way, but it's much nicer than looping around
+* trying to reserve each and every page size.
+*/
+
+   if (no_contig || force_contig || rte_eal_iova_mode() == RTE_IOVA_VA) {
 pg_sz = 0;
+   pg_shift = 0;
 align = RTE_CACHE_LINE_SIZE;


So, assuming dpaa2 as example, I ran testpmd. IOVA=VA is the mode.
pg_sz = 0 is set.
same as before applying the hotplug patchset except that earlier this
decision was purely based on availability of hugepages
(rte_eal_has_hugepages()).
Moving on...


+   } else if (rte_eal_has_hugepages()) {
+   pg_sz = get_min_page_size();
+   pg_shift = rte_bsf32(pg_sz);
+   align = pg_sz;
 } else {
 pg_sz = getpagesize();
 pg_shift = rte_bsf32(pg_sz);
@@ -585,23 +643,34 @@ rte_mempool_populate_default(struct rte_mempool *mp)
 goto fail;
 }

-   mz = rte_memzone_reserve_aligned(mz_name, size,
-   mp->socket_id, mz_flags, align);
-   /* not enough memory, retry with the biggest zone we have */
-   if (mz == NULL)
-   mz = rte_memzone_reserve_aligned(mz_name, 0,
+   if (force_contig) {
+   /*
+* if contiguous memory for entire mempool memory was
+* requested, don't try reserving again if we fail.
+*/
+   mz = rte_memzone_reserve_aligned_contig(mz_name, size,
+   mp->socket_id, mz_flags, align);
+   } else {
+   mz = rte_memzone_reserve_aligned(mz_name, size,
 mp->socket_id, mz_flags, align);
+   /* not enough memory, retry with the biggest zone we
+* have
+*/
+   if (mz == NULL)
+   mz = rte_memzone_reserve_aligned(mz_name, 0,
+ 

Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Somnath Kotur]

On Fri, Mar 23, 2018 at 4:11 PM, Ferruh Yigit 
wrote:

> On 3/23/2018 3:34 AM, Somnath Kotur wrote:
> > Hi Ferruh,
> >
> >
> > On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit  > > wrote:
> >
> > On 2/22/2018 2:58 AM, Somnath Kotur wrote:
> >
> > Please start with lowercase after "net/bnxt: fix"
> >
> > > When user reissues same flow director cmd with a different queue
> > > update the existing filter to redirect flow to the new desired
> > > queue as destination just like the other filters like 5 tuple and
> > > generic flow.
> >
> > Can you please add a fixes line?
> >
> > Actually, I'm not sure if this qualifies as a 'fix for a regression', it
> > prescribes a new behavior for this scenario (of same flow-director cmd ,
> > different queue)  we never handled this before at all , Do you still
> think it
> > needs it ?
>
> Patch title starts with "fix bug with ..." :)
>
Yes i was aware of the irony,  while i was asking the question, was just
thinking /introspecting aloud :)
Thank you for the detailed explanation. All things considered, i think it's
better for me to right now go
with the 'fix' since it helps in so many ways that i hadn't considered ...
So will send out the respin soon with the 'fix' intact


>
> Previously it was returning error when filter exist, now it is updating the
> existing filter with new destination. If this is fix or not depends on
> expected
> behavior [1].
>
> Practical reasons of having fixes tag:
> 1- Stable trees checks for Fixes tag and gets patch to stable trees, if
> you want
> you patch backported the tag is required.
>
> 2- To help other developers that wants to work on your code, these tags
> helps to
> trace easier and understand code easier.
>
> 3- Helps reviewers understand the scope and help on prioritization.
>
>
> If this is not fix please drop "fix" from patch title and be aware that
> this
> won't be included into stable trees. If this is a fix please add the fixes
> line.
>
>
> [1]
> technically a code requires update:
> - To add a new feature (justify a new expectation)
> - To refactor (improve readability, improve re-usability etc.. but same
> functionality)
> - To fix the functionality to make it behave as expected.
>
> It is hard to claim a fix without clear expectations / requirements. And I
> think
> code updates triggered because of expectation changes fits into first
> category.
>
> >
> >
> > Also ./devtools/check-git-log.sh complains about long title, what
> about
> > something like (just a sample):
> > "net/bnxt: fix flow director with same cmd different queue"
> >
> > >
> > > Signed-off-by: Somnath Kotur  > >
> >
> > <...>
> >
> > > @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
> > >   goto free_filter;
> > >   filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
> > >
> > > - match = bnxt_match_fdir(bp, filter);
> > > + if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
> > > + vnic = STAILQ_FIRST(&bp->ff_pool[0]);
> > > + else
> > > + vnic =
> > > + STAILQ_FIRST(&bp->ff_pool[
> fdir->action.rx_queue]);
> >
> > Is this done because of column limit? If so I would prefer a few
> extra chars
> > instead of this assignment.
> >
> > Yes, please thank you , it was going over by 1 character :)
> >
> >
> > btw, not related to this patch, but in this switch there are a few
> "/*
> > FALLTHROUGH */" comments but they may not be required (or wrong),
> can you please
> > check.
> >
> > Sure , will do.
> >
> > <...>
> >
> >
> > Thanks
> > Som
>
>

[dpdk-dev] [PATCH v3 0/7] Change DPAA2 to dynamic logging

[Shreyansh Jain]

::History::
v3:
 - Fixed review comments
 - one debug macro un-removed (CONFIG_RTE_LIBRTE_DPAA2_DEBUG_DRIVER)
   which is continued to be used by dpaa2 mempool driver

v2:
 - Fixed dynamic log identifier names (Ferruh)
 - Added dependency information in cover letter (Hemant)
 - Fixed some checkpatch issues
 - Updated some log messages

::Dependency::

This patchset is dependent on: master (acaa9ee991b)

::Introduction::

DPAA2 devices are enabled by following modules:
 - bus/fslmc
 - net/dpaa2
 - mempool/dpaa2
 - event/dpaa2
 - crypto/dpaa2_sec

This patch series converts the existing static debugging macros - for
control and datapath, both - into dynamic logging macros.

Identified for logs are:
FSLMC bus - bus.fslmc
Mempool Driver - mempool.dpaa2
Ethernet PMD - pmd.net.dpaa2
Eventdev PMD - pmd.event.dpaa2
Crypto PMD - pmd.crypto.dpaa2

This patchset also removed the old unused macros (unsed post dynamic
logging change) for debugging from config and documentation.

Shreyansh Jain (7):
  bus/fslmc: change to dynamic logging
  mempool/dpaa2: change to dynamic logging
  net/dpaa2: change into dynamic logging
  event/dpaa2: change to dynamic logging
  bus/fslmc: remove unused debug macros
  crypto/dpaa2_sec: fix incorrect debugging prints
  crypto/dpaa2_sec: change to dynamic logging

 config/common_base|   8 -
 config/defconfig_arm64-dpaa2-linuxapp-gcc |  15 --
 doc/guides/cryptodevs/dpaa2_sec.rst   |  21 +-
 doc/guides/eventdevs/dpaa2.rst|  14 +-
 doc/guides/nics/dpaa2.rst |  42 +--
 drivers/bus/fslmc/Makefile|   5 -
 drivers/bus/fslmc/fslmc_bus.c |  64 +++--
 drivers/bus/fslmc/fslmc_logs.h|  69 +++--
 drivers/bus/fslmc/fslmc_vfio.c| 151 +--
 drivers/bus/fslmc/portal/dpaa2_hw_dpbp.c  |  12 +-
 drivers/bus/fslmc/portal/dpaa2_hw_dpci.c  |  22 +-
 drivers/bus/fslmc/portal/dpaa2_hw_dpio.c  | 101 ---
 drivers/crypto/dpaa2_sec/Makefile |   5 -
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c   | 370 +-
 drivers/crypto/dpaa2_sec/dpaa2_sec_logs.h |  62 +++--
 drivers/event/dpaa2/dpaa2_eventdev.c  |  49 ++--
 drivers/event/dpaa2/dpaa2_eventdev_logs.h |  10 +-
 drivers/event/dpaa2/dpaa2_hw_dpcon.c  |  15 +-
 drivers/mempool/dpaa2/Makefile|   6 -
 drivers/mempool/dpaa2/dpaa2_hw_mempool.c  |  60 +++--
 drivers/mempool/dpaa2/dpaa2_hw_mempool_logs.h |  38 +++
 drivers/net/dpaa2/Makefile|   6 -
 drivers/net/dpaa2/base/dpaa2_hw_dpni.c|  30 +--
 drivers/net/dpaa2/dpaa2_ethdev.c  | 290 ++--
 drivers/net/dpaa2/dpaa2_pmd_logs.h|  41 +++
 drivers/net/dpaa2/dpaa2_rxtx.c|  59 ++--
 26 files changed, 833 insertions(+), 732 deletions(-)
 create mode 100644 drivers/mempool/dpaa2/dpaa2_hw_mempool_logs.h
 create mode 100644 drivers/net/dpaa2/dpaa2_pmd_logs.h

-- 
2.14.1

[dpdk-dev] [PATCH v3 1/7] bus/fslmc: change to dynamic logging

[Shreyansh Jain]

Signed-off-by: Shreyansh Jain 
Acked-by: Hemant Agrawal 
---
 drivers/bus/fslmc/Makefile   |   5 -
 drivers/bus/fslmc/fslmc_bus.c|  64 +
 drivers/bus/fslmc/fslmc_logs.h   |  31 +++
 drivers/bus/fslmc/fslmc_vfio.c   | 151 +++
 drivers/bus/fslmc/portal/dpaa2_hw_dpbp.c |  12 +--
 drivers/bus/fslmc/portal/dpaa2_hw_dpci.c |  22 ++---
 drivers/bus/fslmc/portal/dpaa2_hw_dpio.c | 101 ++---
 7 files changed, 213 insertions(+), 173 deletions(-)

diff --git a/drivers/bus/fslmc/Makefile b/drivers/bus/fslmc/Makefile
index 952b4c02b..93870bae3 100644
--- a/drivers/bus/fslmc/Makefile
+++ b/drivers/bus/fslmc/Makefile
@@ -10,13 +10,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
 LIB = librte_bus_fslmc.a
 
 CFLAGS += -DALLOW_EXPERIMENTAL_API
-ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT),y)
-CFLAGS += -O0 -g
-CFLAGS += "-Wno-error"
-else
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
-endif
 
 CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc
 CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc/mc
diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index 5ee0beb85..4d29b53b1 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -18,9 +18,9 @@
 
 #include 
 #include 
+#include "fslmc_logs.h"
 
-#define FSLMC_BUS_LOG(level, fmt, args...) \
-   RTE_LOG(level, EAL, fmt "\n", ##args)
+int dpaa2_logtype_bus;
 
 #define VFIO_IOMMU_GROUP_PATH "/sys/kernel/iommu_groups"
 
@@ -93,6 +93,25 @@ insert_in_device_list(struct rte_dpaa2_device *newdev)
TAILQ_INSERT_TAIL(&rte_fslmc_bus.device_list, newdev, next);
 }
 
+static void
+dump_device_list(void)
+{
+   struct rte_dpaa2_device *dev;
+   uint32_t global_log_level;
+   int local_log_level;
+
+   /* Only if the log level has been set to Debugging, print list */
+   global_log_level = rte_log_get_global_level();
+   local_log_level = rte_log_get_level(dpaa2_logtype_bus);
+   if (global_log_level == RTE_LOG_DEBUG ||
+   local_log_level == RTE_LOG_DEBUG) {
+   DPAA2_BUS_DEBUG("List of devices scanned on bus:");
+   TAILQ_FOREACH(dev, &rte_fslmc_bus.device_list, next) {
+   DPAA2_BUS_DEBUG("%s", dev->device.name);
+   }
+   }
+}
+
 static int
 scan_one_fslmc_device(char *dev_name)
 {
@@ -109,7 +128,7 @@ scan_one_fslmc_device(char *dev_name)
/* Creating a temporary copy to perform cut-parse over string */
dup_dev_name = strdup(dev_name);
if (!dup_dev_name) {
-   FSLMC_BUS_LOG(ERR, "Out of memory.");
+   DPAA2_BUS_ERR("Unable to allocate device name memory");
return -ENOMEM;
}
 
@@ -120,7 +139,7 @@ scan_one_fslmc_device(char *dev_name)
 */
dev = calloc(1, sizeof(struct rte_dpaa2_device));
if (!dev) {
-   FSLMC_BUS_LOG(ERR, "Out of memory.");
+   DPAA2_BUS_ERR("Unable to allocate device object");
free(dup_dev_name);
return -ENOMEM;
}
@@ -128,7 +147,7 @@ scan_one_fslmc_device(char *dev_name)
/* Parse the device name and ID */
t_ptr = strtok(dup_dev_name, ".");
if (!t_ptr) {
-   FSLMC_BUS_LOG(ERR, "Incorrect device string observed.");
+   DPAA2_BUS_ERR("Incorrect device name observed");
goto cleanup;
}
if (!strncmp("dpni", t_ptr, 4))
@@ -153,15 +172,14 @@ scan_one_fslmc_device(char *dev_name)
 
t_ptr = strtok(NULL, ".");
if (!t_ptr) {
-   FSLMC_BUS_LOG(ERR, "Incorrect device string observed (%s).",
- t_ptr);
+   DPAA2_BUS_ERR("Incorrect device string observed (%s)", t_ptr);
goto cleanup;
}
 
sscanf(t_ptr, "%hu", &dev->object_id);
dev->device.name = strdup(dev_name);
if (!dev->device.name) {
-   FSLMC_BUS_LOG(ERR, "Out of memory.");
+   DPAA2_BUS_ERR("Unable to clone device name. Out of memory");
goto cleanup;
}
 
@@ -193,8 +211,7 @@ rte_fslmc_scan(void)
int groupid;
 
if (process_once) {
-   FSLMC_BUS_LOG(DEBUG,
- "Fslmc bus already scanned. Not rescanning");
+   DPAA2_BUS_DEBUG("Fslmc bus already scanned. Not rescanning");
return 0;
}
process_once = 1;
@@ -208,7 +225,7 @@ rte_fslmc_scan(void)
groupid);
dir = opendir(fslmc_dirpath);
if (!dir) {
-   FSLMC_BUS_LOG(ERR, "Unable to open VFIO group dir.");
+   DPAA2_BUS_ERR("Unable to open VFIO group directory");
goto scan_fail;
}
 
@@ -224,9 +241,12 @@ rte_fslmc_scan(void)
device_count += 1;
}
 
-   FSLMC_BUS_LOG(INFO, "fslmc: Bus scan completed");
-
closedir(dir);
+
+   DPAA2_BUS_INFO("FSLMC Bus scan complete

[dpdk-dev] [PATCH v3 2/7] mempool/dpaa2: change to dynamic logging

[Shreyansh Jain]

Signed-off-by: Shreyansh Jain 
---
 drivers/mempool/dpaa2/Makefile|  6 ---
 drivers/mempool/dpaa2/dpaa2_hw_mempool.c  | 60 +--
 drivers/mempool/dpaa2/dpaa2_hw_mempool_logs.h | 38 +
 3 files changed, 75 insertions(+), 29 deletions(-)
 create mode 100644 drivers/mempool/dpaa2/dpaa2_hw_mempool_logs.h

diff --git a/drivers/mempool/dpaa2/Makefile b/drivers/mempool/dpaa2/Makefile
index efaac96e7..f0edb32ce 100644
--- a/drivers/mempool/dpaa2/Makefile
+++ b/drivers/mempool/dpaa2/Makefile
@@ -9,14 +9,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
 #
 LIB = librte_mempool_dpaa2.a
 
-ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT),y)
-CFLAGS += -O0 -g
-CFLAGS += "-Wno-error"
-else
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
-endif
-
 CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc
 CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc/qbman/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c 
b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
index 1a618ae1b..ce7a4c577 100644
--- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
+++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
@@ -27,10 +27,14 @@
 #include 
 #include 
 #include "dpaa2_hw_mempool.h"
+#include "dpaa2_hw_mempool_logs.h"
 
 struct dpaa2_bp_info rte_dpaa2_bpid_info[MAX_BPID];
 static struct dpaa2_bp_list *h_bp_list;
 
+/* Dynamic logging identified for mempool */
+int dpaa2_logtype_mempool;
+
 static int
 rte_hw_mbuf_create_pool(struct rte_mempool *mp)
 {
@@ -44,30 +48,30 @@ rte_hw_mbuf_create_pool(struct rte_mempool *mp)
avail_dpbp = dpaa2_alloc_dpbp_dev();
 
if (!avail_dpbp) {
-   PMD_DRV_LOG(ERR, "DPAA2 resources not available");
+   DPAA2_MEMPOOL_ERR("DPAA2 pool not available!");
return -ENOENT;
}
 
if (unlikely(!DPAA2_PER_LCORE_DPIO)) {
ret = dpaa2_affine_qbman_swp();
if (ret) {
-   RTE_LOG(ERR, PMD, "Failure in affining portal\n");
+   DPAA2_MEMPOOL_ERR("Failure in affining portal");
goto err1;
}
}
 
ret = dpbp_enable(&avail_dpbp->dpbp, CMD_PRI_LOW, avail_dpbp->token);
if (ret != 0) {
-   PMD_INIT_LOG(ERR, "Resource enable failure with"
-   " err code: %d\n", ret);
+   DPAA2_MEMPOOL_ERR("Resource enable failure with err code: %d",
+ ret);
goto err1;
}
 
ret = dpbp_get_attributes(&avail_dpbp->dpbp, CMD_PRI_LOW,
  avail_dpbp->token, &dpbp_attr);
if (ret != 0) {
-   PMD_INIT_LOG(ERR, "Resource read failure with"
-" err code: %d\n", ret);
+   DPAA2_MEMPOOL_ERR("Resource read failure with err code: %d",
+ ret);
goto err2;
}
 
@@ -75,7 +79,7 @@ rte_hw_mbuf_create_pool(struct rte_mempool *mp)
 sizeof(struct dpaa2_bp_info),
 RTE_CACHE_LINE_SIZE);
if (!bp_info) {
-   PMD_INIT_LOG(ERR, "No heap memory available for bp_info");
+   DPAA2_MEMPOOL_ERR("Unable to allocate buffer pool memory");
ret = -ENOMEM;
goto err2;
}
@@ -84,7 +88,7 @@ rte_hw_mbuf_create_pool(struct rte_mempool *mp)
bp_list = rte_malloc(NULL, sizeof(struct dpaa2_bp_list),
 RTE_CACHE_LINE_SIZE);
if (!bp_list) {
-   PMD_INIT_LOG(ERR, "No heap memory available");
+   DPAA2_MEMPOOL_ERR("Unable to allocate buffer pool memory");
ret = -ENOMEM;
goto err3;
}
@@ -112,7 +116,7 @@ rte_hw_mbuf_create_pool(struct rte_mempool *mp)
   sizeof(struct dpaa2_bp_info));
mp->pool_data = (void *)bp_info;
 
-   PMD_INIT_LOG(DEBUG, "BP List created for bpid =%d", dpbp_attr.bpid);
+   DPAA2_MEMPOOL_DEBUG("BP List created for bpid =%d", dpbp_attr.bpid);
 
h_bp_list = bp_list;
return 0;
@@ -134,7 +138,7 @@ rte_hw_mbuf_free_pool(struct rte_mempool *mp)
struct dpaa2_dpbp_dev *dpbp_node;
 
if (!mp->pool_data) {
-   PMD_DRV_LOG(ERR, "Not a valid dpaa22 pool");
+   DPAA2_MEMPOOL_ERR("Not a valid dpaa2 buffer pool");
return;
}
 
@@ -180,7 +184,7 @@ rte_dpaa2_mbuf_release(struct rte_mempool *pool 
__rte_unused,
if (unlikely(!DPAA2_PER_LCORE_DPIO)) {
ret = dpaa2_affine_qbman_swp();
if (ret != 0) {
-   RTE_LOG(ERR, PMD, "Failed to allocate IO portal\n");
+   DPAA2_MEMPOOL_ERR("Failed to allocate IO portal");
return;
}
}
@@ -250,7 +254,7 @@ rte_dpaa2_mbuf_alloc_bulk(struct rte_mempool *pool,
bp_info = mempool_to_bpinfo(pool);
 
   

[dpdk-dev] [PATCH v3 4/7] event/dpaa2: change to dynamic logging

[Shreyansh Jain]

Some changes had already been pushed via SHA:72654f090a11 patch. This
patch updates them.
Cc: nipun.gu...@nxp.com

Signed-off-by: Shreyansh Jain 
Acked-by: Nipun Gupta 
---
 doc/guides/eventdevs/dpaa2.rst| 14 -
 drivers/event/dpaa2/dpaa2_eventdev.c  | 49 +++
 drivers/event/dpaa2/dpaa2_eventdev_logs.h | 10 ---
 drivers/event/dpaa2/dpaa2_hw_dpcon.c  | 15 +-
 4 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/doc/guides/eventdevs/dpaa2.rst b/doc/guides/eventdevs/dpaa2.rst
index 5b8da95d7..ad94f24be 100644
--- a/doc/guides/eventdevs/dpaa2.rst
+++ b/doc/guides/eventdevs/dpaa2.rst
@@ -129,7 +129,19 @@ Example:
 
 .. code-block:: console
 
-./your_eventdev_application --vdev="event_dpaa2"
+   ./your_eventdev_application --vdev="event_dpaa2"
+
+Enabling logs
+-
+
+For enabling logs, use the following EAL parameter:
+
+.. code-block:: console
+
+   ./your_eventdev_application  --log-level=pmd.event.dpaa2,
+
+Using ``eventdev.dpaa2`` as log matching criteria, all Event PMD logs can be
+enabled which are lower than logging ``level``.
 
 Limitations
 ---
diff --git a/drivers/event/dpaa2/dpaa2_eventdev.c 
b/drivers/event/dpaa2/dpaa2_eventdev.c
index 8800b47f5..9d9c8d3db 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev.c
@@ -72,7 +72,7 @@ dpaa2_eventdev_enqueue_burst(void *port, const struct 
rte_event ev[],
if (unlikely(!DPAA2_PER_LCORE_DPIO)) {
ret = dpaa2_affine_qbman_swp();
if (ret) {
-   DPAA2_EVENTDEV_ERR("Failure in affining portal\n");
+   DPAA2_EVENTDEV_ERR("Failure in affining portal");
return 0;
}
}
@@ -122,7 +122,8 @@ dpaa2_eventdev_enqueue_burst(void *port, const struct 
rte_event ev[],
if (!loop)
return num_tx;
frames_to_send = loop;
-   DPAA2_EVENTDEV_ERR("Unable to allocate memory");
+   DPAA2_EVENTDEV_ERR(
+   "Unable to allocate event object");
goto send_partial;
}
rte_memcpy(ev_temp, event, sizeof(struct rte_event));
@@ -167,9 +168,9 @@ static void dpaa2_eventdev_dequeue_wait(uint64_t 
timeout_ticks)
 * case to avoid the problem.
 */
if (errno == EINTR) {
-   DPAA2_EVENTDEV_DEBUG("epoll_wait fails\n");
+   DPAA2_EVENTDEV_DEBUG("epoll_wait fails");
if (i++ > 10)
-   DPAA2_EVENTDEV_DEBUG("Dequeue burst Failed\n");
+   DPAA2_EVENTDEV_DEBUG("Dequeue burst Failed");
goto RETRY;
}
}
@@ -227,7 +228,7 @@ dpaa2_eventdev_dequeue_burst(void *port, struct rte_event 
ev[],
if (unlikely(!DPAA2_PER_LCORE_DPIO)) {
ret = dpaa2_affine_qbman_swp();
if (ret) {
-   DPAA2_EVENTDEV_ERR("Failure in affining portal\n");
+   DPAA2_EVENTDEV_ERR("Failure in affining portal");
return 0;
}
}
@@ -263,7 +264,7 @@ dpaa2_eventdev_dequeue_burst(void *port, struct rte_event 
ev[],
rxq->cb(swp, fd, dq, rxq, &ev[num_pkts]);
} else {
qbman_swp_dqrr_consume(swp, dq);
-   DPAA2_EVENTDEV_ERR("Null Return VQ received\n");
+   DPAA2_EVENTDEV_ERR("Null Return VQ received");
return 0;
}
 
@@ -335,7 +336,7 @@ dpaa2_eventdev_configure(const struct rte_eventdev *dev)
priv->event_dev_cfg = conf->event_dev_cfg;
 
DPAA2_EVENTDEV_DEBUG("Configured eventdev devid=%d",
-   dev->data->dev_id);
+dev->data->dev_id);
return 0;
 }
 
@@ -502,8 +503,8 @@ dpaa2_eventdev_port_link(struct rte_eventdev *dev, void 
*port,
CMD_PRI_LOW, dpaa2_portal->dpio_dev->token,
evq_info->dpcon->dpcon_id, &channel_index);
if (ret < 0) {
-   DPAA2_EVENTDEV_ERR("Static dequeue cfg failed with ret: 
%d\n",
-   ret);
+   DPAA2_EVENTDEV_ERR(
+   "Static dequeue config failed: err(%d)", ret);
goto err;
}
 
@@ -587,8 +588,8 @@ dpaa2_eventdev_eth_queue_add_all(const struct rte_eventdev 
*dev,
ret = dpaa2_eth_eventq_attach(eth_dev, i,
dpcon_id, queue_conf);
if (ret) {
-   DPAA2_EVENTDEV_ERR("dpaa2_eth_eventq_attach failed: ret 
%d\n

[dpdk-dev] [PATCH v3 5/7] bus/fslmc: remove unused debug macros

[Shreyansh Jain]

Signed-off-by: Shreyansh Jain 
Acked-by: Hemant Agrawal 
---
 drivers/bus/fslmc/fslmc_logs.h | 40 
 1 file changed, 40 deletions(-)

diff --git a/drivers/bus/fslmc/fslmc_logs.h b/drivers/bus/fslmc/fslmc_logs.h
index 1f3fe8e66..9750b8c8d 100644
--- a/drivers/bus/fslmc/fslmc_logs.h
+++ b/drivers/bus/fslmc/fslmc_logs.h
@@ -38,44 +38,4 @@ extern int dpaa2_logtype_bus;
 #define DPAA2_BUS_DP_WARN(fmt, args...) \
DPAA2_BUS_DP_LOG(WARNING, fmt, ## args)
 
-#define PMD_INIT_LOG(level, fmt, args...) \
-   RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ##args)
-
-#ifdef RTE_LIBRTE_DPAA2_DEBUG_INIT
-#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
-#else
-#define PMD_INIT_FUNC_TRACE() do { } while (0)
-#endif
-
-#ifdef RTE_LIBRTE_DPAA2_DEBUG_RX
-#define PMD_RX_LOG(level, fmt, args...) \
-   RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
-#else
-#define PMD_RX_LOG(level, fmt, args...) do { } while (0)
-#endif
-
-#ifdef RTE_LIBRTE_DPAA2_DEBUG_TX
-#define PMD_TX_LOG(level, fmt, args...) \
-   RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
-#else
-#define PMD_TX_LOG(level, fmt, args...) do { } while (0)
-#endif
-
-#ifdef RTE_LIBRTE_DPAA2_DEBUG_TX_FREE
-#define PMD_TX_FREE_LOG(level, fmt, args...) \
-   RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
-#else
-#define PMD_TX_FREE_LOG(level, fmt, args...) do { } while (0)
-#endif
-
-#ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
-#define PMD_DRV_LOG_RAW(level, fmt, args...) \
-   RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
-#else
-#define PMD_DRV_LOG_RAW(level, fmt, args...) do { } while (0)
-#endif
-
-#define PMD_DRV_LOG(level, fmt, args...) \
-   PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
-
 #endif /* _FSLMC_LOGS_H_ */
-- 
2.14.1

[dpdk-dev] [PATCH v3 6/7] crypto/dpaa2_sec: fix incorrect debugging prints

[Shreyansh Jain]

Digest and IV length variable declarations have changed.
These were escaping builds as the debugging macro was disabled.
During dynamic logging change, they were discoverd.

Fixes: 0fbd75a99fc9 ("cryptodev: move IV parameters to session")
Fixes: 7f0034275a24 ("cryptodev: remove digest length from crypto op")
Cc: pablo.de.lara.gua...@intel.com

Signed-off-by: Shreyansh Jain 
Acked-by: Hemant Agrawal 
---
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c 
b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index 9a7484554..0c28b1d05 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -136,7 +136,7 @@ build_authenc_gcm_sg_fd(dpaa2_sec_session *sess,
   "iv-len=%d data_off: 0x%x\n",
   sym_op->aead.data.offset,
   sym_op->aead.data.length,
-  sym_op->aead.digest.length,
+  sess->digest_length,
   sess->iv.length,
   sym_op->m_src->data_off);
 
@@ -301,7 +301,7 @@ build_authenc_gcm_fd(dpaa2_sec_session *sess,
   "iv-len=%d data_off: 0x%x\n",
   sym_op->aead.data.offset,
   sym_op->aead.data.length,
-  sym_op->aead.digest.length,
+  sess->digest_length,
   sess->iv.length,
   sym_op->m_src->data_off);
 
@@ -433,10 +433,10 @@ build_authenc_sg_fd(dpaa2_sec_session *sess,
"cipher_off: 0x%x/length %d, iv-len=%d data_off: 
0x%x\n",
   sym_op->auth.data.offset,
   sym_op->auth.data.length,
-  sym_op->auth.digest.length,
+  sess->digest_length,
   sym_op->cipher.data.offset,
   sym_op->cipher.data.length,
-  sym_op->cipher.iv.length,
+  sess->iv.length,
   sym_op->m_src->data_off);
 
/* Configure Output FLE with Scatter/Gather Entry */
@@ -877,7 +877,7 @@ build_cipher_sg_fd(dpaa2_sec_session *sess, struct 
rte_crypto_op *op,
"CIPHER SG: cipher_off: 0x%x/length %d,ivlen=%d 
data_off: 0x%x",
   sym_op->cipher.data.offset,
   sym_op->cipher.data.length,
-  sym_op->cipher.iv.length,
+  sess->iv.length,
   sym_op->m_src->data_off);
 
/* o/p fle */
-- 
2.14.1

[dpdk-dev] [PATCH v3 3/7] net/dpaa2: change into dynamic logging

[Shreyansh Jain]

Signed-off-by: Shreyansh Jain 
---
 config/common_base|   5 -
 config/defconfig_arm64-dpaa2-linuxapp-gcc |   8 -
 doc/guides/nics/dpaa2.rst |  42 +++--
 drivers/net/dpaa2/Makefile|   6 -
 drivers/net/dpaa2/base/dpaa2_hw_dpni.c|  30 ++--
 drivers/net/dpaa2/dpaa2_ethdev.c  | 290 +++---
 drivers/net/dpaa2/dpaa2_pmd_logs.h|  41 +
 drivers/net/dpaa2/dpaa2_rxtx.c|  59 +++---
 8 files changed, 259 insertions(+), 222 deletions(-)
 create mode 100644 drivers/net/dpaa2/dpaa2_pmd_logs.h

diff --git a/config/common_base b/config/common_base
index ee10b449b..d87b53c8a 100644
--- a/config/common_base
+++ b/config/common_base
@@ -189,11 +189,6 @@ CONFIG_RTE_LIBRTE_DPAA2_USE_PHYS_IOVA=y
 # Compile burst-oriented NXP DPAA2 PMD driver
 #
 CONFIG_RTE_LIBRTE_DPAA2_PMD=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_DRIVER=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_RX=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_TX=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_TX_FREE=n
 
 #
 # Compile burst-oriented Amazon ENA PMD driver
diff --git a/config/defconfig_arm64-dpaa2-linuxapp-gcc 
b/config/defconfig_arm64-dpaa2-linuxapp-gcc
index afdbc347b..b3b958bf8 100644
--- a/config/defconfig_arm64-dpaa2-linuxapp-gcc
+++ b/config/defconfig_arm64-dpaa2-linuxapp-gcc
@@ -26,15 +26,7 @@ CONFIG_RTE_LIBRTE_VHOST_NUMA=n
 # Compile Support Libraries for DPAA2
 #
 CONFIG_RTE_LIBRTE_DPAA2_USE_PHYS_IOVA=n
-
-#
-# Compile burst-oriented NXP DPAA2 PMD driver
-#
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT=n
 CONFIG_RTE_LIBRTE_DPAA2_DEBUG_DRIVER=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_RX=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_TX=n
-CONFIG_RTE_LIBRTE_DPAA2_DEBUG_TX_FREE=n
 
 #
 # Compile NXP DPAA2 crypto sec driver for CAAM HW
diff --git a/doc/guides/nics/dpaa2.rst b/doc/guides/nics/dpaa2.rst
index 9c66edd45..8e38efff4 100644
--- a/doc/guides/nics/dpaa2.rst
+++ b/doc/guides/nics/dpaa2.rst
@@ -494,28 +494,12 @@ Please note that enabling debugging options may affect 
system performance.
 
 - ``CONFIG_RTE_LIBRTE_DPAA2_DEBUG_DRIVER`` (default ``n``)
 
-  Toggle display of generic debugging messages
+  Toggle display of debugging messages/logic
 
 - ``CONFIG_RTE_LIBRTE_DPAA2_USE_PHYS_IOVA`` (default ``y``)
 
   Toggle to use physical address vs virtual address for hardware accelerators.
 
-- ``CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT`` (default ``n``)
-
-  Toggle display of initialization related messages.
-
-- ``CONFIG_RTE_LIBRTE_DPAA2_DEBUG_RX`` (default ``n``)
-
-  Toggle display of receive fast path run-time message
-
-- ``CONFIG_RTE_LIBRTE_DPAA2_DEBUG_TX`` (default ``n``)
-
-  Toggle display of transmit fast path run-time message
-
-- ``CONFIG_RTE_LIBRTE_DPAA2_DEBUG_TX_FREE`` (default ``n``)
-
-  Toggle display of transmit fast path buffer free run-time message
-
 Driver compilation and testing
 --
 
@@ -532,8 +516,7 @@ for details.
 
.. code-block:: console
 
-  ./arm64-dpaa2-linuxapp-gcc/testpmd -c 0xff -n 1 \
--- -i --portmask=0x3 --nb-cores=1 --no-flush-rx
+  ./testpmd -c 0xff -n 1 -- -i --portmask=0x3 --nb-cores=1 --no-flush-rx
 
   .
   EAL: Registered [pci] bus.
@@ -557,6 +540,27 @@ for details.
   Done
   testpmd>
 
+Enabling logs
+-
+
+For enabling logging for DPAA2 PMD, following log-level prefix can be used:
+
+ .. code-block:: console
+
+  --log-level=bus.fslmc, -- ...
+
+Using ``bus.fslmc`` as log matching criteria, all FSLMC bus logs can be enabled
+which are lower than logging ``level``.
+
+ Or
+
+ .. code-block:: console
+
+  --log-level=pmd.net.dpaa2, -- ...
+
+Using ``pmd.dpaa2`` as log matching criteria, all PMD logs can be enabled
+which are lower than logging ``level``.
+
 Limitations
 ---
 
diff --git a/drivers/net/dpaa2/Makefile b/drivers/net/dpaa2/Makefile
index 068e9d362..1b707adaa 100644
--- a/drivers/net/dpaa2/Makefile
+++ b/drivers/net/dpaa2/Makefile
@@ -10,14 +10,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
 #
 LIB = librte_pmd_dpaa2.a
 
-ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT),y)
-CFLAGS += -O0 -g
-CFLAGS += "-Wno-error"
-else
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
-endif
-
 CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa2
 CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa2/mc
 CFLAGS += -I$(RTE_SDK)/drivers/bus/fslmc
diff --git a/drivers/net/dpaa2/base/dpaa2_hw_dpni.c 
b/drivers/net/dpaa2/base/dpaa2_hw_dpni.c
index 4b60f5610..713a41bf3 100644
--- a/drivers/net/dpaa2/base/dpaa2_hw_dpni.c
+++ b/drivers/net/dpaa2/base/dpaa2_hw_dpni.c
@@ -17,7 +17,7 @@
 #include 
 #include 
 
-#include 
+#include 
 #include 
 #include 
 
@@ -42,7 +42,7 @@ dpaa2_setup_flow_dist(struct rte_eth_dev *eth_dev,
p_params = rte_malloc(
NULL, DIST_PARAM_IOVA_SIZE, RTE_CACHE_LINE_SIZE);
if (!p_params) {
-   PMD_INIT_LOG(ERR, "Memory unavailable");
+   DPAA2_PMD_ERR("Unable to allocate flow-dist parameters");
return -ENOMEM;
}
 

[dpdk-dev] [PATCH v3 7/7] crypto/dpaa2_sec: change to dynamic logging

[Shreyansh Jain]

Signed-off-by: Shreyansh Jain 
Acked-by: Hemant Agrawal 
---
 config/common_base  |   3 -
 config/defconfig_arm64-dpaa2-linuxapp-gcc   |   7 -
 doc/guides/cryptodevs/dpaa2_sec.rst |  21 +-
 drivers/crypto/dpaa2_sec/Makefile   |   5 -
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 366 ++--
 drivers/crypto/dpaa2_sec/dpaa2_sec_logs.h   |  62 +++--
 6 files changed, 228 insertions(+), 236 deletions(-)

diff --git a/config/common_base b/config/common_base
index d87b53c8a..3b109b995 100644
--- a/config/common_base
+++ b/config/common_base
@@ -451,9 +451,6 @@ CONFIG_RTE_LIBRTE_PMD_ARMV8_CRYPTO_DEBUG=n
 # Compile NXP DPAA2 crypto sec driver for CAAM HW
 #
 CONFIG_RTE_LIBRTE_PMD_DPAA2_SEC=n
-CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_INIT=n
-CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_DRIVER=n
-CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_RX=n
 CONFIG_RTE_DPAA2_SEC_PMD_MAX_NB_SESSIONS=2048
 
 #
diff --git a/config/defconfig_arm64-dpaa2-linuxapp-gcc 
b/config/defconfig_arm64-dpaa2-linuxapp-gcc
index b3b958bf8..ecac994bf 100644
--- a/config/defconfig_arm64-dpaa2-linuxapp-gcc
+++ b/config/defconfig_arm64-dpaa2-linuxapp-gcc
@@ -27,10 +27,3 @@ CONFIG_RTE_LIBRTE_VHOST_NUMA=n
 #
 CONFIG_RTE_LIBRTE_DPAA2_USE_PHYS_IOVA=n
 CONFIG_RTE_LIBRTE_DPAA2_DEBUG_DRIVER=n
-
-#
-# Compile NXP DPAA2 crypto sec driver for CAAM HW
-#
-CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_INIT=n
-CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_DRIVER=n
-CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_RX=n
diff --git a/doc/guides/cryptodevs/dpaa2_sec.rst 
b/doc/guides/cryptodevs/dpaa2_sec.rst
index 5460a92da..5558ea593 100644
--- a/doc/guides/cryptodevs/dpaa2_sec.rst
+++ b/doc/guides/cryptodevs/dpaa2_sec.rst
@@ -189,15 +189,6 @@ Please note that enabling debugging options may affect 
system performance.
   By default it is only enabled in defconfig_arm64-dpaa2-* config.
   Toggle compilation of the ``librte_pmd_dpaa2_sec`` driver.
 
-* ``CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_INIT`` (default ``n``)
-  Toggle display of initialization related driver messages
-
-* ``CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_DRIVER`` (default ``n``)
-  Toggle display of driver runtime messages
-
-* ``CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_RX`` (default ``n``)
-  Toggle display of receive fast path run-time message
-
 * ``CONFIG_RTE_DPAA2_SEC_PMD_MAX_NB_SESSIONS``
   By default it is set as 2048 in defconfig_arm64-dpaa2-* config.
   It indicates Number of sessions to create in the session memory pool
@@ -212,3 +203,15 @@ following ``make`` command:
 
cd 
make config T=arm64-dpaa2-linuxapp-gcc install
+
+Enabling logs
+-
+
+For enabling logs, use the following EAL parameter:
+
+.. code-block:: console
+
+   ./your_crypto_application  --log-level=pmd.crypto.dpaa2,
+
+Using ``crypto.dpaa2`` as log matching criteria, all Crypto PMD logs can be
+enabled which are lower than logging ``level``.
diff --git a/drivers/crypto/dpaa2_sec/Makefile 
b/drivers/crypto/dpaa2_sec/Makefile
index cb6c63e69..da3d8f84f 100644
--- a/drivers/crypto/dpaa2_sec/Makefile
+++ b/drivers/crypto/dpaa2_sec/Makefile
@@ -18,13 +18,8 @@ LIB = librte_pmd_dpaa2_sec.a
 
 # build flags
 CFLAGS += -DALLOW_EXPERIMENTAL_API
-ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_INIT),y)
-CFLAGS += -O0 -g
-CFLAGS += "-Wno-error"
-else
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
-endif
 CFLAGS += -D _GNU_SOURCE
 
 ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c 
b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index 0c28b1d05..784b96db8 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -56,6 +56,8 @@ enum rta_sec_era rta_sec_era = RTA_SEC_ERA_8;
 
 static uint8_t cryptodev_driver_id;
 
+int dpaa2_logtype_sec;
+
 static inline int
 build_proto_fd(dpaa2_sec_session *sess,
   struct rte_crypto_op *op,
@@ -113,7 +115,7 @@ build_authenc_gcm_sg_fd(dpaa2_sec_session *sess,
fle = (struct qbman_fle *)rte_malloc(NULL, FLE_SG_MEM_SIZE,
RTE_CACHE_LINE_SIZE);
if (unlikely(!fle)) {
-   RTE_LOG(ERR, PMD, "GCM SG: Memory alloc failed for SGE\n");
+   DPAA2_SEC_ERR("GCM SG: Memory alloc failed for SGE");
return -1;
}
memset(fle, 0, FLE_SG_MEM_SIZE);
@@ -132,7 +134,7 @@ build_authenc_gcm_sg_fd(dpaa2_sec_session *sess,
DPAA2_SET_FD_COMPOUND_FMT(fd);
DPAA2_SET_FD_FLC(fd, DPAA2_VADDR_TO_IOVA(flc));
 
-   PMD_TX_LOG(DEBUG, "GCM SG: auth_off: 0x%x/length %d, digest-len=%d\n"
+   DPAA2_SEC_DP_DEBUG("GCM SG: auth_off: 0x%x/length %d, digest-len=%d\n"
   "iv-len=%d data_off: 0x%x\n",
   sym_op->aead.data.offset,
   sym_op->aead.data.length,
@@ -264,7 +266,7 @@ build_authenc_gcm_fd(dpaa2_sec_session *sess,
 */
retval = rte_mempool_get(priv->fle_pool, (void **)(&fle));
if (retval) {
-   RTE_LOG(ERR, PMD, "GCM: Memory alloc failed for SGE\n");
+   

Re: [dpdk-dev] [PATCH v2 04/18] eal: add lightweight kvarg parsing utility

[Neil Horman]

On Fri, Mar 23, 2018 at 10:31:22AM +0100, Gaëtan Rivet wrote:
> On Thu, Mar 22, 2018 at 08:53:49PM -0400, Neil Horman wrote:
> > On Thu, Mar 22, 2018 at 05:27:51PM +0100, Gaëtan Rivet wrote:
> > > On Thu, Mar 22, 2018 at 10:10:37AM -0400, Neil Horman wrote:
> > > > On Wed, Mar 21, 2018 at 05:32:24PM +, Wiles, Keith wrote:
> > > > > 
> > > > > 
> > > > > > On Mar 21, 2018, at 12:15 PM, Gaetan Rivet  
> > > > > > wrote:
> > > > > > 
> > > > > > This library offers a quick way to parse parameters passed with a
> > > > > > key=value syntax.
> > > > > > 
> > > > > > A single function is needed and finds the relevant element within 
> > > > > > the
> > > > > > text. No dynamic allocation is performed. It is possible to chain 
> > > > > > the
> > > > > > parsing of each pairs for quickly scanning a list.
> > > > > > 
> > > > > > This utility is private to the EAL and should allow avoiding having 
> > > > > > to
> > > > > > move around the more complete librte_kvargs.
> > > > > 
> > > > > What is the big advantage with this code and the librte_kvargs code. 
> > > > > Is it just no allocation, rte_kvargs needs to be build before parts 
> > > > > of EAL or what?
> > > > > 
> > > > > My concern is we have now two flavors one in EAL and one in 
> > > > > librte_kvargs, would it not be more reasonable to improve rte_kvargs 
> > > > > to remove your objections? I am all for fast, better, stronger code 
> > > > > :-)
> > > > > 
> > > > +1, this really doesn't make much sense to me.  Two parsing routines 
> > > > seems like
> > > > its just asking for us to have to fix parsing bugs in two places.  If 
> > > > allocation
> > > > is a concern, I don't see why you can't just change the malloc in
> > > > rte_kvargs_parse to an automatic allocation on the stack, or a 
> > > > preallocation set
> > > > of kvargs that can be shared from init time.
> > > 
> > > I think the existing allocation scheme is fine for other usages (in
> > > drivers and so on). Not for what I wanted to do.
> > > 
> > Ok, but thats an adressable issue.  you can bifurcate the parse function to 
> > an
> > internal function that accepts any preallocated kvargs struct, and export 
> > two
> > wrapper functions, one which allocates the struct from the heap, another 
> > which
> > allocated automatically on the stack.
> > 
> 
> Sure, everything is possible.
> 
Ok.

> > > >   librte_kvargs isn't 
> > > > necessecarily
> > > > the best parsing library ever, but its not bad, and it just seems wrong 
> > > > to go
> > > > re-inventing the wheel.
> > > > 
> > > 
> > > It serves a different purpose than the one I'm pursuing.
> > > 
> > > This helper is lightweight and private. If I wanted to integrate my
> > > needs with librte_kvargs, I would be adding new functionalities, making
> > > it more complex, and for a use-case that is useless for the vast
> > > majority of users of the lib.
> > > 
> > Ok, to that end:
> > 
> > 1) Privacy is not an issue (at least from my understanding of what your 
> > doing).
> > If we start with the assumption that librte_kvargs is capable of satisfying 
> > your
> > needs (even if its not done in an optimal way), the fact that your version 
> > of
> > the function is internal to the library doesn't seem overly relevant, unless
> > theres something critical to that privacy that I'm missing.
> > 
> 
> Privacy is only a point I brought up to say that the impact of this
> function is minimal. People looking to parse their kvargs should not
> have any ambiguity regarding how they should do so. Only librte_kvargs
> is available.
> 
Ok, would you also council others developing dpdk apps to write their own
parsing routines when what they needed was trivial for the existing library?
You are people too :)

> > 2) Lightweight function  seems like something that can be integrated with
> > librte_kvargs.  Looking at it, what may I ask in librte_kvargs is 
> > insufficiently
> > non-performant for your needs, specifically?  We talked about the heap
> > allocation above, is there something else? The string duplication perhaps?
> > 
> > 
> 
> Mostly the way to use it.
> The filter strings are
> bus=value,.../class=value,...
> 
> where either bus= list or class= list can be omitted, but at least one
> must appear.
> 
Ok, so whats the problem with using librte_kvargs for that?  Is it that the list
that acts as the value to the key isn't parsed out into its own set of tokens?
That seems entirely addressable.

> I want to read a single kvarg. I do not want to parse the whole string.
> the '/' signifies the end of the current layer.
> 
This makes it seem like librte_kvargs can handle this as a trivial case of its
functionality.

> librte_kvargs does not care about those points. I cannot ask it to only
> read either bus or class, as it would then throw an error for all the
> other keys (which the EAL has necessarily no knowledge of).
> 
But you can ask it to read both, and within your libraries logic make the

[dpdk-dev] [PATCH] net/bnxt: fix flow director with same cmd different queue

[Somnath Kotur]

When user reissues same flow director cmd with a different queue
update the existing filter to redirect flow to the new desired
queue as destination just like the other filters like 5 tuple and
generic flow.

Fixes: 2d64da097aa0 ("net/bnxt: support FDIR")
Cc: ajit.khapa...@broadcom.com

Signed-off-by: Somnath Kotur 
---
 drivers/net/bnxt/bnxt_ethdev.c | 46 --
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 21c46f8..0b21653 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -2358,7 +2358,8 @@ bnxt_parse_fdir_filter(struct bnxt *bp,
 }
 
 static struct bnxt_filter_info *
-bnxt_match_fdir(struct bnxt *bp, struct bnxt_filter_info *nf)
+bnxt_match_fdir(struct bnxt *bp, struct bnxt_filter_info *nf,
+   struct bnxt_vnic_info **mvnic)
 {
struct bnxt_filter_info *mf = NULL;
int i;
@@ -2396,8 +2397,11 @@ bnxt_match_fdir(struct bnxt *bp, struct bnxt_filter_info 
*nf)
!memcmp(mf->dst_ipaddr, nf->dst_ipaddr,
sizeof(nf->dst_ipaddr)) &&
!memcmp(mf->dst_ipaddr_mask, nf->dst_ipaddr_mask,
-   sizeof(nf->dst_ipaddr_mask)))
+   sizeof(nf->dst_ipaddr_mask))) {
+   if (mvnic)
+   *mvnic = vnic;
return mf;
+   }
}
}
return NULL;
@@ -2411,7 +2415,7 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
struct rte_eth_fdir_filter *fdir  = (struct rte_eth_fdir_filter *)arg;
struct bnxt_filter_info *filter, *match;
-   struct bnxt_vnic_info *vnic;
+   struct bnxt_vnic_info *vnic, *mvnic;
int ret = 0, i;
 
if (filter_op == RTE_ETH_FILTER_NOP)
@@ -2423,7 +2427,6 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
switch (filter_op) {
case RTE_ETH_FILTER_ADD:
case RTE_ETH_FILTER_DELETE:
-   /* FALLTHROUGH */
filter = bnxt_get_unused_filter(bp);
if (filter == NULL) {
PMD_DRV_LOG(ERR,
@@ -2436,11 +2439,31 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
goto free_filter;
filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
 
-   match = bnxt_match_fdir(bp, filter);
+   if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
+   vnic = STAILQ_FIRST(&bp->ff_pool[0]);
+   else
+   vnic = 
STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
+
+   match = bnxt_match_fdir(bp, filter, &mvnic);
if (match != NULL && filter_op == RTE_ETH_FILTER_ADD) {
-   PMD_DRV_LOG(ERR, "Flow already exists.\n");
-   ret = -EEXIST;
-   goto free_filter;
+   if (match->dst_id == vnic->fw_vnic_id) {
+   PMD_DRV_LOG(ERR, "Flow already exists.\n");
+   ret = -EEXIST;
+   goto free_filter;
+   } else {
+   match->dst_id = vnic->fw_vnic_id;
+   ret = bnxt_hwrm_set_ntuple_filter(bp,
+ match->dst_id,
+ match);
+   STAILQ_REMOVE(&mvnic->filter, match,
+ bnxt_filter_info, next);
+   STAILQ_INSERT_TAIL(&vnic->filter, match, next);
+   PMD_DRV_LOG(ERR,
+   "Filter with matching pattern exist\n");
+   PMD_DRV_LOG(ERR,
+   "Updated it to new destination q\n");
+   goto free_filter;
+   }
}
if (match == NULL && filter_op == RTE_ETH_FILTER_DELETE) {
PMD_DRV_LOG(ERR, "Flow does not exist.\n");
@@ -2448,12 +2471,6 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
goto free_filter;
}
 
-   if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
-   vnic = STAILQ_FIRST(&bp->ff_pool[0]);
-   else
-   vnic =
-   STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
-
if (filter_op == RTE_ETH_FILTER_ADD) {
ret = bnxt_hwrm_set_ntuple_filter(bp,
  filter->dst_id,
@@ -2489,7 +2506,6 @@ bnxt_fdir_filter(struct rte_eth

Re: [dpdk-dev] [PATCH v2] net/mlx5: setup RSS regardless of queue count

[Legacy, Allain]

> -Original Message-
> From: Yongseok Koh [mailto:ys...@mellanox.com]
> Sent: Thursday, March 22, 2018 7:38 PM
<..>
> 
> Dahir, Allain
> 
> Did you get a chance to test this patch? It would be good to have 'tested-by'
> tag from you.

I will be able to test it early next week.

Allain

[dpdk-dev] [PATCH v1 0/9] Bunch of flow API-related fixes

[Adrien Mazarguil]

This series contains several fixes for rte_flow and its implementation in
mlx4 and testpmd. Upcoming work on the flow API depends on it.

Adrien Mazarguil (9):
  net/mlx4: fix RSS resource leak in case of error
  net/mlx4: fix ignored RSS hash types
  app/testpmd: fix flow completion for RSS queues
  app/testpmd: fix lack of flow action configuration
  app/testpmd: fix RSS flow action configuration
  app/testpmd: fix missing RSS fields in flow action
  ethdev: fix shallow copy of flow API RSS action
  ethdev: fix missing boolean values in flow command
  ethdev: fix ABI version in meson build

 app/test-pmd/cmdline_flow.c | 255 ---
 app/test-pmd/config.c   | 161 +-
 app/test-pmd/testpmd.h  |  13 ++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |   8 +
 drivers/net/mlx4/mlx4_flow.c|  17 +-
 lib/librte_ether/meson.build|   2 +-
 lib/librte_ether/rte_flow.c | 145 +
 7 files changed, 477 insertions(+), 124 deletions(-)

-- 
2.11.0

[dpdk-dev] [PATCH v1 1/9] net/mlx4: fix RSS resource leak in case of error

[Adrien Mazarguil]

When memory cannot be allocated for a flow rule, its RSS context reference
is not dropped.

Fixes: 078b8b452e6b ("net/mlx4: add RSS flow rule action support")
Cc: sta...@dpdk.org

Signed-off-by: Adrien Mazarguil 
Cc: Shahaf Shuler 
---
 drivers/net/mlx4/mlx4_flow.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index 2d55bfe03..a3b4480b4 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -820,11 +820,14 @@ mlx4_flow_prepare(struct priv *priv,
},
};
 
-   if (!mlx4_zmallocv(__func__, vec, RTE_DIM(vec)))
+   if (!mlx4_zmallocv(__func__, vec, RTE_DIM(vec))) {
+   if (temp.rss)
+   mlx4_rss_put(temp.rss);
return rte_flow_error_set
(error, -rte_errno,
 RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
 "flow rule handle allocation failure");
+   }
/* Most fields will be updated by second pass. */
*flow = (struct rte_flow){
.ibv_attr = temp.ibv_attr,
-- 
2.11.0

[dpdk-dev] [PATCH v1 3/9] app/testpmd: fix flow completion for RSS queues

[Adrien Mazarguil]

The lack of a working completion for RSS queues was overlooked during
development; until now only "end" was displayed as a valid token.

Fixes: 05d34c6e9d2c ("app/testpmd: add queue actions to flow command")
Cc: sta...@dpdk.org

Signed-off-by: Adrien Mazarguil 
Cc: Wenzhuo Lu 
Cc: Jingjing Wu 
---
 app/test-pmd/cmdline_flow.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index a5cf84f79..9cac8e9bf 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -2663,17 +2663,15 @@ static int
 comp_vc_action_rss_queue(struct context *ctx, const struct token *token,
 unsigned int ent, char *buf, unsigned int size)
 {
-   static const char *const str[] = { "", "end", NULL };
-   unsigned int i;
-
(void)ctx;
(void)token;
-   for (i = 0; str[i] != NULL; ++i)
-   if (buf && i == ent)
-   return snprintf(buf, size, "%s", str[i]);
-   if (buf)
-   return -1;
-   return i;
+   if (!buf)
+   return nb_rxq + 1;
+   if (ent < nb_rxq)
+   return snprintf(buf, size, "%u", ent);
+   if (ent == nb_rxq)
+   return snprintf(buf, size, "end");
+   return -1;
 }
 
 /** Internal context. */
-- 
2.11.0

[dpdk-dev] [PATCH v1 2/9] net/mlx4: fix ignored RSS hash types

[Adrien Mazarguil]

When an unsupported hash type is part of a RSS configuration structure, it
is silently ignored instead of triggering an error. This may lead
applications to assume that such types are accepted, while they are in fact
not part of the resulting flow rules.

Fixes: 078b8b452e6b ("net/mlx4: add RSS flow rule action support")
Cc: sta...@dpdk.org

Signed-off-by: Adrien Mazarguil 
Cc: Shahaf Shuler 
---
 drivers/net/mlx4/mlx4_flow.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index a3b4480b4..4d26df326 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -706,6 +706,7 @@ mlx4_flow_prepare(struct priv *priv,
const struct rte_flow_action_queue *queue;
const struct rte_flow_action_rss *rss;
const struct rte_eth_rss_conf *rss_conf;
+   uint64_t fields;
unsigned int i;
 
case RTE_FLOW_ACTION_TYPE_VOID:
@@ -780,10 +781,15 @@ mlx4_flow_prepare(struct priv *priv,
" of the context size";
goto exit_action_not_supported;
}
+   rte_errno = 0;
+   fields = mlx4_conv_rss_hf(priv, rss_conf->rss_hf);
+   if (fields == (uint64_t)-1 && rte_errno) {
+   msg = "unsupported RSS hash type requested";
+   goto exit_action_not_supported;
+   }
flow->rss = mlx4_rss_get
-   (priv,
-mlx4_conv_rss_hf(priv, rss_conf->rss_hf),
-rss_conf->rss_key, rss->num, rss->queue);
+   (priv, fields, rss_conf->rss_key, rss->num,
+rss->queue);
if (!flow->rss) {
msg = "either invalid parameters or not enough"
" resources for additional multi-queue"
-- 
2.11.0

[dpdk-dev] [PATCH v1 4/9] app/testpmd: fix lack of flow action configuration

[Adrien Mazarguil]

Configuration structure is not optional with flow rule actions that expect
one; this pointer is not supposed to be NULL and PMDs should not have to
verify it.

Like pattern item spec/last/mask fields, it is currently set when at least
one configuration parameter is provided on the command line. This patch
sets it as soon as an action is created instead.

Fixes: 7a91969ad35e ("app/testpmd: add various actions to flow command")
Cc: sta...@dpdk.org

Signed-off-by: Adrien Mazarguil 
Cc: Wenzhuo Lu 
Cc: Jingjing Wu 
---
 app/test-pmd/cmdline_flow.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 9cac8e9bf..c2cf415ef 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -1909,6 +1909,7 @@ parse_vc(struct context *ctx, const struct token *token,
return -1;
*action = (struct rte_flow_action){
.type = priv->type,
+   .conf = data_size ? data : NULL,
};
++out->args.vc.actions_n;
ctx->object = action;
@@ -1989,7 +1990,6 @@ parse_vc_conf(struct context *ctx, const struct token 
*token,
  void *buf, unsigned int size)
 {
struct buffer *out = buf;
-   struct rte_flow_action *action;
 
(void)size;
/* Token name must match. */
@@ -1998,14 +1998,9 @@ parse_vc_conf(struct context *ctx, const struct token 
*token,
/* Nothing else to do if there is no buffer. */
if (!out)
return len;
-   if (!out->args.vc.actions_n)
-   return -1;
-   action = &out->args.vc.actions[out->args.vc.actions_n - 1];
/* Point to selected object. */
ctx->object = out->args.vc.data;
ctx->objmask = NULL;
-   /* Update configuration pointer. */
-   action->conf = ctx->object;
return len;
 }
 
-- 
2.11.0

[dpdk-dev] [PATCH v1 5/9] app/testpmd: fix RSS flow action configuration

[Adrien Mazarguil]

Except for a list of queues, RSS configuration (hash key and fields) cannot
be specified from the flow command line and testpmd does not provide safe
defaults either.

In order to validate their implementation with testpmd, PMDs had to
interpret its NULL RSS configuration parameters somehow, however this has
never been valid to begin with.

This patch makes testpmd always provide default values.

Fixes: 05d34c6e9d2c ("app/testpmd: add queue actions to flow command")
Cc: sta...@dpdk.org

Signed-off-by: Adrien Mazarguil 
Cc: Wenzhuo Lu 
Cc: Jingjing Wu 
---
 app/test-pmd/cmdline_flow.c | 104 +
 app/test-pmd/config.c   | 141 +++
 2 files changed, 192 insertions(+), 53 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index c2cf415ef..890c36d8e 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -184,13 +184,19 @@ enum index {
 #define ITEM_RAW_SIZE \
(offsetof(struct rte_flow_item_raw, pattern) + ITEM_RAW_PATTERN_SIZE)
 
-/** Number of queue[] entries in struct rte_flow_action_rss. */
-#define ACTION_RSS_NUM 32
-
-/** Storage size for struct rte_flow_action_rss including queues. */
-#define ACTION_RSS_SIZE \
-   (offsetof(struct rte_flow_action_rss, queue) + \
-sizeof(*((struct rte_flow_action_rss *)0)->queue) * ACTION_RSS_NUM)
+/** Maximum number of queue indices in struct rte_flow_action_rss. */
+#define ACTION_RSS_QUEUE_NUM 32
+
+/** Storage for struct rte_flow_action_rss including external data. */
+union action_rss_data {
+   struct rte_flow_action_rss conf;
+   struct {
+   uint8_t conf_data[offsetof(struct rte_flow_action_rss, queue)];
+   uint16_t queue[ACTION_RSS_QUEUE_NUM];
+   struct rte_eth_rss_conf rss_conf;
+   uint8_t rss_key[RSS_HASH_KEY_LENGTH];
+   } s;
+};
 
 /** Maximum number of subsequent tokens and arguments on the stack. */
 #define CTX_STACK_SIZE 16
@@ -316,6 +322,13 @@ struct token {
.size = (sz), \
})
 
+/** Static initializer for ARGS() with arbitrary offset and size. */
+#define ARGS_ENTRY_ARB(o, s) \
+   (&(const struct arg){ \
+   .offset = (o), \
+   .size = (s), \
+   })
+
 /** Same as ARGS_ENTRY() using network byte ordering. */
 #define ARGS_ENTRY_HTON(s, f) \
(&(const struct arg){ \
@@ -650,6 +663,9 @@ static int parse_vc_spec(struct context *, const struct 
token *,
 const char *, unsigned int, void *, unsigned int);
 static int parse_vc_conf(struct context *, const struct token *,
 const char *, unsigned int, void *, unsigned int);
+static int parse_vc_action_rss(struct context *, const struct token *,
+  const char *, unsigned int, void *,
+  unsigned int);
 static int parse_vc_action_rss_queue(struct context *, const struct token *,
 const char *, unsigned int, void *,
 unsigned int);
@@ -1573,9 +1589,9 @@ static const struct token token_list[] = {
[ACTION_RSS] = {
.name = "rss",
.help = "spread packets among several queues",
-   .priv = PRIV_ACTION(RSS, ACTION_RSS_SIZE),
+   .priv = PRIV_ACTION(RSS, sizeof(union action_rss_data)),
.next = NEXT(action_rss),
-   .call = parse_vc,
+   .call = parse_vc_action_rss,
},
[ACTION_RSS_QUEUES] = {
.name = "queues",
@@ -2004,6 +2020,64 @@ parse_vc_conf(struct context *ctx, const struct token 
*token,
return len;
 }
 
+/** Parse RSS action. */
+static int
+parse_vc_action_rss(struct context *ctx, const struct token *token,
+   const char *str, unsigned int len,
+   void *buf, unsigned int size)
+{
+   struct buffer *out = buf;
+   struct rte_flow_action *action;
+   union action_rss_data *action_rss_data;
+   unsigned int i;
+   int ret;
+
+   ret = parse_vc(ctx, token, str, len, buf, size);
+   if (ret < 0)
+   return ret;
+   /* Nothing else to do if there is no buffer. */
+   if (!out)
+   return ret;
+   if (!out->args.vc.actions_n)
+   return -1;
+   action = &out->args.vc.actions[out->args.vc.actions_n - 1];
+   /* Point to selected object. */
+   ctx->object = out->args.vc.data;
+   ctx->objmask = NULL;
+   /* Set up default configuration. */
+   action_rss_data = ctx->object;
+   *action_rss_data = (union action_rss_data){
+   .conf = (struct rte_flow_action_rss){
+   .rss_conf = &action_rss_data->s.rss_conf,
+   .num = RTE_MIN(nb_rxq, ACTION_RSS_QUEUE_NUM),
+   },
+   };
+   action_rss_data->s.rss_conf = (struct rte_eth_rss_conf){
+ 

[dpdk-dev] [PATCH v1 6/9] app/testpmd: fix missing RSS fields in flow action

[Adrien Mazarguil]

Users cannot override the default RSS settings when entering a RSS action,
only a list of queues can be provided.

This patch enables them to set a RSS hash key and types for a flow rule.

Fixes: 05d34c6e9d2c ("app/testpmd: add queue actions to flow command")
Cc: sta...@dpdk.org

Signed-off-by: Adrien Mazarguil 
Cc: Wenzhuo Lu 
Cc: Jingjing Wu 
---
 app/test-pmd/cmdline_flow.c | 133 ++-
 app/test-pmd/config.c   |  20 ++--
 app/test-pmd/testpmd.h  |  13 +++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |   8 ++
 4 files changed, 163 insertions(+), 11 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 890c36d8e..38aa6099d 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -167,6 +167,10 @@ enum index {
ACTION_DUP,
ACTION_DUP_INDEX,
ACTION_RSS,
+   ACTION_RSS_KEY,
+   ACTION_RSS_KEY_LEN,
+   ACTION_RSS_TYPES,
+   ACTION_RSS_TYPE,
ACTION_RSS_QUEUES,
ACTION_RSS_QUEUE,
ACTION_PF,
@@ -223,6 +227,9 @@ struct context {
 struct arg {
uint32_t hton:1; /**< Use network byte ordering. */
uint32_t sign:1; /**< Value is signed. */
+   uint32_t bounded:1; /**< Value is bounded. */
+   uintmax_t min; /**< Minimum value if bounded. */
+   uintmax_t max; /**< Maximum value if bounded. */
uint32_t offset; /**< Relative offset from ctx->object. */
uint32_t size; /**< Field size. */
const uint8_t *mask; /**< Bit-mask to use instead of offset/size. */
@@ -329,6 +336,16 @@ struct token {
.size = (s), \
})
 
+/** Same as ARGS_ENTRY_ARB() with bounded values. */
+#define ARGS_ENTRY_ARB_BOUNDED(o, s, i, a) \
+   (&(const struct arg){ \
+   .bounded = 1, \
+   .min = (i), \
+   .max = (a), \
+   .offset = (o), \
+   .size = (s), \
+   })
+
 /** Same as ARGS_ENTRY() using network byte ordering. */
 #define ARGS_ENTRY_HTON(s, f) \
(&(const struct arg){ \
@@ -635,6 +652,9 @@ static const enum index action_dup[] = {
 };
 
 static const enum index action_rss[] = {
+   ACTION_RSS_KEY,
+   ACTION_RSS_KEY_LEN,
+   ACTION_RSS_TYPES,
ACTION_RSS_QUEUES,
ACTION_NEXT,
ZERO,
@@ -666,6 +686,9 @@ static int parse_vc_conf(struct context *, const struct 
token *,
 static int parse_vc_action_rss(struct context *, const struct token *,
   const char *, unsigned int, void *,
   unsigned int);
+static int parse_vc_action_rss_type(struct context *, const struct token *,
+   const char *, unsigned int, void *,
+   unsigned int);
 static int parse_vc_action_rss_queue(struct context *, const struct token *,
 const char *, unsigned int, void *,
 unsigned int);
@@ -721,6 +744,8 @@ static int comp_port(struct context *, const struct token *,
 unsigned int, char *, unsigned int);
 static int comp_rule_id(struct context *, const struct token *,
unsigned int, char *, unsigned int);
+static int comp_vc_action_rss_type(struct context *, const struct token *,
+  unsigned int, char *, unsigned int);
 static int comp_vc_action_rss_queue(struct context *, const struct token *,
unsigned int, char *, unsigned int);
 
@@ -1593,6 +1618,43 @@ static const struct token token_list[] = {
.next = NEXT(action_rss),
.call = parse_vc_action_rss,
},
+   [ACTION_RSS_KEY] = {
+   .name = "key",
+   .help = "RSS hash key",
+   .next = NEXT(action_rss, NEXT_ENTRY(STRING)),
+   .args = ARGS(ARGS_ENTRY_ARB
+(((uintptr_t)&((union action_rss_data *)0)->
+  s.rss_conf.rss_key_len),
+ sizeof(((struct rte_eth_rss_conf *)0)->
+rss_key_len)),
+ARGS_ENTRY_ARB
+(((uintptr_t)((union action_rss_data *)0)->
+  s.rss_key),
+ RSS_HASH_KEY_LENGTH)),
+   },
+   [ACTION_RSS_KEY_LEN] = {
+   .name = "key_len",
+   .help = "RSS hash key length in bytes",
+   .next = NEXT(action_rss, NEXT_ENTRY(UNSIGNED)),
+   .args = ARGS(ARGS_ENTRY_ARB_BOUNDED
+(((uintptr_t)&((union action_rss_data *)0)->
+  s.rss_conf.rss_key_len),
+ sizeof(((struct rte_eth_rss_conf *)0)->
+rss_key_len),
+ 0,
+   

[dpdk-dev] [PATCH v1 7/9] ethdev: fix shallow copy of flow API RSS action

[Adrien Mazarguil]

The rss_conf field is defined as a pointer to struct rte_eth_rss_conf.

Even assuming it is permanently allocated and a pointer copy is safe,
pointed data may change and not reflect an applied flow rule anymore.

This patch aligns with testpmd by making a deep copy instead.

Fixes: 18da437b5f63 ("ethdev: add flow rule copy function")
Cc: sta...@dpdk.org
Cc: Gaetan Rivet 

Signed-off-by: Adrien Mazarguil 
Cc: Thomas Monjalon 
---
 lib/librte_ether/rte_flow.c | 145 +++
 1 file changed, 102 insertions(+), 43 deletions(-)

diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
index 38f2d27be..ba6feddee 100644
--- a/lib/librte_ether/rte_flow.c
+++ b/lib/librte_ether/rte_flow.c
@@ -255,60 +255,119 @@ rte_flow_error_set(struct rte_flow_error *error,
return -code;
 }
 
-/** Compute storage space needed by item specification. */
-static void
-flow_item_spec_size(const struct rte_flow_item *item,
-   size_t *size, size_t *pad)
+/** Pattern item specification types. */
+enum item_spec_type {
+   ITEM_SPEC,
+   ITEM_LAST,
+   ITEM_MASK,
+};
+
+/** Compute storage space needed by item specification and copy it. */
+static size_t
+flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
+   enum item_spec_type type)
 {
-   if (!item->spec) {
-   *size = 0;
+   size_t size = 0;
+   const void *item_spec =
+   type == ITEM_SPEC ? item->spec :
+   type == ITEM_LAST ? item->last :
+   type == ITEM_MASK ? item->mask :
+   NULL;
+
+   if (!item_spec)
goto empty;
-   }
switch (item->type) {
union {
const struct rte_flow_item_raw *raw;
-   } spec;
+   } src;
+   union {
+   struct rte_flow_item_raw *raw;
+   } dst;
 
-   /* Not a fall-through */
case RTE_FLOW_ITEM_TYPE_RAW:
-   spec.raw = item->spec;
-   *size = offsetof(struct rte_flow_item_raw, pattern) +
-   spec.raw->length * sizeof(*spec.raw->pattern);
+   src.raw = item_spec;
+   dst.raw = buf;
+   size = offsetof(struct rte_flow_item_raw, pattern) +
+   src.raw->length * sizeof(*src.raw->pattern);
+   if (dst.raw)
+   memcpy(dst.raw, src.raw, size);
break;
default:
-   *size = rte_flow_desc_item[item->type].size;
+   size = rte_flow_desc_item[item->type].size;
+   if (buf)
+   memcpy(buf, item_spec, size);
break;
}
 empty:
-   *pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size;
+   return RTE_ALIGN_CEIL(size, sizeof(double));
 }
 
-/** Compute storage space needed by action configuration. */
-static void
-flow_action_conf_size(const struct rte_flow_action *action,
- size_t *size, size_t *pad)
+/** Compute storage space needed by action configuration and copy it. */
+static size_t
+flow_action_conf_copy(void *buf, const struct rte_flow_action *action)
 {
-   if (!action->conf) {
-   *size = 0;
+   size_t size = 0;
+
+   if (!action->conf)
goto empty;
-   }
switch (action->type) {
union {
const struct rte_flow_action_rss *rss;
-   } conf;
+   } src;
+   union {
+   struct rte_flow_action_rss *rss;
+   } dst;
+   size_t off;
 
-   /* Not a fall-through. */
case RTE_FLOW_ACTION_TYPE_RSS:
-   conf.rss = action->conf;
-   *size = offsetof(struct rte_flow_action_rss, queue) +
-   conf.rss->num * sizeof(*conf.rss->queue);
+   src.rss = action->conf;
+   dst.rss = buf;
+   off = 0;
+   if (dst.rss)
+   *dst.rss = (struct rte_flow_action_rss){
+   .num = src.rss->num,
+   };
+   off += offsetof(struct rte_flow_action_rss, queue);
+   if (src.rss->num) {
+   size = sizeof(*src.rss->queue) * src.rss->num;
+   if (dst.rss)
+   memcpy(dst.rss->queue, src.rss->queue, size);
+   off += size;
+   }
+   off = RTE_ALIGN_CEIL(off, sizeof(double));
+   if (dst.rss) {
+   dst.rss->rss_conf = (void *)((uintptr_t)dst.rss + off);
+   *(struct rte_eth_rss_conf *)(uintptr_t)
+   dst.rss->rss_conf = (struct rte_eth_rss_conf){
+   .rss_key_len = src.rss->rss_conf->rss_key_len,
+   .rss_hf = src.rss->rss_

[dpdk-dev] [PATCH v1 8/9] ethdev: fix missing boolean values in flow command

[Adrien Mazarguil]

Original implementation lacks the on/off toggle.

This patch shows up as a fix because it has been a popular request ever
since the first DPDK release with the original implementation but was never
addressed.

Fixes: abc3d81aca1b ("app/testpmd: add item raw to flow command")
Cc: sta...@dpdk.org

Signed-off-by: Adrien Mazarguil 
---
 app/test-pmd/cmdline_flow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 38aa6099d..f2be10d20 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -2695,6 +2695,7 @@ static const char *const boolean_name[] = {
"false", "true",
"no", "yes",
"N", "Y",
+   "off", "on",
NULL,
 };
 
-- 
2.11.0

[dpdk-dev] [PATCH v1 9/9] ethdev: fix ABI version in meson build

[Adrien Mazarguil]

Must remain synchronized with its Makefile counterpart.

Fixes: a4b0b30723b2 ("ethdev: remove versioning of ethdev filter control 
function")
Cc: Kirill Rybalchenko 

Signed-off-by: Adrien Mazarguil 
---
 lib/librte_ether/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ether/meson.build b/lib/librte_ether/meson.build
index 7fed86056..12bdb6b61 100644
--- a/lib/librte_ether/meson.build
+++ b/lib/librte_ether/meson.build
@@ -2,7 +2,7 @@
 # Copyright(c) 2017 Intel Corporation
 
 name = 'ethdev'
-version = 8
+version = 9
 allow_experimental_apis = true
 sources = files('ethdev_profile.c',
'rte_ethdev.c',
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2 04/18] eal: add lightweight kvarg parsing utility

[Gaëtan Rivet]

On Fri, Mar 23, 2018 at 07:54:11AM -0400, Neil Horman wrote:
> On Fri, Mar 23, 2018 at 10:31:22AM +0100, Gaëtan Rivet wrote:
> > On Thu, Mar 22, 2018 at 08:53:49PM -0400, Neil Horman wrote:
> > > On Thu, Mar 22, 2018 at 05:27:51PM +0100, Gaëtan Rivet wrote:
> > > > On Thu, Mar 22, 2018 at 10:10:37AM -0400, Neil Horman wrote:
> > > > > On Wed, Mar 21, 2018 at 05:32:24PM +, Wiles, Keith wrote:
> > > > > > 
> > > > > > 
> > > > > > > On Mar 21, 2018, at 12:15 PM, Gaetan Rivet 
> > > > > > >  wrote:
> > > > > > > 
> > > > > > > This library offers a quick way to parse parameters passed with a
> > > > > > > key=value syntax.
> > > > > > > 
> > > > > > > A single function is needed and finds the relevant element within 
> > > > > > > the
> > > > > > > text. No dynamic allocation is performed. It is possible to chain 
> > > > > > > the
> > > > > > > parsing of each pairs for quickly scanning a list.
> > > > > > > 
> > > > > > > This utility is private to the EAL and should allow avoiding 
> > > > > > > having to
> > > > > > > move around the more complete librte_kvargs.
> > > > > > 
> > > > > > What is the big advantage with this code and the librte_kvargs 
> > > > > > code. Is it just no allocation, rte_kvargs needs to be build before 
> > > > > > parts of EAL or what?
> > > > > > 
> > > > > > My concern is we have now two flavors one in EAL and one in 
> > > > > > librte_kvargs, would it not be more reasonable to improve 
> > > > > > rte_kvargs to remove your objections? I am all for fast, better, 
> > > > > > stronger code :-)
> > > > > > 
> > > > > +1, this really doesn't make much sense to me.  Two parsing routines 
> > > > > seems like
> > > > > its just asking for us to have to fix parsing bugs in two places.  If 
> > > > > allocation
> > > > > is a concern, I don't see why you can't just change the malloc in
> > > > > rte_kvargs_parse to an automatic allocation on the stack, or a 
> > > > > preallocation set
> > > > > of kvargs that can be shared from init time.
> > > > 
> > > > I think the existing allocation scheme is fine for other usages (in
> > > > drivers and so on). Not for what I wanted to do.
> > > > 
> > > Ok, but thats an adressable issue.  you can bifurcate the parse function 
> > > to an
> > > internal function that accepts any preallocated kvargs struct, and export 
> > > two
> > > wrapper functions, one which allocates the struct from the heap, another 
> > > which
> > > allocated automatically on the stack.
> > > 
> > 
> > Sure, everything is possible.
> > 
> Ok.
> 
> > > > >   librte_kvargs isn't 
> > > > > necessecarily
> > > > > the best parsing library ever, but its not bad, and it just seems 
> > > > > wrong to go
> > > > > re-inventing the wheel.
> > > > > 
> > > > 
> > > > It serves a different purpose than the one I'm pursuing.
> > > > 
> > > > This helper is lightweight and private. If I wanted to integrate my
> > > > needs with librte_kvargs, I would be adding new functionalities, making
> > > > it more complex, and for a use-case that is useless for the vast
> > > > majority of users of the lib.
> > > > 
> > > Ok, to that end:
> > > 
> > > 1) Privacy is not an issue (at least from my understanding of what your 
> > > doing).
> > > If we start with the assumption that librte_kvargs is capable of 
> > > satisfying your
> > > needs (even if its not done in an optimal way), the fact that your 
> > > version of
> > > the function is internal to the library doesn't seem overly relevant, 
> > > unless
> > > theres something critical to that privacy that I'm missing.
> > > 
> > 
> > Privacy is only a point I brought up to say that the impact of this
> > function is minimal. People looking to parse their kvargs should not
> > have any ambiguity regarding how they should do so. Only librte_kvargs
> > is available.
> > 
> Ok, would you also council others developing dpdk apps to write their own
> parsing routines when what they needed was trivial for the existing library?
> You are people too :)
> 
> > > 2) Lightweight function  seems like something that can be integrated with
> > > librte_kvargs.  Looking at it, what may I ask in librte_kvargs is 
> > > insufficiently
> > > non-performant for your needs, specifically?  We talked about the heap
> > > allocation above, is there something else? The string duplication perhaps?
> > > 
> > > 
> > 
> > Mostly the way to use it.
> > The filter strings are
> > bus=value,.../class=value,...
> > 
> > where either bus= list or class= list can be omitted, but at least one
> > must appear.
> > 
> Ok, so whats the problem with using librte_kvargs for that?  Is it that the 
> list
> that acts as the value to the key isn't parsed out into its own set of tokens?
> That seems entirely addressable.
> 
> > I want to read a single kvarg. I do not want to parse the whole string.
> > the '/' signifies the end of the current layer.
> > 
> This makes it seem like librte_kvargs can handle this as a trivial case of its

Re: [dpdk-dev] [PATCH v2 04/18] eal: add lightweight kvarg parsing utility

[Wiles, Keith]

> On Mar 23, 2018, at 4:31 AM, Gaëtan Rivet  wrote:
> 
> On Thu, Mar 22, 2018 at 08:53:49PM -0400, Neil Horman wrote:
>> On Thu, Mar 22, 2018 at 05:27:51PM +0100, Gaëtan Rivet wrote:
>>> On Thu, Mar 22, 2018 at 10:10:37AM -0400, Neil Horman wrote:
 On Wed, Mar 21, 2018 at 05:32:24PM +, Wiles, Keith wrote:
> 
> 
>> On Mar 21, 2018, at 12:15 PM, Gaetan Rivet  
>> wrote:
>> 
>> This library offers a quick way to parse parameters passed with a
>> key=value syntax.
>> 
>> A single function is needed and finds the relevant element within the
>> text. No dynamic allocation is performed. It is possible to chain the
>> parsing of each pairs for quickly scanning a list.
>> 
>> This utility is private to the EAL and should allow avoiding having to
>> move around the more complete librte_kvargs.
> 
> What is the big advantage with this code and the librte_kvargs code. Is 
> it just no allocation, rte_kvargs needs to be build before parts of EAL 
> or what?
> 
> My concern is we have now two flavors one in EAL and one in 
> librte_kvargs, would it not be more reasonable to improve rte_kvargs to 
> remove your objections? I am all for fast, better, stronger code :-)
> 
 +1, this really doesn't make much sense to me.  Two parsing routines seems 
 like
 its just asking for us to have to fix parsing bugs in two places.  If 
 allocation
 is a concern, I don't see why you can't just change the malloc in
 rte_kvargs_parse to an automatic allocation on the stack, or a 
 preallocation set
 of kvargs that can be shared from init time.
>>> 
>>> I think the existing allocation scheme is fine for other usages (in
>>> drivers and so on). Not for what I wanted to do.
>>> 
>> Ok, but thats an adressable issue.  you can bifurcate the parse function to 
>> an
>> internal function that accepts any preallocated kvargs struct, and export two
>> wrapper functions, one which allocates the struct from the heap, another 
>> which
>> allocated automatically on the stack.
>> 
> 
> Sure, everything is possible.
> 
  librte_kvargs isn't 
 necessecarily
 the best parsing library ever, but its not bad, and it just seems wrong to 
 go
 re-inventing the wheel.
 
>>> 
>>> It serves a different purpose than the one I'm pursuing.
>>> 
>>> This helper is lightweight and private. If I wanted to integrate my
>>> needs with librte_kvargs, I would be adding new functionalities, making
>>> it more complex, and for a use-case that is useless for the vast
>>> majority of users of the lib.
>>> 
>> Ok, to that end:
>> 
>> 1) Privacy is not an issue (at least from my understanding of what your 
>> doing).
>> If we start with the assumption that librte_kvargs is capable of satisfying 
>> your
>> needs (even if its not done in an optimal way), the fact that your version of
>> the function is internal to the library doesn't seem overly relevant, unless
>> theres something critical to that privacy that I'm missing.
>> 
> 
> Privacy is only a point I brought up to say that the impact of this
> function is minimal. People looking to parse their kvargs should not
> have any ambiguity regarding how they should do so. Only librte_kvargs
> is available.
> 
>> 2) Lightweight function  seems like something that can be integrated with
>> librte_kvargs.  Looking at it, what may I ask in librte_kvargs is 
>> insufficiently
>> non-performant for your needs, specifically?  We talked about the heap
>> allocation above, is there something else? The string duplication perhaps?
>> 
>> 
> 
> Mostly the way to use it.
> The filter strings are
> bus=value,.../class=value,...
> 
> where either bus= list or class= list can be omitted, but at least one
> must appear.
> 
> I want to read a single kvarg. I do not want to parse the whole string.
> the '/' signifies the end of the current layer.
> 
> librte_kvargs does not care about those points. I cannot ask it to only
> read either bus or class, as it would then throw an error for all the
> other keys (which the EAL has necessarily no knowledge of).
> 
> So I would need to:
> 
>  * Add a custom storage scheme
>  * Add a custom parsing mode stopping at the first kvarg
>  * Add an edge-case to ignore the '/', so as not to throw off the rest
>of the parsing (least it be considered part of the previous kvarg
>value field).
> 
> Seeing this, does adding those really specifics functionality help
> librte_kvargs to be more useful and usable? I do not think so.
> 
> It would only serve to disrupt the library for a marginal use-case, with
> the introduction of edge-cases that will blur the specs of the lib's
> API, making it harder to avoid subtle bugs.
> 
> Only way to do so sanely would be to add rte_parse_kv as part of
> librte_kvargs, as is. But then the whole thing does not make sense IMO:
> no one would care to use it, the maintainance eff

Re: [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling

[Adrien Mazarguil]

Hi Ophir,

On Mon, Mar 19, 2018 at 04:36:50PM +, Ophir Munk wrote:
> Previous to this commit mlx4 CRC stripping was executed by default and
> there was no verbs API to disable it.
> 
> Current support in MLNX_OFED_4.3-1.5.0.0 and above

I suggest to drop this line as it's not exclusive to MLNX_OFED; the
documented minimum requirements are already correct and rdma-core v15 also
supports it.

> Signed-off-by: Ophir Munk 

A few more comments below.

> ---
> v1: initial version
> v2: following internal reviews
> 
>  drivers/net/mlx4/mlx4.c  |  4 
>  drivers/net/mlx4/mlx4.h  |  1 +
>  drivers/net/mlx4/mlx4_rxq.c  | 33 +++--
>  drivers/net/mlx4/mlx4_rxtx.c |  5 -
>  drivers/net/mlx4/mlx4_rxtx.h |  1 +
>  5 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index ee93daf..d4f4323 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -578,6 +578,10 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct 
> rte_pci_device *pci_dev)
>   }
>   DEBUG("supported RSS hash fields mask: %016" PRIx64,
> priv->hw_rss_sup);
> + priv->hw_fcs_strip = !!(device_attr_ex.raw_packet_caps &
> +  IBV_RAW_PACKET_CAP_SCATTER_FCS);

Minor nit, indentation contains one extra space.

> + DEBUG("FCS stripping toggling is %ssupported",
> +   (priv->hw_fcs_strip ? "" : "not "));

Another minor nit, mlx5 prints "configuration" instead of "toggling",
wouldn't it make sense for both PMDs to print the same information?

Also the extra set of parentheses around the conditional expression could be
removed.

>   /* Configure the first MAC address by default. */
>   if (mlx4_get_mac(priv, &mac.addr_bytes)) {
>   ERROR("cannot get MAC address, is mlx4_en loaded?"
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index 19c8a22..3ae3ce6 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -105,6 +105,7 @@ struct priv {
>   uint32_t isolated:1; /**< Toggle isolated mode. */
>   uint32_t hw_csum:1; /**< Checksum offload is supported. */
>   uint32_t hw_csum_l2tun:1; /**< Checksum support for L2 tunnels. */
> + uint32_t hw_fcs_strip:1; /**< FCS stripping toggling is supported. */
>   uint64_t hw_rss_sup; /**< Supported RSS hash fields (Verbs format). */
>   struct rte_intr_handle intr_handle; /**< Port interrupt handle. */
>   struct mlx4_drop *drop; /**< Shared resources for drop flow rules. */
> diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c
> index 7a036ed..6748355 100644
> --- a/drivers/net/mlx4/mlx4_rxq.c
> +++ b/drivers/net/mlx4/mlx4_rxq.c
> @@ -491,6 +491,8 @@ mlx4_rxq_attach(struct rxq *rxq)
>   const char *msg;
>   struct ibv_cq *cq = NULL;
>   struct ibv_wq *wq = NULL;
> + unsigned int create_flags = 0;
> + unsigned int comp_mask = 0;

I suggest using uint32_t to align these with Verb's definitions for struct
ibv_wq_init_attr.

>   volatile struct mlx4_wqe_data_seg (*wqes)[];
>   unsigned int i;
>   int ret;
> @@ -503,6 +505,11 @@ mlx4_rxq_attach(struct rxq *rxq)
>   msg = "CQ creation failure";
>   goto error;
>   }
> + /* By default, FCS (CRC) is stripped by hardware. */
> + if (rxq->crc_present) {
> + create_flags |= IBV_WQ_FLAGS_SCATTER_FCS;
> + comp_mask |= IBV_WQ_INIT_ATTR_FLAGS;
> + }
>   wq = mlx4_glue->create_wq
>   (priv->ctx,
>&(struct ibv_wq_init_attr){
> @@ -511,6 +518,8 @@ mlx4_rxq_attach(struct rxq *rxq)
>   .max_sge = sges_n,
>   .pd = priv->pd,
>   .cq = cq,
> + .comp_mask = comp_mask,
> + .create_flags = create_flags,
>});
>   if (!wq) {
>   ret = errno ? errno : EINVAL;
> @@ -649,9 +658,10 @@ mlx4_rxq_detach(struct rxq *rxq)
>  uint64_t
>  mlx4_get_rx_queue_offloads(struct priv *priv)
>  {
> - uint64_t offloads = DEV_RX_OFFLOAD_SCATTER |
> - DEV_RX_OFFLOAD_CRC_STRIP;
> + uint64_t offloads = DEV_RX_OFFLOAD_SCATTER;
>  
> + if (priv->hw_fcs_strip)
> + offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
>   if (priv->hw_csum)
>   offloads |= DEV_RX_OFFLOAD_CHECKSUM;
>   return offloads;
> @@ -781,6 +791,24 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t 
> idx, uint16_t desc,
> (void *)dev, idx);
>   return -rte_errno;
>   }
> + /* By default, FCS (CRC) is stripped by hardware. */
> + unsigned char crc_present;

This variable must be grouped with others at the beginning of the block and
use the same type as its counterpart in struct rxq for consistency,
uint32_t.

> + if (conf-

[dpdk-dev] [PATCH 1/1] testpmd: add parameters buffersize-before-send and flush-timeout

[Jens Freimann]

Create a fifo to buffer received packets. Once it flows over put
those packets into the actual tx queue. The fifo is created per tx
queue and its size can be set with the --buffersize-before-sending
commandline parameter.

A second commandline parameter is used to set a timeout in
milliseconds after which the fifo is flushed.


Signed-off-by: Jens Freimann 
---
 app/test-pmd/Makefile |  4 
 app/test-pmd/fifo.c   | 16 +
 app/test-pmd/iofwd.c  | 59 ++-
 app/test-pmd/parameters.c | 21 -
 app/test-pmd/testpmd.c| 48 ++
 app/test-pmd/testpmd.h| 15 
 config/common_base|  1 +
 7 files changed, 162 insertions(+), 2 deletions(-)
 create mode 100644 app/test-pmd/fifo.c

diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
index 60ae9b9c1..9254a9440 100644
--- a/app/test-pmd/Makefile
+++ b/app/test-pmd/Makefile
@@ -34,6 +34,10 @@ SRCS-y += csumonly.c
 SRCS-y += icmpecho.c
 SRCS-$(CONFIG_RTE_LIBRTE_IEEE1588) += ieee1588fwd.c
 
+#ifeq ($(CONFIG_RTE_TEST_PMD_NOISY),y)
+#SRCS-y += fifo.c
+#endif
+
 ifeq ($(CONFIG_RTE_LIBRTE_PMD_SOFTNIC)$(CONFIG_RTE_LIBRTE_SCHED),yy)
 SRCS-y += tm.c
 endif
diff --git a/app/test-pmd/fifo.c b/app/test-pmd/fifo.c
new file mode 100644
index 0..2271215e8
--- /dev/null
+++ b/app/test-pmd/fifo.c
@@ -0,0 +1,16 @@
+#ifdef RTE_TEST_PMD_NOISY
+//#include 
+#include "fifo.h"
+//#include "testpmd.h"
+
+
+struct rte_ring * fifo_init(uint32_t qi)
+{
+   struct noisy_config *n = &noisy_cfg[qi];
+
+   n->f = rte_ring_create("noisy ring", 1024, rte_socket_id(),
+   RING_F_SP_ENQ | RING_F_SC_DEQ);
+   return n->f;
+}
+
+#endif
diff --git a/app/test-pmd/iofwd.c b/app/test-pmd/iofwd.c
index 9dce76efe..85fa000f7 100644
--- a/app/test-pmd/iofwd.c
+++ b/app/test-pmd/iofwd.c
@@ -36,6 +36,7 @@
 #include 
 
 #include "testpmd.h"
+#include "fifo.h"
 
 /*
  * Forwarding of packets in I/O mode.
@@ -48,7 +49,7 @@ pkt_burst_io_forward(struct fwd_stream *fs)
 {
struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
uint16_t nb_rx;
-   uint16_t nb_tx;
+   uint16_t nb_tx = 0;
uint32_t retry;
 
 #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
@@ -56,6 +57,15 @@ pkt_burst_io_forward(struct fwd_stream *fs)
uint64_t end_tsc;
uint64_t core_cycles;
 #endif
+#ifdef RTE_TEST_PMD_NOISY
+   const uint64_t freq_khz = rte_get_timer_hz() / 1000;
+   struct noisy_config *ncf = &noisy_cfg[fs->tx_queue];
+   struct rte_mbuf *tmp_pkts[MAX_PKT_BURST];
+   uint16_t nb_enqd;
+   uint16_t nb_deqd = 0;
+   uint64_t delta_ms;
+   uint64_t now;
+#endif
 
 #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
start_tsc = rte_rdtsc();
@@ -73,8 +83,55 @@ pkt_burst_io_forward(struct fwd_stream *fs)
 #ifdef RTE_TEST_PMD_RECORD_BURST_STATS
fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
 #endif
+#ifdef RTE_TEST_PMD_NOISY
+   if (bsize_before_send > 0) {
+   if (rte_ring_free_count(ncf->f) >= nb_rx) {
+   /* enqueue into fifo */
+   nb_enqd = fifo_put(ncf->f, pkts_burst, nb_rx);
+   if (nb_enqd < nb_rx)
+   nb_rx = nb_enqd;
+   } else {
+   /* fifo is full, dequeue first */
+   nb_deqd = fifo_get(ncf->f, tmp_pkts, nb_rx);
+   /* enqueue into fifo */
+   nb_enqd = fifo_put(ncf->f, pkts_burst, nb_deqd);
+   if (nb_enqd < nb_rx)
+   nb_rx = nb_enqd;
+   if (nb_deqd > 0)
+   nb_tx = rte_eth_tx_burst(fs->tx_port,
+   fs->tx_queue, tmp_pkts,
+   nb_deqd);
+   }
+   } else {
+   nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue,
+   pkts_burst, nb_rx);
+   }
+
+   /*
+* TX burst queue drain
+*/
+   if (ncf->prev_time == 0) {
+   now = ncf->prev_time = rte_get_timer_cycles();
+   } else {
+   now = rte_get_timer_cycles();
+   }
+   delta_ms = (now - ncf->prev_time) / freq_khz;
+   if (unlikely(delta_ms >= flush_timer) && flush_timer > 0 && (nb_tx == 
0)) {
+   while (fifo_count(ncf->f) > 0) {
+   nb_deqd = fifo_get(ncf->f, tmp_pkts, nb_rx);
+   nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue,
+tmp_pkts, nb_deqd);
+   if(rte_ring_empty(ncf->f))
+   break;
+   }
+   ncf->prev_time = now;
+   }
+   if (nb_tx < nb_rx && fs->retry_enabled)
+   *pkts_burst = *tmp_pkts;
+#else
nb_tx = rte_eth_tx_burst(f

[dpdk-dev] [RFC 0/1] testpmd: simulating noisy host environment

[Jens Freimann]

This is an addition to testpmd that will allow to simulate a noisy
neighbour VNF.  It was previously discussed here:
http://dpdk.org/ml/archives/dev/2017-October/080763.html

It was not thoroughly tested and doesn't implement all features yet. I'm
sending this only for early feedback about how to approach this. 

Currently I've implmented the logic to buffer incoming packets and flush
the queue after a timeout has expired. The logic resides in the iofwd
code as an example. I think it needs to be in the forwarding mode logic
as packets are send and received from there. To avoid code duplication
some of it could be extracted into new helpers and changes to the
individual forward mode code kept minimal.

What is missing is actual functionality to simulate route lookups (by
doing random r/w memory accesses) and to simulate packet processing.
I'm working on this, but would like to get feedback on what I have so
far.  


regards, Jens 


Jens Freimann (1):
  testpmd: add parameters buffersize-before-send and flush-timeout

 app/test-pmd/Makefile |  4 
 app/test-pmd/fifo.c   | 16 +
 app/test-pmd/iofwd.c  | 59 ++-
 app/test-pmd/parameters.c | 21 -
 app/test-pmd/testpmd.c| 48 ++
 app/test-pmd/testpmd.h| 15 
 config/common_base|  1 +
 7 files changed, 162 insertions(+), 2 deletions(-)
 create mode 100644 app/test-pmd/fifo.c

-- 
2.14.3

Re: [dpdk-dev] [PATCH v4] eal: add asynchronous request API to DPDK IPC

[Tan, Jianfeng]

Hi Anatoly,

Two general comments firstly.

Q1. As I understand, malloc usage as an example, when a primary process 
receives a request in rte_mp_handle thread, it will allocate memory, and 
broadcast an async request to all secondary processes, and it will 
return immediately; then responses are replied from each secondary 
process, which are recorded at rte_mp_async_handle thread firstly; 
either timeout or responses are all collected, rte_mp_async_handle will 
trigger an async action.


I agree the necessity of the async request API; without it, each caller 
who has similar requirement needs lots of code to implement it.
But can we achieve that without creating another pthread by leveraging 
the alarm set? For example, to send an async request,

step 1. set an alarm;
step 2. send the request;
step 3. receive and record responses in mp thread;
step 4. if alarm timeout, trigger the async action with timeout result;
step 5. if all responses are collected, cancel the alarm, and trigger 
the async action with success result.


I don't have strong objection for adding another thread, and actually 
this is an internal thing in ipc system, we can optimize it later.


Q2. Do we really need to register actions for result handler of async 
request? Instead, we can put it as a parameter into 
rte_mp_request_async(), whenever the request fails or succeeds, we just 
invoke the function.


Please see some other comments inline.

On 3/14/2018 1:42 AM, Anatoly Burakov wrote:

This API is similar to the blocking API that is already present,
but reply will be received in a separate callback by the caller.

Under the hood, we create a separate thread to deal with replies to
asynchronous requests, that will just wait to be notified by the
main thread, or woken up on a timer (it'll wake itself up every
minute regardless of whether it was called, but if there are no
requests in the queue, nothing will be done and it'll go to sleep
for another minute).


Wait for 1min seems a little strange to me; it shall wake up for a 
latest timeout of pending async requests?



Signed-off-by: Anatoly Burakov 
---

Notes:
 v4:
   - rebase on top of latest IPC Improvements patchset [2]
 v3:
   - added support for MP_IGN messages introduced in
 IPC improvements v5 patchset
 v2:
   - fixed deadlocks and race conditions by not calling
 callbacks while iterating over sync request list
   - fixed use-after-free by making a copy of request
   - changed API to also give user a copy of original
 request, so that they know to which message the
 callback is a reply to
   - fixed missing .map file entries
 
 This patch is dependent upon previously published patchsets

 for IPC fixes [1] and improvements [2].
 
 rte_mp_action_unregister and rte_mp_async_reply_unregister

 do the same thing - should we perhaps make it one function?
 
 [1] http://dpdk.org/dev/patchwork/bundle/aburakov/IPC_Fixes/

 [2] http://dpdk.org/dev/patchwork/bundle/aburakov/IPC_Improvements/

  lib/librte_eal/common/eal_common_proc.c | 563 ++--
  lib/librte_eal/common/include/rte_eal.h |  72 
  lib/librte_eal/rte_eal_version.map  |   3 +
  3 files changed, 607 insertions(+), 31 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c 
b/lib/librte_eal/common/eal_common_proc.c
index 4131b67..50d6506 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -26,6 +26,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "eal_private.h"

  #include "eal_filesystem.h"
@@ -39,7 +40,11 @@ static pthread_mutex_t mp_mutex_action = 
PTHREAD_MUTEX_INITIALIZER;
  struct action_entry {
TAILQ_ENTRY(action_entry) next;
char action_name[RTE_MP_MAX_NAME_LEN];
-   rte_mp_t action;
+   RTE_STD_C11
+   union {
+   rte_mp_t action;
+   rte_mp_async_reply_t reply;
+   };
  };
  
  /** Double linked list of actions. */

@@ -60,13 +65,37 @@ struct mp_msg_internal {
struct rte_mp_msg msg;
  };
  
+enum mp_request_type {

+   REQUEST_TYPE_SYNC,
+   REQUEST_TYPE_ASYNC
+};
+
+struct async_request_shared_param {
+   struct rte_mp_reply *user_reply;
+   struct timespec *end;


Why we have to make these two as pointers? Operating on pointers 
introduce unnecessary complexity.



+   int n_requests_processed;


It sounds like recording how many requests are sent, but it means how 
many responses are received, right? n_responses sounds better?



+};
+
+struct async_request_param {
+   struct async_request_shared_param *param;
+};
+
+struct sync_request_param {
+   pthread_cond_t cond;
+};
+
  struct sync_request {


I know "sync_" in the original version was for synchronization between 
the blocked thread (who sends the request) and the mp thread.


But it indeed makes the code difficult to understand. We may change the

Re: [dpdk-dev] [PATCH v2 28/41] eal: add support for multiprocess memory hotplug

[Tan, Jianfeng]

On 3/8/2018 12:56 AM, Anatoly Burakov wrote:

This enables multiprocess synchronization for memory hotplug
requests at runtime (as opposed to initialization).

Basic workflow is the following. Primary process always does initial
mapping and unmapping, and secondary processes always follow primary
page map. Only one allocation request can be active at any one time.

When primary allocates memory, it ensures that all other processes
have allocated the same set of hugepages successfully, otherwise
any allocations made are being rolled back, and heap is freed back.
Heap is locked throughout the process, so no race conditions can
happen.

When primary frees memory, it frees the heap, deallocates affected
pages, and notifies other processes of deallocations. Since heap is
freed from that memory chunk, the area basically becomes invisible
to other processes even if they happen to fail to unmap that
specific set of pages, so it's completely safe to ignore results of
sync requests.

When secondary allocates memory, it does not do so by itself.
Instead, it sends a request to primary process to try and allocate
pages of specified size and on specified socket, such that a
specified heap allocation request could complete. Primary process
then sends all secondaries (including the requestor) a separate
notification of allocated pages, and expects all secondary
processes to report success before considering pages as "allocated".

Only after primary process ensures that all memory has been
successfully allocated in all secondary process, it will respond
positively to the initial request, and let secondary proceed with
the allocation. Since the heap now has memory that can satisfy
allocation request, and it was locked all this time (so no other
allocations could take place), secondary process will be able to
allocate memory from the heap.

When secondary frees memory, it hides pages to be deallocated from
the heap. Then, it sends a deallocation request to primary process,
so that it deallocates pages itself, and then sends a separate sync
request to all other processes (including the requestor) to unmap
the same pages. This way, even if secondary fails to notify other
processes of this deallocation, that memory will become invisible
to other processes, and will not be allocated from again.

So, to summarize: address space will only become part of the heap
if primary process can ensure that all other processes have
allocated this memory successfully. If anything goes wrong, the
worst thing that could happen is that a page will "leak" and will
not be available to neither DPDK nor the system, as some process
will still hold onto it. It's not an actual leak, as we can account
for the page - it's just that none of the processes will be able
to use this page for anything useful, until it gets allocated from
by the primary.

Due to underlying DPDK IPC implementation being single-threaded,
some asynchronous magic had to be done, as we need to complete
several requests before we can definitively allow secondary process
to use allocated memory (namely, it has to be present in all other
secondary processes before it can be used). Additionally, only
one allocation request is allowed to be submitted at once.

Memory allocation requests are only allowed when there are no
secondary processes currently initializing. To enforce that,
a shared rwlock is used, that is set to read lock on init (so that
several secondaries could initialize concurrently), and write lock
on making allocation requests (so that either secondary init will
have to wait, or allocation request will have to wait until all
processes have initialized).

Signed-off-by: Anatoly Burakov 
---

Notes:
 v2: - fixed deadlocking on init problem
 - reverted rte_panic changes (fixed by changes in IPC instead)
 
 This problem is evidently complex to solve without multithreaded

 IPC implementation. An alternative approach would be to process
 each individual message in its own thread (or at least spawn a
 thread per incoming request) - that way, we can send requests
 while responding to another request, and this problem becomes
 trivial to solve (and in fact it was solved that way initially,
 before my aversion to certain other programming languages kicked
 in).
 
 Is the added complexity worth saving a couple of thread spin-ups

 here and there?

  lib/librte_eal/bsdapp/eal/Makefile|   1 +
  lib/librte_eal/common/eal_common_memory.c |  16 +-
  lib/librte_eal/common/include/rte_eal_memconfig.h |   3 +
  lib/librte_eal/common/malloc_heap.c   | 255 ++--
  lib/librte_eal/common/malloc_mp.c | 723 ++
  lib/librte_eal/common/malloc_mp.h |  86 +++
  lib/librte_eal/common/meson.build |   1 +
  lib/librte_eal/linuxapp/eal/Makefile  |   1 +
  8 files changed, 1040 insertions(+), 46 deletions(-)
  create mode 1006

Re: [dpdk-dev] [PATCH v4 1/2] eventdev: add device stop flush callback

[Van Haaren, Harry]

> From: Eads, Gage
> Sent: Tuesday, March 20, 2018 2:13 PM
> To: dev@dpdk.org
> Cc: jerin.ja...@caviumnetworks.com; Van Haaren, Harry
> ; hemant.agra...@nxp.com; Richardson, Bruce
> ; santosh.shu...@caviumnetworks.com;
> nipun.gu...@nxp.com
> Subject: [PATCH v4 1/2] eventdev: add device stop flush callback
> 
> When an event device is stopped, it drains all event queues. These events
> may contain pointers, so to prevent memory leaks eventdev now supports a
> user-provided flush callback that is called during the queue drain process.
> This callback is stored in process memory, so the callback must be
> registered by any process that may call rte_event_dev_stop().
> 
> This commit also clarifies the behavior of rte_event_dev_stop().
> 
> This follows this mailing list discussion:
> http://dpdk.org/ml/archives/dev/2018-January/087484.html
> 
> Signed-off-by: Gage Eads 



>  /**
> - * Stop an event device. The device can be restarted with a call to
> - * rte_event_dev_start()
> + * Stop an event device.
> + *
> + * This function causes all queued events to be drained. While draining
> events
> + * out of the device, this function calls the user-provided flush callback
> + * (if one was registered) once per event.
> + *
> + * This function does not drain events from event ports; the application is
> + * responsible for flushing events from all ports before stopping the
> device.


Question about how an application is expected to correctly cleanup all the
events here. Note in particular the last part: "application is responsible
for flushing events from all ports **BEFORE** stopping the device".

Given the event device is still running, how can the application be sure it has
flushed all the events (from the dequeue side in particular)?


In order to drain all events from the ports, I was expecting the following:

// stop scheduling new events to worker cores
rte_event_dev_stop()
---> callback gets called for each event

// to dequeue events from each port, and app cleans them up?
FOR_EACH_PORT( rte_event_dev_dequeue(..., port_id, ...) )


I'd like to avoid the dequeue-each-port() approach in application, as it adds
extra burden to clean up correctly...

What if we say that dequeue() returns zero after stop() (leaving events possibly
in the port-dequeue side SW buffers), and these events which were about to
be dequeued by the worker core are also passed to the dev_stop_flush callback?

Re: [dpdk-dev] [PATCH v5] ethdev: return named opaque type instead of void pointer

[Bruce Richardson]

On Wed, Mar 21, 2018 at 09:04:01AM -0400, Neil Horman wrote:
> On Tue, Mar 20, 2018 at 04:34:04PM +, Ferruh Yigit wrote:
> > "struct rte_eth_rxtx_callback" is defined as internal data structure and
> > used as named opaque type.
> > 
> > So the functions that are adding callbacks can return objects in this
> > type instead of void pointer.
> > 
> > Also const qualifier added to "struct rte_eth_rxtx_callback *" to
> > protect it better from application modification.
> > 
> > Signed-off-by: Ferruh Yigit 
> > ---
> > v2:
> > * keep using struct * in parameters, instead add callback functions
> > return struct rte_eth_rxtx_callback pointer.
> > 
> > v4:
> > * Remove deprecation notice. LIBABIVER already increased in this release
> > 
> > v5:
> > * add const qualifier to rte_eth_rxtx_callback
> I still wish we could find a way to remove the inline functions and truly
> protect that struct, but a const is definately better than nothing
> 
> Acked-by: Neil Horman 
> 
Actually, I think we should do exactly that - convert the rx and tx burst
calls into actual function calls (and consider any other inlined functions
too). The cost would be the overhead of making an additional function call
per-burst, which is likely to be pretty minimal for most common burst sizes
e.g. 32.

We did some quick tests here with the i40e driver, and for a burst size of
32 saw less than 1% perf drop, and for even a small burst of 8 saw less
than 5% drop. Note that this is testing with testpmd, which has nothing but
I/O in the datapath. A real-world app is likely to do far more with the
packets and therefore see proportionally far less perf hit.

Thoughts?

/Bruce

PS: This un-inlining could probably be applied to other device types too,
e.g. cryptodev (though probably not eventdev as it tends to have smaller
bursts in some use-cases).

Re: [dpdk-dev] [PATCH v4 2/2] event/sw: support device stop flush callback

[Van Haaren, Harry]

> From: Eads, Gage
> Sent: Tuesday, March 20, 2018 2:13 PM
> To: dev@dpdk.org
> Cc: jerin.ja...@caviumnetworks.com; Van Haaren, Harry
> ; hemant.agra...@nxp.com; Richardson, Bruce
> ; santosh.shu...@caviumnetworks.com;
> nipun.gu...@nxp.com
> Subject: [PATCH v4 2/2] event/sw: support device stop flush callback
> 
> This commit also adds a flush callback test to the sw eventdev's selftest
> suite.
> 
> Signed-off-by: Gage Eads 
> ---
>  drivers/event/sw/sw_evdev.c  | 25 ++-
>  drivers/event/sw/sw_evdev_selftest.c | 80
> +++-
>  2 files changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
> index 0e89f11..11f394f 100644
> --- a/drivers/event/sw/sw_evdev.c
> +++ b/drivers/event/sw/sw_evdev.c
> @@ -362,8 +362,25 @@ sw_init_qid_iqs(struct sw_evdev *sw)
>  }
> 
>  static void
> -sw_clean_qid_iqs(struct sw_evdev *sw)
> +sw_flush_iq(struct rte_eventdev *dev, struct sw_iq *iq)
>  {
> + struct sw_evdev *sw = sw_pmd_priv(dev);
> +
> + while (iq_count(iq) > 0) {
> + struct rte_event event;
> +
> + iq_dequeue_burst(sw, iq, &event, 1);
> +
> + dev->dev_ops->dev_stop_flush(dev->data->dev_id,
> +  event,
> +  dev->data->dev_stop_flush_arg);


Adding check that dev_stop_flush() is non-NULL?

[Update: Ah I see you do this below already. Still, better check twice
I think, the data path isn't running here anyway in case future me
decides to call sw_flush_iq() without performing the check]

if(dev->dev_ops->dev_stop_flush)
   dev->dev_ops->dev_stop_flush(...);




> @@ -702,7 +723,7 @@ static void
>  sw_stop(struct rte_eventdev *dev)
>  {
>   struct sw_evdev *sw = sw_pmd_priv(dev);
> - sw_clean_qid_iqs(sw);
> + sw_clean_qid_iqs(dev);

Based on the port buffers comment on 1/2, we probably need a
sw_clean_port_buffers(sw); here to return any events in the port
owned SW buffers?


Rest looks good to me!

[dpdk-dev] [PATCH v2] net/bonding: clear dev_started if start fails

[Chas Williams]

From: "Charles (Chas) Williams" 

There are several error paths where the bonding device may not start.
Clear dev_started before we return if we take one of these paths.

Fixes: 2efb58cbab ("bond: new link bonding library")
Cc: sta...@dpdk.org

Signed-off-by: Chas Williams 
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
b/drivers/net/bonding/rte_eth_bond_pmd.c
index b59ba9f7c..1decd8bed 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2034,7 +2034,7 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
 
if (internals->slave_count == 0) {
RTE_BOND_LOG(ERR, "Cannot start port since there are no slave 
devices");
-   return -1;
+   goto out_err;
}
 
if (internals->user_defined_mac == 0) {
@@ -2045,18 +2045,18 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
new_mac_addr = 
&internals->slaves[i].persisted_mac_addr;
 
if (new_mac_addr == NULL)
-   return -1;
+   goto out_err;
 
if (mac_address_set(eth_dev, new_mac_addr) != 0) {
RTE_BOND_LOG(ERR, "bonded port (%d) failed to update 
MAC address",
eth_dev->data->port_id);
-   return -1;
+   goto out_err;
}
}
 
/* Update all slave devices MACs*/
if (mac_address_slaves_update(eth_dev) != 0)
-   return -1;
+   goto out_err;
 
/* If bonded device is configure in promiscuous mode then re-apply 
config */
if (internals->promiscuous_en)
@@ -2081,7 +2081,7 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
"bonded port (%d) failed to reconfigure slave 
device (%d)",
eth_dev->data->port_id,
internals->slaves[i].port_id);
-   return -1;
+   goto out_err;
}
/* We will need to poll for link status if any slave doesn't
 * support interrupts
@@ -2089,6 +2089,7 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
if (internals->slaves[i].link_status_poll_enabled)
internals->link_status_polling_enabled = 1;
}
+
/* start polling if needed */
if (internals->link_status_polling_enabled) {
rte_eal_alarm_set(
@@ -2108,6 +2109,10 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
bond_tlb_enable(internals);
 
return 0;
+
+out_err:
+   eth_dev->data->dev_started = 0;
+   return -1;
 }
 
 static void
-- 
2.13.6

[dpdk-dev] [PATCH 0/4] NFP PF support based on new CPP interface

[Alejandro Lucero]

NFP PMD PF support requires to access the NFP chip for initialization.
Current NFP PMD PF support was added based on the NSPU interface. This
implies to do initialization through the NSP, a embedded ARM processor
which does initialization tasks on demand. The main problem with this
approach is it requires to add support for new NSP commands each time
a new functionality is required, which does not scale well and it is
not really flexible.

Using the new CPP user space interface, the PMD can do whatever could
be done by the NSP, this is current commands and any new functionality
required. This CPP interface allows to access any single chip component
facilitating initialization, firmware uploading, firmware debugging or
extended stats.

The changes just change the PMD PF initialization and do not touch the
datapath at all. No performance changes nor PMD functionalities are affected.

The initial impact using the new CPP interface is the way firmware upload
is handled, which helps the PMD detecting the card type and the firmware file
to upload. Future commits will include extended stats and some sort of debug
channel.

The specific CPP code is contained in the first patch, which has not been
splitted up because is completely internal to the NFP functionality. The
second patch makes the PMD changes required for using the new interface.

Alejandro Lucero (4):
  net/nfp: add NFP CPP support
  net/nfp: update PMD for using new CPP interface
  doc: update NFP guide
  net/nfp: remove files

 doc/guides/nics/nfp.rst   |  31 +-
 drivers/net/nfp/Makefile  |  17 +-
 drivers/net/nfp/nfp_net.c | 342 +---
 drivers/net/nfp/nfp_net_eth.h |  82 --
 drivers/net/nfp/nfp_net_pmd.h |  16 +-
 drivers/net/nfp/nfp_nfpu.c| 108 ---
 drivers/net/nfp/nfp_nfpu.h|  55 --
 drivers/net/nfp/nfp_nspu.c| 642 ---
 drivers/net/nfp/nfp_nspu.h|  83 --
 drivers/net/nfp/nfpcore/nfp-common/nfp_cppat.h| 748 +
 drivers/net/nfp/nfpcore/nfp-common/nfp_platform.h |  62 ++
 drivers/net/nfp/nfpcore/nfp-common/nfp_resid.h| 620 ++
 drivers/net/nfp/nfpcore/nfp6000/nfp6000.h |  68 ++
 drivers/net/nfp/nfpcore/nfp6000/nfp_xpb.h |  54 ++
 drivers/net/nfp/nfpcore/nfp_cpp.h | 803 ++
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c| 962 ++
 drivers/net/nfp/nfpcore/nfp_cppcore.c | 901 
 drivers/net/nfp/nfpcore/nfp_crc.c |  75 ++
 drivers/net/nfp/nfpcore/nfp_crc.h |  45 +
 drivers/net/nfp/nfpcore/nfp_hwinfo.c  | 226 +
 drivers/net/nfp/nfpcore/nfp_hwinfo.h  | 111 +++
 drivers/net/nfp/nfpcore/nfp_mip.c | 180 
 drivers/net/nfp/nfpcore/nfp_mip.h |  47 ++
 drivers/net/nfp/nfpcore/nfp_mutex.c   | 450 ++
 drivers/net/nfp/nfpcore/nfp_nffw.c| 261 ++
 drivers/net/nfp/nfpcore/nfp_nffw.h| 112 +++
 drivers/net/nfp/nfpcore/nfp_nsp.c | 453 ++
 drivers/net/nfp/nfpcore/nfp_nsp.h | 330 
 drivers/net/nfp/nfpcore/nfp_nsp_cmds.c| 135 +++
 drivers/net/nfp/nfpcore/nfp_nsp_eth.c | 691 
 drivers/net/nfp/nfpcore/nfp_resource.c| 291 +++
 drivers/net/nfp/nfpcore/nfp_resource.h|  78 ++
 drivers/net/nfp/nfpcore/nfp_rtsym.c   | 353 
 drivers/net/nfp/nfpcore/nfp_rtsym.h   |  87 ++
 drivers/net/nfp/nfpcore/nfp_target.h  | 605 ++
 35 files changed, 9036 insertions(+), 1088 deletions(-)
 delete mode 100644 drivers/net/nfp/nfp_net_eth.h
 delete mode 100644 drivers/net/nfp/nfp_nfpu.c
 delete mode 100644 drivers/net/nfp/nfp_nfpu.h
 delete mode 100644 drivers/net/nfp/nfp_nspu.c
 delete mode 100644 drivers/net/nfp/nfp_nspu.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp-common/nfp_cppat.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp-common/nfp_platform.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp-common/nfp_resid.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp6000/nfp6000.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp6000/nfp_xpb.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp_cpp.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
 create mode 100644 drivers/net/nfp/nfpcore/nfp_cppcore.c
 create mode 100644 drivers/net/nfp/nfpcore/nfp_crc.c
 create mode 100644 drivers/net/nfp/nfpcore/nfp_crc.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp_hwinfo.c
 create mode 100644 drivers/net/nfp/nfpcore/nfp_hwinfo.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp_mip.c
 create mode 100644 drivers/net/nfp/nfpcore/nfp_mip.h
 create mode 100644 drivers/net/nfp/nfpcore/nfp_mutex.c
 create mode 100644 drivers/net/nfp/n

[dpdk-dev] [PATCH 3/4] doc: update NFP guide

[Alejandro Lucero]

New CPP interface changes the way firmware upload is managed by
the PMD. It also supports different firmware file names for
having specific firmware aplications per card.

Signed-off-by: Alejandro Lucero 
---
 doc/guides/nics/nfp.rst | 31 ---
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/doc/guides/nics/nfp.rst b/doc/guides/nics/nfp.rst
index 99a3b76..6b1f9fc 100644
--- a/doc/guides/nics/nfp.rst
+++ b/doc/guides/nics/nfp.rst
@@ -99,16 +99,33 @@ have a PMD able to work with the PF and VFs at the same 
time and with the PF
 implementing VF management along with other PF-only functionalities/offloads.
 
 The PMD PF has extra work to do which will delay the DPDK app initialization
-like checking if a firmware is already available in the device, uploading the
-firmware if necessary, and configure the Link state properly when starting or
-stopping a PF port. Note that firmware upload is not always necessary which is
-the main delay for NFP PF PMD initialization.
+like uploading the firmware and configure the Link state properly when 
starting or
+stopping a PF port. Since DPDK 18.05 the firmware upload is always happening 
when
+a PF is initialized, which was not always true with older DPDK versions.
 
 Depending on the Netronome product installed in the system, firmware files
 should be available under ``/lib/firmware/netronome``. DPDK PMD supporting the
-PF requires a specific link, ``/lib/firmware/netronome/nic_dpdk_default.nffw``,
-which should be created automatically with Netronome's Agilio products
-installation.
+PF looks for a firmware file in this order:
+
+   1) First try to find a firmware image specific for this device using the
+  NFP serial number:
+
+   serial-00-15-4d-12-20-65-10-ff.nffw
+
+   2) Then try the PCI name:
+
+   pci-:04:00.0.nffw
+
+   3) Finally try the card type and media:
+
+   nic_AMDA0099-0001_2x25.nffw
+
+Netronome's software packages install firmware files under 
``/lib/firmware/netronome``
+for supporting all the Netronome's SmartNICs and also different firmware 
applications.
+This is usually done using file names based on SmartNIC type and media and 
with a
+directory per firmware application. Options 1 and 2 for firmware filenames 
allow to
+have more than one SmartNIC, same type of SmartNIC or different ones, and to 
upload a
+different firmware to each SmartNIC.
 
 PF multiport support
 
-- 
1.9.1

[dpdk-dev] [PATCH 2/4] net/nfp: update PMD for using new CPP interface

[Alejandro Lucero]

PF PMD support was based on NSPU interface. This patch changes the
PMD for using the new CPP user space interface which gives more
flexibility for adding new functionalities, and specifically at this
point, for properly selecting the right firmware file which requires
to know about the card to work with.

This change just changes initialization with the datapath being unaffected.

Signed-off-by: Alejandro Lucero 
---
 drivers/net/nfp/Makefile  |  17 ++-
 drivers/net/nfp/nfp_net.c | 342 +-
 drivers/net/nfp/nfp_net_pmd.h |  16 +-
 3 files changed, 264 insertions(+), 111 deletions(-)

diff --git a/drivers/net/nfp/Makefile b/drivers/net/nfp/Makefile
index aa3b68a..ab4e0a7 100644
--- a/drivers/net/nfp/Makefile
+++ b/drivers/net/nfp/Makefile
@@ -20,11 +20,24 @@ EXPORT_MAP := rte_pmd_nfp_version.map
 
 LIBABIVER := 1
 
+VPATH += $(SRCDIR)/nfpcore
+
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_cppcore.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_cpp_pcie_ops.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_mutex.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_resource.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_crc.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_mip.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_nffw.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_hwinfo.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_rtsym.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_nsp.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_nsp_cmds.c
+SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_nsp_eth.c
+
 #
 # all source are stored in SRCS-y
 #
 SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_net.c
-SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_nfpu.c
-SRCS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += nfp_nspu.c
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index e5bfde6..0657a23 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2015 Netronome Systems, Inc.
+ * Copyright (c) 2014-2018 Netronome Systems, Inc.
  * All rights reserved.
  *
  * Small portions derived from code Copyright(c) 2010-2015 Intel Corporation.
@@ -55,7 +55,13 @@
 #include 
 #include 
 
-#include "nfp_nfpu.h"
+#include "nfpcore/nfp_cpp.h"
+#include "nfpcore/nfp_nffw.h"
+#include "nfpcore/nfp_hwinfo.h"
+#include "nfpcore/nfp_mip.h"
+#include "nfpcore/nfp_rtsym.h"
+#include "nfpcore/nfp_nsp.h"
+
 #include "nfp_net_pmd.h"
 #include "nfp_net_logs.h"
 #include "nfp_net_ctrl.h"
@@ -104,12 +110,8 @@ static int nfp_net_rss_reta_write(struct rte_eth_dev *dev,
 static int nfp_net_rss_hash_write(struct rte_eth_dev *dev,
struct rte_eth_rss_conf *rss_conf);
 
-/*
- * The offset of the queue controller queues in the PCIe Target. These
- * happen to be at the same offset on the NFP6000 and the NFP3200 so
- * we use a single macro here.
- */
-#define NFP_PCIE_QUEUE(_q) (0x800 * ((_q) & 0xff))
+/* The offset of the queue controller queues in the PCIe Target */
+#define NFP_PCIE_QUEUE(_q) (0x8 + (NFP_QCP_QUEUE_ADDR_SZ * ((_q) & 0xff)))
 
 /* Maximum value which can be added to a queue with one transaction */
 #define NFP_QCP_MAX_ADD0x7f
@@ -625,47 +627,29 @@ enum nfp_qcp_ptr {
 #define ETH_ADDR_LEN   6
 
 static void
-nfp_eth_copy_mac_reverse(uint8_t *dst, const uint8_t *src)
+nfp_eth_copy_mac(uint8_t *dst, const uint8_t *src)
 {
int i;
 
for (i = 0; i < ETH_ADDR_LEN; i++)
-   dst[ETH_ADDR_LEN - i - 1] = src[i];
+   dst[i] = src[i];
 }
 
 static int
 nfp_net_pf_read_mac(struct nfp_net_hw *hw, int port)
 {
-   union eth_table_entry *entry;
-   int idx, i;
-
-   idx = port;
-   entry = hw->eth_table;
-
-   /* Reading NFP ethernet table obtained before */
-   for (i = 0; i < NSP_ETH_MAX_COUNT; i++) {
-   if (!(entry->port & NSP_ETH_PORT_LANES_MASK)) {
-   /* port not in use */
-   entry++;
-   continue;
-   }
-   if (idx == 0)
-   break;
-   idx--;
-   entry++;
-   }
-
-   if (i == NSP_ETH_MAX_COUNT)
-   return -EINVAL;
+   struct nfp_eth_table *nfp_eth_table;
 
+   nfp_eth_table = nfp_eth_read_ports(hw->cpp);
/*
 * hw points to port0 private data. We need hw now pointing to
 * right port.
 */
hw += port;
-   nfp_eth_copy_mac_reverse((uint8_t *)&hw->mac_addr,
-(uint8_t *)&entry->mac_addr);
+   nfp_eth_copy_mac((uint8_t *)&hw->mac_addr,
+(uint8_t *)&nfp_eth_table->ports[port].mac_addr);
 
+   free(nfp_eth_table);
return 0;
 }
 
@@ -831,7 +815,7 @@ enum nfp_qcp_ptr {
 
if (hw->is_pf)
/* Configure the physical port up */
-   nfp_nsp_eth_config(hw->nspu_desc, hw->pf_port_idx, 1);
+   nfp_eth_set_configured(hw->cpp, hw->pf_port_idx, 1);
 
hw->ctr

[dpdk-dev] [PATCH 4/4] net/nfp: remove files

[Alejandro Lucero]

New CPP interface makes NSPU interface obsolete. These files are
not needed anymore.

Signed-off-by: Alejandro Lucero 
---
 drivers/net/nfp/nfp_net_eth.h |  82 --
 drivers/net/nfp/nfp_nfpu.c| 108 ---
 drivers/net/nfp/nfp_nfpu.h|  55 
 drivers/net/nfp/nfp_nspu.c| 642 --
 drivers/net/nfp/nfp_nspu.h|  83 --
 5 files changed, 970 deletions(-)
 delete mode 100644 drivers/net/nfp/nfp_net_eth.h
 delete mode 100644 drivers/net/nfp/nfp_nfpu.c
 delete mode 100644 drivers/net/nfp/nfp_nfpu.h
 delete mode 100644 drivers/net/nfp/nfp_nspu.c
 delete mode 100644 drivers/net/nfp/nfp_nspu.h

diff --git a/drivers/net/nfp/nfp_net_eth.h b/drivers/net/nfp/nfp_net_eth.h
deleted file mode 100644
index af57f03..000
--- a/drivers/net/nfp/nfp_net_eth.h
+++ /dev/null
@@ -1,82 +0,0 @@
-/*
- * Copyright (c) 2017 Netronome Systems, Inc.
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are met:
- *
- * 1. Redistributions of source code must retain the above copyright notice,
- *  this list of conditions and the following disclaimer.
- *
- * 2. Redistributions in binary form must reproduce the above copyright
- *  notice, this list of conditions and the following disclaimer in the
- *  documentation and/or other materials provided with the distribution
- *
- * 3. Neither the name of the copyright holder nor the names of its
- *  contributors may be used to endorse or promote products derived from this
- *  software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
- * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
- * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
- * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
- * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
- * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
- * POSSIBILITY OF SUCH DAMAGE.
- */
-
-/*
- * vim:shiftwidth=8:noexpandtab
- *
- * @file dpdk/pmd/nfp_net_eth.h
- *
- * Netronome NFP_NET PDM driver
- */
-
-union eth_table_entry {
-   struct {
-   uint64_t port;
-   uint64_t state;
-   uint8_t mac_addr[6];
-   uint8_t resv[2];
-   uint64_t control;
-   };
-   uint64_t raw[4];
-};
-
-#ifndef BIT_ULL
-#define BIT_ULL(a) (1ULL << (a))
-#endif
-
-#define NSP_ETH_NBI_PORT_COUNT  24
-#define NSP_ETH_MAX_COUNT   (2 * NSP_ETH_NBI_PORT_COUNT)
-#define NSP_ETH_TABLE_SIZE   (NSP_ETH_MAX_COUNT * sizeof(union 
eth_table_entry))
-
-#define NSP_ETH_PORT_LANES  0xf
-#define NSP_ETH_PORT_INDEX  0xff00
-#define NSP_ETH_PORT_LABEL  0x3f
-#define NSP_ETH_PORT_PHYLABEL   0xfc0
-
-#define NSP_ETH_PORT_LANES_MASK rte_cpu_to_le_64(NSP_ETH_PORT_LANES)
-
-#define NSP_ETH_STATE_CONFIGUREDBIT_ULL(0)
-#define NSP_ETH_STATE_ENABLED   BIT_ULL(1)
-#define NSP_ETH_STATE_TX_ENABLEDBIT_ULL(2)
-#define NSP_ETH_STATE_RX_ENABLEDBIT_ULL(3)
-#define NSP_ETH_STATE_RATE  0xf00
-#define NSP_ETH_STATE_INTERFACE 0xff000
-#define NSP_ETH_STATE_MEDIA 0x30
-#define NSP_ETH_STATE_OVRD_CHNG BIT_ULL(22)
-#define NSP_ETH_STATE_ANEG  0x380
-
-#define NSP_ETH_CTRL_CONFIGURED BIT_ULL(0)
-#define NSP_ETH_CTRL_ENABLEDBIT_ULL(1)
-#define NSP_ETH_CTRL_TX_ENABLED BIT_ULL(2)
-#define NSP_ETH_CTRL_RX_ENABLED BIT_ULL(3)
-#define NSP_ETH_CTRL_SET_RATE   BIT_ULL(4)
-#define NSP_ETH_CTRL_SET_LANES  BIT_ULL(5)
-#define NSP_ETH_CTRL_SET_ANEG   BIT_ULL(6)
diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c
deleted file mode 100644
index f11afef..000
--- a/drivers/net/nfp/nfp_nfpu.c
+++ /dev/null
@@ -1,108 +0,0 @@
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include 
-#include 
-
-#include "nfp_nfpu.h"
-
-/* PF BAR and expansion BAR for the NSP interface */
-#define NFP_CFG_PCIE_BAR0
-#define NFP_CFG_EXP_BAR 7
-
-#define NFP_CFG_EXP_BAR_CFG_BASE   0x3
-
-/* There could be other NFP userspace tools using the NSP interface.
- * Make sure there is no other process using it and locking the access for
- * avoiding problems.
- */
-static int
-nspv_aquire_process_lock(nfpu_desc_t *desc)
-{
-   int rc;
-   struct flock lock;
-   char

Re: [dpdk-dev] [PATCH] compressdev: implement API

[Trahe, Fiona]

Hi Tomas,
Sorry for the delay in replying to this

> -Original Message-
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Sunday, February 4, 2018 2:25 PM
> To: Trahe, Fiona 
> Cc: dev@dpdk.org; ahmed.mans...@nxp.com; shally.ve...@cavium.com; De Lara 
> Guarch, Pablo
> 
> Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API
> 
> 02/02/2018 19:25, Fiona Trahe:
> >  config/common_base |   6 +
> >  doc/api/doxy-api-index.md  |   1 +
> >  doc/api/doxy-api.conf  |   1 +
> >  lib/Makefile   |   3 +
> >  lib/librte_compressdev/Makefile|  29 +
> >  lib/librte_compressdev/rte_comp.h  | 503 
> 
> Why rte_comp.h instead of the more consistent rte_compress.h?
[Fiona]  I did originally... but ran into difficulty with horribly names like
RTE_COMPRESS_COMPRESS
RTE_COMPRESS_DECOMPRESS
rte_compress_compress_xform
rte_compress_decompress_xform
So compress is both the module prefix and the name of one of the actions.
I could have used compressdev - but names were very long.
So decided to opt for using 
_compressdev_ in names to do with the device and
_comp_ in names to do with the compression service

Also I could have used compdev instead of compressdev, 
but I felt compress should be in the lib name

> 
> >  lib/librte_compressdev/rte_compressdev.c   | 902 
> > +
> >  lib/librte_compressdev/rte_compressdev.h   | 757 +
> >  lib/librte_compressdev/rte_compressdev_pmd.c   | 163 
> >  lib/librte_compressdev/rte_compressdev_pmd.h   | 439 ++
> >  lib/librte_compressdev/rte_compressdev_version.map |  47 ++
> >  lib/librte_eal/common/include/rte_log.h|   1 +
> >  mk/rte.app.mk  |   1 +
> >  13 files changed, 2853 insertions(+)
> 
> Please update MAINTAINERS file and release notes.
> 
> Maybe it is worth splitting this patch in few shorter parts?
[Fiona] will do
> 
> 
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -535,6 +535,12 @@ CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO=n
> >  CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO_DEBUG=n
> >
> >  #
> > +# Compile generic compression device library
> > +#
> > +CONFIG_RTE_LIBRTE_COMPRESSDEV=y
> > +CONFIG_RTE_COMPRESS_MAX_DEVS=64
> > +
> > +#
> >  # Compile generic security library
> >  #
> >  CONFIG_RTE_LIBRTE_SECURITY=y
> > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> > index d77f205..07b8e75 100644
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -43,6 +43,7 @@ The public API headers are grouped by topics:
> >[rte_tm] (@ref rte_tm.h),
> >[rte_mtr](@ref rte_mtr.h),
> >[bbdev]  (@ref rte_bbdev.h),
> > +  [compressdev](@ref rte_compressdev.h),
> >[cryptodev]  (@ref rte_cryptodev.h),
> >[security]   (@ref rte_security.h),
> >[eventdev]   (@ref rte_eventdev.h),
> 
> Please move it between security and eventdev in these lists.
ok

Re: [dpdk-dev] [PATCH v4] eal: add asynchronous request API to DPDK IPC

[Burakov, Anatoly]

On 23-Mar-18 3:38 PM, Tan, Jianfeng wrote:

Hi Anatoly,

Two general comments firstly.

Q1. As I understand, malloc usage as an example, when a primary process 
receives a request in rte_mp_handle thread, it will allocate memory, and 
broadcast an async request to all secondary processes, and it will 
return immediately; then responses are replied from each secondary 
process, which are recorded at rte_mp_async_handle thread firstly; 
either timeout or responses are all collected, rte_mp_async_handle will 
trigger an async action.


I agree the necessity of the async request API; without it, each caller 
who has similar requirement needs lots of code to implement it.
But can we achieve that without creating another pthread by leveraging 
the alarm set? For example, to send an async request,

step 1. set an alarm;
step 2. send the request;
step 3. receive and record responses in mp thread;
step 4. if alarm timeout, trigger the async action with timeout result;
step 5. if all responses are collected, cancel the alarm, and trigger 
the async action with success result.


That would work, but this "alarm" sounds like another thread. The main 
thread is always blocked in recvmsg(), and interrupting that would 
involve a signal, which is a recipe for disaster (you know, global 
overwritable signal handlers, undefined behavior as to who actually gets 
the signal, stuff like that).


That is, unless you were referring to DPDK's own "alarm" API, in which 
case it's a chicken and egg problem - we want to use alarm for IPC, but 
can't because rte_malloc relies on IPC, which relies on alarm, which 
relies on rte_malloc. So unless we rewrite (or add an option for) alarm 
API to not use rte_malloc'd memory, this isn't going to work.


We of course can do it, but really, i would be inclined to leave this as 
is in the interests of merging this earlier, unless there are strong 
objections to it.




I don't have strong objection for adding another thread, and actually 
this is an internal thing in ipc system, we can optimize it later.


Q2. Do we really need to register actions for result handler of async 
request? Instead, we can put it as a parameter into 
rte_mp_request_async(), whenever the request fails or succeeds, we just 
invoke the function.


That can be done, sure. This would look cleaner on the backend too, so 
i'll probably do that for v5.




Please see some other comments inline.

On 3/14/2018 1:42 AM, Anatoly Burakov wrote:

This API is similar to the blocking API that is already present,
but reply will be received in a separate callback by the caller.

Under the hood, we create a separate thread to deal with replies to
asynchronous requests, that will just wait to be notified by the
main thread, or woken up on a timer (it'll wake itself up every
minute regardless of whether it was called, but if there are no
requests in the queue, nothing will be done and it'll go to sleep
for another minute).


Wait for 1min seems a little strange to me; it shall wake up for a 
latest timeout of pending async requests?


We do. However, we have to wait for *something* if there aren't any 
asynchronous requests pending. There isn't a way to put "wait infinite 
amount" as a time value, so i opted for next best thing - large enough 
to not cause any performance issues. The timeout is arbitrary.



  /** Double linked list of actions. */
@@ -60,13 +65,37 @@ struct mp_msg_internal {
  struct rte_mp_msg msg;
  };
+enum mp_request_type {
+    REQUEST_TYPE_SYNC,
+    REQUEST_TYPE_ASYNC
+};
+
+struct async_request_shared_param {
+    struct rte_mp_reply *user_reply;
+    struct timespec *end;


Why we have to make these two as pointers? Operating on pointers 
introduce unnecessary complexity.


Because those are shared between different pending requests. Each 
pending request gets its own entry in the queue (because it expects 
answer from a particular process), but the request data (callback, 
number of requests processed, etc.) is shared between all requests for 
this sync operation. We don't have the luxury of storing all of that in 
a local variable like we do with synchronous requests :)





+    int n_requests_processed;


It sounds like recording how many requests are sent, but it means how 
many responses are received, right? n_responses sounds better?


No, it doesn't. Well, perhaps "n_requests_processed" should be 
"n_responses_processed", but not "n_responses", because this is a value 
that is indicating how many responses we've already seen (to know when 
to trigger the callback).





+};
+
+struct async_request_param {
+    struct async_request_shared_param *param;
+};
+
+struct sync_request_param {
+    pthread_cond_t cond;
+};
+
  struct sync_request {


I know "sync_" in the original version was for synchronization between 
the blocked thread (who sends the request) and the mp thread.


But it indeed makes the code difficult to understand. We may change the 
name to "pending_request"?


This can be done, su

[dpdk-dev] [PATCH 1/2] kvargs: the life of the party

[Gaetan Rivet]

Signed-off-by: Gaetan Rivet 
---

This is a rough sketch of what would be done to move rte_kvargs
within the EAL and change the parsing in rte_dev to use it.

 drivers/baseband/Makefile  |  2 +-
 lib/Makefile   |  5 +-
 lib/librte_eal/Makefile|  2 +
 lib/librte_eal/common/Makefile |  2 +-
 lib/librte_eal/kvargs/Makefile | 23 ++
 .../kvargs}/meson.build|  0
 .../kvargs}/rte_kvargs.c   | 12 +++--
 .../kvargs}/rte_kvargs.h   |  0
 .../kvargs}/rte_kvargs_version.map |  0
 .../{common/include => kvargs}/rte_string_fns.h|  0
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c|  2 +-
 lib/librte_kvargs/Makefile | 53 --
 12 files changed, 37 insertions(+), 64 deletions(-)
 create mode 100644 lib/librte_eal/kvargs/Makefile
 rename lib/{librte_kvargs => librte_eal/kvargs}/meson.build (100%)
 rename lib/{librte_kvargs => librte_eal/kvargs}/rte_kvargs.c (96%)
 rename lib/{librte_kvargs => librte_eal/kvargs}/rte_kvargs.h (100%)
 rename lib/{librte_kvargs => librte_eal/kvargs}/rte_kvargs_version.map (100%)
 rename lib/librte_eal/{common/include => kvargs}/rte_string_fns.h (100%)
 delete mode 100644 lib/librte_kvargs/Makefile

diff --git a/drivers/baseband/Makefile b/drivers/baseband/Makefile
index 4ec83b0a0..64f6f9ca7 100644
--- a/drivers/baseband/Makefile
+++ b/drivers/baseband/Makefile
@@ -4,7 +4,7 @@
 include $(RTE_SDK)/mk/rte.vars.mk
 
 core-libs := librte_eal librte_mbuf librte_mempool librte_ring
-core-libs += librte_bbdev librte_kvargs librte_cfgfile
+core-libs += librte_bbdev librte_cfgfile
 
 DIRS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_NULL) += null
 DEPDIRS-null = $(core-libs)
diff --git a/lib/Makefile b/lib/Makefile
index 6bb6a8bed..aef95b201 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -20,12 +20,11 @@ DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
 DEPDIRS-librte_cmdline := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ether
 DEPDIRS-librte_ether := librte_net librte_eal librte_mempool librte_ring
-DEPDIRS-librte_ether += librte_mbuf librte_kvargs
+DEPDIRS-librte_ether += librte_mbuf
 DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += librte_bbdev
 DEPDIRS-librte_bbdev := librte_eal librte_mempool librte_mbuf
 DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += librte_cryptodev
 DEPDIRS-librte_cryptodev := librte_eal librte_mempool librte_ring librte_mbuf
-DEPDIRS-librte_cryptodev += librte_kvargs
 DIRS-$(CONFIG_RTE_LIBRTE_SECURITY) += librte_security
 DEPDIRS-librte_security := librte_eal librte_mempool librte_ring librte_mbuf
 DEPDIRS-librte_security += librte_ether
@@ -71,8 +70,6 @@ DEPDIRS-librte_flow_classify :=  librte_net librte_table 
librte_acl
 DIRS-$(CONFIG_RTE_LIBRTE_SCHED) += librte_sched
 DEPDIRS-librte_sched := librte_eal librte_mempool librte_mbuf librte_net
 DEPDIRS-librte_sched += librte_timer
-DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs
-DEPDIRS-librte_kvargs := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_DISTRIBUTOR) += librte_distributor
 DEPDIRS-librte_distributor := librte_eal librte_mbuf librte_ether
 DIRS-$(CONFIG_RTE_LIBRTE_PORT) += librte_port
diff --git a/lib/librte_eal/Makefile b/lib/librte_eal/Makefile
index ccd45cb84..d6b520aeb 100644
--- a/lib/librte_eal/Makefile
+++ b/lib/librte_eal/Makefile
@@ -3,7 +3,9 @@
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
+DIRS-y += kvargs
 DIRS-y += common
+DEPDIRS-common := kvargs
 DIRS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += linuxapp
 DEPDIRS-linuxapp := common
 DIRS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += bsdapp
diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
index 211b21b4b..607ae0447 100644
--- a/lib/librte_eal/common/Makefile
+++ b/lib/librte_eal/common/Makefile
@@ -9,7 +9,7 @@ INC += rte_errno.h rte_launch.h rte_lcore.h
 INC += rte_log.h rte_memory.h rte_memzone.h
 INC += rte_per_lcore.h rte_random.h
 INC += rte_tailq.h rte_interrupts.h rte_alarm.h
-INC += rte_string_fns.h rte_version.h
+INC += rte_version.h
 INC += rte_eal_memconfig.h rte_malloc_heap.h
 INC += rte_hexdump.h rte_devargs.h rte_bus.h rte_dev.h rte_class.h
 INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
diff --git a/lib/librte_eal/kvargs/Makefile b/lib/librte_eal/kvargs/Makefile
new file mode 100644
index 0..65d15744e
--- /dev/null
+++ b/lib/librte_eal/kvargs/Makefile
@@ -0,0 +1,23 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2014 6WIND S.A.
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+# library name
+LIB = librte_kvargs.a
+
+CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
+
+EXPORT_MAP := rte_kvargs_version.map
+
+LIBABIVER := 1
+
+# all source are stored in SRCS-y
+SRCS-$(CONFIG_RTE_LIBRTE_KVARGS) := rte_kvargs.c
+
+# install includes
+INCS := rte_kvargs.h
+INCS += rte_string_fns.h
+SYMLINK-$(CONFIG_RTE_LIBRTE_KVARGS)-include := $(INCS)
+
+include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/l

[dpdk-dev] [PATCH 2/2] dev: use rte_kvargs

[Gaetan Rivet]

Signed-off-by: Gaetan Rivet 
---

Cc: Neil Horman 

I find using rte_parse_kv cleaner.
The function rte_dev_iterator_init is already ugly enough as it is.
This is really not helping.

 lib/librte_eal/common/eal_common_dev.c | 127 +
 lib/librte_eal/linuxapp/eal/Makefile   |   1 +
 2 files changed, 83 insertions(+), 45 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c 
b/lib/librte_eal/common/eal_common_dev.c
index 21703b777..9f1a0ebda 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "eal_private.h"
@@ -270,12 +271,15 @@ rte_eal_hotplug_remove(const char *busname, const char 
*devname)
 }
 
 int __rte_experimental
-rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str)
+rte_dev_iterator_init(struct rte_dev_iterator *it,
+ const char *devstr)
 {
-   struct rte_bus *bus = NULL;
+   struct rte_kvargs *kvlist = NULL;
struct rte_class *cls = NULL;
-   struct rte_kvarg kv;
-   char *slash;
+   struct rte_bus *bus = NULL;
+   struct rte_kvargs_pair *kv;
+   char *slash = NULL;
+   char *str = NULL;
 
/* Having both busstr and clsstr NULL is illegal,
 * marking this iterator as invalid unless
@@ -283,98 +287,131 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, const 
char *str)
 */
it->busstr = NULL;
it->clsstr = NULL;
+   str = strdup(devstr);
+   if (str == NULL) {
+   rte_errno = ENOMEM;
+   goto get_out;
+   }
+   slash = strchr(str, '/');
+   if (slash != NULL) {
+   slash[0] = '\0';
+   slash = strchr(devstr, '/') + 1;
+   }
/* Safety checks and prep-work */
-   if (rte_parse_kv(str, &kv)) {
+   kvlist = rte_kvargs_parse(str, NULL);
+   if (kvlist == NULL) {
RTE_LOG(ERR, EAL, "Could not parse: %s\n", str);
rte_errno = EINVAL;
-   return -rte_errno;
+   goto get_out;
}
it->device = NULL;
it->class_device = NULL;
-   if (strcmp(kv.key, "bus") == 0) {
-   bus = rte_bus_find_by_name(kv.value);
+   kv = &kvlist->pairs[0];
+   if (strcmp(kv->key, "bus") == 0) {
+   bus = rte_bus_find_by_name(kv->value);
if (bus == NULL) {
RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
-   kv.value);
+   kv->value);
rte_errno = EFAULT;
-   return -rte_errno;
+   goto get_out;
}
-   slash = strchr(str, '/');
if (slash != NULL) {
-   if (rte_parse_kv(slash + 1, &kv)) {
+   rte_kvargs_free(kvlist);
+   kvlist = rte_kvargs_parse(slash, NULL);
+   if (kvlist == NULL) {
RTE_LOG(ERR, EAL, "Could not parse: %s\n",
-   slash + 1);
+   slash);
rte_errno = EINVAL;
-   return -rte_errno;
+   goto get_out;
}
-   cls = rte_class_find_by_name(kv.value);
+   kv = &kvlist->pairs[0];
+   if (strcmp(kv->key, "class")) {
+   RTE_LOG(ERR, EAL, "Additional layer must be a 
class\n");
+   rte_errno = EINVAL;
+   goto get_out;
+   }
+   cls = rte_class_find_by_name(kv->value);
if (cls == NULL) {
RTE_LOG(ERR, EAL, "Could not find class 
\"%s\"\n",
-   kv.value);
+   kv->value);
rte_errno = EFAULT;
-   return -rte_errno;
+   goto get_out;
}
}
-   } else if (strcmp(kv.key, "class") == 0) {
-   cls = rte_class_find_by_name(kv.value);
+   } else if (strcmp(kv->key, "class") == 0) {
+   cls = rte_class_find_by_name(kv->value);
if (cls == NULL) {
RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
-   kv.value);
+   kv->value);
rte_errno = EFAULT;
-   return -rte_errno;
+   goto get_out;
}
} else {
rte_errno = EINVAL;
-   return -rte_errno;
+   goto get_out;
}
/* The string sho

Re: [dpdk-dev] [PATCH 01/13] net/bonding: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:15AM +, Andrew Rybchenko wrote:
> Fixes: a0ace286a60b ("net/bonding: build with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 02/13] net/i40e: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:16AM +, Andrew Rybchenko wrote:
> Fixes: e940646b20fa ("drivers/net: build Intel NIC PMDs with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 03/13] net/ixgbe: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:17AM +, Andrew Rybchenko wrote:
> Fixes: e940646b20fa ("drivers/net: build Intel NIC PMDs with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 04/13] net/null: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:18AM +, Andrew Rybchenko wrote:
> Fixes: efd5d1a8d8dd ("drivers/net: build some vdev PMDs with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 05/13] net/ring: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:19AM +, Andrew Rybchenko wrote:
> Fixes: efd5d1a8d8dd ("drivers/net: build some vdev PMDs with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 06/13] bitratestats: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:20AM +, Andrew Rybchenko wrote:
> Fixes: 5b9656b157d3 ("lib: build with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 07/13] cryptodev: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:21AM +, Andrew Rybchenko wrote:
> Fixes: 5b9656b157d3 ("lib: build with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 08/13] eventdev: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:22AM +, Andrew Rybchenko wrote:
> Fixes: 5b9656b157d3 ("lib: build with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 09/13] mempool: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:23AM +, Andrew Rybchenko wrote:
> Fixes: 5b9656b157d3 ("lib: build with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 10/13] pdump: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:24AM +, Andrew Rybchenko wrote:
> Fixes: 5b9656b157d3 ("lib: build with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 11/13] table: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:25AM +, Andrew Rybchenko wrote:
> Fixes: 5b9656b157d3 ("lib: build with meson")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 12/13] ethdev: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:26AM +, Andrew Rybchenko wrote:
> Fixes: 701ac75666b9 ("ethdev: remove versioning of ethdev filter control 
> function")
> 
> Signed-off-by: Andrew Rybchenko 
> ---
>  lib/librte_ether/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ether/meson.build b/lib/librte_ether/meson.build
> index 7fed860..12bdb6b 100644
> --- a/lib/librte_ether/meson.build
> +++ b/lib/librte_ether/meson.build
> @@ -2,7 +2,7 @@
>  # Copyright(c) 2017 Intel Corporation
>  
>  name = 'ethdev'
> -version = 8
> +version = 9

My copy of latest head in git shows this as still being 8. However, I see
it's down as 9 in next-net. Therefore I think we need to put this patch
into next-net, perhaps merging with the appropriate patch there which
updates the Makefile.

Ferruh, perhaps you could look at this one?

/Bruce

Re: [dpdk-dev] [PATCH 13/13] meter: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:27AM +, Andrew Rybchenko wrote:
> Fixes: c06ddf9698e0 ("meter: add configuration profile")
> 
> Signed-off-by: Andrew Rybchenko 
Acked-by: Bruce Richardson 

Re: [dpdk-dev] [PATCH 00/13] build: fix library version in meson build

[Bruce Richardson]

On Tue, Mar 20, 2018 at 11:26:14AM +, Andrew Rybchenko wrote:
> Library version is specified in two places: Makefile and meson.build.
> It is out-of-sync in a number of cases.
> 
> Andrew Rybchenko (13):
>   net/bonding: fix library version in meson build
>   net/i40e: fix library version in meson build
>   net/ixgbe: fix library version in meson build
>   net/null: fix library version in meson build
>   net/ring: fix library version in meson build
>   bitratestats: fix library version in meson build
>   cryptodev: fix library version in meson build
>   eventdev: fix library version in meson build
>   mempool: fix library version in meson build
>   pdump: fix library version in meson build
>   table: fix library version in meson build
>   ethdev: fix library version in meson build
>   meter: fix library version in meson build
> 

Hi Thomas,

these are all trivial fixes, so I think they should go into the main tree
directly rather than going via next-build. Can you apply them, please -
apart from patch 12, which should go to Ferruh's tree, I think?

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] compressdev: implement API

[Thomas Monjalon]

23/03/2018 19:08, Trahe, Fiona:
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > 02/02/2018 19:25, Fiona Trahe:
> > >  lib/librte_compressdev/rte_comp.h  | 503 
> > 
> > Why rte_comp.h instead of the more consistent rte_compress.h?
> [Fiona]  I did originally... but ran into difficulty with horribly names like
> RTE_COMPRESS_COMPRESS
> RTE_COMPRESS_DECOMPRESS
> rte_compress_compress_xform
> rte_compress_decompress_xform
> So compress is both the module prefix and the name of one of the actions.
> I could have used compressdev - but names were very long.
> So decided to opt for using 
> _compressdev_ in names to do with the device and
> _comp_ in names to do with the compression service
> 
> Also I could have used compdev instead of compressdev, 
> but I felt compress should be in the lib name

I understand your concerns.
I don't like "comp" very much because it sounds like "comparison".
However, I don't have a better idea.
Sometimes naming is more difficult than coding :)

Re: [dpdk-dev] [PATCH v5] ethdev: return named opaque type instead of void pointer

[Neil Horman]

On Fri, Mar 23, 2018 at 05:00:34PM +, Bruce Richardson wrote:
> On Wed, Mar 21, 2018 at 09:04:01AM -0400, Neil Horman wrote:
> > On Tue, Mar 20, 2018 at 04:34:04PM +, Ferruh Yigit wrote:
> > > "struct rte_eth_rxtx_callback" is defined as internal data structure and
> > > used as named opaque type.
> > > 
> > > So the functions that are adding callbacks can return objects in this
> > > type instead of void pointer.
> > > 
> > > Also const qualifier added to "struct rte_eth_rxtx_callback *" to
> > > protect it better from application modification.
> > > 
> > > Signed-off-by: Ferruh Yigit 
> > > ---
> > > v2:
> > > * keep using struct * in parameters, instead add callback functions
> > > return struct rte_eth_rxtx_callback pointer.
> > > 
> > > v4:
> > > * Remove deprecation notice. LIBABIVER already increased in this release
> > > 
> > > v5:
> > > * add const qualifier to rte_eth_rxtx_callback
> > I still wish we could find a way to remove the inline functions and truly
> > protect that struct, but a const is definately better than nothing
> > 
> > Acked-by: Neil Horman 
> > 
> Actually, I think we should do exactly that - convert the rx and tx burst
> calls into actual function calls (and consider any other inlined functions
> too). The cost would be the overhead of making an additional function call
> per-burst, which is likely to be pretty minimal for most common burst sizes
> e.g. 32.
> 
> We did some quick tests here with the i40e driver, and for a burst size of
> 32 saw less than 1% perf drop, and for even a small burst of 8 saw less
> than 5% drop. Note that this is testing with testpmd, which has nothing but
> I/O in the datapath. A real-world app is likely to do far more with the
> packets and therefore see proportionally far less perf hit.
> 
> Thoughts?
> 
> /Bruce
> 
> PS: This un-inlining could probably be applied to other device types too,
> e.g. cryptodev (though probably not eventdev as it tends to have smaller
> bursts in some use-cases).
> 
I would be 1000% on board with this conversion.

Best
Neil

Re: [dpdk-dev] [PATCH v2 13/41] eal: replace memseg with memseg lists

[santosh]

Hi Anatoly,


On Wednesday 07 March 2018 10:26 PM, Anatoly Burakov wrote:
> Before, we were aggregating multiple pages into one memseg, so the
> number of memsegs was small. Now, each page gets its own memseg,
> so the list of memsegs is huge. To accommodate the new memseg list
> size and to keep the under-the-hood workings sane, the memseg list
> is now not just a single list, but multiple lists. To be precise,
> each hugepage size available on the system gets one or more memseg
> lists, per socket.
>
> In order to support dynamic memory allocation, we reserve all
> memory in advance. As in, we do an anonymous mmap() of the entire
> maximum size of memory per hugepage size, per socket (which is
> limited to either RTE_MAX_MEMSEG_PER_TYPE pages or
> RTE_MAX_MEM_PER_TYPE gigabytes worth of memory, whichever is the
> smaller one), split over multiple lists (which are limited to
> either RTE_MAX_MEMSEG_PER_LIST memsegs or RTE_MAX_MEM_PER_LIST
> gigabytes per list, whichever is the smaller one).
>
> So, for each hugepage size, we get (by default) up to 128G worth
> of memory, per socket, split into chunks of up to 32G in size.
> The address space is claimed at the start, in eal_common_memory.c.
> The actual page allocation code is in eal_memalloc.c (Linux-only
> for now), and largely consists of copied EAL memory init code.
>
> Pages in the list are also indexed by address. That is, for
> non-legacy mode, in order to figure out where the page belongs,
> one can simply look at base address for a memseg list. Similarly,
> figuring out IOVA address of a memzone is a matter of finding the
> right memseg list, getting offset and dividing by page size to get
> the appropriate memseg. For legacy mode, old behavior of walking
> the memseg list remains.
>
> Due to switch to fbarray and to avoid any intrusive changes,
> secondary processes are not supported in this commit. Also, one
> particular API call (dump physmem layout) no longer makes sense
> and was removed, according to deprecation notice [1].
>
> In legacy mode, nothing is preallocated, and all memsegs are in
> a list like before, but each segment still resides in an appropriate
> memseg list.
>
> The rest of the changes are really ripple effects from the memseg
> change - heap changes, compile fixes, and rewrites to support
> fbarray-backed memseg lists.
>
> [1] http://dpdk.org/dev/patchwork/patch/34002/
>
> Signed-off-by: Anatoly Burakov 
> ---

Thanks for good work!.
Few observations:
# Noticed performance regression for thunderx platform for l3fwd application,
drops by 3%. git bisect shows this changeset is offending commit.
I'm still investigating reason for perf-dip..
Would like to know - have you noticed any regression on x86 platform?

# In next version, pl. make sure that individual patch builds successfully.
Right now, Some patches are dependent on other, leads to build break, observed 
while
git-bisecting.

Few examples are:
>>fa71cdef6963ed795fdd7e7f35085170bb300e39
>>1037fcd989176c5cc83db6223534205cac469765
>> befdec10759d30275a17a829919ee45228d91d3c  
>> 495e60f4e02af8a344c0f817a60d1ee9b9322df4 
[above commits are from your github repo..]

# Nits:
Perhaps you may club all below comits into one single patch,
as changes are identical... that way you'd reduce patch count by few less.
9a1e2a7bd9f6248c680ad3e444b6f173eb92d457 net/vmxnet3: use contiguous allocation 
for DMA memory
46388b194cd559b5cf7079e01b04bf67a99b64d7 net/virtio: use contiguous allocation 
for DMA memory
a3d2eb10bd998ba3ae3a3d39adeaff38d2e53a9d net/qede: use contiguous allocation 
for DMA memory
6f16b23ef1f472db475edf05159dea5ae741dbf8 net/i40e: use contiguous allocation 
for DMA memory
f9f7576eed35cb6aa50793810cdda43bcc0f4642 net/enic: use contiguous allocation 
for DMA memory
2af6c33009b8008da7028a351efed2932b1a13d0 net/ena: use contiguous allocation for 
DMA memory
18003e22bd7087e5e2e03543cb662d554f7bec52 net/cxgbe: use contiguous allocation 
for DMA memory
59f79182502dcb3634dfa3e7b918195829777460 net/bnx2x: use contiguous allocation 
for DMA memory
f481a321e41da82ddfa00f5ddbcb42fc29e6ae76 net/avf: use contiguous allocation for 
DMA memory
5253e9b757c1855a296656d939f5c28e651fea69 crypto/qat: use contiguous allocation 
for DMA memory
297ab037b4c0d9d725aa6cfdd2c33f7cd9396899 ethdev: use contiguous allocation for 
DMA memory

Thanks.

[dpdk-dev] [PATCH] net/sfc: fix type of opaque pointer in perf profile handler

[Andrew Rybchenko]

From: Roman Zhukov 

The 'opaque' pointer in handler function is the last argument
of sfc_kvargs_process() function and it is pointer to the adapter
'evq_flags' that has a uint32_t type. So 'value' must be pointer
to uint32_t.

Fixes: c22d3c508e0c ("net/sfc: support parameter to choose performance profile")
Cc: sta...@dpdk.org

Signed-off-by: Roman Zhukov 
Signed-off-by: Andrew Rybchenko 
---
 drivers/net/sfc/sfc_ev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
index 273a92c..8a5030b 100644
--- a/drivers/net/sfc/sfc_ev.c
+++ b/drivers/net/sfc/sfc_ev.c
@@ -821,7 +821,7 @@ static int
 sfc_kvarg_perf_profile_handler(__rte_unused const char *key,
   const char *value_str, void *opaque)
 {
-   uint64_t *value = opaque;
+   uint32_t *value = opaque;
 
if (strcasecmp(value_str, SFC_KVARG_PERF_PROFILE_THROUGHPUT) == 0)
*value = EFX_EVQ_FLAGS_TYPE_THROUGHPUT;
-- 
2.7.4