DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v5 3/8] ethdev: reserve capability flags for PMD-specific API
From: Adrien Mazarguil @ 2017-01-09 14:41 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Bie, Tiwei, dev@dpdk.org, Lu, Wenzhuo, Mcnamara, John,
	olivier.matz@6wind.com, thomas.monjalon@6wind.com, Zhang, Helin,
	Dai, Wei, Wang, Xiao W
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F10241C@irsmsx105.ger.corp.intel.com>

Hi Konstantin,

On Sun, Jan 08, 2017 at 12:39:55PM +0000, Ananyev, Konstantin wrote:
> 
> Hi Adrien,
> 
> > 
> > Hi Konstantin,
> > 
> > On Thu, Jan 05, 2017 at 11:32:38AM +0000, Ananyev, Konstantin wrote:
> > > Hi Adrien,
[...]
> > > > PMD-specific symbols have nothing to do in the global namespace in my
> > > > opinion, they are not versioned and may evolve without notice. Neither
> > > > applications nor the bonding PMD can rely on them. That's the trade-off.
> > >
> > > Not sure I do understand your reasoning.
> > > For me MACSEC offload is just one of many HW offloads that we support
> > > and should be treated in exactly the same way.
> > > Applications should be able to use it in a transparent and reliable way,
> > > not only under some limited conditions.
> > > Otherwise what is the point to introduce it at all?
> > 
> > Well my first reply to this thread was asking why isn't the whole API global
> > from the start then?
> 
> That's good question, and my preference would always be to have the
> API to configure this feature as generic one.
> I guess the main reason why it is not right now we don't reach an agreement
> how this API should look like: 
> http://dpdk.org/ml/archives/dev/2016-September/047810.html
> But I'll leave it to the author to provide the real reason here. 
> 
> > Given there are valid reasons for it not to and no plan to make it so in the
> > near future, applications must be aware that they are including
> > rte_pmd_ixgbe.h to use it. That in itself is a limiting condition, right?
> 
> Yes, it is definitely a limiting factor.
> Though even if API to configure device to use macsec would be PMD specific right now,
> The API to query that capability and the API to use it at datapath (mbuf.ol_flags) still
> can be (and I think should be) device independent and transparent to use.  

With RESERVED flags, what is global and transparent is the fact the mbuf or
device features in question are PMD-specific and some extra knowledge about
the underlying port is necessary to handle them. I think that could be
useful to applications in order to get the necessary precautions.

> > > Yes, right now it is supported only by ixgbe PMD, but why that should be the
> > > reason to treat is as second-class citizen?
> > > Let say PKT_TX_TUNNEL_* offloads also are supported only by one PMD right now.
> > 
> > You are right about PKT_TX_TUNNEL_*, however these flags exist on their own
> > and are not tied to any API function calls, unlike in this series where
> > PKT_TX_MACSEC can only be used if the DEV_TX_OFFLOAD_MACSEC_INSERT
> > capability is present 
> 
> I don't think PKT_TX_TUNNEL_* 'exists on its own'.
> To use it well behaving app have to:
> 1) Query that device does provide that capability: DEV_TX_OFFLOAD_*_TNL_TSO
> 2) configure PMD( & device) to use that capability
> 3) use that offload at run-time TX code (mb->ol_flags |= ...; mb->tx_offload = ...)
> 
> For PKT_TX_TUNNEL_*  2) is pretty simple - user just need to make sure
> that full-featured TX function will be selected:
> txconf.txq_flags = 0; ...;  rte_eth_tx_queue_setup(..., &txconf);

Pretty much like any other offloads. Any PMD could implement them as well
without exposing additional callbacks.

> For TX_MACSEC, as I understand 2) will be more complicated and
> right now is PMD specific, but anyway the main pattern remains the same.
> So at least 1) and 3) could be kept device neutral.

Yes, however this discussion is precisely because I think it could be a
problem that this API "right now is PMD specific", particularly because it
will remain that way.

> >and the whole thing was configured through
> > rte_pmd_ixgbe_macsec_*() calls after including rte_pmd_ixgbe.h.
> > 
> > To be clear it is not about MACsec per se (as a standardized protocol, I
> > think related definitions for offloads have their place), but it has to do
> > with the fact that the rest of the API is PMD-specific and there is a
> > dependency between them.
> > 
> > > > Therefore until APIs are made global, the safe compromise is to define
> > > > neutral, reserved symbols that any PMD can use to implement their own
> > > > temporary APIs for testing purposes. These can be renamed later without
> > > > changing their value as long as a single PMD uses them.
> > >
> > > Ok, so what we'll gain by introducing PKT_TX_RESERVED instead of PKT_TX_MACSEC?
> > > As I said in my previous mail the redefinition for the same ol_flag bit (and dev capabilities)
> > > by different PMD might create a lot of confusion in future.
> > > Does the potential saving of 1 bit really worth it?
> > 
> > That is one benefit, but my point is mainly to keep applications aware that
> > they are using an API defined by a single PMD, which may be temporary and
> > whose symbols are not versioned.
> 
> As applications have to use PMD specific functions to configure it they definitely are aware.

Therefore they also know if they are only running on top of ixgbe adapters
of a mix with something else. By choosing the PMD-specific path, they know
what they are getting into.

> > Consider this:
> > 
> > rte_mbuf.h:
> > 
> >  #define PKT_TX_RESERVED_0 (1 << 42)
> > 
> > rte_pmd_ixgbe.h:
> > 
> >  #define PKT_TX_MACSEC PKT_TX_RESERVED_0
> > 
> > That way, applications have to get the PKT_TX_MACSEC definition where the
> > rest of the API is also defined.
> > 
> > Other PMDs may reuse PKT_TX_RESERVED_0 and other reserved flags to implement
> > their own experimental APIs.
> 
> That's the main thing I am opposed to.
> I think that by allowing PMD to redefine meaning of
> mbuf.ol_flags and dev_info.(rx| tx)_offload_capa 
> we just asking for trouble.
> 
> Let say tomorrow, i40e will redefine DEV_TX_OFFLOAD_RESERVED_0 and PKT_TX_RESERVED_0
> to implement new specific TX offload (PKT_TX_FEATURE_X).
> Now let say we have an application that works over both ixgbe and i40e
> and would like to use both TX_MACSEC and TC_FEATURE_X offloads whenever they are available.
> As I can see, with the approach you proposed the only way for the application to make it
> is to support 2 different TX code paths (or at least some parts of it).
> To me that way looks inconvenient to the users and source of future troubles.

Remember applications still have to clear PKT_TX_MACSEC and other flags on
ports that do not implement them, just like when the related offload is not
configured even if supported. Whichever approach is taken, applications have
to be careful and need a few extra checks in their TX path. There is no
extra cost involved compared to existing offloads.

> Same for RX:  somewhere at upper layer user got a packet with PKT_RX_RESERVED_0 set.
> What does it really mean if there are different NIC types in the system?

For RX, I agree things are more complicated, applications would have to know
what a given flag means from a particular port. We could define several
RESERVED_X flags that do not overlap on a case basis, so at least
applications know they are dealing with PMD-specific flags.

> > Applications and the bonding PMD can easily be made aware that such reserved
> > flags cannot be shared between ports unless they know what the underlying
> > PMD is, which is already a requirement to use this API in the first place
> > (for instance, calling rte_pmd_ixgbe_macsec_*() functions with another
> > vendor's port_id may crash the application).
> 
> I am talking about that code:
> rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
> {
> ...
> /* Take the first dev's offload capabilities */
> internals->rx_offload_capa = dev_info.rx_offload_capa;
> internals->tx_offload_capa = dev_info.tx_offload_capa;
> ...
> internals->rx_offload_capa &= dev_info.rx_offload_capa;
> internals->tx_offload_capa &= dev_info.tx_offload_capa;
> 
> Obviously with what you are suggesting it is not valid any more.
> Bonded device need to support a MASK of all device reserved offloads to exclude
> them from common subset. 
> Any user app(/lib) that does similar thing would also have to be changed.

Why can't they clear RESERVED flags as well? We just have to document the
expected behavior. Even if the bonding PMD does not, applications that do
not use those flags (since they are reserved) should not be impacted.

Because the bonding PMD won't forward PMD-specific API calls, I think it's
safer to clear them anyway.

By the way, how do you handle the case where the bonding PMD exposes the
MACSEC feature and as a result the application tries to configure MACSEC
support on the bonding port_id? How is the resulting crash documented in a
generic fashion?

> > So the idea if/when the API is made global is to rename PKT_TX_RESERVED_0 to
> > PKT_TX_MACSEC and keep its original value.
> > 
> > If other PMDs also implemented PKT_TX_RESERVED_0 in the meantime, it is
> > redefined using a different value. If there is no room left to do so, these
> > PMDs are out of luck I guess, and their specific API is disabled/removed
> > until something gets re-designed.
> > 
> > How about this?
> 
> I still think that we shouldn't allow PMDs to redefine mbuf.olflags and
> dev_info.(rx|tx)_offload_capa. 
> See above for my reasons.

I'm still not comfortable with partially global/specific APIs such as this,
because we assume applications are fully aware of the underlying port in
order to use them, while at the same time the related flags are part of the
global namespace, I find it confusing.

If application developers have no problem with that (so far they haven't
commented on this topic, which means they likely agree to go with anything),
I have no reason left to go against the majority.

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply

* Re: [PATCH 13/13] i40e: improve message grepability
From: Ferruh Yigit @ 2017-01-09 14:11 UTC (permalink / raw)
  To: Michał Mirosław, dev
In-Reply-To: <39ceb2bf1e5aa61e3957a8d8f9e5b2df28d6d2ad.1481590851.git.mirq-linux@rere.qmqm.pl>

On 12/13/2016 1:08 AM, Michał Mirosław wrote:
> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> ---

<...>

>  	if (ret)
> -		PMD_INIT_LOG(ERR, "Failed to add filter to drop flow control "
> -				  " frames from VSIs.");
> +		PMD_INIT_LOG(ERR, "Failed to add filter to drop flow control frames from VSIs.");

According latest discussion, msg integrity has priority over line
limitation.

But can you please break the lines while keeping whole msg, like:
> +		PMD_INIT_LOG(ERR,
			"Failed to add filter to drop flow control frames from VSIs.");

<...>

>  	if (ret != I40E_SUCCESS) {
> -		PMD_DRV_LOG(ERR, "Fail to debug read from "
> -			    "I40E_GL_SWT_L2TAGCTRL[%d]", reg_id);
> +		PMD_DRV_LOG(ERR, "Fail to debug read from I40E_GL_SWT_L2TAGCTRL[%d]", reg_id);

And can you wrap arguments into next line:
+		PMD_DRV_LOG(ERR,
			"Fail to debug read from I40E_GL_SWT_L2TAGCTRL[%d]",
			reg_id);

<...>

^ permalink raw reply

* Re: [PATCH] app/test: fix aad padding size in SGL operation
From: Kusztal, ArkadiuszX @ 2017-01-09 14:08 UTC (permalink / raw)
  To: De Lara Guarch, Pablo, dev@dpdk.org
  Cc: Trahe, Fiona, Griffin, John, Jain, Deepak K
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D897476C077B@IRSMSX108.ger.corp.intel.com>



> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Monday, January 09, 2017 1:39 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; Griffin, John
> <john.griffin@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>
> Subject: RE: [PATCH] app/test: fix aad padding size in SGL operation
> 
> Hi Arek,
> 
> > -----Original Message-----
> > From: Kusztal, ArkadiuszX
> > Sent: Tuesday, January 03, 2017 3:39 PM
> > To: dev@dpdk.org
> > Cc: Trahe, Fiona; De Lara Guarch, Pablo; Griffin, John; Jain, Deepak
> > K; Kusztal, ArkadiuszX
> > Subject: [PATCH] app/test: fix aad padding size in SGL operation
> >
> > This commit fixes unnecessary padding of aad for GCM using
> > scatter-gather list
> >
> > Fixes: b71990ffa7e4 ("app/test: add SGL tests to cryptodev QAT suite")
> >
> > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> 
> I can merge this patch to the commit above in the subtree, as it has not been
> upstreamed to the mainline.
> Is that OK?

Hi Pablo,

Sure, I think it is the best solution.

Thanks,
Arek

> 
> Thanks,
> Pablo

^ permalink raw reply

* Re: [PATCH v3 0/7] virtio_user as an alternative exception path
From: Bruce Richardson @ 2017-01-09 14:06 UTC (permalink / raw)
  To: Jianfeng Tan; +Cc: dev, yuanhan.liu, ferruh.yigit, cunming.liang
In-Reply-To: <1483502366-140154-1-git-send-email-jianfeng.tan@intel.com>

On Wed, Jan 04, 2017 at 03:59:19AM +0000, Jianfeng Tan wrote:
> v3:
>   - Drop the patch to postpone driver ok sending patch, superseded it
>     with a bug fix to disable all virtqueues and re-init the device.
>     (you might wonder why not just send reset owner msg. Under my test,
>      it causes spinlock deadlock problem when killing the program).
>   - Avoid compiling error on 32-bit system for pointer convert.
>   - Fix a bug in patch "abstract virtio user backend ops", vhostfd is
>     not properly assigned.
>   - Fix a "MQ cannot be used" bug in v2, which is related to strip
>     some feature bits that vhost kernel does not recognize.
>   - Update release note.
> 
> v2: (Lots of them are from yuanhan's comment)
>   - Add offloding feature.
>   - Add multiqueue support.
>   - Add a new patch to postpone the sending of driver ok notification.
>   - Put fix patch ahead of the whole patch series.
>   - Split original 0001 patch into 0003 and 0004 patches.
>   - Remove the original vhost_internal design, just add those into
>     struct virtio_user_dev for simplicity.
>   - Reword "control" to "send_request".
>   - Reword "host_features" to "device_features". 
> 
> In v16.07, we upstreamed a virtual device, virtio_user (with vhost-user
> as the backend). The path to go with a vhost-kernel backend has been
> dropped for bad performance comparing to vhost-user and code simplicity.
> 
> But after a second thought, virtio_user + vhost-kernel is a good 
> candidate as an exceptional path, such as KNI, which exchanges packets
> with kernel networking stack.
>   - maintenance: vhost-net (kernel) is upstreamed and extensively used 
>     kernel module. We don't need any out-of-tree module like KNI.
>   - performance: as with KNI, this solution would use one or more
>     kthreads to send/receive packets from user space DPDK applications,
>     which has little impact on user space polling thread (except that
>     it might enter into kernel space to wake up those kthreads if
>     necessary).
>   - features: vhost-net is born to be a networking solution, which has
>     lots of networking related featuers, like multi queue, tso, multi-seg
>     mbuf, etc.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
Sounds great. However, I think we'll need a how-to doc for this to help
people get it up and running as a KNI replacement for packets to/from
the kernel. Any plans to draw up such a doc? It would be good to include
it in this patchset.

/Bruce

^ permalink raw reply

* Re: [PATCH v2 02/11] crypto/dpaa2_sec: Run time assembler for Descriptor formation
From: De Lara Guarch, Pablo @ 2017-01-09 13:55 UTC (permalink / raw)
  To: Akhil Goyal, dev@dpdk.org
  Cc: thomas.monjalon@6wind.com, Doherty, Declan,
	hemant.agrawal@nxp.com, Mcnamara, John, nhorman@tuxdriver.com,
	Horia Geanta Neag
In-Reply-To: <20161222201700.20020-3-akhil.goyal@nxp.com>



> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Thursday, December 22, 2016 8:17 PM
> To: dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; Doherty, Declan; De Lara Guarch, Pablo;
> hemant.agrawal@nxp.com; Mcnamara, John; nhorman@tuxdriver.com;
> Akhil Goyal; Horia Geanta Neag
> Subject: [PATCH v2 02/11] crypto/dpaa2_sec: Run time assembler for
> Descriptor formation
> 
> A set of header files(hw) which helps in making the descriptors
> that are understood by NXP's SEC hardware.
> This patch provides header files for command words which can be used
> for descriptor formation.
> 
> Signed-off-by: Horia Geanta Neag <horia.geanta@nxp.com>
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---

...

> diff --git a/drivers/crypto/dpaa2_sec/hw/rta.h
> b/drivers/crypto/dpaa2_sec/hw/rta.h
> new file mode 100644
> index 0000000..7eb0455
> --- /dev/null
> +++ b/drivers/crypto/dpaa2_sec/hw/rta.h

...

> +extern enum rta_sec_era rta_sec_era;
> +
> +/**
> + * rta_set_sec_era - Set SEC Era HW block revision for which the RTA
> library
> + *                   will generate the descriptors.
> + * @era: SEC Era (enum rta_sec_era)
> + *
> + * Return: 0 if the ERA was set successfully, -1 otherwise (int)
> + *
> + * Warning 1: Must be called *only once*, *before* using any other RTA
> API
> + * routine.
> + *
> + * Warning 2: *Not thread safe*.
> + */

> +static inline int rta_set_sec_era(enum rta_sec_era era)
> +{

"static inline int" should go in a different line than the function name and parameters.
So it should be:

static inline int
rta_set_sec_era(enum rta_sec_era era)
{

Could you make this change here and in the rest of the functions?

Thanks,
Pablo

^ permalink raw reply

* Re: [PATCH] app/test: fix aad padding size in SGL operation
From: De Lara Guarch, Pablo @ 2017-01-09 13:38 UTC (permalink / raw)
  To: Kusztal, ArkadiuszX, dev@dpdk.org
  Cc: Trahe, Fiona, Griffin, John, Jain, Deepak K
In-Reply-To: <1483457911-9635-1-git-send-email-arkadiuszx.kusztal@intel.com>

Hi Arek,

> -----Original Message-----
> From: Kusztal, ArkadiuszX
> Sent: Tuesday, January 03, 2017 3:39 PM
> To: dev@dpdk.org
> Cc: Trahe, Fiona; De Lara Guarch, Pablo; Griffin, John; Jain, Deepak K;
> Kusztal, ArkadiuszX
> Subject: [PATCH] app/test: fix aad padding size in SGL operation
> 
> This commit fixes unnecessary padding of aad for GCM using
> scatter-gather list
> 
> Fixes: b71990ffa7e4 ("app/test: add SGL tests to cryptodev QAT suite")
> 
> Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>

I can merge this patch to the commit above in the subtree, as it has not been upstreamed to the mainline.
Is that OK?

Thanks,
Pablo

^ permalink raw reply

* Re: [PATCH v2 01/11] librte_cryptodev: Add rte_device pointer in cryptodevice
From: De Lara Guarch, Pablo @ 2017-01-09 13:34 UTC (permalink / raw)
  To: Akhil Goyal, dev@dpdk.org
  Cc: thomas.monjalon@6wind.com, Doherty, Declan,
	hemant.agrawal@nxp.com, Mcnamara, John, nhorman@tuxdriver.com
In-Reply-To: <20161222201700.20020-2-akhil.goyal@nxp.com>

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
> Sent: Thursday, December 22, 2016 8:17 PM
> To: dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; Doherty, Declan; De Lara Guarch, Pablo;
> hemant.agrawal@nxp.com; Mcnamara, John; nhorman@tuxdriver.com;
> Akhil Goyal
> Subject: [dpdk-dev] [PATCH v2 01/11] librte_cryptodev: Add rte_device
> pointer in cryptodevice
> 
> This patch will not be required as some parallel work is going
> on to add it across all crypto devices.
> 
Could you tell me the patch that is going to add this?
In that case, you can drop this and just say that your patchset depends on it.

Also, the title should be "cryptodev: add rte_device pointer in crypto device".
Note that for libraries, you just need the name of the library (i.e. cryptodev).
The other thing is that the first letter should be lowercase. Could you change this in the other patches too?


Thanks,
Pablo

^ permalink raw reply

* Re: Port stats zero when using MLX5 DPDK driver
From: Shahaf Shuler @ 2017-01-09 13:32 UTC (permalink / raw)
  To: george.dit@gmail.com, dev@dpdk.org
In-Reply-To: <CAN9HtFACYzd-OBA8DyNZVH06imNt+vjWJtTrbo-wHXSxU582qQ@mail.gmail.com>

Hi Georgios,

It is not support on Mellanox PMD to read the primary process counters from a secondary process.


--Shahaf


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of george.dit@gmail.com
Sent: Sunday, January 8, 2017 3:38 PM
To: dev@dpdk.org
Subject: [dpdk-dev] Port stats zero when using MLX5 DPDK driver

Hi,

I have a simple setup with a machine that contains a dual port 10 GbE Intel 82599ES NIC and another dual port 100 GbE Mellanox ConnectX-4 NIC. The Intel ports are 0 and 1, while the Mellanox ones are 2 and 3.

I properly compiled DPDK 16.11 and test-pmd works just fine for all 4 ports.
Then, I ran a simple primary application that forwards packets from 0 <-->
1 and 2 <--> 3 and started dpdk-procinfo -- --stats (or --xstats) as a secondary monitoring process, while sending some traffic to all 4 ports.
The problem I see is that the statistics reported by the Mellanox NICs are always zero (Intel ports report just fine).

What is the reason behind this behavior? Is there a bug in the driver (maybe recently fixed by DPDK 17.02 rc?) or is it simply a lack of this functionality?

Thanks and best regards,

-- 
   Georgios Katsikas
   Ph.D. Student and Research Assistant
   Network Systems Lab (NSL)



       *E-Mail:*  george <george.katsikas@imdea.org>.dit@gmail.com
   *Web Site:*  http://www.di.uoa.gr/~katsikas/ <http://people.networks.imdea.org/~george_katsikas/index.html>

^ permalink raw reply

* Re: [PATCH] net/i40e: remove redundant statement and braces
From: Ferruh Yigit @ 2017-01-09 13:31 UTC (permalink / raw)
  To: Yong Wang, helin.zhang, jingjing.wu; +Cc: dev
In-Reply-To: <099e06c8-1fa3-3684-a36c-e4b86f5c6912@intel.com>

On 1/9/2017 1:30 PM, Ferruh Yigit wrote:
> On 1/9/2017 1:48 PM, Yong Wang wrote:
>> In function "reassemble_packets()", the statement "end = secondlast;"
>> is redundant since there is another assignment "start = end = NULL;"
>> 3 lines below. BTW, I removed the redundant braces in the conditional
>> statement "if (end->data_len > rxq->crc_len)".
>>
>> Signed-off-by: Yong Wang <wang.yong19@zte.com.cn>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

^ permalink raw reply

* Re: [PATCH v2 00/11] Introducing NXP DPAA2 SEC based cryptodev PMD
From: De Lara Guarch, Pablo @ 2017-01-09 13:31 UTC (permalink / raw)
  To: Akhil Goyal, dev@dpdk.org
  Cc: thomas.monjalon@6wind.com, Doherty, Declan,
	hemant.agrawal@nxp.com, Mcnamara, John, nhorman@tuxdriver.com
In-Reply-To: <20161222201700.20020-1-akhil.goyal@nxp.com>



> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Thursday, December 22, 2016 8:17 PM
> To: dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; Doherty, Declan; De Lara Guarch, Pablo;
> hemant.agrawal@nxp.com; Mcnamara, John; nhorman@tuxdriver.com;
> Akhil Goyal
> Subject: [PATCH v2 00/11] Introducing NXP DPAA2 SEC based cryptodev
> PMD
> 
> Based over the DPAA2 PMD driver [1], this series of patches introduces the
> DPAA2_SEC PMD which provides DPDK crypto driver for NXP's DPAA2
> CAAM
> Hardware accelerator.
> 
> SEC is NXP DPAA2 SoC's security engine for cryptographic acceleration and
> offloading. It implements block encryption, stream cipher, hashing and
> public key algorithms. It also supports run-time integrity checking, and a
> hardware random number generator.
> 
> Besides the objects exposed in [1], another key object has been added
> through this patch:
> 
>  - DPSECI, refers to SEC block interface
> 
>  :: Patch Layout ::
> 
>  0001	  : lib: Add rte_device pointer in cryptodevice.
>  		This patch may not be needed as some parallel work
> 		is going on on the mailing list.
>  0002~0003: Run Time Assembler(RTA) common library for CAAM
> hardware
>  0004     : Documentation
>  0005~0009: Crytodev PMD
>  0010     : Performance Test
>  0011	  : MAINTAINERS
> 
>  :: Pending/ToDo ::
> 
> - More functionality and algorithms are still work in progress
>         -- Hash followed by Cipher mode
>         -- session-less API
> 	-- Chained mbufs
> 
> - Functional tests would be enhanced in v3


Hi Akhil,

Are you planning to send this v3 anytime soon?

Thanks,
Pablo

^ permalink raw reply

* Re: [PATCH] net/i40e: remove redundant statement and braces
From: Ferruh Yigit @ 2017-01-09 13:30 UTC (permalink / raw)
  To: Yong Wang, helin.zhang, jingjing.wu; +Cc: dev
In-Reply-To: <1483969732-20073-1-git-send-email-wang.yong19@zte.com.cn>

On 1/9/2017 1:48 PM, Yong Wang wrote:
> In function "reassemble_packets()", the statement "end = secondlast;"
> is redundant since there is another assignment "start = end = NULL;"
> 3 lines below. BTW, I removed the redundant braces in the conditional
> statement "if (end->data_len > rxq->crc_len)".
> 
> Signed-off-by: Yong Wang <wang.yong19@zte.com.cn>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply

* Pocket loss on 82545EM Gigabit Ethernet Controller (Copper)
From: Keren Hochman @ 2017-01-09 13:24 UTC (permalink / raw)
  To: dev

Hello,
I have a VM with network card: 82545EM Gigabit Ethernet Controller (Copper)
- SpanSwitch
when I use libpcap of dpdk I see 30% packet loss. Only when I bind the
network card to igb_uio I do not see packet loss anymore. Without dpdk I
there is no packet loss also.

Is there a problem running libpcap on this network card?
Thanks, Keren

^ permalink raw reply

* Re: [PATCH 13/13] i40e: improve message grepability
From: Thomas Monjalon @ 2017-01-09 13:18 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, Wu, Jingjing, Michal Miroslaw, Yigit, Ferruh
In-Reply-To: <20170109120255.GA86544@bricha3-MOBL3.ger.corp.intel.com>

2017-01-09 12:02, Bruce Richardson:
> On Wed, Dec 28, 2016 at 03:51:56AM +0000, Wu, Jingjing wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Miroslaw
> > > Sent: Tuesday, December 13, 2016 9:08 AM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH 13/13] i40e: improve message grepability
> > > 
> > > Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev.c | 198 +++++++++++++++--------------------------
> > >  1 file changed, 73 insertions(+), 125 deletions(-)
> > > 
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> > > index 39fbcfe..4d73aca 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -763,8 +763,7 @@ i40e_add_tx_flow_control_drop_filter(struct i40e_pf
> > > *pf)
> > >  				pf->main_vsi_seid, 0,
> > >  				TRUE, NULL, NULL);
> > >  	if (ret)
> > > -		PMD_INIT_LOG(ERR, "Failed to add filter to drop flow control "
> > > -				  " frames from VSIs.");
> > > +		PMD_INIT_LOG(ERR, "Failed to add filter to drop flow control
> > > frames
> > > +from VSIs.");
> > >  }
> > 
> > You are right, it makes grep easily. But it will break the coding style "Line length is recommended to be not more than 80 characters, including comments."
> > 
> > Any comments from committers?
> > 
> Being able to grep error messages is more important that having lines
> wrapped at 80 characters. I believe checkpatch ignores long strings when
> checking line lengths.

I agree keeping strings is more important than checkpatch conformity.
Please, if checkpatch complains, try to tune the script checkpatch.sh.
There are some options to manage strings.

^ permalink raw reply

* [PATCH] net/i40e: remove redundant statement and braces
From: Yong Wang @ 2017-01-09 13:48 UTC (permalink / raw)
  To: helin.zhang, jingjing.wu; +Cc: dev, Yong Wang

In function "reassemble_packets()", the statement "end = secondlast;"
is redundant since there is another assignment "start = end = NULL;"
3 lines below. BTW, I removed the redundant braces in the conditional
statement "if (end->data_len > rxq->crc_len)".

Signed-off-by: Yong Wang <wang.yong19@zte.com.cn>
---
 drivers/net/i40e/i40e_rxtx_vec_common.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index 6cb5dce..02d834a 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -65,9 +65,9 @@
 				start->ol_flags = end->ol_flags;
 				/* we need to strip crc for the whole packet */
 				start->pkt_len -= rxq->crc_len;
-				if (end->data_len > rxq->crc_len) {
+				if (end->data_len > rxq->crc_len)
 					end->data_len -= rxq->crc_len;
-				} else {
+				else {
 					/* free up last mbuf */
 					struct rte_mbuf *secondlast = start;
 
@@ -77,7 +77,6 @@
 							end->data_len);
 					secondlast->next = NULL;
 					rte_pktmbuf_free_seg(end);
-					end = secondlast;
 				}
 				pkts[pkt_idx++] = start;
 				start = end = NULL;
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH v2] doc: add tso capabilities feature for mlx5
From: Ferruh Yigit @ 2017-01-09 12:31 UTC (permalink / raw)
  To: Elad Persiko, dev
In-Reply-To: <1483892601-6997-1-git-send-email-eladpe@mellanox.com>

On 1/8/2017 4:23 PM, Elad Persiko wrote:
> Feature implemented at:
> commit b007e98ccda9 ("net/mlx5: implement TSO data path")
> commit 085c4137280a ("net/mlx5: support TSO in control plane")
> 
> Signed-off-by: Elad Persiko <eladpe@mellanox.com>
> ---
>  doc/guides/nics/features/mlx4.ini | 1 +
>  doc/guides/nics/features/mlx5.ini | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/doc/guides/nics/features/mlx4.ini b/doc/guides/nics/features/mlx4.ini
> index c9828f7..d74b9dd 100644
> --- a/doc/guides/nics/features/mlx4.ini
> +++ b/doc/guides/nics/features/mlx4.ini
> @@ -10,6 +10,7 @@ Queue start/stop     = Y
>  MTU update           = Y
>  Jumbo frame          = Y
>  Scattered Rx         = Y
> +TSO                  = Y

One more thing, for double check, patch updates only mlx5 files, is
enabling TSO for mlx4 correct?

>  Promiscuous mode     = Y
>  Allmulticast mode    = Y
>  Unicast MAC filter   = Y
<...>

^ permalink raw reply

* Re: [PATCH 2/5] net/mlx5: remove unessecary goto label
From: Ferruh Yigit @ 2017-01-09 12:29 UTC (permalink / raw)
  To: Elad Persiko, dev
In-Reply-To: <1483890123-4854-2-git-send-email-eladpe@mellanox.com>

On 1/8/2017 3:42 PM, Elad Persiko wrote:
> use_dseg label can be deleted as it happens without goto.
> 
> Signed-off-by: Elad Persiko <eladpe@mellanox.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply

* Re: [PATCH v4] null: fake PMD capabilities
From: Ferruh Yigit @ 2017-01-09 12:07 UTC (permalink / raw)
  To: Michał Mirosław, dev
In-Reply-To: <539d7a893864e515b9bdc688ca9d7b975a6835f6.1481742624.git.mirq-linux@rere.qmqm.pl>

On 12/14/2016 7:16 PM, Michał Mirosław wrote:
> From: Paweł Małachowski <pawel.malachowski@atendesoftware.pl>
> 
> This allows for testing code which needs offloads to be supported.
> 
> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> ---
>  drivers/net/null/rte_eth_null.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
> index 836d982..e60516f 100644
> --- a/drivers/net/null/rte_eth_null.c
> +++ b/drivers/net/null/rte_eth_null.c
> @@ -284,6 +284,9 @@ eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
>  	return 0;
>  }
>  
> +static void
> +eth_dev_void_ok(struct rte_eth_dev *dev __rte_unused) { return; }
> +
>  
>  static void
>  eth_dev_info(struct rte_eth_dev *dev,
> @@ -304,6 +307,19 @@ eth_dev_info(struct rte_eth_dev *dev,
>  	dev_info->pci_dev = NULL;
>  	dev_info->reta_size = internals->reta_size;
>  	dev_info->flow_type_rss_offloads = internals->flow_type_rss_offloads;
> +	/* We hereby declare we can RX offload VLAN-s out of thin air (they are not there)
> +	 */
> +	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP |
> +		DEV_RX_OFFLOAD_QINQ_STRIP;
> +	/* We promise we will do any TX offloads before passing packets to /dev/null
> +	 */
> +	dev_info->tx_offload_capa = DEV_TX_OFFLOAD_VLAN_INSERT |
> +		DEV_TX_OFFLOAD_IPV4_CKSUM | DEV_TX_OFFLOAD_UDP_CKSUM |
> +		DEV_TX_OFFLOAD_TCP_CKSUM | DEV_TX_OFFLOAD_SCTP_CKSUM |
> +		DEV_TX_OFFLOAD_TCP_TSO | DEV_TX_OFFLOAD_UDP_TSO |
> +		DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM | DEV_TX_OFFLOAD_QINQ_INSERT |
> +		DEV_TX_OFFLOAD_VXLAN_TNL_TSO | DEV_TX_OFFLOAD_GRE_TNL_TSO |
> +		DEV_TX_OFFLOAD_IPIP_TNL_TSO | DEV_TX_OFFLOAD_GENEVE_TNL_TSO;

What do you think reporting offload capa based on user provided devargs,
as suggested by Konstantin?And by default not report any.

>  }
>  
>  static void
> @@ -477,7 +493,12 @@ static const struct eth_dev_ops ops = {
>  	.reta_update = eth_rss_reta_update,
>  	.reta_query = eth_rss_reta_query,
>  	.rss_hash_update = eth_rss_hash_update,
> -	.rss_hash_conf_get = eth_rss_hash_conf_get
> +	.rss_hash_conf_get = eth_rss_hash_conf_get,
> +	/* Fake our capabilities */
> +	.promiscuous_enable   = eth_dev_void_ok,
> +	.promiscuous_disable  = eth_dev_void_ok,
> +	.allmulticast_enable  = eth_dev_void_ok,
> +	.allmulticast_disable = eth_dev_void_ok
>  };
>  
>  int
> 

^ permalink raw reply

* Re: [PATCH] net/i40e: fix segment num in reassemble process
From: Ferruh Yigit @ 2017-01-09 12:04 UTC (permalink / raw)
  To: Chenghu Yao, helin.zhang, jingjing.wu; +Cc: dev
In-Reply-To: <6e209fe8-98c6-6c2b-d41b-8955c3f61fd2@intel.com>

On 1/9/2017 11:55 AM, Ferruh Yigit wrote:
> On 1/9/2017 3:31 AM, Chenghu Yao wrote:
>> When freeing up last mbuf, start->nb_segs should be decremented
>> by one. See also ixgbe process.
>>
>> Signed-off-by: Chenghu Yao <yao.chenghu@zte.com.cn>
> 
> Fixes: 0e0da28cd888 ("i40e: add vector scatter Rx")
> 
> CC:stable@dpdk.org
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

^ permalink raw reply

* Re: [dpdk-stable] [PATCH] net/af_packet: fix fd use after free
From: Ferruh Yigit @ 2017-01-09 12:04 UTC (permalink / raw)
  To: Timmons C. Player, linville; +Cc: dev, dpdk stable
In-Reply-To: <9109d76d-0d94-2ce4-b030-e2d819709998@intel.com>

On 1/9/2017 11:55 AM, Ferruh Yigit wrote:
> On 1/5/2017 2:33 PM, Timmons C. Player wrote:
>> When using the same file descriptor for both rx and tx, the
>> eth_dev_stop function would close the same fd twice.   This
>> change prevents that from happening.
>>
>> Signed-off-by: Timmons C. Player <timmons.player@spirent.com>
> 
> Fixes: 364e08f2bbc0 ("af_packet: add PMD for AF_PACKET-based virtual
> devices")
> 
> CC:stable@dpdk.org
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

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

^ permalink raw reply

* Re: [PATCH 13/13] i40e: improve message grepability
From: Bruce Richardson @ 2017-01-09 12:02 UTC (permalink / raw)
  To: Wu, Jingjing; +Cc: Michal Miroslaw, dev@dpdk.org, Yigit, Ferruh
In-Reply-To: <9BB6961774997848B5B42BEC655768F810CC0197@SHSMSX103.ccr.corp.intel.com>

On Wed, Dec 28, 2016 at 03:51:56AM +0000, Wu, Jingjing wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Miroslaw
> > Sent: Tuesday, December 13, 2016 9:08 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 13/13] i40e: improve message grepability
> > 
> > Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 198 +++++++++++++++--------------------------
> >  1 file changed, 73 insertions(+), 125 deletions(-)
> > 
> > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> > index 39fbcfe..4d73aca 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -763,8 +763,7 @@ i40e_add_tx_flow_control_drop_filter(struct i40e_pf
> > *pf)
> >  				pf->main_vsi_seid, 0,
> >  				TRUE, NULL, NULL);
> >  	if (ret)
> > -		PMD_INIT_LOG(ERR, "Failed to add filter to drop flow control "
> > -				  " frames from VSIs.");
> > +		PMD_INIT_LOG(ERR, "Failed to add filter to drop flow control
> > frames
> > +from VSIs.");
> >  }
> 
> You are right, it makes grep easily. But it will break the coding style "Line length is recommended to be not more than 80 characters, including comments."
> 
> Any comments from committers?
> 
Being able to grep error messages is more important that having lines
wrapped at 80 characters. I believe checkpatch ignores long strings when
checking line lengths.

/Bruce

^ permalink raw reply

* Re: [PATCH] net/i40e: fix segment num in reassemble process
From: Ferruh Yigit @ 2017-01-09 11:55 UTC (permalink / raw)
  To: Chenghu Yao, helin.zhang, jingjing.wu; +Cc: dev
In-Reply-To: <1483932664-38718-1-git-send-email-yao.chenghu@zte.com.cn>

On 1/9/2017 3:31 AM, Chenghu Yao wrote:
> When freeing up last mbuf, start->nb_segs should be decremented
> by one. See also ixgbe process.
> 
> Signed-off-by: Chenghu Yao <yao.chenghu@zte.com.cn>

Fixes: 0e0da28cd888 ("i40e: add vector scatter Rx")

CC:stable@dpdk.org

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply

* Re: [PATCH] net/af_packet: fix fd use after free
From: Ferruh Yigit @ 2017-01-09 11:55 UTC (permalink / raw)
  To: Timmons C. Player, linville; +Cc: dev, dpdk stable
In-Reply-To: <1483626815-476-1-git-send-email-timmons.player@spirent.com>

On 1/5/2017 2:33 PM, Timmons C. Player wrote:
> When using the same file descriptor for both rx and tx, the
> eth_dev_stop function would close the same fd twice.   This
> change prevents that from happening.
> 
> Signed-off-by: Timmons C. Player <timmons.player@spirent.com>

Fixes: 364e08f2bbc0 ("af_packet: add PMD for AF_PACKET-based virtual
devices")

CC:stable@dpdk.org

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply

* Re: [PATCH] net/bnxt: Add support for new PCI IDs
From: Ferruh Yigit @ 2017-01-09 11:55 UTC (permalink / raw)
  To: Ajit Khaparde, dev
In-Reply-To: <20170106195517.52821-1-ajit.khaparde@broadcom.com>

On 1/6/2017 7:55 PM, Ajit Khaparde wrote:
> Add suupport for PCI IDs which was removed as a part of
> commit 953158857f194991 ("net/bnxt: remove support for few devices")
> 
> Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

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

(fixed referenced commit id)

^ permalink raw reply

* Re: [PATCH v5 3/8] ethdev: reserve capability flags for PMD-specific API
From: Thomas Monjalon @ 2017-01-09 11:26 UTC (permalink / raw)
  To: Tiwei Bie, Ananyev, Konstantin, Adrien Mazarguil
  Cc: dev, Lu, Wenzhuo, Mcnamara, John, olivier.matz, Zhang, Helin,
	Dai, Wei, Wang, Xiao W
In-Reply-To: <20170109035736.GA11691@debian>

2017-01-09 11:57, Tiwei Bie:
> On Sun, Jan 08, 2017 at 08:39:55PM +0800, Ananyev, Konstantin wrote:
> > > Well my first reply to this thread was asking why isn't the whole API global
> > > from the start then?
> > 
> > That's good question, and my preference would always be to have the
> > API to configure this feature as generic one.
> > I guess the main reason why it is not right now we don't reach an agreement
> > how this API should look like: 
> > http://dpdk.org/ml/archives/dev/2016-September/047810.html
> > But I'll leave it to the author to provide the real reason here. 
> 
> Yes, currently this work just provided a thin layer over 82599's
> hardware MACsec offload support to allow users configure 82599's
> MACsec offload engine. The current API may be too specific and may
> need a rework to be used with other NICs.

I think it is a really good approach to start such API privately in a driver.
It will give us more time and experience to design a proper generic API.

Regarding the mbuf flag, it looks straight-forward, and as it is IEEE
standardized, I do not see any objection to add it now.
However, I will wait for the approval of Olivier - as maintainer of mbuf.

^ permalink raw reply

* Re: [PATCH] net/enic: add support for TSO
From: Ferruh Yigit @ 2017-01-09 11:10 UTC (permalink / raw)
  To: John Daley, bruce.richardson; +Cc: dev
In-Reply-To: <20170107044507.2407-1-johndale@cisco.com>

Hi John,

On 1/7/2017 4:45 AM, John Daley wrote:
> The enic TSO implementation requires that the length of the Eth/IP/TCP
> headers be passed to the NIC. Other than that, it's just a matter of
> setting the mss and offload mode on a per packet basis.
> 
> In TSO mode, IP and TCP checksums are offloaded even if not requested
> with mb->ol_flags.
> 
> Signed-off-by: John Daley <johndale@cisco.com>

Can you please update doc/guides/nics/features/enic.ini to document the
TSO feature in this patch?

Thanks,
ferruh

> ---
<...>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox