* Re: [PATCH v4] mempool: use cache in single producer or consumer mode
From: Thomas Monjalon @ 2017-01-13 15:34 UTC (permalink / raw)
To: Wenfeng Liu; +Cc: dev, Olivier Matz, konstantin.ananyev
In-Reply-To: <20170113162327.467538f9@platinum>
2017-01-13 16:23, Olivier Matz:
> On Wed, 11 Jan 2017 02:25:28 +0000, Wenfeng Liu
> <liuwf@arraynetworks.com.cn> wrote:
> > Currently we will check mempool flags when we put/get objects from
> > mempool. However, this makes cache useless when mempool is SC|SP,
> > SC|MP, MC|SP cases.
> > This patch makes cache available in above cases and improves
> > performance.
> >
> > Signed-off-by: Wenfeng Liu <liuwf@arraynetworks.com.cn>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH v4 5/6] example: distributor app showing burst api
From: Bruce Richardson @ 2017-01-13 15:36 UTC (permalink / raw)
To: David Hunt; +Cc: dev
In-Reply-To: <1483948248-91364-6-git-send-email-david.hunt@intel.com>
On Mon, Jan 09, 2017 at 07:50:47AM +0000, David Hunt wrote:
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
> examples/distributor/main.c | 508 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 390 insertions(+), 118 deletions(-)
>
check-git-log complains a bit about the title of the patch, and it would
be good to have a description of the app changes as a commit message
body.
As well as this checkpatch throws up the fact that there is a line
commented out with a C99 style comment. This line should be deleted if
it's commented out.
ERROR:C99_COMMENTS: do not use C99 // comments
#305: FILE: examples/distributor/main.c:331:
+ //struct rte_ring *out_ring = p->dist_tx_ring;
total: 1 errors, 0 warnings, 743 lines checked
/Bruce
^ permalink raw reply
* Re: [PATCH v4 5/6] example: distributor app showing burst api
From: Bruce Richardson @ 2017-01-13 15:38 UTC (permalink / raw)
To: David Hunt; +Cc: dev
In-Reply-To: <1483948248-91364-6-git-send-email-david.hunt@intel.com>
On Mon, Jan 09, 2017 at 07:50:47AM +0000, David Hunt wrote:
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
> examples/distributor/main.c | 508 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 390 insertions(+), 118 deletions(-)
>
> diff --git a/examples/distributor/main.c b/examples/distributor/main.c
> index e7641d2..eebfb74 100644
Compile errors on 32-bit:
/home/bruce/dpdk-clean/examples/distributor/main.c: In function
‘print_stats’:
/home/bruce/dpdk-clean/examples/distributor/main.c:589:14: error: format
‘%ld’ expects argument of type ‘long int’, but argument 2 has type
‘uint64_t {aka volatile long long unsigned int}’ [-Werror=format=]
printf("%ld ", app_stats.worker_bursts[i][j]);
^
^ permalink raw reply
* Re: [PATCH 0/2] make a couple eth_dev_ops const
From: Thomas Monjalon @ 2017-01-13 15:38 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, Ferruh Yigit
In-Reply-To: <088c65d6-e709-23dd-3a86-e90e192fd30f@intel.com>
2017-01-12 21:54, Ferruh Yigit:
> On 1/12/2017 7:12 PM, Stephen Hemminger wrote:
> > Really trivial patches.
>
> Patches are not in patchwork, and not in the mail list.
>
> > I also consider this a litmus test of how long it takes DPDK
> > project to merge trivial maintaince patches.
This is a nice example of why patches are not merged timely,
sometimes they are not received at all :)
In order to reduce the time needed to merge, could you please
run devtools/check-git-log.sh ?
^ permalink raw reply
* Re: [PATCH v5] mbuf: add a function to linearize a packet
From: Kulasek, TomaszX @ 2017-01-13 15:40 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev@dpdk.org, De Lara Guarch, Pablo
In-Reply-To: <20170113163234.1f38b9ee@platinum>
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, January 13, 2017 16:33
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: Re: [PATCH v5] mbuf: add a function to linearize a packet
>
> On Thu, 12 Jan 2017 10:40:44 +0100, Tomasz Kulasek
> <tomaszx.kulasek@intel.com> wrote:
> > This patch adds function rte_pktmbuf_linearize to let crypto PMD
> > coalesce chained mbuf before crypto operation and extend their
> > capabilities to support segmented mbufs when device cannot handle
> > them natively.
> >
> > Included unit tests for rte_pktmbuf_linearize functionality:
> >
> > 1) Creates banch of segmented mbufs with different size and number of
> > segments.
> > 2) Fills noncontigouos mbuf with sequential values.
> > 3) Uses rte_pktmbuf_linearize to coalesce segmented buffer into one
> > contiguous.
> > 4) Verifies data in linearized buffer.
> >
> > Dependencies:
> >
> > This patch is rebased to the dpdk-next-crypto and should be applied
> > before "Chained Mbufs support in SW PMDs" patchset.
> >
> > changes in v5:
> > - name of patch changed,
> > - improved coding style,
> >
> > changes in v4:
> > - separated from "Chained Mbufs support in SW PMDs" patch set for
> > better reviewing,
> > - merged "rte_pktmbuf_linearize" implementation with unit tests,
> >
> > changes in v3:
> > - rebased to dpdk-next-crypto
> >
> > changes in v2:
> > - rte_pktmbuf_coalesce replaced with rte_pktmbuf_linearize
> >
> > Cc: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > Cc: Olivier Matz <olivier.matz@6wind.com>
> > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>
> Just one comment about the changelog for next time: as it's not
> something we need to keep in the git history, you can put it after the
> '---', so it will be dropped when applying the patch with 'git am'.
>
> Thanks,
> Olivier
Ok, thanks.
Tomasz
^ permalink raw reply
* Re: [PATCH v3 2/8] lib: add cryptodev type for the upcoming ARMv8 PMD
From: Zbigniew Bodek @ 2017-01-13 15:50 UTC (permalink / raw)
To: Hemant Agrawal, dev; +Cc: pablo.de.lara.guarch, declan.doherty, jerin.jacob
In-Reply-To: <9c3e96af-7cac-10a4-736a-f6690e9caaff@nxp.com>
On 13.01.2017 09:16, Hemant Agrawal wrote:
> On 1/4/2017 11:03 PM, zbigniew.bodek@caviumnetworks.com wrote:
>> From: Zbigniew Bodek <zbigniew.bodek@caviumnetworks.com>
>>
>> Add type and name for ARMv8 crypto PMD
>>
>> Signed-off-by: Zbigniew Bodek <zbigniew.bodek@caviumnetworks.com>
>> ---
>> lib/librte_cryptodev/rte_cryptodev.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/lib/librte_cryptodev/rte_cryptodev.h
>> b/lib/librte_cryptodev/rte_cryptodev.h
>> index 8f63e8f..6f34f22 100644
>> --- a/lib/librte_cryptodev/rte_cryptodev.h
>> +++ b/lib/librte_cryptodev/rte_cryptodev.h
>> @@ -66,6 +66,8 @@
>> /**< KASUMI PMD device name */
>> #define CRYPTODEV_NAME_ZUC_PMD crypto_zuc
>> /**< KASUMI PMD device name */
>> +#define CRYPTODEV_NAME_ARMV8_PMD crypto_armv8
>> +/**< ARMv8 Crypto PMD device name */
>>
> I will suggest the name as armv8ce or armv8_ce for this driver.
> Do you agree?
CE for "crypto extensions"?
Agreed.
>
>> /** Crypto device type */
>> enum rte_cryptodev_type {
>> @@ -77,6 +79,7 @@ enum rte_cryptodev_type {
>> RTE_CRYPTODEV_KASUMI_PMD, /**< KASUMI PMD */
>> RTE_CRYPTODEV_ZUC_PMD, /**< ZUC PMD */
>> RTE_CRYPTODEV_OPENSSL_PMD, /**< OpenSSL PMD */
>> + RTE_CRYPTODEV_ARMV8_PMD, /**< ARMv8 crypto PMD */
>> };
>>
>> extern const char **rte_cyptodev_names;
>>
>
>
^ permalink raw reply
* Re: [PATCH v3 15/29] crypto/qat: use eal I/O device memory read/write API
From: Ferruh Yigit @ 2017-01-13 15:50 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin, santosh.shukla, John Griffin, Fiona Trahe,
Deepak Kumar Jain
In-Reply-To: <20170113145753.GB13558@localhost.localdomain>
On 1/13/2017 2:57 PM, Jerin Jacob wrote:
> On Fri, Jan 13, 2017 at 11:32:29AM +0000, Ferruh Yigit wrote:
>> On 1/13/2017 8:17 AM, Jerin Jacob wrote:
>>> On Thu, Jan 12, 2017 at 07:09:22PM +0000, Ferruh Yigit wrote:
>>>> Hi Jerin,
>>>>
>>>> On 1/12/2017 9:17 AM, Jerin Jacob wrote:
>>>> <...>
>>>>
>>>>> +#include <rte_io.h>
>>>>> +
>>>>> /* CSR write macro */
>>>>> -#define ADF_CSR_WR(csrAddr, csrOffset, val) \
>>>>> - (void)((*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)) \
>>>>> - = (val)))
>>>>> +#define ADF_CSR_WR(csrAddr, csrOffset, val) \
>>>>> + rte_write32(val, (((uint8_t *)csrAddr) + csrOffset))
>>>>
>>>> For IA, this update introduces an extra compiler barrier (rte_io_wmb()),
>>>> which is indeed not a must, is this correct?
>>>
>>> AFAIK, Compiler barrier is required for IA. I am not an IA expert, if
>>> someone thinks it needs to changed then I can fix it in following commit
>>> in this patch series by making rte_io_wmb() and rte_io_rmb() as empty.
>>>
>>> Let me know.
>>>
>>> AFAIK, Linux kernel code has a barrier in readl/writel for IA.
>>>
>>> Typically we don't use any non relaxed versions in fast path.In fast
>>> typically all the drivers has explicit write barrier for doorbell write
>>> and followed by a relaxed version of write. IMO, In any event, it won't
>>> generate performance regression.
>>>
>>> [dpdk-master] $ git show
>>> 70c343bdc8c33a51a9db23cd58122bdfc120a58f
>>> commit 70c343bdc8c33a51a9db23cd58122bdfc120a58f
>>> Author: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>> Date: Mon Dec 5 06:36:49 2016 +0530
>>>
>>> eal/x86: define I/O device memory barriers for IA
>>>
>>> The patch does not provide any functional change for IA.
>>> I/O barriers are mapped to existing smp barriers.
>>>
>>> CC: Bruce Richardson <bruce.richardson@intel.com>
>>> CC: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>
>>> diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>> b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>> index 00b1cdf..4eac666 100644
>>> --- a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>> +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>> @@ -61,6 +61,12 @@ extern "C" {
>>>
>>> #define rte_smp_rmb() rte_compiler_barrier()
>>>
>>> +#define rte_io_mb() rte_mb()
>>> +
>>> +#define rte_io_wmb() rte_compiler_barrier()
>>> +
>>> +#define rte_io_rmb() rte_compiler_barrier()
>>> +
>>> /*------------------------- 16 bit atomic operations
>>> * -------------------------*/
>>>
>>> #ifndef RTE_FORCE_INTRINSICS
>>>
>>>>
>>>> If so, does it make sense to override these functions for x86, and make
>>>> rte_writeX = rte_writeX_relaxed
>>>> rte_readX = rte_readX_relaxed
>>>>
>>>>>
>>>>> /* CSR read macro */
>>>>> -#define ADF_CSR_RD(csrAddr, csrOffset) \
>>>>> - (*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)))
>>>>> +#define ADF_CSR_RD(csrAddr, csrOffset) \
>>>>> + rte_read32((((uint8_t *)csrAddr) + csrOffset))
>>>>
>>>> This patchset both introduces new rte_readX/rte_writeX functions, also
>>>> applies them into drivers.
>>>>
>>>> While applying them, it changes the behavior.
>>>> Like above code was doing a read, but after update it does read and
>>>> read_memory_barrier.
>>>>
>>>> What do you think this patchset updates usage in a manner that keeps
>>>> behavior exact same. Like using rte_read32_relaxed for this case.
>>>> And doing architecture related updates in a different patchset?
>>>
>>> Need to use rte_read32 at this commit otherwise it will break for ARM.
>>> That's was all point for this patchset.
>>
>> Why it breaks the ARM, is it because rte_*mb() updated for ARM in this
>> patchset (patch 7/29) ?
>
> Yes.
>
>
>>
>> I believe it is good to make these modifications in two phase:
>
> It is in two phases only. First introduced the API with implementation and
> enabled in each driver. Why did you think other-way around it is better?
For two things:
1- If something goes wrong, find the source of problem easier.
2- Make architectural changes obvious, right now it is a little hard to
see, and this again for item 1.
But I also would like to hear more comments before you change/try anything.
> I can rework and test if there is any value addition. If you concerned
> about git bisect ability then I don't think we are loosing that in this
> model.
>
> Thoughts?
>
>> - First replace old usage with rte_readX/rte_writeX while keeping exact
>> same behavior
>>
>> - Second, do architecture specific changes. Both in eal and drivers
>> level if required.
>>
>> Thanks,
>> ferruh
>>
>>> For performance regression, we can always verify by taking delta
>>> between this changeset and the previous changeset. If you think, I need
>>> to make rte_io_wmb()/rte_io_rmb() as empty for IA then I could do that
>>> as well.
>>>
>>>
>>>>
>>>> This both makes easy to see architecture specific updates, and makes
>>>> easy to trace any possible performance issues by this patchset.
>>>>
>>>>>
>>>>> #define ADF_BANK_INT_SRC_SEL_MASK_0 0x4444444CUL
>>>>> #define ADF_BANK_INT_SRC_SEL_MASK_X 0x44444444UL
>>>>>
>>>>
>>
^ permalink raw reply
* Re: [PATCH] app/testpmd: fix static build link ordering
From: Thomas Monjalon @ 2017-01-13 15:53 UTC (permalink / raw)
To: Jerin Jacob; +Cc: Ferruh Yigit, dev, stable
In-Reply-To: <20170113032155.GA31838@localhost.localdomain>
2017-01-13 08:51, Jerin Jacob:
> On Thu, Jan 12, 2017 at 03:27:30PM +0000, Ferruh Yigit wrote:
> > On 1/12/2017 1:58 PM, Jerin Jacob wrote:
> > > On Thu, Jan 12, 2017 at 10:26:08AM +0100, Thomas Monjalon wrote:
> > >> 2017-01-12 13:16, Jerin Jacob:
> > >>> +ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
> > >>> _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> > >>> +endif
> > >>
> > >> _LDLIBS is an internal variable of rte.app.mk.
> > >> Please could you check that there is no issue when using LDLIBS instead
> > >> of _LDLIBS?
> > >
> LDLIBS is not helping the situation as LDLIBS comes before the _LDLIBS-y
> mk/rte.app.mk:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
>
> But moving to EXTRA_LDLIBS looks OK.But it has to be under CONFIG_RTE_LIBRTE_IXGBE_PMD
>
> Thomas, Ferruh
> Let me know if you have any objection on below mentioned diff
>
> -ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
> -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> +ifeq ($(CONFIG_RTE_LIBRTE_IXGBE_PMD),y)
> +EXTRA_LDLIBS += -lrte_pmd_ixgbe
> endif
You need to keep the shared lib check.
Anyway, EXTRA_LDLIBS should be reserved to users and not used in a Makefile.
I prefer your initial patch using _LDLIBS.
Any objection to merge initial proposal?
^ permalink raw reply
* Re: [PATCH v6 00/18] net/ixgbe: Consistent filter API
From: Ferruh Yigit @ 2017-01-13 15:54 UTC (permalink / raw)
To: Wei Zhao, dev
In-Reply-To: <1484295192-34009-1-git-send-email-wei.zhao1@intel.com>
On 1/13/2017 8:12 AM, Wei Zhao wrote:
> The patches mainly finish following functions:
> 1) Store and restore all kinds of filters.
> 2) Parse all kinds of filters.
> 3) Add flow validate function.
> 4) Add flow create function.
> 5) Add flow destroy function.
> 6) Add flow flush function.
>
<...>
>
> zhao wei (18):
> net/ixgbe: store TCP SYN filter
> net/ixgbe: store flow director filter
> net/ixgbe: store L2 tunnel filter
> net/ixgbe: restore n-tuple filter
> net/ixgbe: restore ether type filter
> net/ixgbe: restore TCP SYN filter
> net/ixgbe: restore flow director filter
> net/ixgbe: restore L2 tunnel filter
> net/ixgbe: store and restore L2 tunnel configuration
> net/ixgbe: flush all the filters
> net/ixgbe: parse n-tuple filter
> net/ixgbe: parse ethertype filter
> net/ixgbe: parse TCP SYN filter
> net/ixgbe: parse L2 tunnel filter
> net/ixgbe: parse flow director filter
> net/ixgbe: create consistent filter
> net/ixgbe: destroy consistent filter
> net/ixgbe: flush all the filter list
>
<...>
>
> Acked-by: Beilei Xing <beilei.xing@intel.com>
> Acked-by: Wei Dai <wei.dai@intel.com>
>
Series applied to dpdk-next-net/master, thanks.
^ permalink raw reply
* Re: [PATCH] app/testpmd: fix static build link ordering
From: Ferruh Yigit @ 2017-01-13 15:57 UTC (permalink / raw)
To: Thomas Monjalon, Jerin Jacob; +Cc: dev, stable
In-Reply-To: <2563842.f8ghPspXGG@xps13>
On 1/13/2017 3:53 PM, Thomas Monjalon wrote:
> 2017-01-13 08:51, Jerin Jacob:
>> On Thu, Jan 12, 2017 at 03:27:30PM +0000, Ferruh Yigit wrote:
>>> On 1/12/2017 1:58 PM, Jerin Jacob wrote:
>>>> On Thu, Jan 12, 2017 at 10:26:08AM +0100, Thomas Monjalon wrote:
>>>>> 2017-01-12 13:16, Jerin Jacob:
>>>>>> +ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
>>>>>> _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
>>>>>> +endif
>>>>>
>>>>> _LDLIBS is an internal variable of rte.app.mk.
>>>>> Please could you check that there is no issue when using LDLIBS instead
>>>>> of _LDLIBS?
>>>>
>> LDLIBS is not helping the situation as LDLIBS comes before the _LDLIBS-y
>> mk/rte.app.mk:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
>>
>> But moving to EXTRA_LDLIBS looks OK.But it has to be under CONFIG_RTE_LIBRTE_IXGBE_PMD
>>
>> Thomas, Ferruh
>> Let me know if you have any objection on below mentioned diff
>>
>> -ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
>> +ifeq ($(CONFIG_RTE_LIBRTE_IXGBE_PMD),y)
>> +EXTRA_LDLIBS += -lrte_pmd_ixgbe
>> endif
>
> You need to keep the shared lib check.
> Anyway, EXTRA_LDLIBS should be reserved to users and not used in a Makefile.
> I prefer your initial patch using _LDLIBS.
>
> Any objection to merge initial proposal?
>
LDLIBS should be OK, as long as wrapped with SHARED check. Is following
not working:
-_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
+ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
+ifeq ($(CONFIG_RTE_LIBRTE_IXGBE_PMD),y)
+LDLIBS += -lrte_pmd_ixgbe
+endif
+endif
^ permalink raw reply
* Re: [PATCH] app/testpmd: fix static build link ordering
From: Jerin Jacob @ 2017-01-13 16:01 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Thomas Monjalon, dev, stable
In-Reply-To: <55b009ac-8186-12fe-43b2-b937dc32b8eb@intel.com>
On Fri, Jan 13, 2017 at 03:57:59PM +0000, Ferruh Yigit wrote:
> On 1/13/2017 3:53 PM, Thomas Monjalon wrote:
> > 2017-01-13 08:51, Jerin Jacob:
> >> On Thu, Jan 12, 2017 at 03:27:30PM +0000, Ferruh Yigit wrote:
> >>> On 1/12/2017 1:58 PM, Jerin Jacob wrote:
> >>>> On Thu, Jan 12, 2017 at 10:26:08AM +0100, Thomas Monjalon wrote:
> >>>>> 2017-01-12 13:16, Jerin Jacob:
> >>>>>> +ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
> >>>>>> _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> >>>>>> +endif
> >>>>>
> >>>>> _LDLIBS is an internal variable of rte.app.mk.
> >>>>> Please could you check that there is no issue when using LDLIBS instead
> >>>>> of _LDLIBS?
> >>>>
> >> LDLIBS is not helping the situation as LDLIBS comes before the _LDLIBS-y
> >> mk/rte.app.mk:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
> >>
> >> But moving to EXTRA_LDLIBS looks OK.But it has to be under CONFIG_RTE_LIBRTE_IXGBE_PMD
> >>
> >> Thomas, Ferruh
> >> Let me know if you have any objection on below mentioned diff
> >>
> >> -ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
> >> -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> >> +ifeq ($(CONFIG_RTE_LIBRTE_IXGBE_PMD),y)
> >> +EXTRA_LDLIBS += -lrte_pmd_ixgbe
> >> endif
> >
> > You need to keep the shared lib check.
> > Anyway, EXTRA_LDLIBS should be reserved to users and not used in a Makefile.
> > I prefer your initial patch using _LDLIBS.
> >
> > Any objection to merge initial proposal?
> >
>
> LDLIBS should be OK, as long as wrapped with SHARED check. Is following
> not working:
No, due to the following line
mk/rte.app.mk:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
Again -lrte_pmd_ixgbe comes first.
>
> -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> +ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
> +ifeq ($(CONFIG_RTE_LIBRTE_IXGBE_PMD),y)
> +LDLIBS += -lrte_pmd_ixgbe
> +endif
> +endif
^ permalink raw reply
* Re: [PATCH] app/testpmd: fix static build link ordering
From: Jerin Jacob @ 2017-01-13 16:02 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Ferruh Yigit, dev, stable
In-Reply-To: <2563842.f8ghPspXGG@xps13>
On Fri, Jan 13, 2017 at 04:53:46PM +0100, Thomas Monjalon wrote:
> 2017-01-13 08:51, Jerin Jacob:
> > On Thu, Jan 12, 2017 at 03:27:30PM +0000, Ferruh Yigit wrote:
> > > On 1/12/2017 1:58 PM, Jerin Jacob wrote:
> > > > On Thu, Jan 12, 2017 at 10:26:08AM +0100, Thomas Monjalon wrote:
> > > >> 2017-01-12 13:16, Jerin Jacob:
> > > >>> +ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
> > > >>> _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> > > >>> +endif
> > > >>
> > > >> _LDLIBS is an internal variable of rte.app.mk.
> > > >> Please could you check that there is no issue when using LDLIBS instead
> > > >> of _LDLIBS?
> > > >
> > LDLIBS is not helping the situation as LDLIBS comes before the _LDLIBS-y
> > mk/rte.app.mk:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
> >
> > But moving to EXTRA_LDLIBS looks OK.But it has to be under CONFIG_RTE_LIBRTE_IXGBE_PMD
> >
> > Thomas, Ferruh
> > Let me know if you have any objection on below mentioned diff
> >
> > -ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
> > -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> > +ifeq ($(CONFIG_RTE_LIBRTE_IXGBE_PMD),y)
> > +EXTRA_LDLIBS += -lrte_pmd_ixgbe
> > endif
>
> You need to keep the shared lib check.
> Anyway, EXTRA_LDLIBS should be reserved to users and not used in a Makefile.
> I prefer your initial patch using _LDLIBS.
>
> Any objection to merge initial proposal?
from my side, No
^ permalink raw reply
* Re: [PATCH] scripts: skip capitalization check for commit prefixes
From: Thomas Monjalon @ 2017-01-13 16:08 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
In-Reply-To: <1484312537-22011-1-git-send-email-bruce.richardson@intel.com>
2017-01-13 13:02, Bruce Richardson:
> The prefix in the commit title must be a valid component name and is
> checked in separate checks. For capitalization, just check the part after
> the colon. This is already done for most capitalization checks, just make
> the remainder consistent with this.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] app/testpmd: fix static build link ordering
From: Ferruh Yigit @ 2017-01-13 16:12 UTC (permalink / raw)
To: Jerin Jacob; +Cc: Thomas Monjalon, dev, stable
In-Reply-To: <20170113160124.GA17956@localhost.localdomain>
On 1/13/2017 4:01 PM, Jerin Jacob wrote:
> On Fri, Jan 13, 2017 at 03:57:59PM +0000, Ferruh Yigit wrote:
>> On 1/13/2017 3:53 PM, Thomas Monjalon wrote:
>>> 2017-01-13 08:51, Jerin Jacob:
>>>> On Thu, Jan 12, 2017 at 03:27:30PM +0000, Ferruh Yigit wrote:
>>>>> On 1/12/2017 1:58 PM, Jerin Jacob wrote:
>>>>>> On Thu, Jan 12, 2017 at 10:26:08AM +0100, Thomas Monjalon wrote:
>>>>>>> 2017-01-12 13:16, Jerin Jacob:
>>>>>>>> +ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
>>>>>>>> _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
>>>>>>>> +endif
>>>>>>>
>>>>>>> _LDLIBS is an internal variable of rte.app.mk.
>>>>>>> Please could you check that there is no issue when using LDLIBS instead
>>>>>>> of _LDLIBS?
>>>>>>
>>>> LDLIBS is not helping the situation as LDLIBS comes before the _LDLIBS-y
>>>> mk/rte.app.mk:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
>>>>
>>>> But moving to EXTRA_LDLIBS looks OK.But it has to be under CONFIG_RTE_LIBRTE_IXGBE_PMD
>>>>
>>>> Thomas, Ferruh
>>>> Let me know if you have any objection on below mentioned diff
>>>>
>>>> -ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
>>>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
>>>> +ifeq ($(CONFIG_RTE_LIBRTE_IXGBE_PMD),y)
>>>> +EXTRA_LDLIBS += -lrte_pmd_ixgbe
>>>> endif
>>>
>>> You need to keep the shared lib check.
>>> Anyway, EXTRA_LDLIBS should be reserved to users and not used in a Makefile.
>>> I prefer your initial patch using _LDLIBS.
>>>
>>> Any objection to merge initial proposal?
>>>
>>
>> LDLIBS should be OK, as long as wrapped with SHARED check. Is following
>> not working:
>
> No, due to the following line
> mk/rte.app.mk:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
>
> Again -lrte_pmd_ixgbe comes first.
You are right.
No objection to initial proposal.
^ permalink raw reply
* Re: [PATCH v3 15/29] crypto/qat: use eal I/O device memory read/write API
From: Jerin Jacob @ 2017-01-13 16:21 UTC (permalink / raw)
To: Ferruh Yigit
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin, santosh.shukla, John Griffin, Fiona Trahe,
Deepak Kumar Jain
In-Reply-To: <c10e7cba-badb-3599-9603-c886d43e995d@intel.com>
On Fri, Jan 13, 2017 at 03:50:59PM +0000, Ferruh Yigit wrote:
> On 1/13/2017 2:57 PM, Jerin Jacob wrote:
> > On Fri, Jan 13, 2017 at 11:32:29AM +0000, Ferruh Yigit wrote:
> >> On 1/13/2017 8:17 AM, Jerin Jacob wrote:
> >>> On Thu, Jan 12, 2017 at 07:09:22PM +0000, Ferruh Yigit wrote:
> >>>> Hi Jerin,
> >>>>
> >>>> On 1/12/2017 9:17 AM, Jerin Jacob wrote:
> >>>> <...>
> >>>>
> >>>>> +#include <rte_io.h>
> >>>>> +
> >>>>> /* CSR write macro */
> >>>>> -#define ADF_CSR_WR(csrAddr, csrOffset, val) \
> >>>>> - (void)((*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)) \
> >>>>> - = (val)))
> >>>>> +#define ADF_CSR_WR(csrAddr, csrOffset, val) \
> >>>>> + rte_write32(val, (((uint8_t *)csrAddr) + csrOffset))
> >>>>
> >>>> For IA, this update introduces an extra compiler barrier (rte_io_wmb()),
> >>>> which is indeed not a must, is this correct?
> >>>
> >>> AFAIK, Compiler barrier is required for IA. I am not an IA expert, if
> >>> someone thinks it needs to changed then I can fix it in following commit
> >>> in this patch series by making rte_io_wmb() and rte_io_rmb() as empty.
> >>>
> >>> Let me know.
> >>>
> >>> AFAIK, Linux kernel code has a barrier in readl/writel for IA.
> >>>
> >>> Typically we don't use any non relaxed versions in fast path.In fast
> >>> typically all the drivers has explicit write barrier for doorbell write
> >>> and followed by a relaxed version of write. IMO, In any event, it won't
> >>> generate performance regression.
> >>>
> >>> [dpdk-master] $ git show
> >>> 70c343bdc8c33a51a9db23cd58122bdfc120a58f
> >>> commit 70c343bdc8c33a51a9db23cd58122bdfc120a58f
> >>> Author: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>> Date: Mon Dec 5 06:36:49 2016 +0530
> >>>
> >>> eal/x86: define I/O device memory barriers for IA
> >>>
> >>> The patch does not provide any functional change for IA.
> >>> I/O barriers are mapped to existing smp barriers.
> >>>
> >>> CC: Bruce Richardson <bruce.richardson@intel.com>
> >>> CC: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>>
> >>> diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
> >>> b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
> >>> index 00b1cdf..4eac666 100644
> >>> --- a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
> >>> +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
> >>> @@ -61,6 +61,12 @@ extern "C" {
> >>>
> >>> #define rte_smp_rmb() rte_compiler_barrier()
> >>>
> >>> +#define rte_io_mb() rte_mb()
> >>> +
> >>> +#define rte_io_wmb() rte_compiler_barrier()
> >>> +
> >>> +#define rte_io_rmb() rte_compiler_barrier()
> >>> +
> >>> /*------------------------- 16 bit atomic operations
> >>> * -------------------------*/
> >>>
> >>> #ifndef RTE_FORCE_INTRINSICS
> >>>
> >>>>
> >>>> If so, does it make sense to override these functions for x86, and make
> >>>> rte_writeX = rte_writeX_relaxed
> >>>> rte_readX = rte_readX_relaxed
> >>>>
> >>>>>
> >>>>> /* CSR read macro */
> >>>>> -#define ADF_CSR_RD(csrAddr, csrOffset) \
> >>>>> - (*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)))
> >>>>> +#define ADF_CSR_RD(csrAddr, csrOffset) \
> >>>>> + rte_read32((((uint8_t *)csrAddr) + csrOffset))
> >>>>
> >>>> This patchset both introduces new rte_readX/rte_writeX functions, also
> >>>> applies them into drivers.
> >>>>
> >>>> While applying them, it changes the behavior.
> >>>> Like above code was doing a read, but after update it does read and
> >>>> read_memory_barrier.
> >>>>
> >>>> What do you think this patchset updates usage in a manner that keeps
> >>>> behavior exact same. Like using rte_read32_relaxed for this case.
> >>>> And doing architecture related updates in a different patchset?
> >>>
> >>> Need to use rte_read32 at this commit otherwise it will break for ARM.
> >>> That's was all point for this patchset.
> >>
> >> Why it breaks the ARM, is it because rte_*mb() updated for ARM in this
> >> patchset (patch 7/29) ?
> >
> > Yes.
> >
> >
> >>
> >> I believe it is good to make these modifications in two phase:
> >
> > It is in two phases only. First introduced the API with implementation and
> > enabled in each driver. Why did you think other-way around it is better?
>
> For two things:
> 1- If something goes wrong, find the source of problem easier.
How?
Are you suggesting like this below,
0) Introduce rte_io_?mb()
1) Introduce readxx_relaxed and writexx_relaxed
2) Change all the drivers with readxx_relaxed and writexx_relaxed(15
change sets) to keep the same behavior
3) Introduce readxx and writexx
4) revert step 2 changes and make driver based on readxx and
writexx(again 15 change sets)
Instead of(existing one)
0) Introduce rte_io_?mb()
1) Introduce readxx_relaxed and writexx_relaxed
2) Introduce readxx and writexx
3) Change all the drivers with readxx_[relaxed] and writexx_[relaxed]
Proposed scheme makes driver authors to review two check-ins.
And git bisect fail on fourth case of instead of thrird case with
existing one.
Not sure what we soloving here?
Thomas,
Any thoughts?
> 2- Make architectural changes obvious, right now it is a little hard to
> see, and this again for item 1.
>
> But I also would like to hear more comments before you change/try anything.
>
> > I can rework and test if there is any value addition. If you concerned
> > about git bisect ability then I don't think we are loosing that in this
> > model.
> >
> > Thoughts?
> >
> >> - First replace old usage with rte_readX/rte_writeX while keeping exact
> >> same behavior
> >>
> >> - Second, do architecture specific changes. Both in eal and drivers
> >> level if required.
> >>
> >> Thanks,
> >> ferruh
> >>
> >>> For performance regression, we can always verify by taking delta
> >>> between this changeset and the previous changeset. If you think, I need
> >>> to make rte_io_wmb()/rte_io_rmb() as empty for IA then I could do that
> >>> as well.
> >>>
> >>>
> >>>>
> >>>> This both makes easy to see architecture specific updates, and makes
> >>>> easy to trace any possible performance issues by this patchset.
> >>>>
> >>>>>
> >>>>> #define ADF_BANK_INT_SRC_SEL_MASK_0 0x4444444CUL
> >>>>> #define ADF_BANK_INT_SRC_SEL_MASK_X 0x44444444UL
> >>>>>
> >>>>
> >>
>
^ permalink raw reply
* Re: [PATCH v1] doc: add guidelines on stable and lts releases
From: Thomas Monjalon @ 2017-01-13 16:29 UTC (permalink / raw)
To: Mcnamara, John; +Cc: dev, Liu, Yuanhan
In-Reply-To: <B27915DBBA3421428155699D51E4CFE2026B0556@IRSMSX103.ger.corp.intel.com>
2017-01-13 13:14, Mcnamara, John:
> Just a reminder that DPDK 16.07 was a stable release, 16.11 is a stable
> release and it will also become the 2 year LTS release.
There should be a roadmap for the stable releases,
giving end of life dates.
What about updating this page? http://dpdk.org/dev/roadmap
^ permalink raw reply
* Re: [PATCH] app/testpmd: fix static build link ordering
From: Thomas Monjalon @ 2017-01-13 16:31 UTC (permalink / raw)
To: Jerin Jacob; +Cc: dev, ferruh.yigit, stable
In-Reply-To: <1484207214-13638-1-git-send-email-jerin.jacob@caviumnetworks.com>
2017-01-12 13:16, Jerin Jacob:
> By introducing explicit -lrte_pmd_ixgbe link request in
> testpmd Makefile,"-Wl,-lrte_pmd_ixgbe" provided twice, and linker
> removes the duplication by keeping only first occurrence.
> This moves "-Wl,-lrte_pmd_ixgbe" out of "-Wl,--whole-archive" flag
> and makes symbol generation totally different than previous version
> in case of static build.
> This patch fixes the static build linking order by introducing
> -lrte_pmd_ixgbe under the shared library config
> (CONFIG_RTE_BUILD_SHARED_LIB).
>
> Fixes: 425781ff5afe ("app/testpmd: add ixgbe VF management")
>
> CC: stable@dpdk.org
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Applied, thanks
^ permalink raw reply
* Re: [RFC 0/9] get Rx and Tx used descriptors
From: Olivier Matz @ 2017-01-13 16:44 UTC (permalink / raw)
To: dev
Cc: thomas.monjalon, konstantin.ananyev, wenzhuo.lu, helin.zhang,
Richardson, Bruce
In-Reply-To: <1479981261-19512-1-git-send-email-olivier.matz@6wind.com>
Hi,
On Thu, 24 Nov 2016 10:54:12 +0100, Olivier Matz
<olivier.matz@6wind.com> wrote:
> This RFC patchset introduces a new ethdev API function
> rte_eth_tx_queue_count() which is the tx counterpart of
> rte_eth_rx_queue_count(). It implements this API on some
> Intel drivers for reference, and it also optimizes the
> implementation of rte_eth_rx_queue_count().
>
I'm planning to send a new version of this patchset, fixing the issues
seen by Ferruh, plus a bug fix in the e1000 implementation.
Does anyone have any comment about the new API or about questions
raised in the cover letter? Especially about the real meaning of "used
descriptor": should it include the descriptors hold by the driver?
Any comment about the method (binary search to find the used
descriptors)?
I'm also wondering about adding rte_eth_tx_descriptor_done() in the API
at the same time.
Regards,
Olivier
> The usage of these functions can be:
> - on Rx, anticipate that the cpu is not fast enough to process
> all incoming packets, and take dispositions to solve the
> problem (add more cpus, drop specific packets, ...)
> - on Tx, detect that the link is overloaded, and take dispositions
> to solve the problem (notify flow control, drop specific
> packets)
>
> The tests I've done (instrumenting testpmd) show that browsing
> the descriptors linearly is slow when the ring size increases.
> Accessing the head/tail registers through pci is also slow
> whatever the size of the ring. A binary search is a good compromise
> that gives quite good performance whatever the size of the ring.
>
> Remaining question are about:
> - should we keep this name? I'd say "queue_count" is quite confusing,
> and I would expect it returns the number of queues, not the
> number of used descriptors
> - how shall we count the used descriptors, knowing that the driver
> can hold some to free them by bulk, which reduces the effective
> size of the ring
>
> I would be happy to have some feedback about this RFC before
> I send it as a patch.
>
> Here are some helpers to understand the code more easily (I sometimes
> make some shortcuts between like 1 pkt == 1 desc).
>
> RX side
> =======
>
> - sw advances the tail pointer
> - hw advances the head pointer
> - the software populates the ring with descs to buffers that are
> filled when the hw receives packets
> - head == tail means there is no available buffer for hw to receive a
> packet
> - head points to the next descriptor to be filled
> - hw owns all descriptors between [head...tail]
> - when a packet is written in a descriptor, the DD (descriptor done)
> bit is set, and the head is advanced
> - the driver never reads the head (needs a pci transaction), instead
> it monitors the DD bit of next descriptor
> - when a filled packet is retrieved by the software, the descriptor
> has to be populated with a new empty buffer. This is not done for each
> packet: the driver holds them and waits until it has many
> descriptors to populate, and do it by bulk.
> (by the way, it means that the effective size a queue of size=N is
> lower than N since these descriptors cannot be used by the hw)
>
> rxq->rx_tail: current value of the sw tail (the idx of the next
> packet to be received). The real tail (hw) can be different since the
> driver can hold descriptors.
> rxq->nb_rx_hold: number of held descriptors
> rxq->rxrearm_nb: same, but for vector driver
> rxq->rx_free_thresh: when the number of held descriptors reaches this
> threshold, descriptors are populated with buffers to be filled, and
> sw advances the tail
>
> Example with a ring size of 64:
>
> |----------------------------------------------------------------|
> | xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx |
> | x buffers filled with data by hw x |
> | xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx |
> |----------------------------------------------------------------|
> ^hw_tail=20
> ^sw_tail=35
> ^hw_head=54
> <--- nb_hold -->
> <-pkts in hw queue->
>
> The descriptors marked with 'x' has their DD bit set, the other
> (' ') reference empty buffers.
> The next packet to be received by software is at index 35.
> The software holds 15 descriptors that will be rearmed later.
> There are 19 packets waiting in the hw queue.
>
> We want the function rx_queue_count() to return the number of
> "used" descriptors. The first question is: what does that mean
> exactly? Should it be pkts_in_hw_queue or pkts_in_hw_queue + nb_hold?
> The current implementation returns pkts_in_hw_queue, but including
> nb_hold could be useful to know how many descriptors are really
> free (= size - used).
>
> The current implementation checks the DD bit starting from sw_tail,
> every 4 packets. It can be quite slow for large rings. An alternative
> is to read the head register, but it's also slow.
>
> This patchset optimizes rx_queue_count() by doing a binary
> search (checking for DD) between sw_tail and hw_tail, instead of a
> linear search.
>
> TX side
> =======
>
> - sw advances the tail pointer
> - hw advances the head pointer
> - the software populates the ring with full buffers to be sent by
> the hw
> - head points to the in-progress descriptor.
> - sw writes new descriptors at tail
> - head == tail means that the transmit queue is empty
> - when the hw has processed a descriptor, it sets the DD bit if
> the descriptor has the RS (report status) bit.
> - the driver never reads the head (needs a pci transaction), instead
> it monitors the DD bit of a descriptor that has the RS bit
>
> txq->tx_tail: sw value for tail register
> txq->tx_free_thresh: free buffers if count(free descriptors) < this
> value txq->tx_rs_thresh: RS bit is set every X descriptor
> txq->tx_next_dd: next desc to scan for DD bit
> txq->tx_next_rs: next desc to set RS bit
> txq->last_desc_cleaned: last descriptor that have been cleaned
> txq->nb_tx_free: number of free descriptors
>
> Example:
>
> |----------------------------------------------------------------|
> | D R R R |
> | ............xxxxxxxxxxxxxxxxxxxxxxxxx |
> | <descs sent><- descs not sent yet -> |
> | ............xxxxxxxxxxxxxxxxxxxxxxxxx |
> |----------------------------------------------------------------|
> ^last_desc_cleaned=8 ^next_rs=47
> ^next_dd=15 ^tail=45
> ^hw_head=20
>
> <---- nb_used --------->
>
> The hardware is currently processing the descriptor 20
> 'R' means the descriptor has the RS bit
> 'D' means the descriptor has the DD + RS bits
> 'x' are packets in txq (not sent)
> '.' are packet already sent but not freed by sw
>
> In this example, we have rs_thres=8. On next call to
> ixgbe_tx_free_bufs(), some buffers will be freed.
>
> The new implementation does a binary search (checking for DD) between
> next_dd and tail.
>
>
>
> Olivier Matz (9):
> ethdev: clarify api comments of rx queue count
> ethdev: move queue id check in generic layer
> ethdev: add handler for Tx queue descriptor count
> net/ixgbe: optimize Rx queue descriptor count
> net/ixgbe: add handler for Tx queue descriptor count
> net/igb: optimize rx queue descriptor count
> net/igb: add handler for tx queue descriptor count
> net/e1000: optimize rx queue descriptor count
> net/e1000: add handler for tx queue descriptor count
>
> drivers/net/e1000/e1000_ethdev.h | 10 +++-
> drivers/net/e1000/em_ethdev.c | 1 +
> drivers/net/e1000/em_rxtx.c | 109
> ++++++++++++++++++++++++++++------ drivers/net/e1000/igb_ethdev.c
> | 1 + drivers/net/e1000/igb_rxtx.c | 109
> ++++++++++++++++++++++++++++------ drivers/net/i40e/i40e_rxtx.c
> | 5 -- drivers/net/ixgbe/ixgbe_ethdev.c | 1 +
> drivers/net/ixgbe/ixgbe_ethdev.h | 4 +-
> drivers/net/ixgbe/ixgbe_rxtx.c | 123
> +++++++++++++++++++++++++++++++++------
> drivers/net/ixgbe/ixgbe_rxtx.h | 2 +
> drivers/net/nfp/nfp_net.c | 6 --
> lib/librte_ether/rte_ethdev.h | 48 +++++++++++++-- 12 files
> changed, 344 insertions(+), 75 deletions(-)
>
^ permalink raw reply
* Re: [RFC 0/9] get Rx and Tx used descriptors
From: Richardson, Bruce @ 2017-01-13 17:32 UTC (permalink / raw)
To: Olivier Matz, dev@dpdk.org
Cc: thomas.monjalon@6wind.com, Ananyev, Konstantin, Lu, Wenzhuo,
Zhang, Helin
In-Reply-To: <20170113174409.2f1be0b5@platinum>
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, January 13, 2017 4:44 PM
> To: dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> Helin <helin.zhang@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [RFC 0/9] get Rx and Tx used descriptors
>
> Hi,
>
> On Thu, 24 Nov 2016 10:54:12 +0100, Olivier Matz <olivier.matz@6wind.com>
> wrote:
> > This RFC patchset introduces a new ethdev API function
> > rte_eth_tx_queue_count() which is the tx counterpart of
> > rte_eth_rx_queue_count(). It implements this API on some Intel drivers
> > for reference, and it also optimizes the implementation of
> > rte_eth_rx_queue_count().
> >
>
> I'm planning to send a new version of this patchset, fixing the issues
> seen by Ferruh, plus a bug fix in the e1000 implementation.
>
> Does anyone have any comment about the new API or about questions raised
> in the cover letter? Especially about the real meaning of "used
> descriptor": should it include the descriptors hold by the driver?
For TX, I think we just need used/unused, since for TX any driver will reuse
a slot that has been completed by the NIC, and doesn't hold the mbufs back
for buffering at all.
For RX, strictly speaking, we should have three categories, rather than
trying to work it into 2. I don't see why we can't report a slot as
used/unused/unavailable.
>
> Any comment about the method (binary search to find the used descriptors)?
I think binary search should work ok, though linear search may work better for
smaller ranges as we can prefetch ahead since we know what we will check next.
Linear can also go backward only if we want accuracy (going forward risks having
race conditions between read and NIC write). Overall, though I think binary
search should work well enough.
>
> I'm also wondering about adding rte_eth_tx_descriptor_done() in the API at
> the same time.
>
Let me switch the question around - do we need the queue_count APIs at
all, and is it not more efficient to just supply the descriptor_done() APIs?
If an app wants to know the use of the ring, and take some action based on it,
that app is going to have one or more thresholds for taking the action, right?
In that case, rather than scanning descriptors to find the absolute number
of free/used descriptors, it would be more efficient for the app to just check
the descriptor on the threshold - and take action based just on that value.
Any app that really does need the absolute value of the ring capacity can
presumably do its own binary search or linear search to determine the value
itself. However, I think just doing a done function should encourage people
to use the more efficient solution of just checking the minimum number of
descriptors needed.
Regards,
/Bruce
^ permalink raw reply
* Re: [PATCH] kvargs: make pointers in string arrays const
From: Olivier Matz @ 2017-01-13 17:44 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
In-Reply-To: <1484237907-30717-1-git-send-email-bruce.richardson@intel.com>
On Thu, 12 Jan 2017 16:18:27 +0000, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> Change the parameters of functions from const char *valid[] to
> const char * const valid[]. This additional const is needed to
> allow us to fix some checkpatch warnings, as well as being good
> programming practice.
>
> For the checkpatch warnings, if we have a set of command line
> args that we want to check defined as:
> static const char *args[] = { "arg1", "arg2", NULL };
> kvlist = rte_kvargs_parse(params, args);
>
> checkpatch will complain:
> WARNING:STATIC_CONST_CHAR_ARRAY: static const char *
> array should probably be static const char * const
>
> Adding the additional const to the definition of the args
> will then trigger a compiler error in the absense of this
> change to the kvargs library, as we lose the const in the
> call to kvargs_parse.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
^ permalink raw reply
* Re: [PATCH v1] doc: add guidelines on stable and lts releases
From: Mcnamara, John @ 2017-01-13 17:57 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev@dpdk.org, Liu, Yuanhan
In-Reply-To: <1873516.DJtu264d7Y@xps13>
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, January 13, 2017 4:29 PM
> To: Mcnamara, John <john.mcnamara@intel.com>
> Cc: dev@dpdk.org; Liu, Yuanhan <yuanhan.liu@intel.com>
> Subject: Re: [PATCH v1] doc: add guidelines on stable and lts releases
>
> 2017-01-13 13:14, Mcnamara, John:
> > Just a reminder that DPDK 16.07 was a stable release, 16.11 is a
> > stable release and it will also become the 2 year LTS release.
>
> There should be a roadmap for the stable releases, giving end of life
> dates.
>
> What about updating this page? http://dpdk.org/dev/roadmap
Good point. That was planned. We'll look at pushing up a patch to the Roadmap page.
John
^ permalink raw reply
* Re: [PATCH v3 15/29] crypto/qat: use eal I/O device memory read/write API
From: Ferruh Yigit @ 2017-01-13 18:20 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin, santosh.shukla, John Griffin, Fiona Trahe,
Deepak Kumar Jain
In-Reply-To: <20170113162152.GC17956@localhost.localdomain>
On 1/13/2017 4:21 PM, Jerin Jacob wrote:
> On Fri, Jan 13, 2017 at 03:50:59PM +0000, Ferruh Yigit wrote:
>> On 1/13/2017 2:57 PM, Jerin Jacob wrote:
>>> On Fri, Jan 13, 2017 at 11:32:29AM +0000, Ferruh Yigit wrote:
>>>> On 1/13/2017 8:17 AM, Jerin Jacob wrote:
>>>>> On Thu, Jan 12, 2017 at 07:09:22PM +0000, Ferruh Yigit wrote:
>>>>>> Hi Jerin,
>>>>>>
>>>>>> On 1/12/2017 9:17 AM, Jerin Jacob wrote:
>>>>>> <...>
>>>>>>
>>>>>>> +#include <rte_io.h>
>>>>>>> +
>>>>>>> /* CSR write macro */
>>>>>>> -#define ADF_CSR_WR(csrAddr, csrOffset, val) \
>>>>>>> - (void)((*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)) \
>>>>>>> - = (val)))
>>>>>>> +#define ADF_CSR_WR(csrAddr, csrOffset, val) \
>>>>>>> + rte_write32(val, (((uint8_t *)csrAddr) + csrOffset))
>>>>>>
>>>>>> For IA, this update introduces an extra compiler barrier (rte_io_wmb()),
>>>>>> which is indeed not a must, is this correct?
>>>>>
>>>>> AFAIK, Compiler barrier is required for IA. I am not an IA expert, if
>>>>> someone thinks it needs to changed then I can fix it in following commit
>>>>> in this patch series by making rte_io_wmb() and rte_io_rmb() as empty.
>>>>>
>>>>> Let me know.
>>>>>
>>>>> AFAIK, Linux kernel code has a barrier in readl/writel for IA.
>>>>>
>>>>> Typically we don't use any non relaxed versions in fast path.In fast
>>>>> typically all the drivers has explicit write barrier for doorbell write
>>>>> and followed by a relaxed version of write. IMO, In any event, it won't
>>>>> generate performance regression.
>>>>>
>>>>> [dpdk-master] $ git show
>>>>> 70c343bdc8c33a51a9db23cd58122bdfc120a58f
>>>>> commit 70c343bdc8c33a51a9db23cd58122bdfc120a58f
>>>>> Author: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>> Date: Mon Dec 5 06:36:49 2016 +0530
>>>>>
>>>>> eal/x86: define I/O device memory barriers for IA
>>>>>
>>>>> The patch does not provide any functional change for IA.
>>>>> I/O barriers are mapped to existing smp barriers.
>>>>>
>>>>> CC: Bruce Richardson <bruce.richardson@intel.com>
>>>>> CC: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>>
>>>>> diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>>>> b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>>>> index 00b1cdf..4eac666 100644
>>>>> --- a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>>>> +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
>>>>> @@ -61,6 +61,12 @@ extern "C" {
>>>>>
>>>>> #define rte_smp_rmb() rte_compiler_barrier()
>>>>>
>>>>> +#define rte_io_mb() rte_mb()
>>>>> +
>>>>> +#define rte_io_wmb() rte_compiler_barrier()
>>>>> +
>>>>> +#define rte_io_rmb() rte_compiler_barrier()
>>>>> +
>>>>> /*------------------------- 16 bit atomic operations
>>>>> * -------------------------*/
>>>>>
>>>>> #ifndef RTE_FORCE_INTRINSICS
>>>>>
>>>>>>
>>>>>> If so, does it make sense to override these functions for x86, and make
>>>>>> rte_writeX = rte_writeX_relaxed
>>>>>> rte_readX = rte_readX_relaxed
>>>>>>
>>>>>>>
>>>>>>> /* CSR read macro */
>>>>>>> -#define ADF_CSR_RD(csrAddr, csrOffset) \
>>>>>>> - (*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)))
>>>>>>> +#define ADF_CSR_RD(csrAddr, csrOffset) \
>>>>>>> + rte_read32((((uint8_t *)csrAddr) + csrOffset))
>>>>>>
>>>>>> This patchset both introduces new rte_readX/rte_writeX functions, also
>>>>>> applies them into drivers.
>>>>>>
>>>>>> While applying them, it changes the behavior.
>>>>>> Like above code was doing a read, but after update it does read and
>>>>>> read_memory_barrier.
>>>>>>
>>>>>> What do you think this patchset updates usage in a manner that keeps
>>>>>> behavior exact same. Like using rte_read32_relaxed for this case.
>>>>>> And doing architecture related updates in a different patchset?
>>>>>
>>>>> Need to use rte_read32 at this commit otherwise it will break for ARM.
>>>>> That's was all point for this patchset.
>>>>
>>>> Why it breaks the ARM, is it because rte_*mb() updated for ARM in this
>>>> patchset (patch 7/29) ?
>>>
>>> Yes.
>>>
>>>
>>>>
>>>> I believe it is good to make these modifications in two phase:
>>>
>>> It is in two phases only. First introduced the API with implementation and
>>> enabled in each driver. Why did you think other-way around it is better?
>>
>> For two things:
>> 1- If something goes wrong, find the source of problem easier.
>
> How?
>
> Are you suggesting like this below,
> 0) Introduce rte_io_?mb()
> 1) Introduce readxx_relaxed and writexx_relaxed
> 2) Change all the drivers with readxx_relaxed and writexx_relaxed(15
> change sets) to keep the same behavior
> 3) Introduce readxx and writexx
> 4) revert step 2 changes and make driver based on readxx and
> writexx(again 15 change sets)
Almost yes, instead:
0) Introduce generic rte_io_?mb()
1) Introduce generic readxx_relaxed and writexx_relaxed
2) Introduce generic readxx and writexx
3) Change all the drivers with readxx_relaxed and writexx_relaxed(15
change sets) to keep the same behavior
At this point all same functionality, with new functions.
4) Introduce arch specific rte_io_?mb()
5) Introduce arch specific readxx_relaxed and writexx_relaxed
6) Introduce arch specific readxx and writexx
7) _Update_ step 3 changes and replace some readxx_relaxed and
writexx_relaxed with readxx and writexx in drivers (I expect this to be
less change comparing step 3)
Steps 0,1,2 and 4,5,6 already separated in your patchset.
>
>
> Instead of(existing one)
> 0) Introduce rte_io_?mb()
> 1) Introduce readxx_relaxed and writexx_relaxed
> 2) Introduce readxx and writexx
> 3) Change all the drivers with readxx_[relaxed] and writexx_[relaxed]
>
> Proposed scheme makes driver authors to review two check-ins.
Yes, this brings some extra work, that is why I hesitate.
> And git bisect fail on fourth case of instead of thrird case with
> existing one.
Perhaps you are right here.
>
> Not sure what we soloving here?
Testing first patchset will show if "readxx and writexx" conversion done
correctly or not, and second patchset will show what has been done arch
specific and it's effect.
But not really sure if this worth the effort, so,
If there is no any other objection, let's continue with existing one.
>
> Thomas,
> Any thoughts?
>
>
>> 2- Make architectural changes obvious, right now it is a little hard to
>> see, and this again for item 1.
>>
>> But I also would like to hear more comments before you change/try anything.
>>
>>> I can rework and test if there is any value addition. If you concerned
>>> about git bisect ability then I don't think we are loosing that in this
>>> model.
>>>
>>> Thoughts?
>>>
>>>> - First replace old usage with rte_readX/rte_writeX while keeping exact
>>>> same behavior
>>>>
>>>> - Second, do architecture specific changes. Both in eal and drivers
>>>> level if required.
>>>>
>>>> Thanks,
>>>> ferruh
>>>>
>>>>> For performance regression, we can always verify by taking delta
>>>>> between this changeset and the previous changeset. If you think, I need
>>>>> to make rte_io_wmb()/rte_io_rmb() as empty for IA then I could do that
>>>>> as well.
>>>>>
>>>>>
>>>>>>
>>>>>> This both makes easy to see architecture specific updates, and makes
>>>>>> easy to trace any possible performance issues by this patchset.
>>>>>>
>>>>>>>
>>>>>>> #define ADF_BANK_INT_SRC_SEL_MASK_0 0x4444444CUL
>>>>>>> #define ADF_BANK_INT_SRC_SEL_MASK_X 0x44444444UL
>>>>>>>
>>>>>>
>>>>
>>
^ permalink raw reply
* Re: [PATCH] kvargs: make pointers in string arrays const
From: Thomas Monjalon @ 2017-01-13 18:29 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev, Olivier Matz
In-Reply-To: <20170113184445.7d719306@platinum>
2017-01-13 18:44, Olivier Matz:
> On Thu, 12 Jan 2017 16:18:27 +0000, Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > Change the parameters of functions from const char *valid[] to
> > const char * const valid[]. This additional const is needed to
> > allow us to fix some checkpatch warnings, as well as being good
> > programming practice.
> >
> > For the checkpatch warnings, if we have a set of command line
> > args that we want to check defined as:
> > static const char *args[] = { "arg1", "arg2", NULL };
> > kvlist = rte_kvargs_parse(params, args);
> >
> > checkpatch will complain:
> > WARNING:STATIC_CONST_CHAR_ARRAY: static const char *
> > array should probably be static const char * const
> >
> > Adding the additional const to the definition of the args
> > will then trigger a compiler error in the absense of this
> > change to the kvargs library, as we lose the const in the
> > call to kvargs_parse.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH v9 00/26] Support VFD on i40e - EXPERIMENTAL
From: Ferruh Yigit @ 2017-01-13 18:32 UTC (permalink / raw)
To: Wenzhuo Lu, dev; +Cc: Vincent JARDIN, Zhang, Helin
In-Reply-To: <1484290401-1404-1-git-send-email-wenzhuo.lu@intel.com>
On 1/13/2017 6:52 AM, Wenzhuo Lu wrote:
> 1, VF Daemon (VFD)
> VFD is an idea to control all the VFs from PF.
> As we need to support the scenario kernel PF + DPDK VF, DPDK follows
> the interface between kernel PF + kernel VF.
> We don't want to introduce too many new messages between PF and VF.
> So this patch set adds some new APIs to control VFs directly from PF.
> The new APIs include,
> 1) set VF MAC anti-spoofing
> 2) set VF VLAN anti-spoofing
> 3) set TX loopback
> 4) set VF unicast promiscuous mode
> 5) set VF multicast promiscuous mode
> 6) set VF MTU
> 7) get/reset VF stats
> 8) set VF MAC address
> 9) set VF VLAN stripping
> 10) VF VLAN insertion
> 12) set VF broadcast mode
> 13) set VF VLAN tag
> 14) set VF VLAN filter
> VFD also includes VF to PF mailbox message management by APP.
> When PF receives mailbox messages from VF, PF should call the callback
> provided by APP to know if they're permitted to be processed.
>
> Please aware VFD is an EXPERIMENTAL feature, it can be changed or even
> removed without prior notice.
>
So new version sent with a experimental tag, this can provide some
flexibility to the feature until proper DPDK PF discussion concluded.
Is there any objection to the patchset with experimental tag?
Hi Vincent,
Can this be interim solution until your concerns addressed?
If so, I will proceed working on getting patch into next-net.
Thanks,
ferruh
<...>
> Series-Acked-by: Helin Zhang <helin.zhang@intel.com>
>
^ permalink raw reply
* Re: [PATCH v3 0/8] Add crypto PMD optimized for ARMv8
From: Zbigniew Bodek @ 2017-01-13 18:59 UTC (permalink / raw)
To: Hemant Agrawal, dev; +Cc: pablo.de.lara.guarch, declan.doherty, jerin.jacob
In-Reply-To: <b9eaec28-8c04-9207-c0e3-c7915c70f2cf@nxp.com>
Hello Hemant,
Thank you for your remarks and comments. Please check my answer below.
Kind regards
Zbigniew
On 13.01.2017 09:07, Hemant Agrawal wrote:
> On 1/4/2017 11:03 PM, zbigniew.bodek@caviumnetworks.com wrote:
>> From: Zbigniew Bodek <zbigniew.bodek@caviumnetworks.com>
>>
>> Introduce crypto poll mode driver using ARMv8
>> cryptographic extensions. This PMD is optimized
>> to provide performance boost for chained
>> crypto operations processing, such as:
>> * encryption + HMAC generation
>> * decryption + HMAC validation.
>> In particular, cipher only or hash only
>> operations are not provided.
>
> Do you have a plan to add the crypto only, auth/hash only support into
> this driver?
OpenSSL driver is already implementing that and it is optimized for ARMv8.
> Also, do you plan to add additional cases w.r.t supported by other
> crypto driver?
We may do it in the future but this depends on our resource availability.
>
>> Performance gain can be observed in tests
>> against OpenSSL PMD which also uses ARM
>> crypto extensions for packets processing.
>>
>> Exemplary crypto performance tests comparison:
>>
>> cipher_hash. cipher algo: AES_CBC
>> auth algo: SHA1_HMAC cipher key size=16.
>> burst_size: 64 ops
>>
>> ARMv8 PMD improvement over OpenSSL PMD
>> (Optimized for ARMv8 cipher only and hash
>> only cases):
>>
>> Buffer
>> Size(B) OPS(M) Throughput(Gbps)
>> 64 729 % 742 %
>> 128 577 % 592 %
>> 256 483 % 476 %
>> 512 336 % 351 %
>> 768 300 % 286 %
>> 1024 263 % 250 %
>> 1280 225 % 229 %
>> 1536 214 % 213 %
>> 1792 186 % 203 %
>> 2048 200 % 193 %
>>
>> The driver currently supports AES-128-CBC
>> in combination with: SHA256 HMAC and SHA1 HMAC.
>> The core crypto functionality of this driver is
>> provided by the external armv8_crypto library
>> that can be downloaded from the Cavium repository:
>> https://github.com/caviumnetworks/armv8_crypto
>>
>> CPU compatibility with this virtual device
>> is detected in run-time and virtual crypto
>> device will not be created if CPU doesn't
>> provide AES, SHA1, SHA2 and NEON.
>>
>> The functionality and performance of this
>> code can be tested using generic test application
>> with the following commands:
>> * cryptodev_sw_armv8_autotest
>> * cryptodev_sw_armv8_perftest
>> New test vectors and cases have been added
>> to the general pool. In particular SHA1 and
>> SHA256 HMAC for short cases were introduced.
>> This is because low-level ARM assembly code
>> is using different code paths for long and
>> short data sets, so in order to test the
>> mentioned driver correctly, two different
>> data sets need to be provided.
>>
>> ---
>> v3:
>> * Addressed review remarks
>> * Moved low-level assembly code to the external library
>> * Removed SHA256 MAC cases
>> * Various fixes: interface to the library, digest destination
>> and source address interpreting, missing mbuf manipulations.
>>
>> v2:
>> * Fixed checkpatch warnings
>> * Divide patches into smaller logical parts
>>
>> Zbigniew Bodek (8):
>> mk: fix build of assembly files for ARM64
>> lib: add cryptodev type for the upcoming ARMv8 PMD
>> crypto/armv8: add PMD optimized for ARMv8 processors
>> mk/crypto/armv8: add PMD to the build system
>> doc/armv8: update documentation about crypto PMD
>> crypto/armv8: enable ARMv8 PMD in the configuration
>> crypto/armv8: update MAINTAINERS entry for ARMv8 crypto
>> app/test: add ARMv8 crypto tests and test vectors
>>
>> MAINTAINERS | 6 +
>> app/test/test_cryptodev.c | 63 ++
>> app/test/test_cryptodev_aes_test_vectors.h | 144 +++-
>> app/test/test_cryptodev_blockcipher.c | 4 +
>> app/test/test_cryptodev_blockcipher.h | 1 +
>> app/test/test_cryptodev_perf.c | 480 +++++++++++++
>> config/common_base | 6 +
>> doc/guides/cryptodevs/armv8.rst | 96 +++
>> doc/guides/cryptodevs/index.rst | 1 +
>> doc/guides/rel_notes/release_17_02.rst | 5 +
>> drivers/crypto/Makefile | 1 +
>> drivers/crypto/armv8/Makefile | 73 ++
>> drivers/crypto/armv8/rte_armv8_pmd.c | 926
>> +++++++++++++++++++++++++
>> drivers/crypto/armv8/rte_armv8_pmd_ops.c | 369 ++++++++++
>> drivers/crypto/armv8/rte_armv8_pmd_private.h | 211 ++++++
>> drivers/crypto/armv8/rte_armv8_pmd_version.map | 3 +
>> lib/librte_cryptodev/rte_cryptodev.h | 3 +
>> mk/arch/arm64/rte.vars.mk | 1 -
>> mk/rte.app.mk | 2 +
>> mk/toolchain/gcc/rte.vars.mk | 6 +-
>> 20 files changed, 2390 insertions(+), 11 deletions(-)
>> create mode 100644 doc/guides/cryptodevs/armv8.rst
>> create mode 100644 drivers/crypto/armv8/Makefile
>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd.c
>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd_ops.c
>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd_private.h
>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd_version.map
>>
>
>
^ 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