* Re: [PATCH 1/4] libcrypto_pmd: initial implementation of SW crypto device
From: Mrozowicz, SlawomirX @ 2016-10-11 9:50 UTC (permalink / raw)
To: Jastrzebski, MichalX K, Akhil Goyal, Azarewicz, PiotrX T,
Doherty, Declan, dev@dpdk.org
Cc: Kerlin, MarcinX, Kobylinski, MichalX, Kulasek, TomaszX,
Mrzyglod, DanielX T
In-Reply-To: <60ABE07DBB3A454EB7FAD707B4BB158213AEEE33@IRSMSX109.ger.corp.intel.com>
>-----Original Message-----
>From: Jastrzebski, MichalX K
>Sent: Wednesday, September 14, 2016 10:48 AM
>To: Akhil Goyal <akhil.goyal@nxp.com>; Azarewicz, PiotrX T
><piotrx.t.azarewicz@intel.com>; Doherty, Declan
><declan.doherty@intel.com>; dev@dpdk.org
>Cc: Kerlin, MarcinX <marcinx.kerlin@intel.com>; Mrozowicz, SlawomirX
><slawomirx.mrozowicz@intel.com>; Kobylinski, MichalX
><michalx.kobylinski@intel.com>; Kulasek, TomaszX
><tomaszx.kulasek@intel.com>; Mrzyglod, DanielX T
><danielx.t.mrzyglod@intel.com>
>Subject: RE: [dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial implementation of
>SW crypto device
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
>> Sent: Monday, September 12, 2016 1:17 PM
>> To: Azarewicz, PiotrX T <piotrx.t.azarewicz@intel.com>; Doherty,
>> Declan <declan.doherty@intel.com>; dev@dpdk.org
>> Cc: Kerlin, MarcinX <marcinx.kerlin@intel.com>; Mrozowicz, SlawomirX
>> <slawomirx.mrozowicz@intel.com>; Kobylinski, MichalX
>> <michalx.kobylinski@intel.com>; Kulasek, TomaszX
>> <tomaszx.kulasek@intel.com>; Mrzyglod, DanielX T
>> <danielx.t.mrzyglod@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial
>> implementation of SW crypto device
>>
>> On 8/26/2016 12:51 PM, Piotr Azarewicz wrote:
>>
>> > +/** Provide session for operation */ static struct
>> > +libcrypto_session * get_session(struct libcrypto_qp *qp, struct
>> > +rte_crypto_op *op) {
>> > + struct libcrypto_session *sess = NULL;
>> > +
>> > + if (op->sym->sess_type == RTE_CRYPTO_SYM_OP_WITH_SESSION) {
>> > + /* get existing session */
>> > + if (!unlikely(op->sym->session == NULL ||
>> > + op->sym->session->dev_type !=
>> > + RTE_CRYPTODEV_LIBCRYPTO_PMD))
>> > + sess = (struct libcrypto_session *)
>> > + op->sym->session->_private;
>> > + } else {
>> > + /* provide internal session */
>> > + void *_sess = NULL;
>> > +
>> > + if (!rte_mempool_get(qp->sess_mp, (void **)&_sess)) {
>> > + sess = (struct libcrypto_session *)
>> > + ((struct rte_cryptodev_sym_session *)_sess)
>> > + ->_private;
>> > +
>> > + if (unlikely(libcrypto_set_session_parameters(
>> > + sess, op->sym->xform) != 0)) {
>> > + rte_mempool_put(qp->sess_mp, _sess);
>> > + sess = NULL;
>> > + } else
>> > + op->sym->session = _sess;
>> > + }
>> > + }
>> > +
>> > + if (sess == NULL)
>> > + op->status = RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
>> > +
>> > + return sess;
>> > +}
>> > +
>> > +/*
>> > +
>> > +*------------------------------------------------------------------
>> > +------------
>> > + * Process Operations
>> > +
>> > +*------------------------------------------------------------------
>> > +------------
>> > + */
>> > +
>> > +/** Process standard libcrypto cipher encryption */ static int
>> > +process_libcrypto_cipher_encrypt(uint8_t *src, uint8_t *dst,
>> > + uint8_t *iv, uint8_t *key, int srclen,
>> > + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo) {
>> > + int dstlen, totlen;
>> > +
>> > + if (EVP_EncryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
>> > + goto process_cipher_encrypt_err;
>> [Akhil] this EVP_EncryptInit_ex() can be done for each session instead
>> of each packet. This will improve the performance. Also if there is
>> some change in the parameters later then it can be called again here
>> with the updated parameters only.
>> Same comment is for all cases (hmac, auth, etc)
>
>Hi Akhil,
>Thank You for this comment. We will check if this is possible.
>
>Michal
>
[Sławomir]
We have done some investigation:
- At first glance it is valuable to call this kind of function
only once per session.
- We done test: just remove the functions like EVP_XXXInitXXX
and we reduce packet processing time between 1% to 10%
depending of processed algorithm.
- We analyzed calling only once per session also functions
like EVP_XXXFinalXXX but it is not possible because
this kind of functions finalize cipher or authenticate process
and return result.
- The functions like EVP_XXXFinalXXX change context and
It is not possible to proper continue processing without
reinitialize the context. So after call functions EVP_XXXFinalXXX
we need to call EVP_XXXInitXXX for next operation.
In sum we can't do the proposed performance improvement.
>> > +
>> > + if (EVP_EncryptUpdate(ctx, dst, &dstlen, src, srclen) <= 0)
>> > + goto process_cipher_encrypt_err;
>> > +
>> > + if (EVP_EncryptFinal_ex(ctx, dst + dstlen, &totlen) <= 0)
>> > + goto process_cipher_encrypt_err;
>> > +
>> > + return 0;
>> > +
>> > +process_cipher_encrypt_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto cipher encrypt failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process standard libcrypto cipher decryption */ static int
>> > +process_libcrypto_cipher_decrypt(uint8_t *src, uint8_t *dst,
>> > + uint8_t *iv, uint8_t *key, int srclen,
>> > + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo) {
>> > + int dstlen, totlen;
>> > +
>> > + if (EVP_DecryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + if (EVP_CIPHER_CTX_set_padding(ctx, 0) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + if (EVP_DecryptUpdate(ctx, dst, &dstlen, src, srclen) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + if (EVP_DecryptFinal_ex(ctx, dst + dstlen, &totlen) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + return 0;
>> > +
>> > +process_cipher_decrypt_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto cipher decrypt failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process cipher des 3 ctr encryption, decryption algorithm */
>> > +static int process_libcrypto_cipher_des3ctr(uint8_t *src, uint8_t
>> > +*dst,
>> > + uint8_t *iv, uint8_t *key, int srclen, EVP_CIPHER_CTX *ctx) {
>> > + uint8_t ebuf[8], ctr[8];
>> > + int unused, n;
>> > +
>> > + /* We use 3DES encryption also for decryption.
>> > + * IV is not important for 3DES ecb
>> > + */
>> > + if (EVP_EncryptInit_ex(ctx, EVP_des_ede3_ecb(), NULL, key, NULL)
>> > +<=
>> 0)
>> > + goto process_cipher_des3ctr_err;
>> > +
>> > + memcpy(ctr, iv, 8);
>> > + n = 0;
>> > +
>> > + while (n < srclen) {
>> > + if (n % 8 == 0) {
>> > + if (EVP_EncryptUpdate(ctx, (unsigned char *)&ebuf,
>> &unused,
>> > + (const unsigned char *)&ctr, 8) <= 0)
>> > + goto process_cipher_des3ctr_err;
>> > + ctr_inc(ctr);
>> > + }
>> > + dst[n] = src[n] ^ ebuf[n % 8];
>> > + n++;
>> > + }
>> > +
>> > + return 0;
>> > +
>> > +process_cipher_des3ctr_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto cipher des 3 ede ctr failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process auth gmac algorithm */
>> > +static int
>> > +process_libcrypto_auth_gmac(uint8_t *src, uint8_t *dst,
>> > + uint8_t *iv, uint8_t *key, int srclen, int ivlen,
>> > + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo) {
>> > + int unused;
>> > +
>> > + if (EVP_EncryptInit_ex(ctx, algo, NULL, NULL, NULL) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_IVLEN, ivlen,
>> NULL) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_EncryptInit_ex(ctx, NULL, NULL, key, iv) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_EncryptUpdate(ctx, NULL, &unused, src, srclen) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_EncryptFinal_ex(ctx, NULL, &unused) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_GET_TAG, 16, dst) <=
>> 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + return 0;
>> > +
>> > +process_auth_gmac_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto auth gmac failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process standard libcrypto auth algorithms */ static int
>> > +process_libcrypto_auth(uint8_t *src, uint8_t *dst,
>> > + __rte_unused uint8_t *iv, __rte_unused EVP_PKEY * pkey,
>> > + int srclen, EVP_MD_CTX *ctx, const EVP_MD *algo) {
>> > + size_t dstlen;
>> > +
>> > + if (EVP_DigestInit_ex(ctx, algo, NULL) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestUpdate(ctx, (char *)src, srclen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestFinal_ex(ctx, dst, (unsigned int *)&dstlen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + return 0;
>> > +
>> > +process_auth_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto auth failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process standard libcrypto auth algorithms with hmac */ static
>> > +int process_libcrypto_auth_hmac(uint8_t *src, uint8_t *dst,
>> > + __rte_unused uint8_t *iv, EVP_PKEY *pkey,
>> > + int srclen, EVP_MD_CTX *ctx, const EVP_MD *algo)
>> > +{
>> > + size_t dstlen;
>> > +
>> > + if (EVP_DigestSignInit(ctx, NULL, algo, NULL, pkey) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestSignUpdate(ctx, (char *)src, srclen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestSignFinal(ctx, dst, &dstlen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + return 0;
>> > +
>> > +process_auth_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto auth failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/*-----------------------------------------------------------------
>> > +-----------*/
>> > +
>>
>>
^ permalink raw reply
* Re: [PATCH] net/mlx5: fix init on secondary process
From: Adrien Mazarguil @ 2016-10-11 9:45 UTC (permalink / raw)
To: Olivier Gournet; +Cc: dev
In-Reply-To: <1475072658-29534-1-git-send-email-ogournet@corp.free.fr>
Hi Olivier,
Secondary process support's got overlooked during this refactoring, thanks
for the patch. However can you describe the issue you're addressing as part
of the commit log?
I think problems started when txq got mistakenly converted to
primary_txq_ctrl in 21c8bb4928c9 ("net/mlx5: split Tx queue structure"), you
may add a Fixes line for that one as well.
Otherwise, this patch looks fine to me.
On Wed, Sep 28, 2016 at 04:24:18PM +0200, Olivier Gournet wrote:
> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
>
> Signed-off-by: Olivier Gournet <ogournet@corp.free.fr>
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 130e15d..6f39965 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1308,11 +1308,13 @@ mlx5_secondary_data_setup(struct priv *priv)
> continue;
> primary_txq_ctrl = container_of(primary_txq,
> struct txq_ctrl, txq);
> - txq_ctrl = rte_calloc_socket("TXQ", 1, sizeof(*txq_ctrl), 0,
> + txq_ctrl = rte_calloc_socket("TXQ", 1, sizeof(*txq_ctrl) +
> + primary_txq->elts_n *
> + sizeof(struct rte_mbuf *), 0,
> primary_txq_ctrl->socket);
> if (txq_ctrl != NULL) {
> if (txq_ctrl_setup(priv->dev,
> - primary_txq_ctrl,
> + txq_ctrl,
> primary_txq->elts_n,
> primary_txq_ctrl->socket,
> NULL) == 0) {
> --
> 2.1.4
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH v4] drivers/net:new PMD using tun/tap host interface
From: Ferruh Yigit @ 2016-10-11 9:40 UTC (permalink / raw)
To: Keith Wiles, dev; +Cc: pmatilai, yuanhan.liu
In-Reply-To: <1475592311-25749-1-git-send-email-keith.wiles@intel.com>
Hi Keith,
On 10/4/2016 3:45 PM, Keith Wiles wrote:
> The rte_eth_tap.c PMD creates a device using TUN/TAP interfaces
> on the local host. The PMD allows for DPDK and the host to
> communicate using a raw device interface on the host and in
> the DPDK application. The device created is a Tap device with
> a L2 packet header.
>
> v4 - merge with latest driver changes
> v3 - fix includes by removing ifdef for other type besides Linux.
> Fix the copyright notice in the Makefile
> v2 - merge all of the patches into one patch.
> Fix a typo on naming the tap device.
> Update the maintainers list
>
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
> MAINTAINERS | 5 +
> config/common_linuxapp | 2 +
> doc/guides/nics/tap.rst | 84 ++++
> drivers/net/Makefile | 1 +
> drivers/net/tap/Makefile | 57 +++
> drivers/net/tap/rte_eth_tap.c | 866 ++++++++++++++++++++++++++++++++
> drivers/net/tap/rte_pmd_tap_version.map | 4 +
> mk/rte.app.mk | 1 +
> 8 files changed, 1020 insertions(+)
> create mode 100644 doc/guides/nics/tap.rst
> create mode 100644 drivers/net/tap/Makefile
> create mode 100644 drivers/net/tap/rte_eth_tap.c
> create mode 100644 drivers/net/tap/rte_pmd_tap_version.map
This patch needs to be rebased on top of latest next-net, .init &
.uninit are no more used.
Also patch gives a set of checkpatch warnings, fyi.
Thanks,
ferruh
^ permalink raw reply
* Re: [PATCH 4/4] net/enic: extend fdir support for 1300 series adapters
From: John Daley (johndale) @ 2016-10-11 9:25 UTC (permalink / raw)
To: Ferruh Yigit, bruce.richardson@intel.com; +Cc: dev@dpdk.org
In-Reply-To: <2f02ca98-8250-0c27-7ae4-5e23839521d6@intel.com>
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, October 11, 2016 2:22 AM
> To: John Daley (johndale) <johndale@cisco.com>;
> bruce.richardson@intel.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 4/4] net/enic: extend fdir support for 1300
> series adapters
>
> Hi John,
>
> On 9/29/2016 9:56 PM, John Daley wrote:
> > 1300 series Cisco adapter firmware version 2.0(13) for UCS C-series
> > servers and 3.1(2) for blade servers supports more filtering
> > capabilities. The feature can be enabled via Cisco CIMC or USCM with
> > the 'advanced filters' radio button. When enabled, the these
> > additional flow director modes are available:
> > RTE_ETH_FLOW_NONFRAG_IPV4_OTHER
> > RTE_ETH_FLOW_NONFRAG_IPV4_SCTP
> > RTE_ETH_FLOW_NONFRAG_IPV6_UDP
> > RTE_ETH_FLOW_NONFRAG_IPV6_TCP
> > RTE_ETH_FLOW_NONFRAG_IPV6_SCTP
> > RTE_ETH_FLOW_NONFRAG_IPV6_OTHER
> >
> > Changes:
> > - Detect and set an 'advanced filters' flag dependent on the adapter
> > capability.
> > - Implement RTE_ETH_FILTER_INFO filter op to return the flow types
> > available dependent on whether advanced filters are enabled.
> > - Use a function pointer to select how filters are added to the adapter:
> > copy_fltr_v1() for older firmware/adapters or copy_fltr_v2() for
> > adapters which support advanced filters.
> > - Apply fdir global masks to filters when in advanced filter mode.
> > - Update documentation.
> >
> > Signed-off-by: John Daley <johndale@cisco.com>
> > Reviewed-by: Nelson Escobar <neescoba@cisco.com>
> > ---
>
> <...>
>
> >
> > +void enic_fdir_info_get(struct enic *enic, struct rte_eth_fdir_info
> > +*info) {
> > + info->mode = enic->fdir.modes;
>
> This cause a icc build error:
> .../drivers/net/enic/enic_clsf.c(77):
> error #188: enumerated type mixed with another type
> info->mode = enic->fdir.modes;
> ^
>
> Just casting to the enum fixes it:
> + info->mode = (enum rte_fdir_mode)enic->fdir.modes;
>
> Since the modification is trivial, if you agree with the change, we can apply it
> without needing a new version of the patch?
>
Looks good, thank you and sorry for the trouble.
-john
> <...>
^ permalink raw reply
* Re: [PATCH] net/i40e: add additional prefetch instructions for bulk rx
From: Vladyslav Buslov @ 2016-10-11 9:24 UTC (permalink / raw)
To: Ananyev, Konstantin, Wu, Jingjing, Yigit, Ferruh, Zhang, Helin
Cc: dev@dpdk.org
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0C0408@irsmsx105.ger.corp.intel.com>
Hi Konstantin,
> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Tuesday, October 11, 2016 11:51 AM
> To: Vladyslav Buslov; Wu, Jingjing; Yigit, Ferruh; Zhang, Helin
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/i40e: add additional prefetch
> instructions for bulk rx
>
> Hi Vladislav,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vladyslav Buslov
> > Sent: Monday, October 10, 2016 6:06 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/i40e: add additional prefetch
> > instructions for bulk rx
> >
> > > -----Original Message-----
> > > From: Wu, Jingjing [mailto:jingjing.wu@intel.com]
> > > Sent: Monday, October 10, 2016 4:26 PM
> > > To: Yigit, Ferruh; Vladyslav Buslov; Zhang, Helin
> > > Cc: dev@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH] net/i40e: add additional prefetch
> > > instructions for bulk rx
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yigit, Ferruh
> > > > Sent: Wednesday, September 14, 2016 9:25 PM
> > > > To: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>; Zhang,
> > > > Helin <helin.zhang@intel.com>; Wu, Jingjing
> > > > <jingjing.wu@intel.com>
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] net/i40e: add additional prefetch
> > > > instructions for bulk rx
> > > >
> > > > On 7/14/2016 6:27 PM, Vladyslav Buslov wrote:
> > > > > Added prefetch of first packet payload cacheline in
> > > > > i40e_rx_scan_hw_ring Added prefetch of second mbuf cacheline in
> > > > > i40e_rx_alloc_bufs
> > > > >
> > > > > Signed-off-by: Vladyslav Buslov
> > > > > <vladyslav.buslov@harmonicinc.com>
> > > > > ---
> > > > > drivers/net/i40e/i40e_rxtx.c | 7 +++++--
> > > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > > > b/drivers/net/i40e/i40e_rxtx.c index d3cfb98..e493fb4 100644
> > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > @@ -1003,6 +1003,7 @@ i40e_rx_scan_hw_ring(struct
> i40e_rx_queue
> > > *rxq)
> > > > > /* Translate descriptor info to mbuf parameters */
> > > > > for (j = 0; j < nb_dd; j++) {
> > > > > mb = rxep[j].mbuf;
> > > > > + rte_prefetch0(RTE_PTR_ADD(mb->buf_addr,
> > > > RTE_PKTMBUF_HEADROOM));
> > >
> > > Why did prefetch here? I think if application need to deal with
> > > packet, it is more suitable to put it in application.
> > >
> > > > > qword1 = rte_le_to_cpu_64(\
> > > > > rxdp[j].wb.qword1.status_error_len);
> > > > > pkt_len = ((qword1 &
> > > > I40E_RXD_QW1_LENGTH_PBUF_MASK) >>
> > > > > @@ -1086,9 +1087,11 @@ i40e_rx_alloc_bufs(struct i40e_rx_queue
> > > *rxq)
> > > > >
> > > > > rxdp = &rxq->rx_ring[alloc_idx];
> > > > > for (i = 0; i < rxq->rx_free_thresh; i++) {
> > > > > - if (likely(i < (rxq->rx_free_thresh - 1)))
> > > > > + if (likely(i < (rxq->rx_free_thresh - 1))) {
> > > > > /* Prefetch next mbuf */
> > > > > - rte_prefetch0(rxep[i + 1].mbuf);
> > > > > + rte_prefetch0(&rxep[i + 1].mbuf->cacheline0);
> > > > > + rte_prefetch0(&rxep[i +
> > > > > + 1].mbuf->cacheline1);
>
> I think there are rte_mbuf_prefetch_part1/part2 defined in rte_mbuf.h,
> specially for that case.
Thanks for pointing that out.
I'll submit new patch if you decide to move forward with this development.
>
> > > > > + }
> > > Agree with this change. And when I test it by testpmd with iofwd, no
> > > performance increase is observed but minor decrease.
> > > Can you share will us when it will benefit the performance in your
> scenario ?
> > >
> > >
> > > Thanks
> > > Jingjing
> >
> > Hello Jingjing,
> >
> > Thanks for code review.
> >
> > My use case: We have simple distributor thread that receives packets
> > from port and distributes them among worker threads according to VLAN
> and MAC address hash.
> >
> > While working on performance optimization we determined that most of
> CPU usage of this thread is in DPDK.
> > As and optimization we decided to switch to rx burst alloc function,
> > however that caused additional performance degradation compared to
> scatter rx mode.
> > In profiler two major culprits were:
> > 1. Access to packet data Eth header in application code. (cache miss)
> > 2. Setting next packet descriptor field to NULL in DPDK
> > i40e_rx_alloc_bufs code. (this field is in second descriptor cache
> > line that was not
> > prefetched)
>
> I wonder what will happen if we'll remove any prefetches here?
> Would it make things better or worse (and by how much)?
In our case it causes few per cent PPS degradation on next=NULL assignment but it seems that JingJing's test doesn't confirm it.
>
> > After applying my fixes performance improved compared to scatter rx
> mode.
> >
> > I assumed that prefetch of first cache line of packet data belongs to
> > DPDK because it is done in scatter rx mode. (in
> > i40e_recv_scattered_pkts)
> > It can be moved to application side but IMO it is better to be consistent
> across all rx modes.
>
> I would agree with Jingjing here, probably PMD should avoid to prefetch
> packet's data.
Actually I can see some valid use cases where it is beneficial to have this prefetch in driver.
In our sw distributor case it is trivial to just prefetch next packet on each iteration because packets are processed one by one.
However when we move this functionality to hw by means of RSS/vfunction/FlowDirector(our long term goal) worker threads will receive packets directly from rx queues of NIC.
First operation of worker thread is to perform bulk lookup in hash table by destination MAC. This will cause cache miss on accessing each eth header and can't be easily mitigated in application code.
I assume it is ubiquitous use case for DPDK.
Regards,
Vladyslav
^ permalink raw reply
* Re: [PATCH 4/4] net/enic: extend fdir support for 1300 series adapters
From: Ferruh Yigit @ 2016-10-11 9:22 UTC (permalink / raw)
To: John Daley, bruce.richardson; +Cc: dev
In-Reply-To: <20160929205639.1175-4-johndale@cisco.com>
Hi John,
On 9/29/2016 9:56 PM, John Daley wrote:
> 1300 series Cisco adapter firmware version 2.0(13) for UCS
> C-series servers and 3.1(2) for blade servers supports more
> filtering capabilities. The feature can be enabled via Cisco
> CIMC or USCM with the 'advanced filters' radio button. When
> enabled, the these additional flow director modes are available:
> RTE_ETH_FLOW_NONFRAG_IPV4_OTHER
> RTE_ETH_FLOW_NONFRAG_IPV4_SCTP
> RTE_ETH_FLOW_NONFRAG_IPV6_UDP
> RTE_ETH_FLOW_NONFRAG_IPV6_TCP
> RTE_ETH_FLOW_NONFRAG_IPV6_SCTP
> RTE_ETH_FLOW_NONFRAG_IPV6_OTHER
>
> Changes:
> - Detect and set an 'advanced filters' flag dependent on the adapter
> capability.
> - Implement RTE_ETH_FILTER_INFO filter op to return the flow types
> available dependent on whether advanced filters are enabled.
> - Use a function pointer to select how filters are added to the adapter:
> copy_fltr_v1() for older firmware/adapters or copy_fltr_v2() for
> adapters which support advanced filters.
> - Apply fdir global masks to filters when in advanced filter mode.
> - Update documentation.
>
> Signed-off-by: John Daley <johndale@cisco.com>
> Reviewed-by: Nelson Escobar <neescoba@cisco.com>
> ---
<...>
>
> +void enic_fdir_info_get(struct enic *enic, struct rte_eth_fdir_info *info)
> +{
> + info->mode = enic->fdir.modes;
This cause a icc build error:
.../drivers/net/enic/enic_clsf.c(77):
error #188: enumerated type mixed with another type
info->mode = enic->fdir.modes;
^
Just casting to the enum fixes it:
+ info->mode = (enum rte_fdir_mode)enic->fdir.modes;
Since the modification is trivial, if you agree with the change, we can
apply it without needing a new version of the patch?
<...>
^ permalink raw reply
* Re: OpenSSL libcrypto PMD name
From: Thomas Monjalon @ 2016-10-11 9:14 UTC (permalink / raw)
To: Declan Doherty; +Cc: pablo.de.lara.guarch, dev
In-Reply-To: <2d5bb5ac-dd3a-19c7-9253-961110adc938@intel.com>
2016-10-11 09:53, Declan Doherty:
> On 10/10/16 12:36, Thomas Monjalon wrote:
> > Hi,
> >
> > I would like to raise a naming issue in crypto.
> >
> > In the crypto side of DPDK, we have a library (similar to ethdev)
> > for crypto API and device interface:
> > http://dpdk.org/browse/dpdk/tree/lib/librte_cryptodev
> > There are also some drivers (which are some libraries):
> > http://dpdk.org/browse/dpdk/tree/drivers/crypto
> > Most of them (6/8) are some DPDK wrappers for external libraries.
> >
> > Recently was introduced the libcrypto PMD which is a wrapper for
> > the OpenSSL libcrypto.
> > As we already have a lot of crypto libraries, I'm afraid the name
> > libcrypto is really confusing. Why not call it openssl PMD?
> >
> > PS: I know OpenSSL has 2 libraries - ssl and crypto - but I do not
> > expect any high-level SSL feature in a crypto driver.
> > So drivers/crypto/openssl should not be confusing.
>
>
> Hey Thomas,
>
> I can see the how this could get pretty confusion especially to those
> not familiar with the implementation details. I think the current name
> makes sense using the rational that we are only using the libcrypto
> library from openssl and not libssl but it doesn't make things exactly
> clear within DPDK.
>
> My thought is that we could just call the PMD "base_sw", as this is the
> function which it is intended to provide, a base implementation of
> algorithms for which there isn't an optimized/vectorised software
> implementation or a fall back for systems which don't support the
> required vector or CPU instructions for the optimized libraries. Also
> this would allow us at a later date extend beyond the scope of Openssl
> if required.
Ah, I'm remembering that before creating a new library we should impose
to define the scope first :)
There are already some PMDs using other libraries.
Do you really want to extend this one beyond of OpenSSL? It looks a weird
use case to me. The question is: how do we choose a crypto library rather
than another one?
By the way, the name "base_sw" is worst :) Please call a marketing-qualified
person ;)
^ permalink raw reply
* Re: 17.02 Roadmap
From: O'Driscoll, Tim @ 2016-10-11 9:09 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev@dpdk.org
In-Reply-To: <1998191.9HGrB6oKr3@xps13>
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>
> Thanks Tim for the interesting inputs.
> Some of them may require a dedicated thread to continue the discussion
> based on some preliminary specifications or drafts.
Agreed. To avoid me acting as a middle man, I'm going to ask the individual developers to respond initially to this thread. If more discussion is required on some of the items, then we should create separate threads.
^ permalink raw reply
* Re: DPDK patchwork upgrade
From: Stephen Finucane @ 2016-10-11 9:05 UTC (permalink / raw)
To: Maxim Uvarov; +Cc: Thomas Monjalon, De Lara Guarch, Pablo, dev@dpdk.org
In-Reply-To: <CAD8XO3aRe+YsyM7kJqerxAwJAxFYmnrzi9mPKPE_AEvwG3f1_g@mail.gmail.com>
> > You sure can - use the 'retag' command [1]:
> >
> > ./manage.py retag
> >
> > I should probably have included that in the upgrade notes...
> >
> > Stephen
> >
> > [1] https://github.com/getpatchwork/patchwork/blob/stable/1.1/patchwork/management/commands/> retag.py
>
> That is looks really good and should save a lot of time for
> maintainer taking already validated patches. How hard is connection
> between patchwork, mailing list tags and testing system? Is that
> all inside patchwork or you wrote it specially for dpdk? I would
> really like to have the same thing for my projects.
Not difficult at all, and improving all the time. dpdk.org uses upstream
Patchwork, so I'd suggest looking at the documentation [1] for some
advice of getting both developer and deployment versions configured.
Alternatively, Andy Doan in Linaro is working on Patchwork and might be
able to help you out some?
Stephen
[1] patchwork.readthedocs.io/en/latest/development/
^ permalink raw reply
* Re: [PATCH v3 03/16] mbuf: move packet type definitions in a new file
From: Olivier MATZ @ 2016-10-11 9:01 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, cunming.liang, john.mcnamara, andrey.chilikin,
konstantin.ananyev
In-Reply-To: <2178965.8ItUXrjgAs@xps13>
Hi Thomas,
On 10/10/2016 04:52 PM, Thomas Monjalon wrote:
> 2016-10-03 10:38, Olivier Matz:
>> The file rte_mbuf.h starts to be quite big, and next commits
>> will introduce more functions related to packet types. Let's
>> move them in a new file.
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>> lib/librte_mbuf/Makefile | 2 +-
>> lib/librte_mbuf/rte_mbuf.h | 495 +----------------------------------
>> lib/librte_mbuf/rte_mbuf_ptype.h | 552 +++++++++++++++++++++++++++++++++++++++
>
> Why not moving packet type and other packet flags in librte_net?
>
These are mbuf features.
I can reverse the question: why moving them in librte_net? :)
^ permalink raw reply
* Re: [PATCH v2] vhost: Only access header if offloading is supported in dequeue path
From: Yuanhan Liu @ 2016-10-11 9:01 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, mst, jianfeng.tan, olivier.matz, stephen
In-Reply-To: <1476171927-10134-1-git-send-email-maxime.coquelin@redhat.com>
On Tue, Oct 11, 2016 at 09:45:27AM +0200, Maxime Coquelin wrote:
> @@ -684,12 +699,12 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
> struct rte_mempool *mbuf_pool)
> {
> struct vring_desc *desc;
> - uint64_t desc_addr;
> + uint64_t desc_addr = 0;
> uint32_t desc_avail, desc_offset;
> uint32_t mbuf_avail, mbuf_offset;
> uint32_t cpy_len;
> struct rte_mbuf *cur = m, *prev = m;
> - struct virtio_net_hdr *hdr;
> + struct virtio_net_hdr *hdr = NULL;
> /* A counter to avoid desc dead loop chain */
> uint32_t nr_desc = 1;
>
> @@ -698,12 +713,14 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
> (desc->flags & VRING_DESC_F_INDIRECT))
> return -1;
>
> - desc_addr = gpa_to_vva(dev, desc->addr);
> - if (unlikely(!desc_addr))
> - return -1;
> + if (virtio_net_with_host_offload(dev)) {
> + desc_addr = gpa_to_vva(dev, desc->addr);
> + if (unlikely(!desc_addr))
> + return -1;
>
> - hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr);
> - rte_prefetch0(hdr);
> + hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr);
> + rte_prefetch0(hdr);
> + }
>
> /*
> * A virtio driver normally uses at least 2 desc buffers
> @@ -720,18 +737,24 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
> if (unlikely(!desc_addr))
> return -1;
>
> - rte_prefetch0((void *)(uintptr_t)desc_addr);
> -
> desc_offset = 0;
> desc_avail = desc->len;
> nr_desc += 1;
> -
> - PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);
> } else {
> + if (!desc_addr) {
> + desc_addr = gpa_to_vva(dev, desc->addr);
> + if (unlikely(!desc_addr))
> + return -1;
> + }
> +
I think this piece of code make things a bit complex. I think what you
want to achieve is, besides saving hdr prefetch, to save one call to
gpa_to_vva() for the non-ANY_LAYOUT case. Does that matter too much?
How about just saving the hdr prefetch?
if (virtio_net_with_host_offload(dev)) {
hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr);
rte_prefetch0(hdr);
}
--yliu
> desc_avail = desc->len - dev->vhost_hlen;
> desc_offset = dev->vhost_hlen;
> }
>
> + rte_prefetch0((void *)(uintptr_t)(desc_addr + desc_offset));
> +
> + PRINT_PACKET(dev, (uintptr_t)(desc_addr + desc_offset), desc_avail, 0);
> +
> mbuf_offset = 0;
> mbuf_avail = m->buf_len - RTE_PKTMBUF_HEADROOM;
> while (1) {
> @@ -795,7 +818,7 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
> prev->data_len = mbuf_offset;
> m->pkt_len += mbuf_offset;
>
> - if (hdr->flags != 0 || hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE)
> + if (virtio_net_with_host_offload(dev))
> vhost_dequeue_offload(hdr, m);
>
> return 0;
> --
> 2.7.4
^ permalink raw reply
* Re: OpenSSL libcrypto PMD name
From: Declan Doherty @ 2016-10-11 8:53 UTC (permalink / raw)
To: Thomas Monjalon, pablo.de.lara.guarch; +Cc: dev
In-Reply-To: <2254713.m5JxJRtkTJ@xps13>
On 10/10/16 12:36, Thomas Monjalon wrote:
> Hi,
>
> I would like to raise a naming issue in crypto.
>
> In the crypto side of DPDK, we have a library (similar to ethdev)
> for crypto API and device interface:
> http://dpdk.org/browse/dpdk/tree/lib/librte_cryptodev
> There are also some drivers (which are some libraries):
> http://dpdk.org/browse/dpdk/tree/drivers/crypto
> Most of them (6/8) are some DPDK wrappers for external libraries.
>
> Recently was introduced the libcrypto PMD which is a wrapper for
> the OpenSSL libcrypto.
> As we already have a lot of crypto libraries, I'm afraid the name
> libcrypto is really confusing. Why not call it openssl PMD?
>
> PS: I know OpenSSL has 2 libraries - ssl and crypto - but I do not
> expect any high-level SSL feature in a crypto driver.
> So drivers/crypto/openssl should not be confusing.
>
Hey Thomas,
I can see the how this could get pretty confusion especially to those
not familiar with the implementation details. I think the current name
makes sense using the rational that we are only using the libcrypto
library from openssl and not libssl but it doesn't make things exactly
clear within DPDK.
My thought is that we could just call the PMD "base_sw", as this is the
function which it is intended to provide, a base implementation of
algorithms for which there isn't an optimized/vectorised software
implementation or a fall back for systems which don't support the
required vector or CPU instructions for the optimized libraries. Also
this would allow us at a later date extend beyond the scope of Openssl
if required.
Declan
^ permalink raw reply
* Re: [PATCH v5 1/2] librte_ether: add protection against overwrite device data
From: Thomas Monjalon @ 2016-10-11 8:52 UTC (permalink / raw)
To: Kerlin, MarcinX; +Cc: dev, De Lara Guarch, Pablo
In-Reply-To: <68D830D942438745AD09BAFA99E33E812BE66F@IRSMSX102.ger.corp.intel.com>
2016-10-07 12:23, Kerlin, MarcinX:
> Hi Thomas,
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, October 06, 2016 4:53 PM
> > To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> > device data
> >
> > 2016-09-30 16:00, Marcin Kerlin:
> > > Added protection against overwrite device data in array
> > > rte_eth_dev_data[] for the next secondary applications. Secondary
> > > process appends in the first free place rather than at the beginning.
> > > This behavior prevents overwriting devices data of primary process by
> > secondary process.
> >
> > It would be good to state what is a secondary process.
> > You are trying to extend its capabilities to be able to initialize devices.
> > So primary and secondary processes are almost equivalent?
> > What happens if we do not create any device in the primary?
> > Answer from code review: "Cannot allocate memzone for ethernet port data\n"
> >
> > The secondary process is a hack to me.
> > But it is fine to have such hack for debug or monitoring purpose.
> > I would like to understand what are the other use cases?
>
> It's true, it is fine for debug or monitoring but If DPDK allow run secondary app with
> devices then it should be safe or completely not allowed.
>
> This bug has been observed while running secondary testpmd with virtual devices.
>
> I will adapt to the decision of maintainers regards to design of secondary process.
>
> >
> > By the way, the code managing the shared data of a device should be at the
> > EAL level in order to be used by other interfaces like crypto.
> >
> > > @@ -631,6 +692,8 @@ int
> > > rte_eth_dev_detach(uint8_t port_id, char *name) {
> > > struct rte_pci_addr addr;
> > > + struct rte_eth_dev_data *eth_dev_data = NULL;
> > > + char device[RTE_ETH_NAME_MAX_LEN];
> > > int ret = -1;
> > >
> > > if (name == NULL) {
> > > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> > > if (rte_eth_dev_is_detachable(port_id))
> > > goto err;
> > >
> > > + /* get device name by port id */
> > > + if (rte_eth_dev_get_name_by_port(port_id, device))
> > > + goto err;
> > > +
> > > + /* look for an entry in the shared device data */
> > > + eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> > > + if (eth_dev_data == NULL)
> > > + goto err;
> >
> > Why not getting eth_dev_data from rte_eth_devices[port_id].data ?
>
> because rte_eth_devices[port_id].data for some drivers (mainly virtual devices)
> is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other drivers).
> This causes that local eth_dev_data is clearing rather than shared in memzone.
>
> Naming is unique so if device was added then there (shared memzone) has to be.
Not sure to understand. Isn't it a bug to have local eth_dev_data?
It means these devices are not shared with secondary process?
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > /**
> > > * @internal
> > > + * Release device data kept in shared memory for all processes.
> > > + *
> > > + * @param port_id
> > > + * The port identifier of the device to release device data.
> > > + * @return
> > > + * - 0 on success, negative on error
> > > + */
> > > +int rte_eth_dev_release_dev_data(uint8_t port_id);
> >
> > Why this function? It is not used.
> > You already have done the job in the detach function.
>
> This function is using in testpmd.c:1640, basic wrapper for clean up.
Please explain the need for cleaning on testpmd exit.
Is it cleaning every devices even those used by the primary process?
I feel it is very weak to not clearly define which process owns a device.
> Detach function is working only for detachable devices, release_dev_data()
> no matter just clean up shared array before next run secondary e.g testpmd.
Yes freeing device resources is for detachable devices.
^ permalink raw reply
* Re: [PATCH] net/i40e: add additional prefetch instructions for bulk rx
From: Ananyev, Konstantin @ 2016-10-11 8:51 UTC (permalink / raw)
To: Vladyslav Buslov, Wu, Jingjing, Yigit, Ferruh, Zhang, Helin; +Cc: dev@dpdk.org
In-Reply-To: <MWHPR11MB1360602B685F5573E38429859DDB0@MWHPR11MB1360.namprd11.prod.outlook.com>
Hi Vladislav,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vladyslav Buslov
> Sent: Monday, October 10, 2016 6:06 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: add additional prefetch instructions for bulk rx
>
> > -----Original Message-----
> > From: Wu, Jingjing [mailto:jingjing.wu@intel.com]
> > Sent: Monday, October 10, 2016 4:26 PM
> > To: Yigit, Ferruh; Vladyslav Buslov; Zhang, Helin
> > Cc: dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] net/i40e: add additional prefetch
> > instructions for bulk rx
> >
> >
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh
> > > Sent: Wednesday, September 14, 2016 9:25 PM
> > > To: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>; Zhang, Helin
> > > <helin.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] net/i40e: add additional prefetch
> > > instructions for bulk rx
> > >
> > > On 7/14/2016 6:27 PM, Vladyslav Buslov wrote:
> > > > Added prefetch of first packet payload cacheline in
> > > > i40e_rx_scan_hw_ring Added prefetch of second mbuf cacheline in
> > > > i40e_rx_alloc_bufs
> > > >
> > > > Signed-off-by: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>
> > > > ---
> > > > drivers/net/i40e/i40e_rxtx.c | 7 +++++--
> > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > > b/drivers/net/i40e/i40e_rxtx.c index d3cfb98..e493fb4 100644
> > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > @@ -1003,6 +1003,7 @@ i40e_rx_scan_hw_ring(struct i40e_rx_queue
> > *rxq)
> > > > /* Translate descriptor info to mbuf parameters */
> > > > for (j = 0; j < nb_dd; j++) {
> > > > mb = rxep[j].mbuf;
> > > > + rte_prefetch0(RTE_PTR_ADD(mb->buf_addr,
> > > RTE_PKTMBUF_HEADROOM));
> >
> > Why did prefetch here? I think if application need to deal with packet, it is
> > more suitable to put it in application.
> >
> > > > qword1 = rte_le_to_cpu_64(\
> > > > rxdp[j].wb.qword1.status_error_len);
> > > > pkt_len = ((qword1 &
> > > I40E_RXD_QW1_LENGTH_PBUF_MASK) >>
> > > > @@ -1086,9 +1087,11 @@ i40e_rx_alloc_bufs(struct i40e_rx_queue
> > *rxq)
> > > >
> > > > rxdp = &rxq->rx_ring[alloc_idx];
> > > > for (i = 0; i < rxq->rx_free_thresh; i++) {
> > > > - if (likely(i < (rxq->rx_free_thresh - 1)))
> > > > + if (likely(i < (rxq->rx_free_thresh - 1))) {
> > > > /* Prefetch next mbuf */
> > > > - rte_prefetch0(rxep[i + 1].mbuf);
> > > > + rte_prefetch0(&rxep[i + 1].mbuf->cacheline0);
> > > > + rte_prefetch0(&rxep[i + 1].mbuf->cacheline1);
I think there are rte_mbuf_prefetch_part1/part2 defined in rte_mbuf.h,
specially for that case.
> > > > + }
> > Agree with this change. And when I test it by testpmd with iofwd, no
> > performance increase is observed but minor decrease.
> > Can you share will us when it will benefit the performance in your scenario ?
> >
> >
> > Thanks
> > Jingjing
>
> Hello Jingjing,
>
> Thanks for code review.
>
> My use case: We have simple distributor thread that receives packets from port and distributes them among worker threads according to
> VLAN and MAC address hash.
>
> While working on performance optimization we determined that most of CPU usage of this thread is in DPDK.
> As and optimization we decided to switch to rx burst alloc function, however that caused additional performance degradation compared to
> scatter rx mode.
> In profiler two major culprits were:
> 1. Access to packet data Eth header in application code. (cache miss)
> 2. Setting next packet descriptor field to NULL in DPDK i40e_rx_alloc_bufs code. (this field is in second descriptor cache line that was not
> prefetched)
I wonder what will happen if we'll remove any prefetches here?
Would it make things better or worse (and by how much)?
> After applying my fixes performance improved compared to scatter rx mode.
>
> I assumed that prefetch of first cache line of packet data belongs to DPDK because it is done in scatter rx mode. (in
> i40e_recv_scattered_pkts)
> It can be moved to application side but IMO it is better to be consistent across all rx modes.
I would agree with Jingjing here, probably PMD should avoid to prefetch packet's data.
Konstantin
^ permalink raw reply
* Re: DPDK patchwork upgrade
From: Maxim Uvarov @ 2016-10-11 8:42 UTC (permalink / raw)
To: Stephen Finucane; +Cc: Thomas Monjalon, De Lara Guarch, Pablo, dev@dpdk.org
In-Reply-To: <DB5PR0301MB2117F19266B6189D35E97756A3DB0@DB5PR0301MB2117.eurprd03.prod.outlook.com>
That is looks really good and should save a lot of time for maintainer
taking already validated patches. How hard is connection between patchwork,
mailing list tags and testing system? Is that all inside patchwork or you
wrote it specially for dpdk? I would really like to have the same thing
for my projects.
Thank you,
Maxim.
On 10 October 2016 at 21:09, Stephen Finucane <stephenfinucane@hotmail.com>
wrote:
> > 2016-09-07 16:40, De Lara Guarch, Pablo:
> > > I am looking at it, and I see two issues:
> > > - There are some patches that have been acked, but I don't see that in
> patchwork
> > > (e.g. http://dpdk.org/dev/patchwork/patch/15381/)
> >
> > Yes the new columns are empty.
> > I guess they will be filled starting from now.
> > Stephen, could we re-parse old emails (from August/September) to fill
> > A/R/T without breaking the database?
>
> You sure can - use the 'retag' command [1]:
>
> ./manage.py retag
>
> I should probably have included that in the upgrade notes...
>
> Stephen
>
> [1] https://github.com/getpatchwork/patchwork/blob/stable/1.1/patchwork/
> management/commands/retag.py
^ permalink raw reply
* Re: [RFC v2] Generic flow director/filtering/classification API
From: Adrien Mazarguil @ 2016-10-11 8:21 UTC (permalink / raw)
To: Zhao1, Wei; +Cc: dev@dpdk.org
In-Reply-To: <A2573D2ACFCADC41BB3BE09C6DE313CA01FDBE41@PGSMSX103.gar.corp.intel.com>
Hi Wei,
On Tue, Oct 11, 2016 at 01:47:53AM +0000, Zhao1, Wei wrote:
> Hi Adrien Mazarguil,
> There is a struct rte_flow_action_rss in rte_flow.txt, the member rss_conf is a pointer type, is there any convenience in using pointer?
> Why not using struct rte_eth_rss_conf rss_conf type, as rte_flow_item_ipv4/ rte_flow_item_ipv6 struct member?
>
> Thank you.
>
> struct rte_flow_action_rss {
> struct rte_eth_rss_conf *rss_conf; /**< RSS parameters. */
> uint16_t queues; /**< Number of entries in queue[]. */
> uint16_t queue[]; /**< Queues indices to use. */
> };
Well I thought it made sharing flow RSS configuration with its counterpart
in struct rte_eth_conf easier (this pointer should even be const). Also,
while ABI breakage would still occur if rte_eth_rss_conf happened to be
modified, the impact on this API would be limited as it would not cause a
change in structure size. We'd ideally need some kind of version field to be
completely safe but I guess that would be somewhat overkill.
Now considering this API was written without an initial implementation, all
structure definitions that do not make sense are still open to debate, we
can adjust them as needed.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH v7] net/virtio: add set_mtu in virtio
From: Kavanagh, Mark B @ 2016-10-11 8:12 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Dey, stephen@networkplumber.org, dev@dpdk.org
In-Reply-To: <20161011040119.GJ1597@yliu-dev.sh.intel.com>
>-----Original Message-----
>From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
>Sent: Tuesday, October 11, 2016 5:01 AM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>
>Cc: Dey <sodey@sonusnet.com>; stephen@networkplumber.org; dev@dpdk.org
>Subject: Re: [PATCH v7] net/virtio: add set_mtu in virtio
>
>On Mon, Oct 10, 2016 at 10:49:22AM +0000, Kavanagh, Mark B wrote:
>> >
>> >On Thu, Sep 29, 2016 at 04:31:30PM -0400, Dey wrote:
>> >> /*
>> >> * dev_ops for virtio, bare necessities for basic operation
>> >> */
>> >> @@ -677,7 +685,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
>> >> .allmulticast_enable = virtio_dev_allmulticast_enable,
>> >> .allmulticast_disable = virtio_dev_allmulticast_disable,
>> >> + .mtu_set = virtio_mtu_set,
>> >> .dev_infos_get = virtio_dev_info_get,
>> >> .stats_get = virtio_dev_stats_get,
>> >> .xstats_get = virtio_dev_xstats_get,
>> >> --
>> >> 2.9.3.windows.1
>> >
>> >Your patch is malformed: I got an error while trying to apply it.
>> >
>> > patch: **** malformed patch at line 167: * dev_ops for virtio,
>> > bare necessities for basic operation
>> >
>> >Seems like the way you were generating the patch is wrong.
>> >
>> >Anyway, I applied it manually, with the "- frame_size" fix as well
>> >as few more minor coding style fixes.
>> >
>> >So applied to dpdk-next-virtio.
>>
>> Hi Yuanhan/Souvik,
>>
>> Given my contributions to this patch (and in particular since comments from here -
>http://dpdk.org/ml/archives/dev/2016-September/047208.html - were copied directly into the
>commit message), I think that I should have been added as co-author of the patch?
>
>Mark,
>
>I appreciate your contributions here. But for this case, I think it
>might not be enough for adding you as the co-author: you don't co-write
>the code with Souvik after all.
>
>However, I'd suggest you to add your "Reviewed-by" if a patch looks to
>you after your review effort. This is another way to recognize your
>contributions to a patch.
No problem Yuanhan - thanks for clarifying.
Reviewed-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>
>Thanks.
>
> --yliu
>
>
>>
>> Let me know if you think that I am mistaken.
>>
>> Thanks in advance,
>> Mark
>> >
>> > --yliu
^ permalink raw reply
* Re: [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init
From: Thomas Monjalon @ 2016-10-11 8:08 UTC (permalink / raw)
To: John Ousterhout; +Cc: dev
In-Reply-To: <20161010223933.5924-1-ouster@cs.stanford.edu>
2016-10-10 15:39, John Ousterhout:
> Before this patch, application-specific loggers could not be
> installed before rte_eal_init completed (the initialization process
> called rte_openlog_stream, overwriting any previously installed
> logger). This made it impossible for an application to capture the
> initial log messages generated during rte_eal_init. This patch changes
> initialization so that information from a previous call to
> rte_openlog_stream is not lost. Specifically:
> * The default log stream is now maintained separately from an
> application-specific log stream installed with rte_openlog_stream.
> * rte_eal_common_log_init has been renamed to rte_eal_log_set_default,
> since this is all it does. It no longer invokes rte_openlog_stream; it
> just updates the default stream. Also, this method now returns void,
> rather than int, since there are no errors.
> * The "early log" mechanism (e.g. rte_eal_log_early_init) has been
> removed; all of the desired functionality can be achieved by calling
> rte_eal_log_set_default.
>
> Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
> ----
> v2:
> * Removed the early log mechanism, renamed rte_eal_common_log_init.
>
> Note: I see from the code that Linux and BSD set different default streams:
> Linux uses stdout, while BSD uses stderr. This patch retains the distinction,
> though I'm not sure why it is there.
I don't know either.
What is best between stdout and stderr for logs?
[...]
> -int
> -rte_eal_log_early_init(void)
> -{
> - rte_openlog_stream(stderr);
> - return 0;
> + rte_eal_set_default(stderr);
It should be rte_eal_log_set_default.
[...]
> /*
> - * called by environment-specific log init function
> + * Called by environment-specific initialization functions.
> */
> -int
> -rte_eal_common_log_init(FILE *default_log)
> +void
> +rte_eal_log_set_default(FILE *default_log)
> {
> default_log_stream = default_log;
> - rte_openlog_stream(default_log);
>
> #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
> RTE_LOG(NOTICE, EAL, "Debug logs available - lower performance\n");
> #endif
> -
> - return 0;
> }
Do we really need a function for that?
Why not just initialize default_log_stream statically?
[...]
> /**
> - * Common log initialization function (private to eal).
> + * Common log initialization function (private to eal). Determines
> + * where log data is written when no call to eal_openlog_stream is
> + * in effect.
It should be rte_openlog_stream.
^ permalink raw reply
* [PATCH v2] vhost: Only access header if offloading is supported in dequeue path
From: Maxime Coquelin @ 2016-10-11 7:45 UTC (permalink / raw)
To: yuanhan.liu, dev
Cc: mst, jianfeng.tan, olivier.matz, stephen, Maxime Coquelin
In-Reply-To: <1475773241-5714-1-git-send-email-maxime.coquelin@redhat.com>
If offloading features are not negotiated, parsing the virtio header
is not needed.
Micro-benchmark with testpmd shows that the gain is +4% with indirect
descriptors, +1% when using direct descriptors.
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
Changes since v1:
=================
- Rebased
- Fix early out check in vhost_dequeue_offload
lib/librte_vhost/virtio_net.c | 47 ++++++++++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index a59c39b..5e2fd75 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -548,6 +548,18 @@ rte_vhost_enqueue_burst(int vid, uint16_t queue_id,
return virtio_dev_rx(dev, queue_id, pkts, count);
}
+static inline bool
+virtio_net_with_host_offload(struct virtio_net *dev)
+{
+ if (dev->features &
+ (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
+ VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 |
+ VIRTIO_NET_F_HOST_UFO))
+ return true;
+
+ return false;
+}
+
static void
parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
{
@@ -600,6 +612,9 @@ vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
void *l4_hdr = NULL;
struct tcp_hdr *tcp_hdr = NULL;
+ if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
+ return;
+
parse_ethernet(m, &l4_proto, &l4_hdr);
if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
if (hdr->csum_start == (m->l2_len + m->l3_len)) {
@@ -684,12 +699,12 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
struct rte_mempool *mbuf_pool)
{
struct vring_desc *desc;
- uint64_t desc_addr;
+ uint64_t desc_addr = 0;
uint32_t desc_avail, desc_offset;
uint32_t mbuf_avail, mbuf_offset;
uint32_t cpy_len;
struct rte_mbuf *cur = m, *prev = m;
- struct virtio_net_hdr *hdr;
+ struct virtio_net_hdr *hdr = NULL;
/* A counter to avoid desc dead loop chain */
uint32_t nr_desc = 1;
@@ -698,12 +713,14 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
(desc->flags & VRING_DESC_F_INDIRECT))
return -1;
- desc_addr = gpa_to_vva(dev, desc->addr);
- if (unlikely(!desc_addr))
- return -1;
+ if (virtio_net_with_host_offload(dev)) {
+ desc_addr = gpa_to_vva(dev, desc->addr);
+ if (unlikely(!desc_addr))
+ return -1;
- hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr);
- rte_prefetch0(hdr);
+ hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr);
+ rte_prefetch0(hdr);
+ }
/*
* A virtio driver normally uses at least 2 desc buffers
@@ -720,18 +737,24 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
if (unlikely(!desc_addr))
return -1;
- rte_prefetch0((void *)(uintptr_t)desc_addr);
-
desc_offset = 0;
desc_avail = desc->len;
nr_desc += 1;
-
- PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);
} else {
+ if (!desc_addr) {
+ desc_addr = gpa_to_vva(dev, desc->addr);
+ if (unlikely(!desc_addr))
+ return -1;
+ }
+
desc_avail = desc->len - dev->vhost_hlen;
desc_offset = dev->vhost_hlen;
}
+ rte_prefetch0((void *)(uintptr_t)(desc_addr + desc_offset));
+
+ PRINT_PACKET(dev, (uintptr_t)(desc_addr + desc_offset), desc_avail, 0);
+
mbuf_offset = 0;
mbuf_avail = m->buf_len - RTE_PKTMBUF_HEADROOM;
while (1) {
@@ -795,7 +818,7 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
prev->data_len = mbuf_offset;
m->pkt_len += mbuf_offset;
- if (hdr->flags != 0 || hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE)
+ if (virtio_net_with_host_offload(dev))
vhost_dequeue_offload(hdr, m);
return 0;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
From: Thomas Monjalon @ 2016-10-11 7:27 UTC (permalink / raw)
To: Shreyansh Jain; +Cc: Neil Horman, david.marchand@6wind.com, dev
In-Reply-To: <cb6fab09-018d-aaf1-8b61-253263bed801@nxp.com>
2016-10-11 12:06, Shreyansh Jain:
> On Monday 10 October 2016 06:26 PM, Neil Horman wrote:
> > On Sat, Oct 08, 2016 at 01:00:59PM +0000, Shreyansh Jain wrote:
> >> Hi Thomas,
> >>
> >>> -----Original Message-----
> >>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >>> Sent: Friday, October 07, 2016 7:22 PM
> >>> To: Shreyansh Jain <shreyansh.jain@nxp.com>
> >>> Cc: david.marchand@6wind.com; dev@dpdk.org
> >>> Subject: Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
> >>>
> >>> 2016-10-07 19:11, Shreyansh Jain:
> >>>> --- a/mk/internal/rte.compile-pre.mk
> >>>> +++ b/mk/internal/rte.compile-pre.mk
> >>>> @@ -87,7 +87,7 @@ endif
> >>>> PMDINFO_GEN = $(RTE_SDK_BIN)/app/dpdk-pmdinfogen $@ $@.pmd.c
> >>>> PMDINFO_CC = $(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@.pmd.o $@.pmd.c
> >>>> PMDINFO_LD = $(CROSS)ld $(LDFLAGS) -r -o $@.o $@.pmd.o $@
> >>>> -PMDINFO_TO_O = if grep -q 'DRIVER_REGISTER_.*(.*)' $<; then \
> >>>> +PMDINFO_TO_O = if grep 'EAL_REGISTER_.*(.*)' $<; then \
> >>>> echo "$(if $V,$(PMDINFO_GEN), PMDINFO $@.pmd.c)" && \
> >>>> $(PMDINFO_GEN) && \
> >>>> echo "$(if $V,$(PMDINFO_CC), CC $@.pmd.o)" && \
> >>>>
> >>>> --->8---
> >>>> CC eal_pci_vfio.o
> >>>> PMDINFO eal_pci_vfio.o.pmd.c
> >>>> /bin/sh: 1:
> >>>> /home/shreyansh/build/DPDK/02_dpdk/x86_64-native-linuxapp-gcc/app/dpdk-
> >>> pmdinfogen:
> >>>> not found
> >>>> /home/shreyansh/build/DPDK/02_dpdk/mk/internal/rte.compile-pre.mk:138:
> >>>> recipe for target 'eal_pci_vfio.o' failed
> >>>> --->8---
> >>>>
> >>>> I don't think PMDINFO should be running on eal_pci_vfio file. Isn't it?
> >>>
> >>> Every files are scanned for the pattern.
> >>
> >> Sorry, I should have been clearer in my statement.
> >> I meant, I didn't think eal_pci_vfio.o had anything of interest for the PMD tool and hence the mk files would have skipped over it in absence of a match.
> >> I understand that PMDINFO would run on all files.
> >>
> > Thats incorrect, the Makefile does a REGEX search for appropriate registration
> > macros that imply the need for pmdinfo to run. If its running on an
> > inappropriate file its because your new macros inadvertently match the current
> > regex, hence my suggestion that the regex should be tuned to be more specific
>
> Agree. Thats is what I wanted to clarify as stated below:
> "...EAL_REGISTER_* (macro name has changed since) is matching
> EAL_REGISTER_TAILQ..".
>
> As for 'more specific' match - I did suggest [2] a longer more specific
> version but Thomas had a different view point [1]. You can have a look
> at [2] and let me know your suggestion or if that is wrong.
>
> [1] http://dpdk.org/ml/archives/dev/2016-October/048425.html
> [2] http://dpdk.org/ml/archives/dev/2016-October/048407.html
I do not have a different point of view :)
We need to be more specific than EAL_REGISTER_*.
And RTE_PMD_REGISTER_* is specific enough. That's all :)
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature
From: Yuanhan Liu @ 2016-10-11 6:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Wang, Zhihong, Maxime Coquelin, Stephen Hemminger, dev@dpdk.org,
qemu-devel@nongnu.org
In-Reply-To: <20161010073721-mutt-send-email-mst@kernel.org>
On Mon, Oct 10, 2016 at 07:39:59AM +0300, Michael S. Tsirkin wrote:
> > > > > > 1. why is this memset expensive?
> > > > >
> > > > > I don't have the exact answer, but just some rough thoughts:
> > > > >
> > > > > It's an external clib function: there is a call stack and the
> > > > > IP register will bounch back and forth.
> > > >
> > > > for memset 0? gcc 5.3.1 on fedora happily inlines it.
> > >
> > > Good to know!
> > >
> > > > > overkill to use that for resetting 14 bytes structure.
> > > > >
> > > > > Some trick like
> > > > > *(struct virtio_net_hdr *)hdr = {0, };
> > > > >
> > > > > Or even
> > > > > hdr->xxx = 0;
> > > > > hdr->yyy = 0;
> > > > >
> > > > > should behaviour better.
> > > > >
> > > > > There was an example: the vhost enqueue optmization patchset from
> > > > > Zhihong [0] uses memset, and it introduces more than 15% drop (IIRC)
> > > > > on my Ivybridge server: it has no such issue on his server though.
> > > > >
> > > > > [0]: http://dpdk.org/ml/archives/dev/2016-August/045272.html
> > > > >
> > > > > --yliu
> > > >
> > > > I'd say that's weird. what's your config? any chance you
> > > > are using an old compiler?
> > >
> > > Not really, it's gcc 5.3.1. Maybe Zhihong could explain more. IIRC,
> > > he said the memset is not well optimized for Ivybridge server.
> >
> > The dst is remote in that case. It's fine on Haswell but has complication
> > in Ivy Bridge which (wasn't supposed to but) causes serious frontend issue.
> >
> > I don't think gcc inlined it there. I'm using fc24 gcc 6.1.1.
>
>
> So try something like this then:
Yes, I saw memset is inlined when this diff is applied.
So, mind to send a formal patch? You might want to try build at least:
it doesn't build.
> Generally pointer chasing in vq->hw->vtnet_hdr_size can't be good
> for performance. Move fields used on data path into vq
> and use from there to avoid indirections?
Good suggestion!
--yliu
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature
From: Yuanhan Liu @ 2016-10-11 6:49 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: Michael S. Tsirkin, Stephen Hemminger, dev, qemu-devel
In-Reply-To: <c82dd14f-9cc9-26b5-6463-059becc16a98@redhat.com>
On Tue, Oct 11, 2016 at 08:39:54AM +0200, Maxime Coquelin wrote:
>
>
> On 10/11/2016 08:04 AM, Yuanhan Liu wrote:
> >On Mon, Oct 10, 2016 at 04:54:39PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >>On 10/10/2016 04:42 PM, Yuanhan Liu wrote:
> >>>On Mon, Oct 10, 2016 at 02:40:44PM +0200, Maxime Coquelin wrote:
> >>>>>>>At that time, a packet always use 2 descs. Since indirect desc is
> >>>>>>>enabled (by default) now, the assumption is not true then. What's
> >>>>>>>worse, it might even slow things a bit down. That should also be
> >>>>>>>part of the reason why performance is slightly worse than before.
> >>>>>>>
> >>>>>>> --yliu
> >>>>>>
> >>>>>>I'm not sure I get what you are saying
> >>>>>>
> >>>>>>>commit 1d41d77cf81c448c1b09e1e859bfd300e2054a98
> >>>>>>>Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>>>>Date: Mon May 2 17:46:17 2016 -0700
> >>>>>>>
> >>>>>>> vhost: optimize dequeue for small packets
> >>>>>>>
> >>>>>>> A virtio driver normally uses at least 2 desc buffers for Tx: the
> >>>>>>> first for storing the header, and the others for storing the data.
> >>>>>>>
> >>>>>>> Therefore, we could fetch the first data desc buf before the main
> >>>>>>> loop, and do the copy first before the check of "are we done yet?".
> >>>>>>> This could save one check for small packets that just have one data
> >>>>>>> desc buffer and need one mbuf to store it.
> >>>>>>>
> >>>>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>>>> Acked-by: Huawei Xie <huawei.xie@intel.com>
> >>>>>>> Tested-by: Rich Lane <rich.lane@bigswitch.com>
> >>>>>>
> >>>>>>This fast-paths the 2-descriptors format but it's not active
> >>>>>>for indirect descriptors. Is this what you mean?
> >>>>>
> >>>>>Yes. It's also not active when ANY_LAYOUT is actually turned on.
> >>>>>>Should be a simple matter to apply this optimization for indirect.
> >>>>>
> >>>>>Might be.
> >>>>
> >>>>If I understand the code correctly, indirect descs also benefit from this
> >>>>optimization, or am I missing something?
> >>>
> >>>Aha..., you are right!
> >>
> >>The interesting thing is that the patch I send on Thursday that removes
> >>header access when no offload has been negotiated[0] seems to reduce
> >>almost to zero the performance seen with indirect descriptors enabled.
> >
> >Didn't follow that.
> >
> >>I see this with 64 bytes packets using testpmd on both ends.
> >>
> >>When I did the patch, I would have expected the same gain with both
> >>modes, whereas I measured +1% for direct and +4% for indirect.
> >
> >IIRC, I did a test before (remove those offload code piece), and the
> >performance was basically the same before and after that. Well, there
> >might be some small difference, say 1% as you said. But the result has
> >never been steady.
> >
> >Anyway, I think your patch is good to have: I just didn't see v2.
>
> I waited to gather some comments/feedback before sending the v2.
> I'll send it today or tomorrow.
Interesting, I saw a deadlock then: I haven't looked at the code
carefully once you said there is a v2, thus I'm waiting for it.
However, you are waitting for my review. :)
Anyway, I will take time to look at it shortly.
--yliu
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature
From: Maxime Coquelin @ 2016-10-11 6:39 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Michael S. Tsirkin, Stephen Hemminger, dev, qemu-devel
In-Reply-To: <20161011060456.GM1597@yliu-dev.sh.intel.com>
On 10/11/2016 08:04 AM, Yuanhan Liu wrote:
> On Mon, Oct 10, 2016 at 04:54:39PM +0200, Maxime Coquelin wrote:
>>
>>
>> On 10/10/2016 04:42 PM, Yuanhan Liu wrote:
>>> On Mon, Oct 10, 2016 at 02:40:44PM +0200, Maxime Coquelin wrote:
>>>>>>> At that time, a packet always use 2 descs. Since indirect desc is
>>>>>>> enabled (by default) now, the assumption is not true then. What's
>>>>>>> worse, it might even slow things a bit down. That should also be
>>>>>>> part of the reason why performance is slightly worse than before.
>>>>>>>
>>>>>>> --yliu
>>>>>>
>>>>>> I'm not sure I get what you are saying
>>>>>>
>>>>>>> commit 1d41d77cf81c448c1b09e1e859bfd300e2054a98
>>>>>>> Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>>>> Date: Mon May 2 17:46:17 2016 -0700
>>>>>>>
>>>>>>> vhost: optimize dequeue for small packets
>>>>>>>
>>>>>>> A virtio driver normally uses at least 2 desc buffers for Tx: the
>>>>>>> first for storing the header, and the others for storing the data.
>>>>>>>
>>>>>>> Therefore, we could fetch the first data desc buf before the main
>>>>>>> loop, and do the copy first before the check of "are we done yet?".
>>>>>>> This could save one check for small packets that just have one data
>>>>>>> desc buffer and need one mbuf to store it.
>>>>>>>
>>>>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>>>> Acked-by: Huawei Xie <huawei.xie@intel.com>
>>>>>>> Tested-by: Rich Lane <rich.lane@bigswitch.com>
>>>>>>
>>>>>> This fast-paths the 2-descriptors format but it's not active
>>>>>> for indirect descriptors. Is this what you mean?
>>>>>
>>>>> Yes. It's also not active when ANY_LAYOUT is actually turned on.
>>>>>> Should be a simple matter to apply this optimization for indirect.
>>>>>
>>>>> Might be.
>>>>
>>>> If I understand the code correctly, indirect descs also benefit from this
>>>> optimization, or am I missing something?
>>>
>>> Aha..., you are right!
>>
>> The interesting thing is that the patch I send on Thursday that removes
>> header access when no offload has been negotiated[0] seems to reduce
>> almost to zero the performance seen with indirect descriptors enabled.
>
> Didn't follow that.
>
>> I see this with 64 bytes packets using testpmd on both ends.
>>
>> When I did the patch, I would have expected the same gain with both
>> modes, whereas I measured +1% for direct and +4% for indirect.
>
> IIRC, I did a test before (remove those offload code piece), and the
> performance was basically the same before and after that. Well, there
> might be some small difference, say 1% as you said. But the result has
> never been steady.
>
> Anyway, I think your patch is good to have: I just didn't see v2.
I waited to gather some comments/feedback before sending the v2.
I'll send it today or tomorrow.
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
From: Shreyansh Jain @ 2016-10-11 6:36 UTC (permalink / raw)
To: Neil Horman; +Cc: Thomas Monjalon, david.marchand@6wind.com, dev@dpdk.org
In-Reply-To: <20161010125624.GA19423@hmsreliant.think-freely.org>
On Monday 10 October 2016 06:26 PM, Neil Horman wrote:
> On Sat, Oct 08, 2016 at 01:00:59PM +0000, Shreyansh Jain wrote:
>> Hi Thomas,
>>
>>> -----Original Message-----
>>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>>> Sent: Friday, October 07, 2016 7:22 PM
>>> To: Shreyansh Jain <shreyansh.jain@nxp.com>
>>> Cc: david.marchand@6wind.com; dev@dpdk.org
>>> Subject: Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
>>>
>>> 2016-10-07 19:11, Shreyansh Jain:
>>>> --- a/mk/internal/rte.compile-pre.mk
>>>> +++ b/mk/internal/rte.compile-pre.mk
>>>> @@ -87,7 +87,7 @@ endif
>>>> PMDINFO_GEN = $(RTE_SDK_BIN)/app/dpdk-pmdinfogen $@ $@.pmd.c
>>>> PMDINFO_CC = $(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@.pmd.o $@.pmd.c
>>>> PMDINFO_LD = $(CROSS)ld $(LDFLAGS) -r -o $@.o $@.pmd.o $@
>>>> -PMDINFO_TO_O = if grep -q 'DRIVER_REGISTER_.*(.*)' $<; then \
>>>> +PMDINFO_TO_O = if grep 'EAL_REGISTER_.*(.*)' $<; then \
>>>> echo "$(if $V,$(PMDINFO_GEN), PMDINFO $@.pmd.c)" && \
>>>> $(PMDINFO_GEN) && \
>>>> echo "$(if $V,$(PMDINFO_CC), CC $@.pmd.o)" && \
>>>>
>>>> --->8---
>>>> CC eal_pci_vfio.o
>>>> PMDINFO eal_pci_vfio.o.pmd.c
>>>> /bin/sh: 1:
>>>> /home/shreyansh/build/DPDK/02_dpdk/x86_64-native-linuxapp-gcc/app/dpdk-
>>> pmdinfogen:
>>>> not found
>>>> /home/shreyansh/build/DPDK/02_dpdk/mk/internal/rte.compile-pre.mk:138:
>>>> recipe for target 'eal_pci_vfio.o' failed
>>>> --->8---
>>>>
>>>> I don't think PMDINFO should be running on eal_pci_vfio file. Isn't it?
>>>
>>> Every files are scanned for the pattern.
>>
>> Sorry, I should have been clearer in my statement.
>> I meant, I didn't think eal_pci_vfio.o had anything of interest for the PMD tool and hence the mk files would have skipped over it in absence of a match.
>> I understand that PMDINFO would run on all files.
>>
> Thats incorrect, the Makefile does a REGEX search for appropriate registration
> macros that imply the need for pmdinfo to run. If its running on an
> inappropriate file its because your new macros inadvertently match the current
> regex, hence my suggestion that the regex should be tuned to be more specific
Agree. Thats is what I wanted to clarify as stated below:
"...EAL_REGISTER_* (macro name has changed since) is matching
EAL_REGISTER_TAILQ..".
As for 'more specific' match - I did suggest [2] a longer more specific
version but Thomas had a different view point [1]. You can have a look
at [2] and let me know your suggestion or if that is wrong.
[1] http://dpdk.org/ml/archives/dev/2016-October/048425.html
[2] http://dpdk.org/ml/archives/dev/2016-October/048407.html
>
> Neil
>
>>>
>>>> Is it because EAL_REGISTER_* is matching EAL_REGISTER_TAILQ like macro
>>>> as well?
>>>
>>> Probably.
>>> That's another argument in favor of good prefixes.
>>> I think you should use RTE_DRIVER_REGISTER_ or better, RTE_PMD_REGISTER_
>>
>> I thought of EAL_PMD_REGISTER_* but dropped it because for PCI_TABLE like macros, the name got really long (EAL_PMD_REGISTER_PCI_TABLE()).
>> Anyways, I will use RTE_PMD_REGISTER_* now and send another version.
>>
>>>>
>>>> I am not very well versed with how PMDINFO is linking with the build
>>>> system so any hints/insight into this would be really helpful.
>>>>
>>>> One I get more clarity on this, I will push a new version of this patch.
>>>
>>> Thanks
>>
>> -
>> Shreyansh
>>
>
-
Shreyansh
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature
From: Yuanhan Liu @ 2016-10-11 6:04 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: Michael S. Tsirkin, Stephen Hemminger, dev, qemu-devel
In-Reply-To: <18372cc2-19d3-f455-728d-2f2ed405d800@redhat.com>
On Mon, Oct 10, 2016 at 04:54:39PM +0200, Maxime Coquelin wrote:
>
>
> On 10/10/2016 04:42 PM, Yuanhan Liu wrote:
> >On Mon, Oct 10, 2016 at 02:40:44PM +0200, Maxime Coquelin wrote:
> >>>>>At that time, a packet always use 2 descs. Since indirect desc is
> >>>>>enabled (by default) now, the assumption is not true then. What's
> >>>>>worse, it might even slow things a bit down. That should also be
> >>>>>part of the reason why performance is slightly worse than before.
> >>>>>
> >>>>> --yliu
> >>>>
> >>>>I'm not sure I get what you are saying
> >>>>
> >>>>>commit 1d41d77cf81c448c1b09e1e859bfd300e2054a98
> >>>>>Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>>Date: Mon May 2 17:46:17 2016 -0700
> >>>>>
> >>>>> vhost: optimize dequeue for small packets
> >>>>>
> >>>>> A virtio driver normally uses at least 2 desc buffers for Tx: the
> >>>>> first for storing the header, and the others for storing the data.
> >>>>>
> >>>>> Therefore, we could fetch the first data desc buf before the main
> >>>>> loop, and do the copy first before the check of "are we done yet?".
> >>>>> This could save one check for small packets that just have one data
> >>>>> desc buffer and need one mbuf to store it.
> >>>>>
> >>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>> Acked-by: Huawei Xie <huawei.xie@intel.com>
> >>>>> Tested-by: Rich Lane <rich.lane@bigswitch.com>
> >>>>
> >>>>This fast-paths the 2-descriptors format but it's not active
> >>>>for indirect descriptors. Is this what you mean?
> >>>
> >>>Yes. It's also not active when ANY_LAYOUT is actually turned on.
> >>>>Should be a simple matter to apply this optimization for indirect.
> >>>
> >>>Might be.
> >>
> >>If I understand the code correctly, indirect descs also benefit from this
> >>optimization, or am I missing something?
> >
> >Aha..., you are right!
>
> The interesting thing is that the patch I send on Thursday that removes
> header access when no offload has been negotiated[0] seems to reduce
> almost to zero the performance seen with indirect descriptors enabled.
Didn't follow that.
> I see this with 64 bytes packets using testpmd on both ends.
>
> When I did the patch, I would have expected the same gain with both
> modes, whereas I measured +1% for direct and +4% for indirect.
IIRC, I did a test before (remove those offload code piece), and the
performance was basically the same before and after that. Well, there
might be some small difference, say 1% as you said. But the result has
never been steady.
Anyway, I think your patch is good to have: I just didn't see v2.
--yliu
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox