date:20170816

Re: [dpdk-dev] [PATCH] crypto/openssl: performance improvements

2017-08-16 Thread Akhil Goyal


Hi Pablo,
On 8/15/2017 12:56 PM, De Lara Guarch, Pablo wrote:

Hi,


-Original Message-
From: Akhil Goyal [mailto:akhil.go...@nxp.com]
Sent: Tuesday, August 15, 2017 7:45 AM
To: De Lara Guarch, Pablo ;
dev@dpdk.org; Doherty, Declan 
Cc: hemant.agra...@nxp.com
Subject: Re: [PATCH] crypto/openssl: performance improvements

On 8/14/2017 7:47 PM, De Lara Guarch, Pablo wrote:

Hi Akhil,


-Original Message-
From: Akhil Goyal [mailto:akhil.go...@nxp.com]
Sent: Friday, July 28, 2017 12:08 PM
To: dev@dpdk.org; Doherty, Declan 
Cc: De Lara Guarch, Pablo ;
hemant.agra...@nxp.com; Akhil Goyal 
Subject: [PATCH] crypto/openssl: performance improvements

key and algo are added in the openssl ctx during session
initialization instead of adding it for each packet.

Also in case of HMAC the openssl APIs HMAC_XXX give better
performance for all HMAC cases.

Signed-off-by: Akhil Goyal 


Thanks for the patch, nice optimization!
Could you split this into two patches, as you are doing two different

things here?

One for the first sentence and another one for the second sentence.
Also, as you do that, could you rename the title to be more explicit?
Like: crypto/openssl: initialize cipher key at session init

Finally, I was looking at GCM, and I think it could benefit from this.
I will send a separate patch for it, unless you want to integrate it in this

patchset yourself.




Ok I would split the patches.
For GCM I will try to incorporate in this patchset, if I get some performance
improvement, or I would send a different patch later if some issue comes.


Thanks Ahkil. Since I am working on AES-CCM for this PMD, I have the change
already done. I have seen performance improvements, but it is not as straight 
forward
as the cipher algorithms, because GMAC is also affected, which is in a 
different code path,
but requires GCM to be set.



If you have the change and it is working fine, then you can send your 
patch, no issues in that.


Thanks,
Akhil

Re: [dpdk-dev] [RFC PATCH 1/4] rte_security: API definitions

2017-08-16 Thread Akhil Goyal


On 8/15/2017 4:34 PM, Radu Nicolau wrote:


On 8/15/2017 7:35 AM, Akhil Goyal wrote:

Detailed description is added in the coverletter

Signed-off-by: Akhil Goyal 
---
  lib/librte_cryptodev/rte_security.c | 171 +++
  lib/librte_cryptodev/rte_security.h | 409 


  2 files changed, 580 insertions(+)
  create mode 100644 lib/librte_cryptodev/rte_security.c
  create mode 100644 lib/librte_cryptodev/rte_security.h




+int
+rte_security_session_init(uint16_t dev_id,
+  struct rte_security_session *sess,
+  struct rte_security_sess_conf *conf,
+  struct rte_mempool *mp)
+{
+struct rte_cryptodev *cdev = NULL;
+struct rte_eth_dev *dev = NULL;
+uint8_t index;
+int ret;
+
+if (sess == NULL || conf == NULL)
+return -EINVAL;
+
+switch (conf->action_type) {
+case RTE_SECURITY_SESS_CRYPTO_PROTO_OFFLOAD:
+if (!rte_cryptodev_pmd_is_valid_dev(dev_id))
+return -EINVAL;
+cdev = rte_cryptodev_pmd_get_dev(dev_id);
+index = cdev->driver_id;
+if (sess->sess_private_data[index] == NULL) {
+ret = cdev->sec_ops->session_configure(cdev, conf, sess, 
mp);

+if (ret < 0) {
+CDEV_LOG_ERR(
+"cdev_id %d failed to configure session details",
+dev_id);
+return ret;
+}
+}
+break;
+case RTE_SECURITY_SESS_ETH_INLINE_CRYPTO:
+case RTE_SECURITY_SESS_ETH_PROTO_OFFLOAD:
+dev = &rte_eth_devices[dev_id];
+index = dev->data->port_id;
+if (sess->sess_private_data[index] == NULL) {
+//ret = dev->sec_ops->session_configure(dev, conf, sess, 
mp);

+//if (ret < 0) {
+//CDEV_LOG_ERR(
+//"dev_id %d failed to configure session details",
+//dev_id);
+//return ret;
+//}
The commented lines above suggests that also eth devices will have a 
sec_ops field, (which makes sense). Is this correct?
Also, if the above is correct, session_configure and session_clear 
should accept both crypto and eth devices as first parameter.


Yes you are correct both these ops should accept void *dev and 
internally in the driver should typecast to respective device.

Please consider the following diff over this patch


diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h 
b/lib/librte_cryptodev/rte_cryptodev_pmd.h

index 219fba6..ab3ecf7 100644
--- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
+++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
@@ -371,7 +371,7 @@ struct rte_cryptodev_ops {
  *  - Returns -ENOTSUP if crypto device does not support the crypto 
transform.

  *  - Returns -ENOMEM if the private session could not be allocated.
  */
-typedef int (*security_configure_session_t)(struct rte_cryptodev *dev,
+typedef int (*security_configure_session_t)(void *dev,
struct rte_security_sess_conf *conf,
struct rte_security_session *sess,
struct rte_mempool *mp);
@@ -382,7 +382,7 @@ typedef int (*security_configure_session_t)(struct 
rte_cryptodev *dev,

  * @param  dev Crypto device pointer
  * @param  sessSecurity session structure
  */
-typedef void (*security_free_session_t)(struct rte_cryptodev *dev,
+typedef void (*security_free_session_t)(void *dev,
struct rte_security_session *sess);

 /** Security operations function pointer table */
diff --git a/lib/librte_cryptodev/rte_security.c 
b/lib/librte_cryptodev/rte_security.c

index 7c73c93..a7558bb 100644
--- a/lib/librte_cryptodev/rte_security.c
+++ b/lib/librte_cryptodev/rte_security.c
@@ -87,7 +87,8 @@ rte_security_session_init(uint16_t dev_id,
cdev = rte_cryptodev_pmd_get_dev(dev_id);
index = cdev->driver_id;
if (sess->sess_private_data[index] == NULL) {
-   ret = cdev->sec_ops->session_configure(cdev, 
conf, sess, mp);

+   ret = cdev->sec_ops->session_configure((void *)cdev,
+   conf, sess, mp);
if (ret < 0) {
CDEV_LOG_ERR(
"cdev_id %d failed to configure 
session details",

@@ -101,7 +102,8 @@ rte_security_session_init(uint16_t dev_id,
dev = &rte_eth_devices[dev_id];
index = dev->data->port_id;
if (sess->sess_private_data[index] == NULL) {
-// ret = dev->sec_ops->session_configure(dev, conf, 
sess, mp);

+// ret = dev->sec_ops->session_configure((void *)dev,
+// conf, sess, mp);

Thanks,
Akhil

Re: [dpdk-dev] [PATCH] net/failsafe: fix tx sub device deactivating

2017-08-16 Thread Gaëtan Rivet

Hi Matan,

Thanks for spotting this, a few nits below.

On Tue, Aug 15, 2017 at 09:59:19AM +0300, Matan Azrad wrote:
> The corrupted code couldn't recognize that all sub devices
> were not ready for tx traffic when failsafe PMD was trying
> to switch device because of an unreachable condition using.
> 
> Hence, the current tx sub device variable was not updated
> correctly.
> 
> The fix removed the unreachable condition and adds condition
> in the right place to handle non tx device ready scenario.
> 

It should be reworded as

  Make the condition reachable by moving it in the right place to
  handle the scenario when no TX device is ready.

If the condition is removed and then added, I find it clearer to say
that it was moved.

> Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> 

The root commit introducing the issue is the first one, but
this fix only applies to the second.
So I don't know which commit is actually fixed by this, but I find
peculiar to have two commits targeted by a fix.

In doubt, probably leave both, but maybe someone has a better idea about
it?

> Signed-off-by: Matan Azrad 
> Cc: sta...@dpdk.org

The Cc: stable line should immediately follow the Fixes: line.

> ---
>  drivers/net/failsafe/failsafe_private.h | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Hi Gaetan
> I didn't find any real scenario which cause to problematic
> behavior because of the previous code.
> But it may be.
> 
> diff --git a/drivers/net/failsafe/failsafe_private.h 
> b/drivers/net/failsafe/failsafe_private.h
> index 0361cf4..dc97aec 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
>   PRIV(dev)->subs_tx = i;
>   break;
>   }
> - } else if (txd && txd->state < req_state) {
> - DEBUG("No device ready, deactivating tx_dev");
> - PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> + if (i >= PRIV(dev)->subs_tail || !sdev) {

`!sdev` should be `sdev == NULL`, see [1].

> + DEBUG("No device ready, deactivating tx_dev");
> + PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> + }
>   } else {
>   return;
>   }
> -- 
> 2.7.4
> 

With these changes,

Acked-by: Gaetan Rivet 

[1]:
http://dpdk.org/doc/guides/contributing/coding_style.html#c-statement-style-and-conventions
-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] net/failsafe: fix tx sub device deactivating

2017-08-16 Thread Matan Azrad

Hi Gaetan

> -Original Message-
> From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com]
> Sent: Wednesday, August 16, 2017 11:47 AM
> To: Matan Azrad 
> Cc: dev@dpdk.org; sta...@dpdk.org
> Subject: Re: [PATCH] net/failsafe: fix tx sub device deactivating
> 
> Hi Matan,
> 
> Thanks for spotting this, a few nits below.
> 
> On Tue, Aug 15, 2017 at 09:59:19AM +0300, Matan Azrad wrote:
> > The corrupted code couldn't recognize that all sub devices were not
> > ready for tx traffic when failsafe PMD was trying to switch device
> > because of an unreachable condition using.
> >
> > Hence, the current tx sub device variable was not updated correctly.
> >
> > The fix removed the unreachable condition and adds condition in the
> > right place to handle non tx device ready scenario.
> >
> 
> It should be reworded as
> 
>   Make the condition reachable by moving it in the right place to
>   handle the scenario when no TX device is ready.
> 
> If the condition is removed and then added, I find it clearer to say that it 
> was
> moved.

But the two conditions are different,
The old condition can't handle the scenario we want.

> 
> > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> >
> 
> The root commit introducing the issue is the first one, but this fix only 
> applies
> to the second.
> So I don't know which commit is actually fixed by this, but I find peculiar to
> have two commits targeted by a fix.
> 
> In doubt, probably leave both, but maybe someone has a better idea about
> it?

I also thought about it, and found the two are necessary for future review. 
 
> 
> > Signed-off-by: Matan Azrad 
> > Cc: sta...@dpdk.org
> 
> The Cc: stable line should immediately follow the Fixes: line.
> 

Will be fixed.

> > ---
> >  drivers/net/failsafe/failsafe_private.h | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > Hi Gaetan
> > I didn't find any real scenario which cause to problematic behavior
> > because of the previous code.
> > But it may be.
> >
> > diff --git a/drivers/net/failsafe/failsafe_private.h
> > b/drivers/net/failsafe/failsafe_private.h
> > index 0361cf4..dc97aec 100644
> > --- a/drivers/net/failsafe/failsafe_private.h
> > +++ b/drivers/net/failsafe/failsafe_private.h
> > @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
> > PRIV(dev)->subs_tx = i;
> > break;
> > }
> > -   } else if (txd && txd->state < req_state) {
> > -   DEBUG("No device ready, deactivating tx_dev");
> > -   PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > +   if (i >= PRIV(dev)->subs_tail || !sdev) {
> 
> `!sdev` should be `sdev == NULL`, see [1].
OK, will be fixed.

> 
> > +   DEBUG("No device ready, deactivating tx_dev");
> > +   PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > +   }
> > } else {
> > return;
> > }
> > --
> > 2.7.4
> >
> 
> With these changes,
> 
> Acked-by: Gaetan Rivet 
> 
> [1]:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> k.org%2Fdoc%2Fguides%2Fcontributing%2Fcoding_style.html%23c-
> statement-style-and-
> conventions&data=02%7C01%7Cmatan%40mellanox.com%7C6283c71dcc2b4
> ebe5f6608d4e48350cd%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%
> 7C636384700025293702&sdata=nNMTElzhe3RlEMc3vB67QlwAYYYQ%2ByNNp
> 9ebXgSsMM8%3D&reserved=0
> --
> Gaëtan Rivet
> 6WIND

Thanks 
Matan Azrad

Re: [dpdk-dev] [PATCH] doc: add i40e firmware upgrade guide

2017-08-16 Thread Yang, Qiming

> -Original Message-
> From: Yigit, Ferruh
> Sent: Tuesday, August 15, 2017 4:52 PM
> To: Yang, Qiming ; dev@dpdk.org
> Cc: Wu, Jingjing ; Xing, Beilei 
> Subject: Re: [dpdk-dev] [PATCH] doc: add i40e firmware upgrade guide
> 
> On 8/15/2017 4:26 AM, Qiming Yang wrote:
> > This patch adds one link to DPDK i40e doc, which is for users on how
> > to upgrade firmware.
> 
> Does it make sense to add a link to download the FW too?
> 
This one is good enough, it has detail about updating process, and in chapter 
3.0 includes the link to get the latest NVM.
So it's no need to add another link to download the FW.
> >
> > Signed-off-by: Qiming Yang 
> > ---
> >  doc/guides/nics/i40e.rst | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst index
> > bc200d3..606375d 100644
> > --- a/doc/guides/nics/i40e.rst
> > +++ b/doc/guides/nics/i40e.rst
> > @@ -78,6 +78,8 @@ Prerequisites
> >  - To get better performance on Intel platforms, please follow the "How to 
> > get
> best performance with NICs on Intel platforms"
> >section of the :ref:`Getting Started Guide for Linux `.
> >
> > +- Upgrade the NVM/FW version follow the `Intel® Ethernet NVM Update
> > +Tool Quick Usage Guide for Linux
> > +   ssl.intel.com/content/www/us/en/embedded/products/networking/nvm-
> update-tool-quick-linux-usage-guide.html>`_ if need.
> >
> >  Pre-Installation Configuration
> >  --
> >

[dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically

2017-08-16 Thread Pablo de Lara

When register a crypto driver, a cryptodev driver
structure was being allocated, using malloc.
Since this call may fail, it is safer to allocate
this memory statically in each PMD, so driver registration
will never fail.

Coverity issue: 158645

Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")

Signed-off-by: Pablo de Lara 
---

Changes in v3:

- Added documentation (removed deprecation notice and added API Change)
- Modified title to something more explicit

Changes in v2:

- Allocate statically the cryptodev driver structure,
  instead of using malloc, that can potentially fail.

 doc/guides/rel_notes/deprecation.rst|  6 --
 doc/guides/rel_notes/release_17_11.rst  |  5 +
 drivers/crypto/aesni_gcm/aesni_gcm_pmd.c|  5 -
 drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +-
 drivers/crypto/armv8/rte_armv8_pmd.c|  9 ++---
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 -
 drivers/crypto/kasumi/rte_kasumi_pmd.c  |  5 -
 drivers/crypto/null/null_crypto_pmd.c   |  5 -
 drivers/crypto/openssl/rte_openssl_pmd.c|  5 -
 drivers/crypto/qat/rte_qat_cryptodev.c  |  7 +--
 drivers/crypto/scheduler/scheduler_pmd.c|  5 -
 drivers/crypto/snow3g/rte_snow3g_pmd.c  |  5 -
 drivers/crypto/zuc/rte_zuc_pmd.c|  5 -
 lib/librte_cryptodev/rte_cryptodev.c| 18 +
 lib/librte_cryptodev/rte_cryptodev.h| 20 ---
 lib/librte_cryptodev/rte_cryptodev_pmd.h| 30 +
 16 files changed, 88 insertions(+), 53 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst 
b/doc/guides/rel_notes/deprecation.rst
index 3362f33..8be626e 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -106,12 +106,6 @@ Deprecation Notices
 
   - ``rte_cryptodev_vdev_pmd_init``
 
-* cryptodev: the following function will have an extra parameter, passing a
-  statically allocated crypto driver structure, instead of calling malloc,
-  in 17.11:
-
-  - ``rte_cryptodev_allocate_driver``
-
 * librte_meter: The API will change to accommodate configuration profiles.
   Most of the API functions will have an additional opaque parameter.
 
diff --git a/doc/guides/rel_notes/release_17_11.rst 
b/doc/guides/rel_notes/release_17_11.rst
index 170f4f9..cdb99f4 100644
--- a/doc/guides/rel_notes/release_17_11.rst
+++ b/doc/guides/rel_notes/release_17_11.rst
@@ -110,6 +110,11 @@ API Changes
Also, make sure to start the actual text at the margin.
=
 
+* **Modified the rte_cryptodev_allocate_driver function in the cryptodev 
library.**
+
+  The function ``rte_cryptodev_allocate_driver()`` has been modified.
+  An extra parameter ``struct cryptodev_driver *crypto_drv`` has been added.
+
 
 ABI Changes
 ---
diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c 
b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
index d9c91d0..b59f399 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
@@ -630,10 +630,13 @@ static struct rte_vdev_driver aesni_gcm_pmd_drv = {
.remove = aesni_gcm_remove
 };
 
+static struct cryptodev_driver aesni_gcm_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_AESNI_GCM_PMD, aesni_gcm_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_AESNI_GCM_PMD, cryptodev_aesni_gcm_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_AESNI_GCM_PMD,
"max_nb_queue_pairs= "
"max_nb_sessions= "
"socket_id=");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_gcm_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_gcm_crypto_drv, aesni_gcm_pmd_drv,
+   cryptodev_driver_id);
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c 
b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
index 16e1451..5846bc9 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
@@ -823,10 +823,14 @@ static struct rte_vdev_driver cryptodev_aesni_mb_pmd_drv 
= {
.remove = cryptodev_aesni_mb_remove
 };
 
+static struct cryptodev_driver aesni_mb_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_AESNI_MB_PMD, cryptodev_aesni_mb_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_AESNI_MB_PMD, cryptodev_aesni_mb_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_AESNI_MB_PMD,
"max_nb_queue_pairs= "
"max_nb_sessions= "
"socket_id=");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_aesni_mb_pmd_drv, 
cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_mb_crypto_drv,
+   cryptodev_aesni_mb_pmd_drv,
+   cryptodev_driver_id);
diff --git a/drivers/crypto/armv8/rte_armv8_pmd.c 
b/drivers/crypto/armv8/rte_armv8_pmd.c
index a5c39c9..b0d68d1 100644
--- a/drivers/crypto/armv8/rte_armv8_pmd.c
+++ b/drivers/crypto/armv8/rte_armv8_pmd.c
@@ -882,15 +882,18 @@ cryptodev_armv8_crypto_uninit(struct rte_vdev_device 
*

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

2017-08-16 Thread Neil Horman

On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> This patchset reworks the service apis to be more user
> friendly. In particular, the various rte_service_* functions
> now take an integer id parameter instead of a service pointer.
> This both reduces the API surface (no service_get_from_id()),
> and allows easier debugging (gdb function calls with integer args),
> and various other benefits (better encapsulation, less pointers :)
> 
> Finally, some APIs are changed or renamed for consistency and
> clarity of what they do. See commit messages for details.
> Note that the service library is merged as EXPERIMENTAL in
> the 17.08 release, allowing API improvements for 17.11 release.
> 
> I hope to merge this patchset early in the 17.11 timeframe,
> so please review ASAP to allow time for other DPDK components
> to utilize services in this release :)
> 
> Feedback and input welcome, -Harry
> 
You need to add a deprecation note in the rel notes area so that people are
aware of the upcomming ABI changes
Neil

> ---
> 
> There is one checkpatch warning: "macro with flow control", however
> this same type of macro is used extensively in Ethdev and others,
> I presume it is a false-positive.
> 
> Harry van Haaren (8):
>   service: rework probe and get name to use ids
>   service: rework lcore to service map functions
>   service: rework register to return service id
>   service: rework service start stop to runstate
>   service: rework service stats functions
>   service: rework unregister api to use integers
>   service: rework get by name function to use id
>   service: clarify documentation for register
> 
>  drivers/event/sw/sw_evdev.c|   7 +-
>  drivers/event/sw/sw_evdev.h|   1 +
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map  |  11 +-
>  lib/librte_eal/common/include/rte_service.h| 144 +++---
>  .../common/include/rte_service_component.h |  13 +-
>  lib/librte_eal/common/rte_service.c| 167 
> +
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map|  11 +-
>  test/test/test_service_cores.c | 123 +++
>  8 files changed, 215 insertions(+), 262 deletions(-)
> 
> -- 
> 2.7.4
> 
>

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

2017-08-16 Thread Van Haaren, Harry

> From: Neil Horman [mailto:nhor...@tuxdriver.com]
> Sent: Wednesday, August 16, 2017 12:16 PM
> To: Van Haaren, Harry 
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> 
> On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> > This patchset reworks the service apis to be more user
> > friendly. In particular, the various rte_service_* functions
> > now take an integer id parameter instead of a service pointer.
> > This both reduces the API surface (no service_get_from_id()),
> > and allows easier debugging (gdb function calls with integer args),
> > and various other benefits (better encapsulation, less pointers :)
> >
> > Finally, some APIs are changed or renamed for consistency and
> > clarity of what they do. See commit messages for details.
> > Note that the service library is merged as EXPERIMENTAL in
> > the 17.08 release, allowing API improvements for 17.11 release.
> >
> > I hope to merge this patchset early in the 17.11 timeframe,
> > so please review ASAP to allow time for other DPDK components
> > to utilize services in this release :)
> >
> > Feedback and input welcome, -Harry
> >
> You need to add a deprecation note in the rel notes area so that people are
> aware of the upcomming ABI changes

Service cores was merged into 17.08 with the EXPERIMENTAL tag, which indicates 
that the API and ABI are not stable. The version map file has the service cores 
functions added in the Experimental staging area, instead of in the 17.08 
stable ABI[1]. To make this very visible to the users, the documentation has 
large "Warning: Experimental, this API may change without prior notice" 
marks[2], and the MAINTAINERS file[3] has the Experimental tag.

As far as I am aware, those are all the requirements to be able to remove / 
change / update / fix APIs. It was discussed on #IRC that it would be better to 
merge service-cores as experimental to allow faster iteration, and to get 
improvements out the door, and I'm still of that opinion.

Given the above, I don't see any issue with merging service-core changes into 
the 17.11 release.

[1] 
http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/rte_eal_version.map#n212
[2] 
http://dpdk.org/doc/api/rte__service_8h.html#aea7fce2a101bf2c00194dffb30bfc4ea
[3] http://dpdk.org/browse/dpdk/tree/MAINTAINERS#n138

[dpdk-dev] [PATCH] ethdev: fix device state on close

2017-08-16 Thread Shahaf Shuler

Currently device state moves between ATTACHED when device was
successfully probed to UNUSED when device is detached or released.

The device state following rte_eth_dev_close() operation is inconsist,
The device is still in ATTACHED state, however it cannot be used
in any way till it will be probed again.

Fixing it by changing the state to UNUSED.

Fixes: d52268a8b24b ("ethdev: expose device states")
Cc: gaetan.ri...@6wind.com
Cc: sta...@dpdk.org

Signed-off-by: Shahaf Shuler 
---
 lib/librte_ether/rte_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0597641ee..98d9e929c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id)
dev->data->nb_tx_queues = 0;
rte_free(dev->data->tx_queues);
dev->data->tx_queues = NULL;
+
+   dev->state = RTE_ETH_DEV_UNUSED;
 }
 
 int
-- 
2.12.0

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

2017-08-16 Thread Van Haaren, Harry

> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Van Haaren, Harry
> Sent: Wednesday, August 16, 2017 12:32 PM
> To: Neil Horman 
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> 
> > From: Neil Horman [mailto:nhor...@tuxdriver.com]
> > Sent: Wednesday, August 16, 2017 12:16 PM
> > To: Van Haaren, Harry 
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> >
> > On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> > > This patchset reworks the service apis to be more user
> > > friendly. In particular, the various rte_service_* functions
> > > now take an integer id parameter instead of a service pointer.
> > > This both reduces the API surface (no service_get_from_id()),
> > > and allows easier debugging (gdb function calls with integer args),
> > > and various other benefits (better encapsulation, less pointers :)
> > >
> > > Finally, some APIs are changed or renamed for consistency and
> > > clarity of what they do. See commit messages for details.
> > > Note that the service library is merged as EXPERIMENTAL in
> > > the 17.08 release, allowing API improvements for 17.11 release.
> > >
> > > I hope to merge this patchset early in the 17.11 timeframe,
> > > so please review ASAP to allow time for other DPDK components
> > > to utilize services in this release :)
> > >
> > > Feedback and input welcome, -Harry
> > >
> > You need to add a deprecation note in the rel notes area so that people are
> > aware of the upcomming ABI changes
> 
> Service cores was merged into 17.08 with the EXPERIMENTAL tag, which 
> indicates that the
> API and ABI are not stable. The version map file has the service cores 
> functions added in
> the Experimental staging area, instead of in the 17.08 stable ABI[1]. To make 
> this very
> visible to the users, the documentation has large "Warning: Experimental, 
> this API may
> change without prior notice" marks[2], and the MAINTAINERS file[3] has the 
> Experimental
> tag.
> 
> As far as I am aware, those are all the requirements to be able to remove / 
> change /
> update / fix APIs. It was discussed on #IRC that it would be better to merge 
> service-cores
> as experimental to allow faster iteration, and to get improvements out the 
> door, and I'm
> still of that opinion.
> 
> Given the above, I don't see any issue with merging service-core changes into 
> the 17.11
> release.
> 
> [1] 
> http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/rte_eal_version.map#n212
> [2] 
> http://dpdk.org/doc/api/rte__service_8h.html#aea7fce2a101bf2c00194dffb30bfc4ea
> [3] http://dpdk.org/browse/dpdk/tree/MAINTAINERS#n138

Self-reply;

On re-reading, perhaps I understood you wrong; did you mean that I need to add 
a section to the Release notes that the service core APIs have been updated in 
17.11 itself?

That's a very valid point - and I'll fix that in v2, (some other typo fixes to 
fix too).

-Harry

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

2017-08-16 Thread Gaëtan Rivet

Hello Shahaf,

On Wed, Aug 16, 2017 at 02:43:08PM +0300, Shahaf Shuler wrote:
> Currently device state moves between ATTACHED when device was
> successfully probed to UNUSED when device is detached or released.
> 
> The device state following rte_eth_dev_close() operation is inconsist,
> The device is still in ATTACHED state, however it cannot be used
> in any way till it will be probed again.
> 
> Fixing it by changing the state to UNUSED.
> 

You are right that simply closing the device leaves it in a unusable
state.

However it seems to be by design.
Most drivers call `rte_eth_dev_release_port` when being removed, which
sets the state to RTE_ETH_DEV_UNUSED.

If I'm not mistaken, the API of rte_eth_dev_close is that the only
available action should then be to detach the driver. At least PCI and
vdev buses expects a `remove` callback from their driver, which can be
called by the user (previously using specific API like
`rte_eal_vdev_uninit` for example, now using `rte_eal_hotplug_remove` or
`rte_eth_dev_detach` from the ether layer).

So, it seems that this burden lies with the driver which should call the
proper API when removing their device.

Maybe Thomas will have a better insight about the scope of the
`rte_eth_dev_close` function. But IMO the API is respected.
After all, until the proper `dev_detach` function is called, the device
is still attached, even if closed.

If you disagree, there might possibly be an argument to make about
either adding finer-grained device states or streamlining the API. This
is however a discussion about API design and not about its implementation
anymore.

> Fixes: d52268a8b24b ("ethdev: expose device states")
> Cc: gaetan.ri...@6wind.com
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Shahaf Shuler 
> ---
>  lib/librte_ether/rte_ethdev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0597641ee..98d9e929c 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id)
>   dev->data->nb_tx_queues = 0;
>   rte_free(dev->data->tx_queues);
>   dev->data->tx_queues = NULL;
> +
> + dev->state = RTE_ETH_DEV_UNUSED;
>  }
>  
>  int
> -- 
> 2.12.0
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] net/failsafe: fix tx sub device deactivating

2017-08-16 Thread Gaëtan Rivet

On Wed, Aug 16, 2017 at 09:02:31AM +, Matan Azrad wrote:
> Hi Gaetan
> 
> > -Original Message-
> > From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com]
> > Sent: Wednesday, August 16, 2017 11:47 AM
> > To: Matan Azrad 
> > Cc: dev@dpdk.org; sta...@dpdk.org
> > Subject: Re: [PATCH] net/failsafe: fix tx sub device deactivating
> > 
> > Hi Matan,
> > 
> > Thanks for spotting this, a few nits below.
> > 
> > On Tue, Aug 15, 2017 at 09:59:19AM +0300, Matan Azrad wrote:
> > > The corrupted code couldn't recognize that all sub devices were not
> > > ready for tx traffic when failsafe PMD was trying to switch device
> > > because of an unreachable condition using.
> > >
> > > Hence, the current tx sub device variable was not updated correctly.
> > >
> > > The fix removed the unreachable condition and adds condition in the
> > > right place to handle non tx device ready scenario.
> > >
> > 
> > It should be reworded as
> > 
> >   Make the condition reachable by moving it in the right place to
> >   handle the scenario when no TX device is ready.
> > 
> > If the condition is removed and then added, I find it clearer to say that 
> > it was
> > moved.
> 
> But the two conditions are different,
> The old condition can't handle the scenario we want.
>   

Yes you're right, but the commit log should still be written in the
present tense:

  Remove the unreachable branch and add one in the right place respecting
  the original intent.

Or something like it :)

> > 
> > > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> > > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> > >
> > 
> > The root commit introducing the issue is the first one, but this fix only 
> > applies
> > to the second.
> > So I don't know which commit is actually fixed by this, but I find peculiar 
> > to
> > have two commits targeted by a fix.
> > 
> > In doubt, probably leave both, but maybe someone has a better idea about
> > it?
> 
> I also thought about it, and found the two are necessary for future review. 
>  
> > 
> > > Signed-off-by: Matan Azrad 
> > > Cc: sta...@dpdk.org
> > 
> > The Cc: stable line should immediately follow the Fixes: line.
> > 
> 
> Will be fixed.
> 
> > > ---
> > >  drivers/net/failsafe/failsafe_private.h | 7 ---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > Hi Gaetan
> > > I didn't find any real scenario which cause to problematic behavior
> > > because of the previous code.
> > > But it may be.
> > >
> > > diff --git a/drivers/net/failsafe/failsafe_private.h
> > > b/drivers/net/failsafe/failsafe_private.h
> > > index 0361cf4..dc97aec 100644
> > > --- a/drivers/net/failsafe/failsafe_private.h
> > > +++ b/drivers/net/failsafe/failsafe_private.h
> > > @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
> > >   PRIV(dev)->subs_tx = i;
> > >   break;
> > >   }
> > > - } else if (txd && txd->state < req_state) {
> > > - DEBUG("No device ready, deactivating tx_dev");
> > > - PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > > + if (i >= PRIV(dev)->subs_tail || !sdev) {
> > 
> > `!sdev` should be `sdev == NULL`, see [1].
> OK, will be fixed.
> 
> > 
> > > + DEBUG("No device ready, deactivating tx_dev");
> > > + PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > > + }
> > >   } else {
> > >   return;
> > >   }
> > > --
> > > 2.7.4
> > >
> > 
> > With these changes,
> > 
> > Acked-by: Gaetan Rivet 
> > 
> > [1]:
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> > k.org%2Fdoc%2Fguides%2Fcontributing%2Fcoding_style.html%23c-
> > statement-style-and-
> > conventions&data=02%7C01%7Cmatan%40mellanox.com%7C6283c71dcc2b4
> > ebe5f6608d4e48350cd%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%
> > 7C636384700025293702&sdata=nNMTElzhe3RlEMc3vB67QlwAYYYQ%2ByNNp
> > 9ebXgSsMM8%3D&reserved=0
> > --
> > Gaëtan Rivet
> > 6WIND
> 
> Thanks 
> Matan Azrad

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

2017-08-16 Thread Neil Horman

On Wed, Aug 16, 2017 at 12:07:11PM +, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Van Haaren, Harry
> > Sent: Wednesday, August 16, 2017 12:32 PM
> > To: Neil Horman 
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> > 
> > > From: Neil Horman [mailto:nhor...@tuxdriver.com]
> > > Sent: Wednesday, August 16, 2017 12:16 PM
> > > To: Van Haaren, Harry 
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> > >
> > > On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> > > > This patchset reworks the service apis to be more user
> > > > friendly. In particular, the various rte_service_* functions
> > > > now take an integer id parameter instead of a service pointer.
> > > > This both reduces the API surface (no service_get_from_id()),
> > > > and allows easier debugging (gdb function calls with integer args),
> > > > and various other benefits (better encapsulation, less pointers :)
> > > >
> > > > Finally, some APIs are changed or renamed for consistency and
> > > > clarity of what they do. See commit messages for details.
> > > > Note that the service library is merged as EXPERIMENTAL in
> > > > the 17.08 release, allowing API improvements for 17.11 release.
> > > >
> > > > I hope to merge this patchset early in the 17.11 timeframe,
> > > > so please review ASAP to allow time for other DPDK components
> > > > to utilize services in this release :)
> > > >
> > > > Feedback and input welcome, -Harry
> > > >
> > > You need to add a deprecation note in the rel notes area so that people 
> > > are
> > > aware of the upcomming ABI changes
> > 
> > Service cores was merged into 17.08 with the EXPERIMENTAL tag, which 
> > indicates that the
> > API and ABI are not stable. The version map file has the service cores 
> > functions added in
> > the Experimental staging area, instead of in the 17.08 stable ABI[1]. To 
> > make this very
> > visible to the users, the documentation has large "Warning: Experimental, 
> > this API may
> > change without prior notice" marks[2], and the MAINTAINERS file[3] has the 
> > Experimental
> > tag.
> > 
> > As far as I am aware, those are all the requirements to be able to remove / 
> > change /
> > update / fix APIs. It was discussed on #IRC that it would be better to 
> > merge service-cores
> > as experimental to allow faster iteration, and to get improvements out the 
> > door, and I'm
> > still of that opinion.
> > 
> > Given the above, I don't see any issue with merging service-core changes 
> > into the 17.11
> > release.
> > 
> > [1] 
> > http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/rte_eal_version.map#n212
> > [2] 
> > http://dpdk.org/doc/api/rte__service_8h.html#aea7fce2a101bf2c00194dffb30bfc4ea
> > [3] http://dpdk.org/browse/dpdk/tree/MAINTAINERS#n138
> 
> Self-reply;
> 
> On re-reading, perhaps I understood you wrong; did you mean that I need to 
> add a section to the Release notes that the service core APIs have been 
> updated in 17.11 itself?
> 
> That's a very valid point - and I'll fix that in v2, (some other typo fixes 
> to fix too).
> 
I hadn't noted the experimental tag, so you likely don't need a deprecation
warning, but yes, a release note to indicate the change to the API in 17.11
would be good.

Best
NEil

> -Harry
>

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

2017-08-16 Thread Shahaf Shuler

Wednesday, August 16, 2017 3:42 PM, Gaëtan Rivet:
> On Wed, Aug 16, 2017 at 02:43:08PM +0300, Shahaf Shuler wrote:
> > Currently device state moves between ATTACHED when device was
> > successfully probed to UNUSED when device is detached or released.
> >
> > The device state following rte_eth_dev_close() operation is inconsist,
> > The device is still in ATTACHED state, however it cannot be used in
> > any way till it will be probed again.
> >
> > Fixing it by changing the state to UNUSED.
> >
> 
> You are right that simply closing the device leaves it in a unusable state.
> 
> However it seems to be by design.
> Most drivers call `rte_eth_dev_release_port` when being removed, which
> sets the state to RTE_ETH_DEV_UNUSED.
> 
> If I'm not mistaken, the API of rte_eth_dev_close is that the only available
> action should then be to detach the driver. At least PCI and vdev buses
> expects a `remove` callback from their driver, which can be called by the user
> (previously using specific API like `rte_eal_vdev_uninit` for example, now
> using `rte_eal_hotplug_remove` or `rte_eth_dev_detach` from the ether
> layer).
> 
> So, it seems that this burden lies with the driver which should call the 
> proper
> API when removing their device.

Even though it is reasonable for driver to call the rte_eth_dev_port_release, I 
still think the ethdev layer should protect from such bad behavior from the 
application side.
It is more robust than counting on the different PMD to implement such release. 

> 
> Maybe Thomas will have a better insight about the scope of the
> `rte_eth_dev_close` function. But IMO the API is respected.
> After all, until the proper `dev_detach` function is called, the device is 
> still
> attached, even if closed.
> 
> If you disagree, there might possibly be an argument to make about either
> adding finer-grained device states or streamlining the API. This is however a
> discussion about API design and not about its implementation anymore.

Well my first thought when I created this patch was to add fine-grained device 
states. However then I read the commit log which changed the device states, 
specifically :

" "DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that
the emptiness of a slot is not necessarily the result of detaching a
device."

Which convince me to reuse the UNUSED state to reflect that this device cannot 
longer be used (even though it is still attached). 

> 
> > Fixes: d52268a8b24b ("ethdev: expose device states")
> > Cc: gaetan.ri...@6wind.com
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Shahaf Shuler 
> > ---
> >  lib/librte_ether/rte_ethdev.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 0597641ee..98d9e929c 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id)
> > dev->data->nb_tx_queues = 0;
> > rte_free(dev->data->tx_queues);
> > dev->data->tx_queues = NULL;
> > +
> > +   dev->state = RTE_ETH_DEV_UNUSED;
> >  }
> >
> >  int
> > --
> > 2.12.0
> >
> 
> --
> Gaëtan Rivet
> 6WIND

[dpdk-dev] [PATCH v2] net/failsafe: fix tx sub device deactivating

2017-08-16 Thread Matan Azrad

The corrupted code couldn't recognize that all sub devices
were not ready for tx traffic when failsafe PMD was trying
to switch device because of an unreachable condition using.

Hence, the current tx sub device variable was not updated
correctly.

The fix removed the unreachable branch and added new one
in the right place respecting the original intent.

Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
Cc: sta...@dpdk.org

Signed-off-by: Matan Azrad 
Acked-by: Gaetan Rivet 
---
 drivers/net/failsafe/failsafe_private.h | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_private.h 
b/drivers/net/failsafe/failsafe_private.h
index 0361cf4..ef646db 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,
} else if ((txd && txd->state < req_state) ||
   txd == NULL ||
   txd == banned) {
-   struct sub_device *sdev;
+   struct sub_device *sdev = NULL;
uint8_t i;
 
/* Using acceptable device */
@@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
PRIV(dev)->subs_tx = i;
break;
}
-   } else if (txd && txd->state < req_state) {
-   DEBUG("No device ready, deactivating tx_dev");
-   PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+   if (i >= PRIV(dev)->subs_tail || sdev == NULL) {
+   DEBUG("No device ready, deactivating tx_dev");
+   PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+   }
} else {
return;
}
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix tx sub device deactivating

2017-08-16 Thread Gaëtan Rivet

On Wed, Aug 16, 2017 at 05:19:28PM +0300, Matan Azrad wrote:
> The corrupted code couldn't recognize that all sub devices
> were not ready for tx traffic when failsafe PMD was trying
> to switch device because of an unreachable condition using.
> 
> Hence, the current tx sub device variable was not updated
> correctly.
> 
> The fix removed the unreachable branch and added new one
> in the right place respecting the original intent.
> 
> Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Matan Azrad 
> Acked-by: Gaetan Rivet 
> ---
>  drivers/net/failsafe/failsafe_private.h | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_private.h 
> b/drivers/net/failsafe/failsafe_private.h
> index 0361cf4..ef646db 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,
>   } else if ((txd && txd->state < req_state) ||
>  txd == NULL ||
>  txd == banned) {
> - struct sub_device *sdev;
> + struct sub_device *sdev = NULL;

Good catch, actually this makes me think that the FOREACH_SUBDEV_STATE
macro is not following the usual tailq API (which sets the iterator to
NULL upon terminating). This can throw-off the unsuspecting writer.

I will fix this soon™. In the meantime, sdev should be initialized to
NULL.

>   uint8_t i;
>  
>   /* Using acceptable device */
> @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
>   PRIV(dev)->subs_tx = i;
>   break;
>   }
> - } else if (txd && txd->state < req_state) {
> - DEBUG("No device ready, deactivating tx_dev");
> - PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> + if (i >= PRIV(dev)->subs_tail || sdev == NULL) {
> + DEBUG("No device ready, deactivating tx_dev");
> + PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> + }
>   } else {
>   return;
>   }
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix tx sub device deactivating

2017-08-16 Thread Matan Azrad

Hi

> -Original Message-
> From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com]
> Sent: Wednesday, August 16, 2017 5:39 PM
> To: Matan Azrad 
> Cc: dev@dpdk.org; sta...@dpdk.org
> Subject: Re: [PATCH v2] net/failsafe: fix tx sub device deactivating
> 
> On Wed, Aug 16, 2017 at 05:19:28PM +0300, Matan Azrad wrote:
> > The corrupted code couldn't recognize that all sub devices were not
> > ready for tx traffic when failsafe PMD was trying to switch device
> > because of an unreachable condition using.
> >
> > Hence, the current tx sub device variable was not updated correctly.
> >
> > The fix removed the unreachable branch and added new one in the right
> > place respecting the original intent.
> >
> > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Matan Azrad 
> > Acked-by: Gaetan Rivet 
> > ---
> >  drivers/net/failsafe/failsafe_private.h | 9 +
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/failsafe/failsafe_private.h
> > b/drivers/net/failsafe/failsafe_private.h
> > index 0361cf4..ef646db 100644
> > --- a/drivers/net/failsafe/failsafe_private.h
> > +++ b/drivers/net/failsafe/failsafe_private.h
> > @@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,
> > } else if ((txd && txd->state < req_state) ||
> >txd == NULL ||
> >txd == banned) {
> > -   struct sub_device *sdev;
> > +   struct sub_device *sdev = NULL;
> 
> Good catch, actually this makes me think that the FOREACH_SUBDEV_STATE
> macro is not following the usual tailq API (which sets the iterator to NULL
> upon terminating). This can throw-off the unsuspecting writer.

I think you right.
Else, you should go all over this macro using and initiate sdev to NULL.

> 
> I will fix this soon™. In the meantime, sdev should be initialized to NULL.

Great 

> 
> > uint8_t i;
> >
> > /* Using acceptable device */
> > @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
> > PRIV(dev)->subs_tx = i;
> > break;
> > }
> > -   } else if (txd && txd->state < req_state) {
> > -   DEBUG("No device ready, deactivating tx_dev");
> > -   PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > +   if (i >= PRIV(dev)->subs_tail || sdev == NULL) {
> > +   DEBUG("No device ready, deactivating tx_dev");
> > +   PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > +   }
> > } else {
> > return;
> > }
> > --
> > 2.7.4
> >
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

2017-08-16 Thread Gaëtan Rivet

On Wed, Aug 16, 2017 at 02:17:16PM +, Shahaf Shuler wrote:
> Wednesday, August 16, 2017 3:42 PM, Gaëtan Rivet:
> > On Wed, Aug 16, 2017 at 02:43:08PM +0300, Shahaf Shuler wrote:
> > > Currently device state moves between ATTACHED when device was
> > > successfully probed to UNUSED when device is detached or released.
> > >
> > > The device state following rte_eth_dev_close() operation is inconsist,
> > > The device is still in ATTACHED state, however it cannot be used in
> > > any way till it will be probed again.
> > >
> > > Fixing it by changing the state to UNUSED.
> > >
> > 
> > You are right that simply closing the device leaves it in a unusable state.
> > 
> > However it seems to be by design.
> > Most drivers call `rte_eth_dev_release_port` when being removed, which
> > sets the state to RTE_ETH_DEV_UNUSED.
> > 
> > If I'm not mistaken, the API of rte_eth_dev_close is that the only available
> > action should then be to detach the driver. At least PCI and vdev buses
> > expects a `remove` callback from their driver, which can be called by the 
> > user
> > (previously using specific API like `rte_eal_vdev_uninit` for example, now
> > using `rte_eal_hotplug_remove` or `rte_eth_dev_detach` from the ether
> > layer).
> > 
> > So, it seems that this burden lies with the driver which should call the 
> > proper
> > API when removing their device.
> 
> Even though it is reasonable for driver to call the rte_eth_dev_port_release, 
> I still think the ethdev layer should protect from such bad behavior from the 
> application side.
> It is more robust than counting on the different PMD to implement such 
> release. 
> 

The ethdev layer does so in the rte_eth_dev_detach function, which
is currently the only way to detach a device from the ethdev level.

`rte_eth_dev_detach` setting the device state seems to me to be a
crutch against badly implemented drivers. If I was nitpicky I would
actually remove it and ideally everyone should enforce the call of
rte_eth_dev_release_port from device removal functions when reviewing
PMD implementations.

The hotplug API is available to applications and the only way to have
consistent devices states after calling rte_eal_hotplug_remove is to
have drivers using a hook in the ethdev layer allowing to clean-up
resources upon release. While it is trivial in its current state, such
entry-point in the ethdev layer will be useful and should be kept and
enforced IMO.

I'm thinking about the 16-bit portid scheduled for v17.11, which implies
an arbitrary number of port available. This would imply a dynamic
allocation of rte_eth_devices, which would *need* such release hook to
be available. Well the API should not be designed from conjectures or
speculations of course, but I think it should be useful and there is no
reason to remove it.

Going further, I find it dangerous to have two points where the port is
semantically released (device state set to UNUSED). If the API of the
port release changes, we now have two points where we need to enforce
the changes. While trivial concerning an enum, it could become more
complex / dangerous if this veered toward memory management.

> > 
> > Maybe Thomas will have a better insight about the scope of the
> > `rte_eth_dev_close` function. But IMO the API is respected.
> > After all, until the proper `dev_detach` function is called, the device is 
> > still
> > attached, even if closed.
> > 
> > If you disagree, there might possibly be an argument to make about either
> > adding finer-grained device states or streamlining the API. This is however 
> > a
> > discussion about API design and not about its implementation anymore.
> 
> Well my first thought when I created this patch was to add fine-grained 
> device states. However then I read the commit log which changed the device 
> states, specifically :
> 
> " "DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that
> the emptiness of a slot is not necessarily the result of detaching a
> device."
> 
> Which convince me to reuse the UNUSED state to reflect that this device 
> cannot longer be used (even though it is still attached). 
> 

When the device is closed, it could still be in the
`RTE_ETH_DEV_DEFERRED` state. This state means that the device is
managed by a third party (application or whatever). It is forbidden for
the ethdev layer to change the state of the device unless said
third-party releases it.

The only place where the device state could unequivocally be set to
UNUSED is upon the proper release of the device. As far as the ethdev
layer is concerned, this is currently within rte_eth_dev_detach.

> > 
> > > Fixes: d52268a8b24b ("ethdev: expose device states")
> > > Cc: gaetan.ri...@6wind.com
> > > Cc: sta...@dpdk.org
> > >
> > > Signed-off-by: Shahaf Shuler 
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c index 0597641ee..98d9e929

Re: [dpdk-dev] [RFC PATCH 1/4] rte_security: API definitions

2017-08-16 Thread Hemant Agrawal

Hi Thomas,
Can we get a next-security tree to do development around this proposal?

Also, we can discuss about this proposal in general in next techboard meeting.

Regards,
Hemant


> -Original Message-
> From: Akhil Goyal [mailto:akhil.go...@nxp.com]
> Sent: Wednesday, August 16, 2017 1:10 PM
> To: Radu Nicolau ; dev@dpdk.org;
> declan.dohe...@intel.com; tho...@monjalon.net;
> avia...@mellanox.com; bor...@mellanox.com;
> pablo.de.lara.gua...@intel.com; sergio.gonzalez.mon...@intel.com
> Cc: Hemant Agrawal ; Sandeep Malik
> 
> Subject: Re: [RFC PATCH 1/4] rte_security: API definitions
> 
> On 8/15/2017 4:34 PM, Radu Nicolau wrote:
> >
> > On 8/15/2017 7:35 AM, Akhil Goyal wrote:
> >> Detailed description is added in the coverletter
> >>
> >> Signed-off-by: Akhil Goyal 
> >> ---
> >>   lib/librte_cryptodev/rte_security.c | 171 +++
> >>   lib/librte_cryptodev/rte_security.h | 409
> >> 
> >>   2 files changed, 580 insertions(+)
> >>   create mode 100644 lib/librte_cryptodev/rte_security.c
> >>   create mode 100644 lib/librte_cryptodev/rte_security.h
> >>
> 
> >> +int
> >> +rte_security_session_init(uint16_t dev_id,
> >> +  struct rte_security_session *sess,
> >> +  struct rte_security_sess_conf *conf,
> >> +  struct rte_mempool *mp) {
> >> +struct rte_cryptodev *cdev = NULL;
> >> +struct rte_eth_dev *dev = NULL;
> >> +uint8_t index;
> >> +int ret;
> >> +
> >> +if (sess == NULL || conf == NULL)
> >> +return -EINVAL;
> >> +
> >> +switch (conf->action_type) {
> >> +case RTE_SECURITY_SESS_CRYPTO_PROTO_OFFLOAD:
> >> +if (!rte_cryptodev_pmd_is_valid_dev(dev_id))
> >> +return -EINVAL;
> >> +cdev = rte_cryptodev_pmd_get_dev(dev_id);
> >> +index = cdev->driver_id;
> >> +if (sess->sess_private_data[index] == NULL) {
> >> +ret = cdev->sec_ops->session_configure(cdev, conf, sess,
> >> mp);
> >> +if (ret < 0) {
> >> +CDEV_LOG_ERR(
> >> +"cdev_id %d failed to configure session details",
> >> +dev_id);
> >> +return ret;
> >> +}
> >> +}
> >> +break;
> >> +case RTE_SECURITY_SESS_ETH_INLINE_CRYPTO:
> >> +case RTE_SECURITY_SESS_ETH_PROTO_OFFLOAD:
> >> +dev = &rte_eth_devices[dev_id];
> >> +index = dev->data->port_id;
> >> +if (sess->sess_private_data[index] == NULL) {
> >> +//ret = dev->sec_ops->session_configure(dev, conf, sess,
> >> mp);
> >> +//if (ret < 0) {
> >> +//CDEV_LOG_ERR(
> >> +//"dev_id %d failed to configure session details",
> >> +//dev_id);
> >> +//return ret;
> >> +//}
> > The commented lines above suggests that also eth devices will have a
> > sec_ops field, (which makes sense). Is this correct?
> > Also, if the above is correct, session_configure and session_clear
> > should accept both crypto and eth devices as first parameter.
> 
> Yes you are correct both these ops should accept void *dev and internally in
> the driver should typecast to respective device.
> Please consider the following diff over this patch
> 
> 
> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h
> b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> index 219fba6..ab3ecf7 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> @@ -371,7 +371,7 @@ struct rte_cryptodev_ops {
>*  - Returns -ENOTSUP if crypto device does not support the crypto
> transform.
>*  - Returns -ENOMEM if the private session could not be allocated.
>*/
> -typedef int (*security_configure_session_t)(struct rte_cryptodev *dev,
> +typedef int (*security_configure_session_t)(void *dev,
>  struct rte_security_sess_conf *conf,
>  struct rte_security_session *sess,
>  struct rte_mempool *mp);
> @@ -382,7 +382,7 @@ typedef int (*security_configure_session_t)(struct
> rte_cryptodev *dev,
>* @param  dev Crypto device pointer
>* @param  sessSecurity session structure
>*/
> -typedef void (*security_free_session_t)(struct rte_cryptodev *dev,
> +typedef void (*security_free_session_t)(void *dev,
>  struct rte_security_session *sess);
> 
>   /** Security operations function pointer table */
> diff --git a/lib/librte_cryptodev/rte_security.c
> b/lib/librte_cryptodev/rte_security.c
> index 7c73c93..a7558bb 100644
> --- a/lib/librte_cryptodev/rte_security.c
> +++ b/lib/librte_cryptodev/rte_security.c
> @@ -87,7 +87,8 @@ rte_security_session_init(uint16_t dev_id,
>  cdev = rte_cryptodev_pmd_get_dev(dev_id);
>  index = cdev->driver_id;
>  if (sess->sess_private_data[index] == NULL) {
> -   ret = cdev->sec_ops->

Re: [dpdk-dev] [PATCH v6 08/12] linuxapp/eal: auto detect iova mode

2017-08-16 Thread Aaron Conole

Santosh Shukla  writes:

> - Moving late bus scanning to up..just after eal_parsing.
> - Auto detect iova mapping mode, based on the result of
>   rte_bus_scan_iommu_class.
>
> Signed-off-by: Santosh Shukla 
> Signed-off-by: Jerin Jacob 
> Reviewed-by: Maxime Coquelin 
> ---
>  lib/librte_eal/linuxapp/eal/eal.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c 
> b/lib/librte_eal/linuxapp/eal/eal.c
> index febbafdb3..5382f6c00 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -798,6 +798,15 @@ rte_eal_init(int argc, char **argv)
>   return -1;
>   }
>  
> + if (rte_bus_scan()) {
> + rte_eal_init_alert("Cannot scan the buses for devices\n");
> + rte_errno = ENODEV;

Since this now happens before hugetlbs are allocated, is it possible to
retry?  If so, then I would say to clear the run_once variable.

> + return -1;
> + }
> +
> + /* autodetect the iova mapping mode (default is iova_pa) */
> + rte_eal_get_configuration()->iova_mode = rte_bus_get_iommu_class();
> +
>   if (internal_config.no_hugetlbfs == 0 &&
>   internal_config.process_type != RTE_PROC_SECONDARY &&
>   internal_config.xen_dom0_support == 0 &&
> @@ -900,12 +909,6 @@ rte_eal_init(int argc, char **argv)
>   return -1;
>   }
>  
> - if (rte_bus_scan()) {
> - rte_eal_init_alert("Cannot scan the buses for devices\n");
> - rte_errno = ENODEV;
> - return -1;
> - }
> -
>   RTE_LCORE_FOREACH_SLAVE(i) {
>  
>   /*

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

2017-08-16 Thread Shahaf Shuler

Wednesday, August 16, 2017 6:26 PM, Gaëtan Rivet:
> > Even though it is reasonable for driver to call the
> rte_eth_dev_port_release, I still think the ethdev layer should protect from
> such bad behavior from the application side.
> > It is more robust than counting on the different PMD to implement such
> release.
> >
> 
> The ethdev layer does so in the rte_eth_dev_detach function, which is
> currently the only way to detach a device from the ethdev level.
> 
> `rte_eth_dev_detach` setting the device state seems to me to be a crutch
> against badly implemented drivers. If I was nitpicky I would actually remove 
> it
> and ideally everyone should enforce the call of rte_eth_dev_release_port
> from device removal functions when reviewing PMD implementations.
> 
> The hotplug API is available to applications and the only way to have
> consistent devices states after calling rte_eal_hotplug_remove is to have
> drivers using a hook in the ethdev layer allowing to clean-up resources upon
> release. While it is trivial in its current state, such entry-point in the 
> ethdev
> layer will be useful and should be kept and enforced IMO.
> 
> I'm thinking about the 16-bit portid scheduled for v17.11, which implies an
> arbitrary number of port available. This would imply a dynamic allocation of
> rte_eth_devices, which would *need* such release hook to be available.
> Well the API should not be designed from conjectures or speculations of
> course, but I think it should be useful and there is no reason to remove it.
> 
> Going further, I find it dangerous to have two points where the port is
> semantically released (device state set to UNUSED). If the API of the port
> release changes, we now have two points where we need to enforce the
> changes. While trivial concerning an enum, it could become more complex /
> dangerous if this veered toward memory management.

Those are valid concerns, and you convinced me the RTE_ETH_DEV_UNUSED cannot be 
set after device close.

I still think the ethdev layer missing protection against driver calls (other 
than detach) following a device close. The API not allows, but the ethdev 
should enforce it.
Considering some driver memset to 0 their control structs after device close, 
with no protection such bad calls might lead to segfault, which is not what we 
want even under bad behavior. 

One could suggest to protect it inside the driver by replacing the vtable of 
the driver ops, however it will introduce a lot of duplicated code which can 
easily be avoided if ethdev would not pass such
Calls after device close.

So, how about adding another state which will indicate the device is closed, 
cannot be used, yet still attached?

[dpdk-dev] [ovs-dpdk-tests] where is ovs-dpdk test case?

2017-08-16 Thread Sam

Hi all,

I'm working with ovs-dpdk, I want to run ovs-dpdk test case. But I found
there is no test case for 'netdev' type bridge and no test case for
ovs-dpdk datapath(which is pmd_thread_main).

So my question is where could I find these test cases?

Also I change some code in dpdk-vhost client mode, how could I find test
case for this module?

Thank you~

Re: [dpdk-dev] [PATCH] crypto/openssl: performance improvements

Re: [dpdk-dev] [RFC PATCH 1/4] rte_security: API definitions

Re: [dpdk-dev] [PATCH] net/failsafe: fix tx sub device deactivating

Re: [dpdk-dev] [PATCH] net/failsafe: fix tx sub device deactivating

Re: [dpdk-dev] [PATCH] doc: add i40e firmware upgrade guide

[dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

[dpdk-dev] [PATCH] ethdev: fix device state on close

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

Re: [dpdk-dev] [PATCH] net/failsafe: fix tx sub device deactivating

Re: [dpdk-dev] [PATCH 0/8] service: rework for usability

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

[dpdk-dev] [PATCH v2] net/failsafe: fix tx sub device deactivating

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix tx sub device deactivating

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix tx sub device deactivating

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

Re: [dpdk-dev] [RFC PATCH 1/4] rte_security: API definitions

Re: [dpdk-dev] [PATCH v6 08/12] linuxapp/eal: auto detect iova mode

Re: [dpdk-dev] [PATCH] ethdev: fix device state on close

[dpdk-dev] [ovs-dpdk-tests] where is ovs-dpdk test case?

22 matches

Site Navigation

Mail list logo

Footer information