* Re: [PATCH] test: adding AES cipher-only tests on QAT PMD
From: Kusztal, ArkadiuszX @ 2016-11-25 14:13 UTC (permalink / raw)
To: Trahe, Fiona, dev@dpdk.org; +Cc: De Lara Guarch, Pablo, Griffin, John
In-Reply-To: <1480012164-17395-1-git-send-email-fiona.trahe@intel.com>
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Thursday, November 24, 2016 6:29 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Griffin, John <john.griffin@intel.com>; Kusztal,
> ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Subject: [PATCH] test: adding AES cipher-only tests on QAT PMD
>
> Extended functional AES-CBC and AES-CTR cipher-only tests to run on QAT
> PMD.
> Added AES_CBC cipher-only performance tests on QAT PMD.
> No driver changes, but as now tested, QAT documentation is updated to
> remove constraint.
>
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
> app/test/test_cryptodev.c | 18 ++++++
> app/test/test_cryptodev_aes_test_vectors.h | 36 +++++++----
> app/test/test_cryptodev_perf.c | 96 +++++++++++++++++++----------
> -
> doc/guides/cryptodevs/qat.rst | 1 -
> 4 files changed, 102 insertions(+), 49 deletions(-)
>
> --
> 2.5.0
Acked-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Andrew Rybchenko @ 2016-11-25 13:23 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Ferruh Yigit, dev, Bruce Richardson
In-Reply-To: <4008902.bqyKbWRKzy@xps13>
On 11/25/2016 04:00 PM, Thomas Monjalon wrote:
> 2016-11-25 12:43, Ferruh Yigit:
>> On 11/25/2016 12:02 PM, Andrew Rybchenko wrote:
>>> On 11/25/2016 01:24 PM, Ferruh Yigit wrote:
>>>> On 11/23/2016 7:49 AM, Andrew Rybchenko wrote:
>>>>> On 11/23/2016 03:02 AM, Ferruh Yigit wrote:
>>>>>> Also folder structure is drivers/net/sfc/efx/<all_src_files>, why /sfc/
>>>>>> layer is created?
>>>>>> sfc is company name (solarflare communications), right? Other driver
>>>>>> folders not structured based on company, what about using
>>>>>> drivers/net/efx/* ?
>>>>> I've tried to explain it above in item (2):
>>>>>
>>>>> >>>
>>>>>
>>>>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>>>>> is considered and could be added in the future. Code for data path
>>>>> should be shared by these two drivers. libefx-based PMD is put into
>>>>> 'efx' subdirectory to have a space for another PMD and shared code.
>>>>>
>>>>> <<<
>>>>>
>>>>> So, main reason is to have location for the code shared by two Solarflare
>>>>> network PMDs. May be it better to relocate when we really have it.
>>>>> I'm open for other ideas/suggestions.
>>>> If there will be another PMD that shares code with current one, the
>>>> logic seems good, but I am not sure about start using company names, I
>>>> am not against it, just I don't know.
>>> I think that mlx4 and mlx5 are tightly bound to the company name (plus
>>> adapter generation, I guess).
>>>
>>>> Let's relocate later, this buys some time to think / get feedback on issue.
>>> Do I understand correctly that you suggest to avoid extra level inside
>>> for now
>>> and relocate later if required? If so, that's fine for me.
>>>
>>> As for naming, we think that just "efx" is a bad idea. The prefix is
>>> occupied by
>>> the libefx functions and driver should use something else. We have chosen
>>> "sfc" mainly to follow naming used in Linux kernel for Solarflare driver
>>> (the first level of Ethernet driver names is company bound in the Linux
>>> kernel).
>>> If we avoid extra level as discussed above, I think "sfc_efx", "sfcefx"
>>> (may be it
>>> will look better nearby other drivers) or just "sfc" are fine for us.
>>>
>> Thomas, Bruce, any comment on this?
> You can add multiple drivers in the same library. As an example, ixgbe
> directory have several drivers for PF/VF, scalar/vector, etc.
> If you really want separate directories for your drivers while sharing some
> code, you can link some files from the other directory.
Thanks for ideas/examples. I'll remove extra level.
> About the name of this directory, I have no strong opinion.
> sfcefx looks good.
If there is no strong opinion against "sfc" , we'd prefer to keep it:
1. sfc is used in Linux (and, hopefully, well know)
2. sfc is shorter and already used in sources
3. sfc seems to be more future proof if we would like to add more drivers
inside (efx binds it to libefx)
4. sfc could be read as well as Solarflare Flareon Controller ;)
Andrew.
^ permalink raw reply
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Thomas Monjalon @ 2016-11-25 13:09 UTC (permalink / raw)
To: Bruce Richardson, Jerin Jacob
Cc: dev, harry.van.haaren, hemant.agrawal, gage.eads
In-Reply-To: <20161125110053.GA149796@bricha3-MOBL3.ger.corp.intel.com>
2016-11-25 11:00, Bruce Richardson:
> On Fri, Nov 25, 2016 at 05:53:34AM +0530, Jerin Jacob wrote:
> > On Thu, Nov 24, 2016 at 04:35:56PM +0100, Thomas Monjalon wrote:
> > > 2016-11-24 07:29, Jerin Jacob:
> > > > On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > > > > 2016-11-18 11:14, Jerin Jacob:
> > > > > > +#define EVENTDEV_NAME_SKELETON_PMD event_skeleton
> > > > > > +/**< Skeleton event device PMD name */
> > > > >
> > > > > I do not understand this #define.
> > > >
> > > > Applications can explicitly request the a specific driver though driver
> > > > name. This will go as argument to rte_event_dev_get_dev_id(const char *name).
> > > > The reason for keeping this #define in rte_eventdev.h is that,
> > > > application needs to include only rte_eventdev.h not rte_eventdev_pmd.h.
> > >
> > > So each driver must register its name in the API?
> > > Is it really needed?
> >
> > Otherwise how application knows the name of the driver.
> > The similar scheme used in cryptodev.
> > http://dpdk.org/browse/dpdk/tree/lib/librte_cryptodev/rte_cryptodev.h#n53
> > No strong opinion here. Open for suggestions.
> >
>
> I like having a name registered. I think we need a scheme where an app
> can find and use an implementation using a specific driver.
I do not like having the driver names in the API.
An API should not know its drivers.
If an application do some driver-specific processing, it knows
the driver name as well. The driver name is written in the driver.
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Thomas Monjalon @ 2016-11-25 13:00 UTC (permalink / raw)
To: Andrew Rybchenko; +Cc: Ferruh Yigit, dev, Bruce Richardson
In-Reply-To: <bf0be13a-c424-a559-1c9e-43fff3df3c65@intel.com>
2016-11-25 12:43, Ferruh Yigit:
> On 11/25/2016 12:02 PM, Andrew Rybchenko wrote:
> > On 11/25/2016 01:24 PM, Ferruh Yigit wrote:
> >> On 11/23/2016 7:49 AM, Andrew Rybchenko wrote:
> >>> On 11/23/2016 03:02 AM, Ferruh Yigit wrote:
> >>>> Also folder structure is drivers/net/sfc/efx/<all_src_files>, why /sfc/
> >>>> layer is created?
> >>>> sfc is company name (solarflare communications), right? Other driver
> >>>> folders not structured based on company, what about using
> >>>> drivers/net/efx/* ?
> >>> I've tried to explain it above in item (2):
> >>>
> >>> >>>
> >>>
> >>> 2. Another Solarflare PMD with in-kernel part (for control operations)
> >>> is considered and could be added in the future. Code for data path
> >>> should be shared by these two drivers. libefx-based PMD is put into
> >>> 'efx' subdirectory to have a space for another PMD and shared code.
> >>>
> >>> <<<
> >>>
> >>> So, main reason is to have location for the code shared by two Solarflare
> >>> network PMDs. May be it better to relocate when we really have it.
> >>> I'm open for other ideas/suggestions.
> >> If there will be another PMD that shares code with current one, the
> >> logic seems good, but I am not sure about start using company names, I
> >> am not against it, just I don't know.
> >
> > I think that mlx4 and mlx5 are tightly bound to the company name (plus
> > adapter generation, I guess).
> >
> >> Let's relocate later, this buys some time to think / get feedback on issue.
> >
> > Do I understand correctly that you suggest to avoid extra level inside
> > for now
> > and relocate later if required? If so, that's fine for me.
> >
> > As for naming, we think that just "efx" is a bad idea. The prefix is
> > occupied by
> > the libefx functions and driver should use something else. We have chosen
> > "sfc" mainly to follow naming used in Linux kernel for Solarflare driver
> > (the first level of Ethernet driver names is company bound in the Linux
> > kernel).
> > If we avoid extra level as discussed above, I think "sfc_efx", "sfcefx"
> > (may be it
> > will look better nearby other drivers) or just "sfc" are fine for us.
> >
>
> Thomas, Bruce, any comment on this?
You can add multiple drivers in the same library. As an example, ixgbe
directory have several drivers for PF/VF, scalar/vector, etc.
If you really want separate directories for your drivers while sharing some
code, you can link some files from the other directory.
About the name of this directory, I have no strong opinion.
sfcefx looks good.
^ permalink raw reply
* Re: [PATCH 00/16] e1000 base code update
From: Ferruh Yigit @ 2016-11-25 12:58 UTC (permalink / raw)
To: Wenzhuo Lu, dev
In-Reply-To: <1479921780-9813-1-git-send-email-wenzhuo.lu@intel.com>
Hi Wenzhuo,
On 11/23/2016 5:22 PM, Wenzhuo Lu wrote:
> Updated e1000 base code to fix several bugs and support
> i219 NICs.
>
> Wenzhuo Lu (16):
> e1000/base: increased ULP timer
> e1000/base: increase PHY PLL clock gate timing
> e1000/base: try more times to get HW mailbox lock
> e1000/base: add getting HW version support for i354
> e1000/base: expose e1000_write_vfta_i350
> e1000/base: add max RX jumbo frame define
> e1000/base: restore link speed after ULP exit
> e1000/base: clear ULP configuration register on ULP exit
> e1000/base: increase LANPHYPC low duration
> e1000/base: workaround for ULP entry flow
> e1000/base: enable new i219 devices
> e1000/base: always request clock during K1 at 1G link speed
> e1000/base: ability to force K1-off disabled
> e1000/base: support more i219 devices
> e1000/base: update readme
> e1000: add new i219 devices
>
> drivers/net/e1000/base/README | 4 +-
> drivers/net/e1000/base/e1000_82575.c | 1 -
> drivers/net/e1000/base/e1000_82575.h | 1 +
> drivers/net/e1000/base/e1000_api.c | 19 +
> drivers/net/e1000/base/e1000_defines.h | 9 +
> drivers/net/e1000/base/e1000_hw.h | 21 +-
> drivers/net/e1000/base/e1000_ich8lan.c | 865 +++++++++++++++++++++++++++++++--
> drivers/net/e1000/base/e1000_ich8lan.h | 21 +-
> drivers/net/e1000/base/e1000_mbx.c | 36 +-
> drivers/net/e1000/base/e1000_nvm.c | 1 +
> drivers/net/e1000/base/e1000_regs.h | 7 +
> drivers/net/e1000/em_ethdev.c | 34 +-
> 12 files changed, 949 insertions(+), 70 deletions(-)
>
Based on this pathset.
Can you also please send another patch to:
1- add I219 to supported nics list
2- announce new supported nic in release notes.
Also as far as I can see there is no igb/e1000 documentation under
doc/guides/nics/*, it can good to provide one, not with above requested
patches perhaps, but in some suitable time.
Thanks,
ferruh
^ permalink raw reply
* Re: [PATCH 00/16] e1000 base code update
From: Ferruh Yigit @ 2016-11-25 12:54 UTC (permalink / raw)
To: Wenzhuo Lu, dev
In-Reply-To: <1479921780-9813-1-git-send-email-wenzhuo.lu@intel.com>
On 11/23/2016 5:22 PM, Wenzhuo Lu wrote:
> Updated e1000 base code to fix several bugs and support
> i219 NICs.
>
> Wenzhuo Lu (16):
> e1000/base: increased ULP timer
> e1000/base: increase PHY PLL clock gate timing
> e1000/base: try more times to get HW mailbox lock
> e1000/base: add getting HW version support for i354
> e1000/base: expose e1000_write_vfta_i350
> e1000/base: add max RX jumbo frame define
> e1000/base: restore link speed after ULP exit
> e1000/base: clear ULP configuration register on ULP exit
> e1000/base: increase LANPHYPC low duration
> e1000/base: workaround for ULP entry flow
> e1000/base: enable new i219 devices
> e1000/base: always request clock during K1 at 1G link speed
> e1000/base: ability to force K1-off disabled
> e1000/base: support more i219 devices
> e1000/base: update readme
> e1000: add new i219 devices
>
> drivers/net/e1000/base/README | 4 +-
> drivers/net/e1000/base/e1000_82575.c | 1 -
> drivers/net/e1000/base/e1000_82575.h | 1 +
> drivers/net/e1000/base/e1000_api.c | 19 +
> drivers/net/e1000/base/e1000_defines.h | 9 +
> drivers/net/e1000/base/e1000_hw.h | 21 +-
> drivers/net/e1000/base/e1000_ich8lan.c | 865 +++++++++++++++++++++++++++++++--
> drivers/net/e1000/base/e1000_ich8lan.h | 21 +-
> drivers/net/e1000/base/e1000_mbx.c | 36 +-
> drivers/net/e1000/base/e1000_nvm.c | 1 +
> drivers/net/e1000/base/e1000_regs.h | 7 +
> drivers/net/e1000/em_ethdev.c | 34 +-
> 12 files changed, 949 insertions(+), 70 deletions(-)
>
Series applied to dpdk-next-net/master, thanks.
Some modifications done in both commit subject and logs, can you please
double check the updates.
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Andrew Rybchenko @ 2016-11-25 12:44 UTC (permalink / raw)
To: Ferruh Yigit, dev
In-Reply-To: <9cdced57-ba4c-fd0e-c509-904d333aab50@intel.com>
On 11/23/2016 03:02 AM, Ferruh Yigit wrote:
> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>> The patch series adds Solarflare libefx-based network PMD.
>>
>> This version of the driver supports Solarflare SFN7xxx and SFN8xxx
>> families of 10/40 Gbps adapters.
>>
>> libefx is a platform-independent library to implement drivers for
>> Solarflare network adapters. It provides unified adapter family
>> independent interface (if possible). FreeBSD [1] and illumos [2]
>> drivers are built on top of the library.
>>
>> The patch series could be logically structured into 5 sub-series:
>> 1. (1) add the driver skeleton including documentation
>> 2. (2-30) import libefx and include it in build with the latest patch
>> 3. (31-43) implement minimal device level operations in steps
>> 4. (44-51) implement Rx subsystem
>> 5. (52-56) implement Tx subsystem
>>
>> Functional driver with multi-queue support capable to send and receive
>> traffic appears with the last patch in the series.
>>
>> The following design decisions are made during development:
>>
>> 1. Since libefx uses positive errno return codes, positive errno
>> return codes are used inside the driver and coversion to negative
>> is done on return from eth_dev_ops callbacks. We think that it
>> is the less error-prone way.
>>
>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>> is considered and could be added in the future. Code for data path
>> should be shared by these two drivers. libefx-based PMD is put into
>> 'efx' subdirectory to have a space for another PMD and shared code.
>>
>> 3. Own event queue (a way to deliver events from HW to host CPU) is
>> used for house-keeping (e.g. link status notifications), each Tx
>> and each Rx queue. No locks on datapath are requires in this case.
>>
>> 4. Alarm is used to periodically poll house-keeping event queue.
>> The event queue is used to deliver link status change notifications,
>> Rx/Tx queue flush events, SRAM events. It is not used on datapath.
>> The event queue polling is protected using spin-lock since
>> concurrent access from different contexts is possible (e.g. device
>> stop when polling alarm is running).
>>
>> [1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/
>> [2] https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/
>>
>> ---
>>
>> Andrew Rybchenko (49):
>> net/sfc: libefx-based PMD stub sufficient to build and init
>> net/sfc: import libefx base
>> net/sfc: import libefx register definitions
>> net/sfc: import libefx filters support
>> net/sfc: import libefx MCDI definition
>> net/sfc: import libefx MCDI implementation
>> net/sfc: import libefx MCDI logging support
>> net/sfc: import libefx MCDI proxy authorization support
>> net/sfc: import libefx 5xxx/6xxx family support
>> net/sfc: import libefx SFN7xxx family support
>> net/sfc: import libefx SFN8xxx family support
>> net/sfc: import libefx diagnostics support
>> net/sfc: import libefx built-in selftest support
>> net/sfc: import libefx software per-queue statistics support
>> net/sfc: import libefx PHY flags control support
>> net/sfc: import libefx PHY statistics support
>> net/sfc: import libefx PHY LEDs control support
>> net/sfc: import libefx MAC statistics support
>> net/sfc: import libefx event prefetch support
>> net/sfc: import libefx Rx scatter support
>> net/sfc: import libefx RSS support
>> net/sfc: import libefx loopback control support
>> net/sfc: import libefx monitors statistics support
>> net/sfc: import libefx support to access monitors via MCDI
>> net/sfc: import libefx support for Rx packed stream mode
>> net/sfc: import libefx NVRAM support
>> net/sfc: import libefx VPD support
>> net/sfc: import libefx bootrom configuration support
>> net/sfc: import libefx licensing support
>> net/sfc: implement dummy callback to get device information
>> net/sfc: implement driver operation to init device on attach
>> net/sfc: add device configure and close stubs
>> net/sfc: add device configuration checks
>> net/sfc: implement device start and stop operations
>> net/sfc: make available resources estimation and allocation
>> net/sfc: interrupts support sufficient for event queue init
>> net/sfc: implement event queue support
>> net/sfc: implement EVQ dummy exception handling
>> net/sfc: maintain management event queue
>> net/sfc: periodic management EVQ polling using alarm
>> net/sfc: minimum port control sufficient to receive traffic
>> net/sfc: implement Rx subsystem stubs
>> net/sfc: check configured rxmode
>> net/sfc: implement Rx queue setup release operations
>> net/sfc: calculate Rx buffer size which may be used
>> net/sfc: validate Rx queue buffers setup
>> net/sfc: implement Rx queue start and stop operations
>> net/sfc: implement device callback to Rx burst of packets
>> net/sfc: discard scattered packet on Rx correctly
>>
>> Artem Andreev (2):
>> net/sfc: include libefx in build
>> net/sfc: implement device operation to retrieve link info
>>
>> Ivan Malov (5):
>> net/sfc: provide basic stubs for Tx subsystem
>> net/sfc: add function to check configured Tx mode
>> net/sfc: add callbacks to set up and release Tx queues
>> net/sfc: implement transmit path start / stop
>> net/sfc: add callback to send bursts of packets
>>
> Hi Andrew,
>
> Thank you for the patch, I have encounter with a few minor issues, can
> you please check them [1]?
>
> Also folder structure is drivers/net/sfc/efx/<all_src_files>, why /sfc/
> layer is created?
> sfc is company name (solarflare communications), right? Other driver
> folders not structured based on company, what about using
> drivers/net/efx/* ?
>
>
> [1]:
> 1- There are a few (non-base) checkpatch warnings, can you please check
> patch 36, 49, 50 and 55 please?
>
> 2- Got following compile issues, not investigated, directly sharing here:
>
> <...>
>
> c) icc compiler errors:
> =======================================
> In file included from
> .../x86_64-native-linuxapp-icc/include/rte_ethdev.h(185),
> from .../drivers/net/sfc/efx/sfc.h(35),
> from .../drivers/net/sfc/efx/sfc.c(37):
> .../x86_64-native-linuxapp-icc/include/rte_ether.h(258): warning #2203:
> cast discards qualifiers from target type
> uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes);
> ^
I think this one is not mine.
> .../drivers/net/sfc/efx/base/efx_mcdi.c(1157): warning #3179: deprecated
> conversion of string literal to char* (should be const char*)
> .../drivers/net/sfc/efx/base/ef10_filter.c(1276): warning #188:
> enumerated type mixed with another type
> : "unknown assertion";
> ^
It looks like output is mixed here. Looking at the code I think the
first is false
warning. Assigned variable is 'const char *' and all assigned values are
string literals.
> filter_flags = 0;
> ^
Will fix in v2.
> .../drivers/net/sfc/efx/base/efx_mcdi.c(1426): warning #188: enumerated
> type mixed with another type
> epp->ep_fixed_port_type =
> ^
Wil try to fix in v2.
> .../drivers/net/sfc/efx/base/efx_nic.c(556): warning #188: enumerated
> type mixed with another type
> enp->en_family = 0;
> ^
Will fix in v2.
Andrew.
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Ferruh Yigit @ 2016-11-25 12:43 UTC (permalink / raw)
To: Andrew Rybchenko, dev; +Cc: Thomas Monjalon, Bruce Richardson
In-Reply-To: <edfdf094-9c4d-b019-2239-5d6c94722384@solarflare.com>
On 11/25/2016 12:02 PM, Andrew Rybchenko wrote:
> On 11/25/2016 01:24 PM, Ferruh Yigit wrote:
>> On 11/23/2016 7:49 AM, Andrew Rybchenko wrote:
>>> On 11/23/2016 03:02 AM, Ferruh Yigit wrote:
>>>> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>>>>> The patch series adds Solarflare libefx-based network PMD.
>>>>>
>>>>> This version of the driver supports Solarflare SFN7xxx and SFN8xxx
>>>>> families of 10/40 Gbps adapters.
>>>>>
>>>>> libefx is a platform-independent library to implement drivers for
>>>>> Solarflare network adapters. It provides unified adapter family
>>>>> independent interface (if possible). FreeBSD [1] and illumos [2]
>>>>> drivers are built on top of the library.
>>>>>
>>>>> The patch series could be logically structured into 5 sub-series:
>>>>> 1. (1) add the driver skeleton including documentation
>>>>> 2. (2-30) import libefx and include it in build with the latest patch
>>>>> 3. (31-43) implement minimal device level operations in steps
>>>>> 4. (44-51) implement Rx subsystem
>>>>> 5. (52-56) implement Tx subsystem
>>>>>
>>>>> Functional driver with multi-queue support capable to send and receive
>>>>> traffic appears with the last patch in the series.
>>>>>
>>>>> The following design decisions are made during development:
>>>>>
>>>>> 1. Since libefx uses positive errno return codes, positive errno
>>>>> return codes are used inside the driver and coversion to negative
>>>>> is done on return from eth_dev_ops callbacks. We think that it
>>>>> is the less error-prone way.
>>>>>
>>>>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>>>>> is considered and could be added in the future. Code for data path
>>>>> should be shared by these two drivers. libefx-based PMD is put into
>>>>> 'efx' subdirectory to have a space for another PMD and shared code.
>>>>>
>>>>> 3. Own event queue (a way to deliver events from HW to host CPU) is
>>>>> used for house-keeping (e.g. link status notifications), each Tx
>>>>> and each Rx queue. No locks on datapath are requires in this case.
>>>>>
>>>>> 4. Alarm is used to periodically poll house-keeping event queue.
>>>>> The event queue is used to deliver link status change notifications,
>>>>> Rx/Tx queue flush events, SRAM events. It is not used on datapath.
>>>>> The event queue polling is protected using spin-lock since
>>>>> concurrent access from different contexts is possible (e.g. device
>>>>> stop when polling alarm is running).
>>>>>
>>>>> [1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/
>>>>> [2] https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/
>>>>>
>>>>> ---
>>>>>
>>>>> Andrew Rybchenko (49):
>>>>> net/sfc: libefx-based PMD stub sufficient to build and init
>>>>> net/sfc: import libefx base
>>>>> net/sfc: import libefx register definitions
>>>>> net/sfc: import libefx filters support
>>>>> net/sfc: import libefx MCDI definition
>>>>> net/sfc: import libefx MCDI implementation
>>>>> net/sfc: import libefx MCDI logging support
>>>>> net/sfc: import libefx MCDI proxy authorization support
>>>>> net/sfc: import libefx 5xxx/6xxx family support
>>>>> net/sfc: import libefx SFN7xxx family support
>>>>> net/sfc: import libefx SFN8xxx family support
>>>>> net/sfc: import libefx diagnostics support
>>>>> net/sfc: import libefx built-in selftest support
>>>>> net/sfc: import libefx software per-queue statistics support
>>>>> net/sfc: import libefx PHY flags control support
>>>>> net/sfc: import libefx PHY statistics support
>>>>> net/sfc: import libefx PHY LEDs control support
>>>>> net/sfc: import libefx MAC statistics support
>>>>> net/sfc: import libefx event prefetch support
>>>>> net/sfc: import libefx Rx scatter support
>>>>> net/sfc: import libefx RSS support
>>>>> net/sfc: import libefx loopback control support
>>>>> net/sfc: import libefx monitors statistics support
>>>>> net/sfc: import libefx support to access monitors via MCDI
>>>>> net/sfc: import libefx support for Rx packed stream mode
>>>>> net/sfc: import libefx NVRAM support
>>>>> net/sfc: import libefx VPD support
>>>>> net/sfc: import libefx bootrom configuration support
>>>>> net/sfc: import libefx licensing support
>>>>> net/sfc: implement dummy callback to get device information
>>>>> net/sfc: implement driver operation to init device on attach
>>>>> net/sfc: add device configure and close stubs
>>>>> net/sfc: add device configuration checks
>>>>> net/sfc: implement device start and stop operations
>>>>> net/sfc: make available resources estimation and allocation
>>>>> net/sfc: interrupts support sufficient for event queue init
>>>>> net/sfc: implement event queue support
>>>>> net/sfc: implement EVQ dummy exception handling
>>>>> net/sfc: maintain management event queue
>>>>> net/sfc: periodic management EVQ polling using alarm
>>>>> net/sfc: minimum port control sufficient to receive traffic
>>>>> net/sfc: implement Rx subsystem stubs
>>>>> net/sfc: check configured rxmode
>>>>> net/sfc: implement Rx queue setup release operations
>>>>> net/sfc: calculate Rx buffer size which may be used
>>>>> net/sfc: validate Rx queue buffers setup
>>>>> net/sfc: implement Rx queue start and stop operations
>>>>> net/sfc: implement device callback to Rx burst of packets
>>>>> net/sfc: discard scattered packet on Rx correctly
>>>>>
>>>>> Artem Andreev (2):
>>>>> net/sfc: include libefx in build
>>>>> net/sfc: implement device operation to retrieve link info
>>>>>
>>>>> Ivan Malov (5):
>>>>> net/sfc: provide basic stubs for Tx subsystem
>>>>> net/sfc: add function to check configured Tx mode
>>>>> net/sfc: add callbacks to set up and release Tx queues
>>>>> net/sfc: implement transmit path start / stop
>>>>> net/sfc: add callback to send bursts of packets
>>>>>
>>>> Hi Andrew,
>>>>
>>>> Thank you for the patch, I have encounter with a few minor issues, can
>>>> you please check them [1]?
>>>>
>>>> Also folder structure is drivers/net/sfc/efx/<all_src_files>, why /sfc/
>>>> layer is created?
>>>> sfc is company name (solarflare communications), right? Other driver
>>>> folders not structured based on company, what about using
>>>> drivers/net/efx/* ?
>>> I've tried to explain it above in item (2):
>>>
>>> >>>
>>>
>>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>>> is considered and could be added in the future. Code for data path
>>> should be shared by these two drivers. libefx-based PMD is put into
>>> 'efx' subdirectory to have a space for another PMD and shared code.
>>>
>>> <<<
>>>
>>> So, main reason is to have location for the code shared by two Solarflare
>>> network PMDs. May be it better to relocate when we really have it.
>>> I'm open for other ideas/suggestions.
>> If there will be another PMD that shares code with current one, the
>> logic seems good, but I am not sure about start using company names, I
>> am not against it, just I don't know.
>
> I think that mlx4 and mlx5 are tightly bound to the company name (plus
> adapter generation, I guess).
>
>> Let's relocate later, this buys some time to think / get feedback on issue.
>
> Do I understand correctly that you suggest to avoid extra level inside
> for now
> and relocate later if required? If so, that's fine for me.
>
> As for naming, we think that just "efx" is a bad idea. The prefix is
> occupied by
> the libefx functions and driver should use something else. We have chosen
> "sfc" mainly to follow naming used in Linux kernel for Solarflare driver
> (the first level of Ethernet driver names is company bound in the Linux
> kernel).
> If we avoid extra level as discussed above, I think "sfc_efx", "sfcefx"
> (may be it
> will look better nearby other drivers) or just "sfc" are fine for us.
>
Thomas, Bruce, any comment on this?
>>>> [1]:
>>>> 1- There are a few (non-base) checkpatch warnings, can you please check
>>>> patch 36, 49, 50 and 55 please?
>>> Thanks, I'll fix spelling in v2.
>>> 36, 49 and 55 also ask to check multiple assignments. IMHO, it is the
>>> right usage
>>> of multiple assignment when logically bound variables must have the same
>>> value.
>>>
>>>> 2- Got following compile issues, not investigated, directly sharing here:
>>>>
>>>> b) for icc getting following warnings:
>>>> =======================================
>>>> icc: command line warning #10006: ignoring unknown option '-Wno-empty-body'
>>>> icc: command line warning #10006: ignoring unknown option
>>>> '-Waggregate-return'
>>>> icc: command line warning #10006: ignoring unknown option
>>>> '-Wbad-function-cast'
>>>> icc: command line warning #10006: ignoring unknown option '-Wnested-externs'
>>>>
>>>>
>>>> c) icc compiler errors:
>>>> =======================================
>>>> In file included from
>>>> .../x86_64-native-linuxapp-icc/include/rte_ethdev.h(185),
>>>> from .../drivers/net/sfc/efx/sfc.h(35),
>>>> from .../drivers/net/sfc/efx/sfc.c(37):
>>>> .../x86_64-native-linuxapp-icc/include/rte_ether.h(258): warning #2203:
>>>> cast discards qualifiers from target type
>>>> uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes);
>>>> ^
>>>>
>>>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1157): warning #3179: deprecated
>>>> conversion of string literal to char* (should be const char*)
>>>> .../drivers/net/sfc/efx/base/ef10_filter.c(1276): warning #188:
>>>> enumerated type mixed with another type
>>>> : "unknown assertion";
>>>> ^
>>>>
>>>> filter_flags = 0;
>>>> ^
>>>>
>>>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1426): warning #188: enumerated
>>>> type mixed with another type
>>>> epp->ep_fixed_port_type =
>>>> ^
>>>>
>>>> .../drivers/net/sfc/efx/base/efx_nic.c(556): warning #188: enumerated
>>>> type mixed with another type
>>>> enp->en_family = 0;
>>> Yes, I have no ICC compilers. I'll try to fix these warnings, but I
>>> can't be sure without checking it.
>>> Also we cannot claim ICC supported without building and testing the
>>> generated binary.
>> Please update the code at least to not break the icc compilation,
>> specially since this PMD will be default enabled. If you prefer I can
>> verify compilation offline before you send the patchset.
>
> I'll do. I would be thankful if you help with ICC check. Should I send the
> patch series directly to you?
Just for icc check, you can send directly to me to reduce the noise in
the list, or you can send to the list, both are OK for me.
>
> Andrew.
^ permalink raw reply
* Re: [PATCH v3] kni: avoid using lsb_release script
From: Ferruh Yigit @ 2016-11-25 12:24 UTC (permalink / raw)
To: Robin Jarry; +Cc: dev
In-Reply-To: <1480076537-16630-1-git-send-email-robin.jarry@6wind.com>
On 11/25/2016 12:22 PM, Robin Jarry wrote:
> The lsb_release script is part of an optional package which is not
> always installed. On the other hand, /etc/lsb-release is always present
> even on minimal Ubuntu installations.
>
> root@ubuntu1604:~# dpkg -S /etc/lsb-release
> base-files: /etc/lsb-release
>
> Read the file if present and use the variables defined in it.
>
> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
^ permalink raw reply
* [PATCH v3] kni: avoid using lsb_release script
From: Robin Jarry @ 2016-11-25 12:22 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev
In-Reply-To: <1480073634-14079-1-git-send-email-robin.jarry@6wind.com>
The lsb_release script is part of an optional package which is not
always installed. On the other hand, /etc/lsb-release is always present
even on minimal Ubuntu installations.
root@ubuntu1604:~# dpkg -S /etc/lsb-release
base-files: /etc/lsb-release
Read the file if present and use the variables defined in it.
Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
---
lib/librte_eal/linuxapp/kni/Makefile | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/librte_eal/linuxapp/kni/Makefile b/lib/librte_eal/linuxapp/kni/Makefile
index 4e99e07e7aec..1e4756123d0c 100644
--- a/lib/librte_eal/linuxapp/kni/Makefile
+++ b/lib/librte_eal/linuxapp/kni/Makefile
@@ -44,8 +44,10 @@ MODULE_CFLAGS += -I$(RTE_OUTPUT)/include -I$(SRCDIR)/ethtool/ixgbe -I$(SRCDIR)/e
MODULE_CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h
MODULE_CFLAGS += -Wall -Werror
-ifeq ($(shell lsb_release -si 2>/dev/null),Ubuntu)
-MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(shell lsb_release -sr | tr -d .)
+-include /etc/lsb-release
+
+ifeq ($(DISTRIB_ID),Ubuntu)
+MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(DISTRIB_RELEASE))
UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE $(RTE_KERNELDIR)/include/generated/utsrelease.h \
| cut -d '"' -f2 | cut -d- -f1,2 | tr .- ,`,1)
MODULE_CFLAGS += -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))"
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Richardson, Bruce @ 2016-11-25 12:09 UTC (permalink / raw)
To: Van Haaren, Harry, Jerin Jacob, Thomas Monjalon
Cc: dev@dpdk.org, hemant.agrawal@nxp.com, Eads, Gage
In-Reply-To: <E923DB57A917B54B9182A2E928D00FA6129C56BD@IRSMSX102.ger.corp.intel.com>
> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Friday, November 25, 2016 11:59 AM
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> hemant.agrawal@nxp.com; Eads, Gage <gage.eads@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 1/4] eventdev: introduce event driven
> programming model
>
> Hi All,
>
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Friday, November 25, 2016 12:24 AM
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Van
> > Haaren, Harry <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com;
> > Eads, Gage <gage.eads@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/4] eventdev: introduce event driven
> > programming model
> >
> > On Thu, Nov 24, 2016 at 04:35:56PM +0100, Thomas Monjalon wrote:
> > > 2016-11-24 07:29, Jerin Jacob:
> > > > On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > > > > 2016-11-18 11:14, Jerin Jacob:
>
> > > > > > +#define RTE_EVENT_TYPE_ETHDEV 0x0
> > > > > > +/**< The event generated from ethdev subsystem */
> > > > > > +#define RTE_EVENT_TYPE_CRYPTODEV 0x1
> > > > > > +/**< The event generated from crypodev subsystem */
> > > > > > +#define RTE_EVENT_TYPE_TIMERDEV 0x2
> > > > > > +/**< The event generated from timerdev subsystem */
> > > > > > +#define RTE_EVENT_TYPE_CORE 0x3
> > > > > > +/**< The event generated from core.
> > > > >
> > > > > What is core?
> > > >
> > > > The event are generated by lcore for pipeling. Any suggestion for
> > > > better name? lcore?
> > >
> > > What about CPU or SW?
> >
> > No strong opinion here. I will go with CPU then
>
>
> +1 for CPU (as SW is the software PMD name).
>
Fine, I'm outvoted. I'll learn to live with it. :-)
/Bruce
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Andrew Rybchenko @ 2016-11-25 12:02 UTC (permalink / raw)
To: Ferruh Yigit, dev
In-Reply-To: <576edd32-01be-0210-f5f0-5a6d54450acf@intel.com>
On 11/25/2016 01:24 PM, Ferruh Yigit wrote:
> On 11/23/2016 7:49 AM, Andrew Rybchenko wrote:
>> On 11/23/2016 03:02 AM, Ferruh Yigit wrote:
>>> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>>>> The patch series adds Solarflare libefx-based network PMD.
>>>>
>>>> This version of the driver supports Solarflare SFN7xxx and SFN8xxx
>>>> families of 10/40 Gbps adapters.
>>>>
>>>> libefx is a platform-independent library to implement drivers for
>>>> Solarflare network adapters. It provides unified adapter family
>>>> independent interface (if possible). FreeBSD [1] and illumos [2]
>>>> drivers are built on top of the library.
>>>>
>>>> The patch series could be logically structured into 5 sub-series:
>>>> 1. (1) add the driver skeleton including documentation
>>>> 2. (2-30) import libefx and include it in build with the latest patch
>>>> 3. (31-43) implement minimal device level operations in steps
>>>> 4. (44-51) implement Rx subsystem
>>>> 5. (52-56) implement Tx subsystem
>>>>
>>>> Functional driver with multi-queue support capable to send and receive
>>>> traffic appears with the last patch in the series.
>>>>
>>>> The following design decisions are made during development:
>>>>
>>>> 1. Since libefx uses positive errno return codes, positive errno
>>>> return codes are used inside the driver and coversion to negative
>>>> is done on return from eth_dev_ops callbacks. We think that it
>>>> is the less error-prone way.
>>>>
>>>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>>>> is considered and could be added in the future. Code for data path
>>>> should be shared by these two drivers. libefx-based PMD is put into
>>>> 'efx' subdirectory to have a space for another PMD and shared code.
>>>>
>>>> 3. Own event queue (a way to deliver events from HW to host CPU) is
>>>> used for house-keeping (e.g. link status notifications), each Tx
>>>> and each Rx queue. No locks on datapath are requires in this case.
>>>>
>>>> 4. Alarm is used to periodically poll house-keeping event queue.
>>>> The event queue is used to deliver link status change notifications,
>>>> Rx/Tx queue flush events, SRAM events. It is not used on datapath.
>>>> The event queue polling is protected using spin-lock since
>>>> concurrent access from different contexts is possible (e.g. device
>>>> stop when polling alarm is running).
>>>>
>>>> [1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/
>>>> [2] https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/
>>>>
>>>> ---
>>>>
>>>> Andrew Rybchenko (49):
>>>> net/sfc: libefx-based PMD stub sufficient to build and init
>>>> net/sfc: import libefx base
>>>> net/sfc: import libefx register definitions
>>>> net/sfc: import libefx filters support
>>>> net/sfc: import libefx MCDI definition
>>>> net/sfc: import libefx MCDI implementation
>>>> net/sfc: import libefx MCDI logging support
>>>> net/sfc: import libefx MCDI proxy authorization support
>>>> net/sfc: import libefx 5xxx/6xxx family support
>>>> net/sfc: import libefx SFN7xxx family support
>>>> net/sfc: import libefx SFN8xxx family support
>>>> net/sfc: import libefx diagnostics support
>>>> net/sfc: import libefx built-in selftest support
>>>> net/sfc: import libefx software per-queue statistics support
>>>> net/sfc: import libefx PHY flags control support
>>>> net/sfc: import libefx PHY statistics support
>>>> net/sfc: import libefx PHY LEDs control support
>>>> net/sfc: import libefx MAC statistics support
>>>> net/sfc: import libefx event prefetch support
>>>> net/sfc: import libefx Rx scatter support
>>>> net/sfc: import libefx RSS support
>>>> net/sfc: import libefx loopback control support
>>>> net/sfc: import libefx monitors statistics support
>>>> net/sfc: import libefx support to access monitors via MCDI
>>>> net/sfc: import libefx support for Rx packed stream mode
>>>> net/sfc: import libefx NVRAM support
>>>> net/sfc: import libefx VPD support
>>>> net/sfc: import libefx bootrom configuration support
>>>> net/sfc: import libefx licensing support
>>>> net/sfc: implement dummy callback to get device information
>>>> net/sfc: implement driver operation to init device on attach
>>>> net/sfc: add device configure and close stubs
>>>> net/sfc: add device configuration checks
>>>> net/sfc: implement device start and stop operations
>>>> net/sfc: make available resources estimation and allocation
>>>> net/sfc: interrupts support sufficient for event queue init
>>>> net/sfc: implement event queue support
>>>> net/sfc: implement EVQ dummy exception handling
>>>> net/sfc: maintain management event queue
>>>> net/sfc: periodic management EVQ polling using alarm
>>>> net/sfc: minimum port control sufficient to receive traffic
>>>> net/sfc: implement Rx subsystem stubs
>>>> net/sfc: check configured rxmode
>>>> net/sfc: implement Rx queue setup release operations
>>>> net/sfc: calculate Rx buffer size which may be used
>>>> net/sfc: validate Rx queue buffers setup
>>>> net/sfc: implement Rx queue start and stop operations
>>>> net/sfc: implement device callback to Rx burst of packets
>>>> net/sfc: discard scattered packet on Rx correctly
>>>>
>>>> Artem Andreev (2):
>>>> net/sfc: include libefx in build
>>>> net/sfc: implement device operation to retrieve link info
>>>>
>>>> Ivan Malov (5):
>>>> net/sfc: provide basic stubs for Tx subsystem
>>>> net/sfc: add function to check configured Tx mode
>>>> net/sfc: add callbacks to set up and release Tx queues
>>>> net/sfc: implement transmit path start / stop
>>>> net/sfc: add callback to send bursts of packets
>>>>
>>> Hi Andrew,
>>>
>>> Thank you for the patch, I have encounter with a few minor issues, can
>>> you please check them [1]?
>>>
>>> Also folder structure is drivers/net/sfc/efx/<all_src_files>, why /sfc/
>>> layer is created?
>>> sfc is company name (solarflare communications), right? Other driver
>>> folders not structured based on company, what about using
>>> drivers/net/efx/* ?
>> I've tried to explain it above in item (2):
>>
>> >>>
>>
>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>> is considered and could be added in the future. Code for data path
>> should be shared by these two drivers. libefx-based PMD is put into
>> 'efx' subdirectory to have a space for another PMD and shared code.
>>
>> <<<
>>
>> So, main reason is to have location for the code shared by two Solarflare
>> network PMDs. May be it better to relocate when we really have it.
>> I'm open for other ideas/suggestions.
> If there will be another PMD that shares code with current one, the
> logic seems good, but I am not sure about start using company names, I
> am not against it, just I don't know.
I think that mlx4 and mlx5 are tightly bound to the company name (plus
adapter generation, I guess).
> Let's relocate later, this buys some time to think / get feedback on issue.
Do I understand correctly that you suggest to avoid extra level inside
for now
and relocate later if required? If so, that's fine for me.
As for naming, we think that just "efx" is a bad idea. The prefix is
occupied by
the libefx functions and driver should use something else. We have chosen
"sfc" mainly to follow naming used in Linux kernel for Solarflare driver
(the first level of Ethernet driver names is company bound in the Linux
kernel).
If we avoid extra level as discussed above, I think "sfc_efx", "sfcefx"
(may be it
will look better nearby other drivers) or just "sfc" are fine for us.
>>> [1]:
>>> 1- There are a few (non-base) checkpatch warnings, can you please check
>>> patch 36, 49, 50 and 55 please?
>> Thanks, I'll fix spelling in v2.
>> 36, 49 and 55 also ask to check multiple assignments. IMHO, it is the
>> right usage
>> of multiple assignment when logically bound variables must have the same
>> value.
>>
>>> 2- Got following compile issues, not investigated, directly sharing here:
>>>
>>> b) for icc getting following warnings:
>>> =======================================
>>> icc: command line warning #10006: ignoring unknown option '-Wno-empty-body'
>>> icc: command line warning #10006: ignoring unknown option
>>> '-Waggregate-return'
>>> icc: command line warning #10006: ignoring unknown option
>>> '-Wbad-function-cast'
>>> icc: command line warning #10006: ignoring unknown option '-Wnested-externs'
>>>
>>>
>>> c) icc compiler errors:
>>> =======================================
>>> In file included from
>>> .../x86_64-native-linuxapp-icc/include/rte_ethdev.h(185),
>>> from .../drivers/net/sfc/efx/sfc.h(35),
>>> from .../drivers/net/sfc/efx/sfc.c(37):
>>> .../x86_64-native-linuxapp-icc/include/rte_ether.h(258): warning #2203:
>>> cast discards qualifiers from target type
>>> uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes);
>>> ^
>>>
>>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1157): warning #3179: deprecated
>>> conversion of string literal to char* (should be const char*)
>>> .../drivers/net/sfc/efx/base/ef10_filter.c(1276): warning #188:
>>> enumerated type mixed with another type
>>> : "unknown assertion";
>>> ^
>>>
>>> filter_flags = 0;
>>> ^
>>>
>>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1426): warning #188: enumerated
>>> type mixed with another type
>>> epp->ep_fixed_port_type =
>>> ^
>>>
>>> .../drivers/net/sfc/efx/base/efx_nic.c(556): warning #188: enumerated
>>> type mixed with another type
>>> enp->en_family = 0;
>> Yes, I have no ICC compilers. I'll try to fix these warnings, but I
>> can't be sure without checking it.
>> Also we cannot claim ICC supported without building and testing the
>> generated binary.
> Please update the code at least to not break the icc compilation,
> specially since this PMD will be default enabled. If you prefer I can
> verify compilation offline before you send the patchset.
I'll do. I would be thankful if you help with ICC check. Should I send the
patch series directly to you?
Andrew.
^ permalink raw reply
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Van Haaren, Harry @ 2016-11-25 11:59 UTC (permalink / raw)
To: Jerin Jacob, Thomas Monjalon
Cc: dev@dpdk.org, Richardson, Bruce, hemant.agrawal@nxp.com,
Eads, Gage
In-Reply-To: <20161125002334.GA21048@svelivela-lt.caveonetworks.com>
Hi All,
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Friday, November 25, 2016 12:24 AM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Eads, Gage <gage.eads@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/4] eventdev: introduce event driven programming model
>
> On Thu, Nov 24, 2016 at 04:35:56PM +0100, Thomas Monjalon wrote:
> > 2016-11-24 07:29, Jerin Jacob:
> > > On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > > > 2016-11-18 11:14, Jerin Jacob:
> > > > > +Eventdev API - EXPERIMENTAL
> > > > > +M: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > +F: lib/librte_eventdev/
> > > >
> > > > OK to mark it experimental.
> > > > What is the plan to remove the experimental word?
> > >
> > > IMO, EXPERIMENTAL status can be changed when
> > > - At least two event drivers available(Intel and Cavium are working on
> > > SW and HW event drivers)
> > > - Functional test applications are fine with at least two drivers
> > > - Portable example application to showcase the features of the library
> > > - eventdev integration with another dpdk subsystem such as ethdev
> > >
> > > Thoughts?. I am not sure the criteria used in cryptodev case.
> >
> > Sounds good.
> > We will be more confident when drivers and tests will be implemented.
> >
> > I think the roadmap for the SW driver targets the release 17.05.
> > Do you still plan 17.02 for this API and the Cavium driver?
>
> No. 17.02 too short for up-streaming the Cavium driver.However, I think API and
> skeleton event driver can go in 17.02 if there are no objections.
>
> >
> > > > > +#define EVENTDEV_NAME_SKELETON_PMD event_skeleton
> > > > > +/**< Skeleton event device PMD name */
> > > >
> > > > I do not understand this #define.
> > >
> > > Applications can explicitly request the a specific driver though driver
> > > name. This will go as argument to rte_event_dev_get_dev_id(const char *name).
> > > The reason for keeping this #define in rte_eventdev.h is that,
> > > application needs to include only rte_eventdev.h not rte_eventdev_pmd.h.
> >
> > So each driver must register its name in the API?
> > Is it really needed?
>
> Otherwise how application knows the name of the driver.
> The similar scheme used in cryptodev.
> http://dpdk.org/browse/dpdk/tree/lib/librte_cryptodev/rte_cryptodev.h#n53
> No strong opinion here. Open for suggestions.
>
> >
> > > > > +struct rte_event_dev_config {
> > > > > + uint32_t dequeue_wait_ns;
> > > > > + /**< rte_event_dequeue() wait for *dequeue_wait_ns* ns on this device.
> > > >
> > > > Please explain exactly when the wait occurs and why.
> > >
> > > Here is the explanation from rte_event_dequeue() API definition,
> > > -
> > > @param wait
> > > 0 - no-wait, returns immediately if there is no event.
> > > >0 - wait for the event, if the device is configured with
> > > RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT then this function will wait until
> > > the event available or *wait* time.
> > > if the device is not configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT
> > > then this function will wait until the event available or *dequeue_wait_ns*
> > > ^^^^^^^^^^^^^^^^^^^^^^
> > > ns which was previously supplied to rte_event_dev_configure()
> > > -
> > > This is provides the application to have control over, how long the
> > > implementation should wait if event is not available.
> > >
> > > Let me know what exact changes are required if details are not enough in
> > > rte_event_dequeue() API definition.
> >
> > Maybe that timeout would be a better name.
> > It waits only if there is nothing in the queue.
> > It can be interesting to highlight in this comment that this parameter
> > makes the dequeue function a blocking call.
>
> OK. I will change to timeout then
>
> >
> > > > > +/** Event port configuration structure */
> > > > > +struct rte_event_port_conf {
> > > > > + int32_t new_event_threshold;
> > > > > + /**< A backpressure threshold for new event enqueues on this port.
> > > > > + * Use for *closed system* event dev where event capacity is limited,
> > > > > + * and cannot exceed the capacity of the event dev.
> > > > > + * Configuring ports with different thresholds can make higher priority
> > > > > + * traffic less likely to be backpressured.
> > > > > + * For example, a port used to inject NIC Rx packets into the event dev
> > > > > + * can have a lower threshold so as not to overwhelm the device,
> > > > > + * while ports used for worker pools can have a higher threshold.
> > > > > + * This value cannot exceed the *nb_events_limit*
> > > > > + * which previously supplied to rte_event_dev_configure()
> > > > > + */
> > > > > + uint8_t dequeue_depth;
> > > > > + /**< Configure number of bulk dequeues for this event port.
> > > > > + * This value cannot exceed the *nb_event_port_dequeue_depth*
> > > > > + * which previously supplied to rte_event_dev_configure()
> > > > > + */
> > > > > + uint8_t enqueue_depth;
> > > > > + /**< Configure number of bulk enqueues for this event port.
> > > > > + * This value cannot exceed the *nb_event_port_enqueue_depth*
> > > > > + * which previously supplied to rte_event_dev_configure()
> > > > > + */
> > > > > +};
> > > >
> > > > The depth configuration is not clear to me.
> > >
> > > Basically the maximum number of events can be enqueued/dequeued at time
> > > from a given event port. depth of one == non burst mode.
> >
> > OK so depth is the queue size. Please could you reword?
>
> OK
>
> >
> > > > > +/* Event types to classify the event source */
> > > >
> > > > Why this classification is needed?
> > >
> > > This for application pipeling and the cases like, if application wants to know which
> > > subsystem generated the event.
> > >
> > > example packet forwarding loop on the worker cores:
> > > while(1) {
> > > ev = dequeue()
> > > // event from ethdev subsystem
> > > if (ev.event_type == RTE_EVENT_TYPE_ETHDEV) {
> > > - swap the mac address
> > > - push to atomic queue for ingress flow order maintenance
> > > by CORE
> > > /* events from core */
> > > } else if (ev.event_type == RTE_EVENT_TYPE_CORE) {
> > >
> > > }
> > > enqueue(ev);
> > > }
> >
> > I don't know why but I feel this classification is weak.
> > You need to track the source of the event. Does it make sense to go beyond
> > and identify the source device?
>
> No, dequeue has dev_id argument, so event comes only from that device
>
> >
> > > > > +#define RTE_EVENT_TYPE_ETHDEV 0x0
> > > > > +/**< The event generated from ethdev subsystem */
> > > > > +#define RTE_EVENT_TYPE_CRYPTODEV 0x1
> > > > > +/**< The event generated from crypodev subsystem */
> > > > > +#define RTE_EVENT_TYPE_TIMERDEV 0x2
> > > > > +/**< The event generated from timerdev subsystem */
> > > > > +#define RTE_EVENT_TYPE_CORE 0x3
> > > > > +/**< The event generated from core.
> > > >
> > > > What is core?
> > >
> > > The event are generated by lcore for pipeling. Any suggestion for
> > > better name? lcore?
> >
> > What about CPU or SW?
>
> No strong opinion here. I will go with CPU then
+1 for CPU (as SW is the software PMD name).
> > > > > + /**< Opaque event pointer */
> > > > > + struct rte_mbuf *mbuf;
> > > > > + /**< mbuf pointer if dequeued event is associated with mbuf */
> > > >
> > > > How do we know that an event is associated with mbuf?
> > >
> > > By looking at the event source/type RTE_EVENT_TYPE_*
> > >
> > > > Does it mean that such events are always converted into mbuf even if the
> > > > application does not need it?
> > >
> > > Hardware has dependency on getting physical address of the event, so any
> > > struct that has "phys_addr_t buf_physaddr" works.
> >
> > I do not understand.
>
> In HW based implementations, the event pointer will be submitted to HW.
> As you know, since HW can't understand the virtual address and it needs
> to converted to the physical address, any DPDK object that provides phys_addr_t
> such as mbuf can be used with libeventdev.
>
> >
> > I tought that decoding the event would be the responsibility of the app
> > by calling a function like
> > rte_eventdev_convert_to_mbuf(struct rte_event *, struct rte_mbuf *).
>
> It can be. But it is costly.i.e Yet another function pointer based
> driver interface on fastpath. Instead, if the driver itself can
> convert to mbuf(in case of ETHDEV device) and tag the source/event type
> as RTE_EVENT_TYPE_ETHDEV.
> IMO the proposed schemed helps in SW based implementation as their no real
> mbuf conversation. Something we can revisit in ethdev integration if
> required.
>
> >
> > > > > +struct rte_eventdev_driver;
> > > > > +struct rte_eventdev_ops;
> > > >
> > > > I think it is better to split API and driver interface in two files.
> > > > (we should do this split in ethdev)
> > >
> > > I thought so, but then the "static inline" versions of northbound
> > > API(like rte_event_enqueue) will go another file(due to the fact that
> > > implementation need to deference "dev->data->ports[port_id]"). Do you want that way?
> > > I would like to keep all northbound API in rte_eventdev.h and not any of them
> > > in rte_eventdev_pmd.h.
> >
> > My comment was confusing.
> > You are doing 2 files, one for API (what you call northbound I think)
> > and the other one for driver interface (what you call southbound I think),
> > it's very fine.
> >
> > > > > +/**
> > > > > + * Enqueue the event object supplied in the *rte_event* structure on an
> > > > > + * event device designated by its *dev_id* through the event port specified by
> > > > > + * *port_id*. The event object specifies the event queue on which this
> > > > > + * event will be enqueued.
> > > > > + *
> > > > > + * @param dev_id
> > > > > + * Event device identifier.
> > > > > + * @param port_id
> > > > > + * The identifier of the event port.
> > > > > + * @param ev
> > > > > + * Pointer to struct rte_event
> > > > > + *
> > > > > + * @return
> > > > > + * - 0 on success
> > > > > + * - <0 on failure. Failure can occur if the event port's output queue is
> > > > > + * backpressured, for instance.
> > > > > + */
> > > > > +static inline int
> > > > > +rte_event_enqueue(uint8_t dev_id, uint8_t port_id, struct rte_event *ev)
> > > >
> > > > Is it really needed to have non-burst variant of enqueue/dequeue?
> > >
> > > Yes. certain HW can work only with non burst variants.
> >
> > Same comment as Bruce, we must keep only the burst variant.
> > We cannot have different API for different HW.
>
> I don't think there is any portability issue here, I can explain.
>
> The application level, we have two more use case to deal with non burst
> variant
>
> - latency critical work
> - on dequeue, if application wants to deal with only one flow(i.e to
> avoid processing two different application flows to avoid cache trashing)
>
> Selection of the burst variants will be based on
> rte_event_dev_info_get() and rte_event_dev_configure()(see, max_event_port_dequeue_depth,
> max_event_port_enqueue_depth, nb_event_port_dequeue_depth, nb_event_port_enqueue_depth )
> So I don't think their is portability issue here and I don't want to waste my
> CPU cycles on the for loop if application known to be working with non
> bursts variant like below
>
> nb_events = rte_event_dequeue_burst();
> for(i=0; i < nb_events; i++){
> process ev[i]
> }
>
> And mostly importantly the NPU can get almost same throughput
> without burst variant so why not?
Perhaps I'm mis-understanding, but can you not just dequeue 1 from the burst() function?
struct rte_event ev;
rte_event_dequeue_burst(dev, port, &ev, 1, wait);
process( &ev );
I mean, if an application *demands* to not use bursts, the above allows it. Of course it won't scale to other implementations that would benefit from burst - but that's the application authors choice?
> > > > > +/**
> > > > > + * Converts nanoseconds to *wait* value for rte_event_dequeue()
> > > > > + *
> > > > > + * If the device is configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT flag then
> > > > > + * application can use this function to convert wait value in nanoseconds to
> > > > > + * implementations specific wait value supplied in rte_event_dequeue()
> > > >
> > > > Why is it implementation-specific?
> > > > Why this conversion is not internal in the driver?
> > >
> > > This is for performance optimization, otherwise in drivers
> > > need to convert ns to ticks in "fast path"
> >
> > So why not defining the unit of this timeout as CPU cycles like the ones
> > returned by rte_get_timer_cycles()?
>
> Because HW co-processor can run in different clock domain. Need not be at
> CPU frequency.
^ permalink raw reply
* Re: [PATCH v2] kni: avoid using lsb_release script
From: Ferruh Yigit @ 2016-11-25 11:35 UTC (permalink / raw)
To: Robin Jarry; +Cc: dev
In-Reply-To: <1480073634-14079-1-git-send-email-robin.jarry@6wind.com>
On 11/25/2016 11:33 AM, Robin Jarry wrote:
> The lsb_release script is part of an optional package which is not
> always installed. On the other hand, /etc/lsb-release is always present
> even on minimal Ubuntu installations.
>
> root@ubuntu1604:~# dpkg -S /etc/lsb-release
> base-files: /etc/lsb-release
>
> Read the file if present and use the variables defined in it.
>
> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> lib/librte_eal/linuxapp/kni/Makefile | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/kni/Makefile b/lib/librte_eal/linuxapp/kni/Makefile
> index 4e99e07e7aec..62a957ce8534 100644
> --- a/lib/librte_eal/linuxapp/kni/Makefile
> +++ b/lib/librte_eal/linuxapp/kni/Makefile
> @@ -44,8 +44,12 @@ MODULE_CFLAGS += -I$(RTE_OUTPUT)/include -I$(SRCDIR)/ethtool/ixgbe -I$(SRCDIR)/e
> MODULE_CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h
> MODULE_CFLAGS += -Wall -Werror
>
> -ifeq ($(shell lsb_release -si 2>/dev/null),Ubuntu)
> -MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(shell lsb_release -sr | tr -d .)
> +ifneq ($(wildcard /etc/lsb-release),)
I mean removing this check completely, and having only below line, do
you think does it works that way?
> +-include /etc/lsb-release
> +endif
> +
> +ifeq ($(DISTRIB_ID),Ubuntu)
> +MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(DISTRIB_RELEASE))
> UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE $(RTE_KERNELDIR)/include/generated/utsrelease.h \
> | cut -d '"' -f2 | cut -d- -f1,2 | tr .- ,`,1)
> MODULE_CFLAGS += -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))"
>
^ permalink raw reply
* [PATCH v2] kni: avoid using lsb_release script
From: Robin Jarry @ 2016-11-25 11:33 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev
In-Reply-To: <1480067361-6208-1-git-send-email-robin.jarry@6wind.com>
The lsb_release script is part of an optional package which is not
always installed. On the other hand, /etc/lsb-release is always present
even on minimal Ubuntu installations.
root@ubuntu1604:~# dpkg -S /etc/lsb-release
base-files: /etc/lsb-release
Read the file if present and use the variables defined in it.
Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
---
lib/librte_eal/linuxapp/kni/Makefile | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/lib/librte_eal/linuxapp/kni/Makefile b/lib/librte_eal/linuxapp/kni/Makefile
index 4e99e07e7aec..62a957ce8534 100644
--- a/lib/librte_eal/linuxapp/kni/Makefile
+++ b/lib/librte_eal/linuxapp/kni/Makefile
@@ -44,8 +44,12 @@ MODULE_CFLAGS += -I$(RTE_OUTPUT)/include -I$(SRCDIR)/ethtool/ixgbe -I$(SRCDIR)/e
MODULE_CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h
MODULE_CFLAGS += -Wall -Werror
-ifeq ($(shell lsb_release -si 2>/dev/null),Ubuntu)
-MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(shell lsb_release -sr | tr -d .)
+ifneq ($(wildcard /etc/lsb-release),)
+-include /etc/lsb-release
+endif
+
+ifeq ($(DISTRIB_ID),Ubuntu)
+MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(DISTRIB_RELEASE))
UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE $(RTE_KERNELDIR)/include/generated/utsrelease.h \
| cut -d '"' -f2 | cut -d- -f1,2 | tr .- ,`,1)
MODULE_CFLAGS += -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))"
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] kni: avoid using lsb_release script
From: Robin Jarry @ 2016-11-25 11:28 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev
In-Reply-To: <f88de78b-1963-cfb7-acdb-84bfb5fccac0@intel.com>
2016-11-25, Ferruh Yigit:
>What about:
>+-include /etc/lsb-release
Sure, that cannot hurt.
>s/DISTRIB_ID/DISTRIB_RELEASE
Oops last minute typo... I'll submit a v2 right away.
--
Robin
^ permalink raw reply
* Re: [PATCH] kni: avoid using lsb_release script
From: Ferruh Yigit @ 2016-11-25 11:27 UTC (permalink / raw)
To: Robin Jarry; +Cc: dev
In-Reply-To: <1480067575-6543-1-git-send-email-robin.jarry@6wind.com>
On 11/25/2016 9:52 AM, Robin Jarry wrote:
> The lsb_release script is part of an optional package which is not
> always installed. On the other hand, /etc/lsb-release is always present
> even on minimal Ubuntu installations.
>
> root@ubuntu1604:~# dpkg -S /etc/lsb-release
> base-files: /etc/lsb-release
>
> Read the file if present and use the variables defined in it.
>
> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> lib/librte_eal/linuxapp/kni/Makefile | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/kni/Makefile b/lib/librte_eal/linuxapp/kni/Makefile
> index 4e99e07e7aec..63b9ac8779f8 100644
> --- a/lib/librte_eal/linuxapp/kni/Makefile
> +++ b/lib/librte_eal/linuxapp/kni/Makefile
> @@ -44,8 +44,12 @@ MODULE_CFLAGS += -I$(RTE_OUTPUT)/include -I$(SRCDIR)/ethtool/ixgbe -I$(SRCDIR)/e
> MODULE_CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h
> MODULE_CFLAGS += -Wall -Werror
>
> -ifeq ($(shell lsb_release -si 2>/dev/null),Ubuntu)
> -MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(shell lsb_release -sr | tr -d .)
> +ifneq ($(wildcard /etc/lsb-release),)
> +include /etc/lsb-release
> +endif
What about:
+-include /etc/lsb-release
> +
> +ifeq ($(DISTRIB_ID),Ubuntu)
> +MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(DISTRIB_ID))
s/DISTRIB_ID/DISTRIB_RELEASE
> UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE $(RTE_KERNELDIR)/include/generated/utsrelease.h \
> | cut -d '"' -f2 | cut -d- -f1,2 | tr .- ,`,1)
> MODULE_CFLAGS += -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))"
>
^ permalink raw reply
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-11-25 11:00 UTC (permalink / raw)
To: Jerin Jacob
Cc: Thomas Monjalon, dev, harry.van.haaren, hemant.agrawal, gage.eads
In-Reply-To: <20161125002334.GA21048@svelivela-lt.caveonetworks.com>
On Fri, Nov 25, 2016 at 05:53:34AM +0530, Jerin Jacob wrote:
> On Thu, Nov 24, 2016 at 04:35:56PM +0100, Thomas Monjalon wrote:
> > 2016-11-24 07:29, Jerin Jacob:
> > > On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > > > 2016-11-18 11:14, Jerin Jacob:
> > > > > +Eventdev API - EXPERIMENTAL
> > > > > +M: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > +F: lib/librte_eventdev/
> > > >
> > > > OK to mark it experimental.
> > > > What is the plan to remove the experimental word?
> > >
> > > IMO, EXPERIMENTAL status can be changed when
> > > - At least two event drivers available(Intel and Cavium are working on
> > > SW and HW event drivers)
> > > - Functional test applications are fine with at least two drivers
> > > - Portable example application to showcase the features of the library
> > > - eventdev integration with another dpdk subsystem such as ethdev
> > >
> > > Thoughts?. I am not sure the criteria used in cryptodev case.
> >
> > Sounds good.
> > We will be more confident when drivers and tests will be implemented.
> >
> > I think the roadmap for the SW driver targets the release 17.05.
> > Do you still plan 17.02 for this API and the Cavium driver?
>
> No. 17.02 too short for up-streaming the Cavium driver.However, I think API and
> skeleton event driver can go in 17.02 if there are no objections.
>
> >
> > > > > +#define EVENTDEV_NAME_SKELETON_PMD event_skeleton
> > > > > +/**< Skeleton event device PMD name */
> > > >
> > > > I do not understand this #define.
> > >
> > > Applications can explicitly request the a specific driver though driver
> > > name. This will go as argument to rte_event_dev_get_dev_id(const char *name).
> > > The reason for keeping this #define in rte_eventdev.h is that,
> > > application needs to include only rte_eventdev.h not rte_eventdev_pmd.h.
> >
> > So each driver must register its name in the API?
> > Is it really needed?
>
> Otherwise how application knows the name of the driver.
> The similar scheme used in cryptodev.
> http://dpdk.org/browse/dpdk/tree/lib/librte_cryptodev/rte_cryptodev.h#n53
> No strong opinion here. Open for suggestions.
>
I like having a name registered. I think we need a scheme where an app
can find and use an implementation using a specific driver.
> >
> > > > > +struct rte_event_dev_config {
> > > > > + uint32_t dequeue_wait_ns;
> > > > > + /**< rte_event_dequeue() wait for *dequeue_wait_ns* ns on this device.
> > > >
> > > > Please explain exactly when the wait occurs and why.
> > >
> > > Here is the explanation from rte_event_dequeue() API definition,
> > > -
> > > @param wait
> > > 0 - no-wait, returns immediately if there is no event.
> > > >0 - wait for the event, if the device is configured with
> > > RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT then this function will wait until
> > > the event available or *wait* time.
> > > if the device is not configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT
> > > then this function will wait until the event available or *dequeue_wait_ns*
> > > ^^^^^^^^^^^^^^^^^^^^^^
> > > ns which was previously supplied to rte_event_dev_configure()
> > > -
> > > This is provides the application to have control over, how long the
> > > implementation should wait if event is not available.
> > >
> > > Let me know what exact changes are required if details are not enough in
> > > rte_event_dequeue() API definition.
> >
> > Maybe that timeout would be a better name.
> > It waits only if there is nothing in the queue.
> > It can be interesting to highlight in this comment that this parameter
> > makes the dequeue function a blocking call.
>
> OK. I will change to timeout then
>
> >
> > > > > +/** Event port configuration structure */
> > > > > +struct rte_event_port_conf {
> > > > > + int32_t new_event_threshold;
> > > > > + /**< A backpressure threshold for new event enqueues on this port.
> > > > > + * Use for *closed system* event dev where event capacity is limited,
> > > > > + * and cannot exceed the capacity of the event dev.
> > > > > + * Configuring ports with different thresholds can make higher priority
> > > > > + * traffic less likely to be backpressured.
> > > > > + * For example, a port used to inject NIC Rx packets into the event dev
> > > > > + * can have a lower threshold so as not to overwhelm the device,
> > > > > + * while ports used for worker pools can have a higher threshold.
> > > > > + * This value cannot exceed the *nb_events_limit*
> > > > > + * which previously supplied to rte_event_dev_configure()
> > > > > + */
> > > > > + uint8_t dequeue_depth;
> > > > > + /**< Configure number of bulk dequeues for this event port.
> > > > > + * This value cannot exceed the *nb_event_port_dequeue_depth*
> > > > > + * which previously supplied to rte_event_dev_configure()
> > > > > + */
> > > > > + uint8_t enqueue_depth;
> > > > > + /**< Configure number of bulk enqueues for this event port.
> > > > > + * This value cannot exceed the *nb_event_port_enqueue_depth*
> > > > > + * which previously supplied to rte_event_dev_configure()
> > > > > + */
> > > > > +};
> > > >
> > > > The depth configuration is not clear to me.
> > >
> > > Basically the maximum number of events can be enqueued/dequeued at time
> > > from a given event port. depth of one == non burst mode.
> >
> > OK so depth is the queue size. Please could you reword?
>
> OK
>
> >
> > > > > +/* Event types to classify the event source */
> > > >
> > > > Why this classification is needed?
> > >
> > > This for application pipeling and the cases like, if application wants to know which
> > > subsystem generated the event.
> > >
> > > example packet forwarding loop on the worker cores:
> > > while(1) {
> > > ev = dequeue()
> > > // event from ethdev subsystem
> > > if (ev.event_type == RTE_EVENT_TYPE_ETHDEV) {
> > > - swap the mac address
> > > - push to atomic queue for ingress flow order maintenance
> > > by CORE
> > > /* events from core */
> > > } else if (ev.event_type == RTE_EVENT_TYPE_CORE) {
> > >
> > > }
> > > enqueue(ev);
> > > }
> >
> > I don't know why but I feel this classification is weak.
> > You need to track the source of the event. Does it make sense to go beyond
> > and identify the source device?
>
> No, dequeue has dev_id argument, so event comes only from that device
>
> >
> > > > > +#define RTE_EVENT_TYPE_ETHDEV 0x0
> > > > > +/**< The event generated from ethdev subsystem */
> > > > > +#define RTE_EVENT_TYPE_CRYPTODEV 0x1
> > > > > +/**< The event generated from crypodev subsystem */
> > > > > +#define RTE_EVENT_TYPE_TIMERDEV 0x2
> > > > > +/**< The event generated from timerdev subsystem */
> > > > > +#define RTE_EVENT_TYPE_CORE 0x3
> > > > > +/**< The event generated from core.
> > > >
> > > > What is core?
> > >
> > > The event are generated by lcore for pipeling. Any suggestion for
> > > better name? lcore?
> >
> > What about CPU or SW?
>
> No strong opinion here. I will go with CPU then
If you have no strong opinion, I think I'd prefer SW to CPU, as the main
difference to my mind is that this comes from another SW entity rather
than a hardware block.
>
> >
> > > > > + /**< Opaque event pointer */
> > > > > + struct rte_mbuf *mbuf;
> > > > > + /**< mbuf pointer if dequeued event is associated with mbuf */
> > > >
> > > > How do we know that an event is associated with mbuf?
> > >
> > > By looking at the event source/type RTE_EVENT_TYPE_*
> > >
> > > > Does it mean that such events are always converted into mbuf even if the
> > > > application does not need it?
> > >
> > > Hardware has dependency on getting physical address of the event, so any
> > > struct that has "phys_addr_t buf_physaddr" works.
> >
> > I do not understand.
>
> In HW based implementations, the event pointer will be submitted to HW.
> As you know, since HW can't understand the virtual address and it needs
> to converted to the physical address, any DPDK object that provides phys_addr_t
> such as mbuf can be used with libeventdev.
>
> >
> > I tought that decoding the event would be the responsibility of the app
> > by calling a function like
> > rte_eventdev_convert_to_mbuf(struct rte_event *, struct rte_mbuf *).
>
> It can be. But it is costly.i.e Yet another function pointer based
> driver interface on fastpath. Instead, if the driver itself can
> convert to mbuf(in case of ETHDEV device) and tag the source/event type
> as RTE_EVENT_TYPE_ETHDEV.
> IMO the proposed schemed helps in SW based implementation as their no real
> mbuf conversation. Something we can revisit in ethdev integration if
> required.
>
> >
> > > > > +struct rte_eventdev_driver;
> > > > > +struct rte_eventdev_ops;
> > > >
> > > > I think it is better to split API and driver interface in two files.
> > > > (we should do this split in ethdev)
> > >
> > > I thought so, but then the "static inline" versions of northbound
> > > API(like rte_event_enqueue) will go another file(due to the fact that
> > > implementation need to deference "dev->data->ports[port_id]"). Do you want that way?
> > > I would like to keep all northbound API in rte_eventdev.h and not any of them
> > > in rte_eventdev_pmd.h.
> >
> > My comment was confusing.
> > You are doing 2 files, one for API (what you call northbound I think)
> > and the other one for driver interface (what you call southbound I think),
> > it's very fine.
> >
> > > > > +/**
> > > > > + * Enqueue the event object supplied in the *rte_event* structure on an
> > > > > + * event device designated by its *dev_id* through the event port specified by
> > > > > + * *port_id*. The event object specifies the event queue on which this
> > > > > + * event will be enqueued.
> > > > > + *
> > > > > + * @param dev_id
> > > > > + * Event device identifier.
> > > > > + * @param port_id
> > > > > + * The identifier of the event port.
> > > > > + * @param ev
> > > > > + * Pointer to struct rte_event
> > > > > + *
> > > > > + * @return
> > > > > + * - 0 on success
> > > > > + * - <0 on failure. Failure can occur if the event port's output queue is
> > > > > + * backpressured, for instance.
> > > > > + */
> > > > > +static inline int
> > > > > +rte_event_enqueue(uint8_t dev_id, uint8_t port_id, struct rte_event *ev)
> > > >
> > > > Is it really needed to have non-burst variant of enqueue/dequeue?
> > >
> > > Yes. certain HW can work only with non burst variants.
> >
> > Same comment as Bruce, we must keep only the burst variant.
> > We cannot have different API for different HW.
>
> I don't think there is any portability issue here, I can explain.
>
> The application level, we have two more use case to deal with non burst
> variant
>
> - latency critical work
> - on dequeue, if application wants to deal with only one flow(i.e to
> avoid processing two different application flows to avoid cache trashing)
>
> Selection of the burst variants will be based on
> rte_event_dev_info_get() and rte_event_dev_configure()(see, max_event_port_dequeue_depth,
> max_event_port_enqueue_depth, nb_event_port_dequeue_depth, nb_event_port_enqueue_depth )
> So I don't think their is portability issue here and I don't want to waste my
> CPU cycles on the for loop if application known to be working with non
> bursts variant like below
>
If the application is known to be working on non-burst varients, then
they always request a burst-size of 1, and skip the loop completely.
There is no extra performance hit in that case in either the app or the
driver (since the non-burst driver always returns 1, irrespective of the
number requested).
> nb_events = rte_event_dequeue_burst();
> for(i=0; i < nb_events; i++){
> process ev[i]
> }
>
> And mostly importantly the NPU can get almost same throughput
> without burst variant so why not?
>
> >
> > > > > +/**
> > > > > + * Converts nanoseconds to *wait* value for rte_event_dequeue()
> > > > > + *
> > > > > + * If the device is configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT flag then
> > > > > + * application can use this function to convert wait value in nanoseconds to
> > > > > + * implementations specific wait value supplied in rte_event_dequeue()
> > > >
> > > > Why is it implementation-specific?
> > > > Why this conversion is not internal in the driver?
> > >
> > > This is for performance optimization, otherwise in drivers
> > > need to convert ns to ticks in "fast path"
> >
> > So why not defining the unit of this timeout as CPU cycles like the ones
> > returned by rte_get_timer_cycles()?
>
> Because HW co-processor can run in different clock domain. Need not be at
> CPU frequency.
>
While I've no huge objection to this API, since it will not be
implemented by our SW implementation, I'm just curious as to how much
having this will save. How complicated is the arithmetic that needs to
be done, and how many cycles on your platform is that going to take?
/Bruce
^ permalink raw reply
* Re: [PATCH v2 6/7] pci: Combine rte_eal_pci_scan and rte_eal_pci_probe
From: Shreyansh Jain @ 2016-11-25 10:56 UTC (permalink / raw)
To: Ben Walker; +Cc: dev, david.marchand@6wind.com, Ferruh Yigit
In-Reply-To: <1479931644-78960-6-git-send-email-benjamin.walker@intel.com>
On Thursday 24 November 2016 01:37 AM, Ben Walker wrote:
> Two functions is both confusing and unnecessary. Previously,
> rte_eal_pci_scan populated an internal list of devices by
> scanning sysfs. Then, rte_eal_pci_probe would match registered
> drivers to that internal list. These are not really useful
> operations to perform separately, though, so
> simplify the api down to just rte_eal_pci_probe which can
> be called repeatedly through the lifetime of the application
> to scan for new or removed PCI devices and load or unload
> drivers as required.
Agree with this.
And similar case exists for rte_eal_dev_init() for which there is
another patch floated by Jerin [1].
Also, I am already merging these two (EAL Bus model) [2]. So, we have:
- Only a probe called from EAL for all devices, whether PCI, VDEV or
another other type
- Probe in turns performs all scans and driver->probes()
Concern I have is that with the change of placement for device scan,
would it impact the external modules/PMDs as currently the scanned list
is created *before* eal_plugins_init(). But, now the list is created
*after* plugin initialization.
There are other similar inits like creation of slave threads. As well as
concern raised by Ferruh about device enumeration.
I don't have clear idea of all such dependencies. Maybe David and Ferruh
in CC might be able to comment better.
[1] http://dpdk.org/ml/archives/dev/2016-November/050415.html
[2] http://dpdk.org/ml/archives/dev/2016-November/050301.html
>
> Signed-off-by: Ben Walker <benjamin.walker@intel.com>
> ---
> app/test/test_pci.c | 2 +-
> lib/librte_eal/bsdapp/eal/eal.c | 3 ---
> lib/librte_eal/bsdapp/eal/eal_pci.c | 17 +----------------
> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 -
> lib/librte_eal/common/eal_common_pci.c | 6 ++++++
> lib/librte_eal/common/eal_private.h | 14 +++++---------
> lib/librte_eal/common/include/rte_pci.h | 17 +++++------------
> lib/librte_eal/linuxapp/eal/eal.c | 3 ---
> lib/librte_eal/linuxapp/eal/eal_pci.c | 18 +-----------------
> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 -
> 10 files changed, 19 insertions(+), 63 deletions(-)
>
> diff --git a/app/test/test_pci.c b/app/test/test_pci.c
> index cda186d..fdd84f7 100644
> --- a/app/test/test_pci.c
> +++ b/app/test/test_pci.c
> @@ -180,7 +180,7 @@ test_pci_setup(void)
> TAILQ_INSERT_TAIL(&real_pci_device_list, dev, next);
> }
>
> - ret = rte_eal_pci_scan();
> + ret = rte_eal_pci_probe();
> TEST_ASSERT_SUCCESS(ret, "failed to scan PCI bus");
> rte_eal_pci_dump(stdout);
>
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 35e3117..fd44528 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -561,9 +561,6 @@ rte_eal_init(int argc, char **argv)
> if (rte_eal_timer_init() < 0)
> rte_panic("Cannot init HPET or TSC timers\n");
>
> - if (rte_eal_pci_init() < 0)
> - rte_panic("Cannot init PCI\n");
> -
> eal_check_mem_on_local_socket();
>
> if (eal_plugins_init() < 0)
> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
> index 8b3ed88..6c3a169 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> @@ -361,7 +361,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
> * list. Call pci_scan_one() for each pci entry found.
> */
> int
> -rte_eal_pci_scan(void)
> +pci_scan(void)
> {
> int fd;
> unsigned dev_count = 0;
> @@ -667,18 +667,3 @@ rte_eal_pci_ioport_unmap(struct rte_pci_ioport *p)
>
> return ret;
> }
> -
> -/* Init the PCI EAL subsystem */
> -int
> -rte_eal_pci_init(void)
> -{
> - /* for debug purposes, PCI can be disabled */
> - if (internal_config.no_pci)
> - return 0;
> -
> - if (rte_eal_pci_scan() < 0) {
> - RTE_LOG(ERR, EAL, "%s(): Cannot scan PCI bus\n", __func__);
> - return -1;
> - }
> - return 0;
> -}
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 2f81f7c..67c469c 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -44,7 +44,6 @@ DPDK_2.0 {
> rte_eal_pci_probe;
> rte_eal_pci_probe_one;
> rte_eal_pci_register;
> - rte_eal_pci_scan;
> rte_eal_pci_unregister;
> rte_eal_process_type;
> rte_eal_remote_launch;
> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> index 4f8c3a0..d50a534 100644
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -81,6 +81,7 @@
> #include <rte_devargs.h>
>
> #include "eal_private.h"
> +#include "eal_internal_cfg.h"
>
> struct pci_driver_list pci_driver_list =
> TAILQ_HEAD_INITIALIZER(pci_driver_list);
> @@ -423,6 +424,11 @@ rte_eal_pci_probe(void)
> int probe_all = 0;
> int ret = 0;
>
> + if (internal_config.no_pci)
> + return 0;
> +
> + pci_scan();
> +
So, no check for error reported by scan?
I think we should, 1) because pci_scan() returns one, and 2) because in
case scan failed, there is no reason probe would do anything worthwhile.
> if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) == 0)
> probe_all = 1;
>
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 9e7d8f6..54f18ea 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -108,18 +108,14 @@ int rte_eal_timer_init(void);
> */
> int rte_eal_log_init(const char *id, int facility);
>
> -/**
> - * Init the PCI infrastructure
> +struct rte_pci_driver;
> +struct rte_pci_device;
> +
> +/* Scan the PCI bus for devices
> *
> * This function is private to EAL.
> - *
> - * @return
> - * 0 on success, negative on error
> */
> -int rte_eal_pci_init(void);
> -
> -struct rte_pci_driver;
> -struct rte_pci_device;
> +int pci_scan(void);
>
> /**
> * Update a pci device object by asking the kernel for the latest information.
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 5d0feac..2154a54 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -386,20 +386,13 @@ RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> void rte_eal_pci_unregister(struct rte_pci_driver *driver);
>
> /**
> - * Scan the content of the PCI bus, and the devices in the devices
> - * list
> - *
> - * @return
> - * 0 on success, negative on error
> - */
> -int rte_eal_pci_scan(void);
> -
> -/**
> - * Probe the PCI bus for registered drivers.
> + * Scan the PCI bus for devices and match them to their driver.
> *
> * Scan the content of the PCI bus, and call the probe() function for
> - * all registered drivers that have a matching entry in its id_table
> - * for discovered devices.
> + * all registered drivers that have a matching entry in their id_table.
> + * If a device already has a driver loaded, probe will not be called.
> + * If a previously discovered device is no longer present on the system,
> + * the associated driver's remove() callback will be called.
> *
> * @return
> * - 0 on success.
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 2075282..f47f361 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -802,9 +802,6 @@ rte_eal_init(int argc, char **argv)
> if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0)
> rte_panic("Cannot init logs\n");
>
> - if (rte_eal_pci_init() < 0)
> - rte_panic("Cannot init PCI\n");
> -
> #ifdef VFIO_PRESENT
> if (rte_eal_vfio_setup() < 0)
> rte_panic("Cannot init VFIO\n");
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 936f076..5146385 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -479,7 +479,7 @@ parse_pci_addr_format(const char *buf, int bufsize, struct rte_pci_addr *addr)
> * list
> */
> int
> -rte_eal_pci_scan(void)
> +pci_scan(void)
> {
> struct dirent *e;
> DIR *dir;
> @@ -810,19 +810,3 @@ rte_eal_pci_ioport_unmap(struct rte_pci_ioport *p)
>
> return ret;
> }
> -
> -/* Init the PCI EAL subsystem */
> -int
> -rte_eal_pci_init(void)
> -{
> - /* for debug purposes, PCI can be disabled */
> - if (internal_config.no_pci)
> - return 0;
> -
> - if (rte_eal_pci_scan() < 0) {
> - RTE_LOG(ERR, EAL, "%s(): Cannot scan PCI bus\n", __func__);
> - return -1;
> - }
> -
> - return 0;
> -}
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index 83721ba..856728e 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -44,7 +44,6 @@ DPDK_2.0 {
> rte_eal_pci_probe;
> rte_eal_pci_probe_one;
> rte_eal_pci_register;
> - rte_eal_pci_scan;
> rte_eal_pci_unregister;
> rte_eal_process_type;
> rte_eal_remote_launch;
>
--
-
Shreyansh
^ permalink raw reply
* Re: [PATCH v2 4/7] pci: rte_eal_pci_scan now handles removal of PCI devices
From: Shreyansh Jain @ 2016-11-25 10:44 UTC (permalink / raw)
To: Ben Walker; +Cc: dev
In-Reply-To: <1479931644-78960-4-git-send-email-benjamin.walker@intel.com>
Hi Ben,
On Thursday 24 November 2016 01:37 AM, Ben Walker wrote:
> rte_eal_pci_scan can be called repeatedly to re-scan the PCI
> bus. If a device was removed from the system, the associated
> driver will automatically be unloaded.
>
> Signed-off-by: Ben Walker <benjamin.walker@intel.com>
> ---
[...]
While reviewing, I found that there are some checkpatch warnings on this
patch:
--->8---
### [PATCH v2 4/7] pci: rte_eal_pci_scan now handles removal of PCI devices
WARNING:LONG_LINE_COMMENT: line over 80 characters
#76: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:490:
+ /* Search the device list for devices that are no longer present
on the system
WARNING:LONG_LINE_STRING: line over 80 characters
#105: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:519:
+ RTE_LOG(DEBUG, EAL, "PCI device "PCI_PRI_FMT"
was removed.\n",
WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a
separate line
#111: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:525:
+ * Unload it. */
WARNING:LONG_LINE_STRING: line over 80 characters
#112: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:526:
+ RTE_LOG(DEBUG, EAL, " Unload driver:
%x:%x %s\n",
WARNING:LONG_LINE: line over 80 characters
#113: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:527:
+ dev->id.vendor_id,
dev->id.device_id,
WARNING:LONG_LINE_COMMENT: line over 80 characters
#117: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:531:
+ /* It doesn't matter what remove
returns -
WARNING:LONG_LINE_COMMENT: line over 80 characters
#118: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:532:
+ * we're removing the device
either way. */
WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a
separate line
#118: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:532:
+ * we're removing the device
either way. */
WARNING:LONG_LINE: line over 80 characters
#125: FILE: lib/librte_eal/linuxapp/eal/eal_pci.c:539:
+ if (dev->driver->drv_flags &
RTE_PCI_DRV_NEED_MAPPING)
total: 0 errors, 9 warnings, 69 lines checked
--->8---
^ permalink raw reply
* Re: [PATCH v2 3/7] pci: Pass rte_pci_addr to functions instead of separate args
From: Shreyansh Jain @ 2016-11-25 10:33 UTC (permalink / raw)
To: Ben Walker; +Cc: dev
In-Reply-To: <1479931644-78960-3-git-send-email-benjamin.walker@intel.com>
On Thursday 24 November 2016 01:37 AM, Ben Walker wrote:
> Instead of passing domain, bus, devid, func, just pass
> an rte_pci_addr.
>
> Signed-off-by: Ben Walker <benjamin.walker@intel.com>
> ---
> lib/librte_eal/linuxapp/eal/eal_pci.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 876ba38..073af5f 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -267,8 +267,7 @@ pci_parse_sysfs_resource(const char *filename, struct rte_pci_device *dev)
>
> /* Scan one pci sysfs entry, and fill the devices list from it. */
> static int
> -pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
> - uint8_t devid, uint8_t function)
> +pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
> {
> char filename[PATH_MAX];
> unsigned long tmp;
> @@ -281,10 +280,7 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
> return -1;
>
> memset(dev, 0, sizeof(*dev));
> - dev->addr.domain = domain;
> - dev->addr.bus = bus;
> - dev->addr.devid = devid;
> - dev->addr.function = function;
> + dev->addr = *addr;
>
> /* get vendor id */
> snprintf(filename, sizeof(filename), "%s/vendor", dirname);
> @@ -429,16 +425,14 @@ pci_update_device(const struct rte_pci_addr *addr)
> pci_get_sysfs_path(), addr->domain, addr->bus, addr->devid,
> addr->function);
>
> - return pci_scan_one(filename, addr->domain, addr->bus, addr->devid,
> - addr->function);
> + return pci_scan_one(filename, addr);
> }
>
> /*
> * split up a pci address into its constituent parts.
> */
> static int
> -parse_pci_addr_format(const char *buf, int bufsize, uint16_t *domain,
> - uint8_t *bus, uint8_t *devid, uint8_t *function)
> +parse_pci_addr_format(const char *buf, int bufsize, struct rte_pci_addr *addr)
> {
> /* first split on ':' */
> union splitaddr {
> @@ -466,10 +460,10 @@ parse_pci_addr_format(const char *buf, int bufsize, uint16_t *domain,
>
> /* now convert to int values */
> errno = 0;
> - *domain = (uint16_t)strtoul(splitaddr.domain, NULL, 16);
> - *bus = (uint8_t)strtoul(splitaddr.bus, NULL, 16);
> - *devid = (uint8_t)strtoul(splitaddr.devid, NULL, 16);
> - *function = (uint8_t)strtoul(splitaddr.function, NULL, 10);
> + addr->domain = (uint16_t)strtoul(splitaddr.domain, NULL, 16);
> + addr->bus = (uint8_t)strtoul(splitaddr.bus, NULL, 16);
> + addr->devid = (uint8_t)strtoul(splitaddr.devid, NULL, 16);
> + addr->function = (uint8_t)strtoul(splitaddr.function, NULL, 10);
> if (errno != 0)
> goto error;
>
> @@ -490,8 +484,7 @@ rte_eal_pci_scan(void)
> struct dirent *e;
> DIR *dir;
> char dirname[PATH_MAX];
> - uint16_t domain;
> - uint8_t bus, devid, function;
> + struct rte_pci_addr addr;
>
> dir = opendir(pci_get_sysfs_path());
> if (dir == NULL) {
> @@ -500,20 +493,21 @@ rte_eal_pci_scan(void)
> return -1;
> }
>
> +
Unnecessary new line.
> while ((e = readdir(dir)) != NULL) {
> if (e->d_name[0] == '.')
> continue;
>
> - if (parse_pci_addr_format(e->d_name, sizeof(e->d_name), &domain,
> - &bus, &devid, &function) != 0)
> + if (parse_pci_addr_format(e->d_name, sizeof(e->d_name), &addr) != 0)
> continue;
>
> snprintf(dirname, sizeof(dirname), "%s/%s",
> pci_get_sysfs_path(), e->d_name);
> - if (pci_scan_one(dirname, domain, bus, devid, function) < 0)
> + if (pci_scan_one(dirname, &addr) < 0)
> goto error;
> }
> closedir(dir);
> +
> return 0;
>
> error:
>
This is much more cleaner than passing all the BDF entries.
Except the above unnecessary new line:
Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Ferruh Yigit @ 2016-11-25 10:25 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <0982edd5-0ee6-2d2a-0c15-38c1edab5d7a@solarflare.com>
On 11/24/2016 4:15 PM, Andrew Rybchenko wrote:
> On 11/23/2016 06:29 PM, Ferruh Yigit wrote:
>> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>>> The patch series adds Solarflare libefx-based network PMD.
>>>
>>> This version of the driver supports Solarflare SFN7xxx and SFN8xxx
>>> families of 10/40 Gbps adapters.
>>>
>>> libefx is a platform-independent library to implement drivers for
>>> Solarflare network adapters. It provides unified adapter family
>>> independent interface (if possible). FreeBSD [1] and illumos [2]
>>> drivers are built on top of the library.
>>>
>>> The patch series could be logically structured into 5 sub-series:
>>> 1. (1) add the driver skeleton including documentation
>>> 2. (2-30) import libefx and include it in build with the latest patch
>>> 3. (31-43) implement minimal device level operations in steps
>>> 4. (44-51) implement Rx subsystem
>>> 5. (52-56) implement Tx subsystem
>>>
>>> Functional driver with multi-queue support capable to send and receive
>>> traffic appears with the last patch in the series.
>>>
>>> The following design decisions are made during development:
>>>
>>> 1. Since libefx uses positive errno return codes, positive errno
>>> return codes are used inside the driver and coversion to negative
>>> is done on return from eth_dev_ops callbacks. We think that it
>>> is the less error-prone way.
>>>
>>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>>> is considered and could be added in the future. Code for data path
>>> should be shared by these two drivers. libefx-based PMD is put into
>>> 'efx' subdirectory to have a space for another PMD and shared code.
>>>
>>> 3. Own event queue (a way to deliver events from HW to host CPU) is
>>> used for house-keeping (e.g. link status notifications), each Tx
>>> and each Rx queue. No locks on datapath are requires in this case.
>>>
>>> 4. Alarm is used to periodically poll house-keeping event queue.
>>> The event queue is used to deliver link status change notifications,
>>> Rx/Tx queue flush events, SRAM events. It is not used on datapath.
>>> The event queue polling is protected using spin-lock since
>>> concurrent access from different contexts is possible (e.g. device
>>> stop when polling alarm is running).
>>>
>>> [1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/
>>> [2] https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/
>>>
>>> ---
>> I would like to note that very well organized patchset. Thank you for
>> your effort.
>
> Thanks a lot, it is very pleasant to read it.
>
> Please, see my questions in thread for patches 01/56 (about compiler
> versions to test on) and 30/56 (about libefx configuration documentation).
>
> Also I'd like to ask a question about the further patches submission.
> We have about 40 patches which support various features (RSS, stats,
> flow control and many others). What is the preferred way to submit it?
> A. Separately since they are not so tightly related (but in fact cannot be
> applied in random order since some touch the same lines in code)
> B. As a series to process everything in one go.
If they can't be applied in random order, it is better send as a patchset.
But if you can make multiple logically independent patchsets with small
effort, please do, smaller patchsets are easier to chew.
>
> Thanks,
> Andrew.
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Ferruh Yigit @ 2016-11-25 10:24 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <5d041f12-ec8d-77da-47ac-671eb3a39a5b@solarflare.com>
On 11/23/2016 7:49 AM, Andrew Rybchenko wrote:
> On 11/23/2016 03:02 AM, Ferruh Yigit wrote:
>> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>>> The patch series adds Solarflare libefx-based network PMD.
>>>
>>> This version of the driver supports Solarflare SFN7xxx and SFN8xxx
>>> families of 10/40 Gbps adapters.
>>>
>>> libefx is a platform-independent library to implement drivers for
>>> Solarflare network adapters. It provides unified adapter family
>>> independent interface (if possible). FreeBSD [1] and illumos [2]
>>> drivers are built on top of the library.
>>>
>>> The patch series could be logically structured into 5 sub-series:
>>> 1. (1) add the driver skeleton including documentation
>>> 2. (2-30) import libefx and include it in build with the latest patch
>>> 3. (31-43) implement minimal device level operations in steps
>>> 4. (44-51) implement Rx subsystem
>>> 5. (52-56) implement Tx subsystem
>>>
>>> Functional driver with multi-queue support capable to send and receive
>>> traffic appears with the last patch in the series.
>>>
>>> The following design decisions are made during development:
>>>
>>> 1. Since libefx uses positive errno return codes, positive errno
>>> return codes are used inside the driver and coversion to negative
>>> is done on return from eth_dev_ops callbacks. We think that it
>>> is the less error-prone way.
>>>
>>> 2. Another Solarflare PMD with in-kernel part (for control operations)
>>> is considered and could be added in the future. Code for data path
>>> should be shared by these two drivers. libefx-based PMD is put into
>>> 'efx' subdirectory to have a space for another PMD and shared code.
>>>
>>> 3. Own event queue (a way to deliver events from HW to host CPU) is
>>> used for house-keeping (e.g. link status notifications), each Tx
>>> and each Rx queue. No locks on datapath are requires in this case.
>>>
>>> 4. Alarm is used to periodically poll house-keeping event queue.
>>> The event queue is used to deliver link status change notifications,
>>> Rx/Tx queue flush events, SRAM events. It is not used on datapath.
>>> The event queue polling is protected using spin-lock since
>>> concurrent access from different contexts is possible (e.g. device
>>> stop when polling alarm is running).
>>>
>>> [1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/
>>> [2] https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/
>>>
>>> ---
>>>
>>> Andrew Rybchenko (49):
>>> net/sfc: libefx-based PMD stub sufficient to build and init
>>> net/sfc: import libefx base
>>> net/sfc: import libefx register definitions
>>> net/sfc: import libefx filters support
>>> net/sfc: import libefx MCDI definition
>>> net/sfc: import libefx MCDI implementation
>>> net/sfc: import libefx MCDI logging support
>>> net/sfc: import libefx MCDI proxy authorization support
>>> net/sfc: import libefx 5xxx/6xxx family support
>>> net/sfc: import libefx SFN7xxx family support
>>> net/sfc: import libefx SFN8xxx family support
>>> net/sfc: import libefx diagnostics support
>>> net/sfc: import libefx built-in selftest support
>>> net/sfc: import libefx software per-queue statistics support
>>> net/sfc: import libefx PHY flags control support
>>> net/sfc: import libefx PHY statistics support
>>> net/sfc: import libefx PHY LEDs control support
>>> net/sfc: import libefx MAC statistics support
>>> net/sfc: import libefx event prefetch support
>>> net/sfc: import libefx Rx scatter support
>>> net/sfc: import libefx RSS support
>>> net/sfc: import libefx loopback control support
>>> net/sfc: import libefx monitors statistics support
>>> net/sfc: import libefx support to access monitors via MCDI
>>> net/sfc: import libefx support for Rx packed stream mode
>>> net/sfc: import libefx NVRAM support
>>> net/sfc: import libefx VPD support
>>> net/sfc: import libefx bootrom configuration support
>>> net/sfc: import libefx licensing support
>>> net/sfc: implement dummy callback to get device information
>>> net/sfc: implement driver operation to init device on attach
>>> net/sfc: add device configure and close stubs
>>> net/sfc: add device configuration checks
>>> net/sfc: implement device start and stop operations
>>> net/sfc: make available resources estimation and allocation
>>> net/sfc: interrupts support sufficient for event queue init
>>> net/sfc: implement event queue support
>>> net/sfc: implement EVQ dummy exception handling
>>> net/sfc: maintain management event queue
>>> net/sfc: periodic management EVQ polling using alarm
>>> net/sfc: minimum port control sufficient to receive traffic
>>> net/sfc: implement Rx subsystem stubs
>>> net/sfc: check configured rxmode
>>> net/sfc: implement Rx queue setup release operations
>>> net/sfc: calculate Rx buffer size which may be used
>>> net/sfc: validate Rx queue buffers setup
>>> net/sfc: implement Rx queue start and stop operations
>>> net/sfc: implement device callback to Rx burst of packets
>>> net/sfc: discard scattered packet on Rx correctly
>>>
>>> Artem Andreev (2):
>>> net/sfc: include libefx in build
>>> net/sfc: implement device operation to retrieve link info
>>>
>>> Ivan Malov (5):
>>> net/sfc: provide basic stubs for Tx subsystem
>>> net/sfc: add function to check configured Tx mode
>>> net/sfc: add callbacks to set up and release Tx queues
>>> net/sfc: implement transmit path start / stop
>>> net/sfc: add callback to send bursts of packets
>>>
>> Hi Andrew,
>>
>> Thank you for the patch, I have encounter with a few minor issues, can
>> you please check them [1]?
>>
>> Also folder structure is drivers/net/sfc/efx/<all_src_files>, why /sfc/
>> layer is created?
>> sfc is company name (solarflare communications), right? Other driver
>> folders not structured based on company, what about using
>> drivers/net/efx/* ?
>
> I've tried to explain it above in item (2):
>
> >>>
>
> 2. Another Solarflare PMD with in-kernel part (for control operations)
> is considered and could be added in the future. Code for data path
> should be shared by these two drivers. libefx-based PMD is put into
> 'efx' subdirectory to have a space for another PMD and shared code.
>
> <<<
>
> So, main reason is to have location for the code shared by two Solarflare
> network PMDs. May be it better to relocate when we really have it.
> I'm open for other ideas/suggestions.
If there will be another PMD that shares code with current one, the
logic seems good, but I am not sure about start using company names, I
am not against it, just I don't know.
Let's relocate later, this buys some time to think / get feedback on issue.
>
>> [1]:
>> 1- There are a few (non-base) checkpatch warnings, can you please check
>> patch 36, 49, 50 and 55 please?
>
> Thanks, I'll fix spelling in v2.
> 36, 49 and 55 also ask to check multiple assignments. IMHO, it is the
> right usage
> of multiple assignment when logically bound variables must have the same
> value.
>
>> 2- Got following compile issues, not investigated, directly sharing here:
>>
>> b) for icc getting following warnings:
>> =======================================
>> icc: command line warning #10006: ignoring unknown option '-Wno-empty-body'
>> icc: command line warning #10006: ignoring unknown option
>> '-Waggregate-return'
>> icc: command line warning #10006: ignoring unknown option
>> '-Wbad-function-cast'
>> icc: command line warning #10006: ignoring unknown option '-Wnested-externs'
>>
>>
>> c) icc compiler errors:
>> =======================================
>> In file included from
>> .../x86_64-native-linuxapp-icc/include/rte_ethdev.h(185),
>> from .../drivers/net/sfc/efx/sfc.h(35),
>> from .../drivers/net/sfc/efx/sfc.c(37):
>> .../x86_64-native-linuxapp-icc/include/rte_ether.h(258): warning #2203:
>> cast discards qualifiers from target type
>> uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes);
>> ^
>>
>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1157): warning #3179: deprecated
>> conversion of string literal to char* (should be const char*)
>> .../drivers/net/sfc/efx/base/ef10_filter.c(1276): warning #188:
>> enumerated type mixed with another type
>> : "unknown assertion";
>> ^
>>
>> filter_flags = 0;
>> ^
>>
>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1426): warning #188: enumerated
>> type mixed with another type
>> epp->ep_fixed_port_type =
>> ^
>>
>> .../drivers/net/sfc/efx/base/efx_nic.c(556): warning #188: enumerated
>> type mixed with another type
>> enp->en_family = 0;
>
> Yes, I have no ICC compilers. I'll try to fix these warnings, but I
> can't be sure without checking it.
> Also we cannot claim ICC supported without building and testing the
> generated binary.
Please update the code at least to not break the icc compilation,
specially since this PMD will be default enabled. If you prefer I can
verify compilation offline before you send the patchset.
>
> Many thanks,
> Andrew.
>
^ permalink raw reply
* Re: [PATCH 30/56] net/sfc: include libefx in build
From: Ferruh Yigit @ 2016-11-25 10:24 UTC (permalink / raw)
To: Andrew Rybchenko, dev; +Cc: Artem Andreev
In-Reply-To: <0cc9d168-a15f-abb7-303f-8c22132227ed@solarflare.com>
On 11/24/2016 3:44 PM, Andrew Rybchenko wrote:
> See one question below.
>
> On 11/23/2016 06:26 PM, Ferruh Yigit wrote:
>> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>>> From: Artem Andreev <Artem.Andreev@oktetlabs.ru>
>>>
>>> Implement efsys.h for the PMD.
>>>
>>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>>> Signed-off-by: Artem Andreev <Artem.Andreev@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>> drivers/net/sfc/efx/Makefile | 54 +++
>>> drivers/net/sfc/efx/efsys.h | 767 +++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 821 insertions(+)
>>> create mode 100644 drivers/net/sfc/efx/efsys.h
>>>
>>> diff --git a/drivers/net/sfc/efx/Makefile b/drivers/net/sfc/efx/Makefile
>>> index 71f07ca..de95ea8 100644
>>> --- a/drivers/net/sfc/efx/Makefile
>>> +++ b/drivers/net/sfc/efx/Makefile
>>> @@ -33,6 +33,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>> #
>>> LIB = librte_pmd_sfc_efx.a
>>>
>>> +CFLAGS += -I$(SRCDIR)/base/
>>> +CFLAGS += -I$(SRCDIR)
>>> CFLAGS += -O3
>>>
>>> # Enable basic warnings but disable some which are accepted
>>> @@ -60,6 +62,17 @@ CFLAGS += -Wstrict-prototypes
>>> CFLAGS += -Wundef
>>> CFLAGS += -Wwrite-strings
>>>
>>> +# Extra CFLAGS for base driver files
>>> +CFLAGS_BASE_DRIVER += -Wno-unused-variable
>>> +CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
>> clang complain about this one:
>> warning: unknown warning option '-Wno-unused-but-set-variable'; did you
>> mean '-Wno-unused-const-variable'? [-Wunknown-warning-option]
>
> Will fix in v2
>
>>> +
>>> +#
>>> +# List of base driver object files for which
>>> +# special CFLAGS above should be applied
>>> +#
>>> +BASE_DRIVER_OBJS=$(patsubst %.c,%.o,$(notdir $(wildcard $(SRCDIR)/base/*.c)))
>>> +$(foreach obj, $(BASE_DRIVER_OBJS), $(eval CFLAGS+=$(CFLAGS_BASE_DRIVER)))
>> This cause multiple "-Wno-unused-variable -Wno-unused-but-set-variable"
>> params in final command, I guess the intention is:
>>
>> $(foreach obj, $(BASE_DRIVER_OBJS), $(eval
>> CFLAGS_$(obj)+=$(CFLAGS_BASE_DRIVER)))
>>
>> Fixing this may generate a few compiler warnings.
>
> Many thanks, will fix in v2.
>
>> <...>
>>
>>> diff --git a/drivers/net/sfc/efx/efsys.h b/drivers/net/sfc/efx/efsys.h
>>> new file mode 100644
>>> index 0000000..2eef996
>>> --- /dev/null
>>> +++ b/drivers/net/sfc/efx/efsys.h
>>> @@ -0,0 +1,767 @@
>> <...>
>>
>> I guess below is hardcoded compile time configuration for libefx, do you
>> think does it make sense to document this default configuration?
>
> Yes, it is libefx configuration and more options will be enabled when
> corresponding
> functionality is supported in the PMD.
> I'm sorry, but I don't understand what would you like to see in the
> documentation.
> Could you clarify, please?
This is mostly a question, following defines how libefx behaves, and a
little hard to find, do you think does it make sense to document these
in nic documentation, guides/nics/sfc_efx.rst, to highlight default
configuration.
Like by default filtering capabilities and SFN7xxx family support
enabled but 5xxx/6xxx family support disabled... These can be listed in
a bullet listed way in two groups (default enabled / default disabled) ?
>
>>> +
>>> +#define EFSYS_OPT_NAMES 0
>>> +
>>> +#define EFSYS_OPT_SIENA 0
>>> +#define EFSYS_OPT_HUNTINGTON 1
>>> +#define EFSYS_OPT_MEDFORD 1
>>> +#ifdef RTE_LIBRTE_SFC_EFX_DEBUG
>>> +#define EFSYS_OPT_CHECK_REG 1
>>> +#else
>>> +#define EFSYS_OPT_CHECK_REG 0
>>> +#endif
>>> +
>>> +#define EFSYS_OPT_MCDI 1
>>> +#define EFSYS_OPT_MCDI_LOGGING 0
>>> +#define EFSYS_OPT_MCDI_PROXY_AUTH 0
>>> +
>>> +#define EFSYS_OPT_MAC_STATS 0
>>> +
>>> +#define EFSYS_OPT_LOOPBACK 0
>>> +
>>> +#define EFSYS_OPT_MON_MCDI 0
>>> +#define EFSYS_OPT_MON_STATS 0
>>> +
>>> +#define EFSYS_OPT_PHY_STATS 0
>>> +#define EFSYS_OPT_BIST 0
>>> +#define EFSYS_OPT_PHY_LED_CONTROL 0
>>> +#define EFSYS_OPT_PHY_FLAGS 0
>>> +
>>> +#define EFSYS_OPT_VPD 0
>>> +#define EFSYS_OPT_NVRAM 0
>>> +#define EFSYS_OPT_BOOTCFG 0
>>> +
>>> +#define EFSYS_OPT_DIAG 0
>>> +#define EFSYS_OPT_RX_SCALE 0
>>> +#define EFSYS_OPT_QSTATS 0
>>> +#define EFSYS_OPT_FILTER 1
>>> +#define EFSYS_OPT_RX_SCATTER 0
>>> +
>>> +#define EFSYS_OPT_EV_PREFETCH 0
>>> +
>>> +#define EFSYS_OPT_DECODE_INTR_FATAL 0
>>> +
>>> +#define EFSYS_OPT_LICENSING 0
>>> +
>>> +#define EFSYS_OPT_ALLOW_UNCONFIGURED_NIC 0
>>> +
>>> +#define EFSYS_OPT_RX_PACKED_STREAM 0
>> <...>
>
>
^ permalink raw reply
* Re: [PATCH 01/56] net/sfc: libefx-based PMD stub sufficient to build and init
From: Ferruh Yigit @ 2016-11-25 10:17 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <f300afdb-5120-ad2a-0e4a-f182d0361499@solarflare.com>
On 11/24/2016 3:59 PM, Andrew Rybchenko wrote:
> On 11/23/2016 06:26 PM, Ferruh Yigit wrote:
>> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>>> The PMD is put into the sfc/efx subdirectory to have a place for
>>> the second PMD and library shared by both.
>>>
>>> Enable the PMD by default on supported configuratons.
>>>
>>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>> MAINTAINERS | 6 ++
>>> config/common_base | 6 ++
>>> config/defconfig_arm-armv7a-linuxapp-gcc | 1 +
>>> config/defconfig_arm64-armv8a-linuxapp-gcc | 1 +
>>> config/defconfig_i686-native-linuxapp-gcc | 5 +
>>> config/defconfig_i686-native-linuxapp-icc | 5 +
>>> config/defconfig_ppc_64-power8-linuxapp-gcc | 1 +
>>> config/defconfig_tile-tilegx-linuxapp-gcc | 1 +
>>> config/defconfig_x86_64-native-linuxapp-icc | 5 +
>>> config/defconfig_x86_x32-native-linuxapp-gcc | 5 +
>>> doc/guides/nics/features/sfc_efx.ini | 10 ++
>>> doc/guides/nics/index.rst | 1 +
>>> doc/guides/nics/sfc_efx.rst | 109 +++++++++++++++++++++
>> Can you also update release notes please, to announce new driver.
>
> Thanks, will do in v2.
>
>> <...>
>>
>>> diff --git a/drivers/net/sfc/efx/Makefile b/drivers/net/sfc/efx/Makefile
>>> new file mode 100644
>>> index 0000000..71f07ca
>>> --- /dev/null
>>> +++ b/drivers/net/sfc/efx/Makefile
>>> @@ -0,0 +1,81 @@
>> <...>
>>> +
>>> +include $(RTE_SDK)/mk/rte.vars.mk
>>> +
>>> +#
>>> +# library name
>>> +#
>>> +LIB = librte_pmd_sfc_efx.a
>>> +
>>> +CFLAGS += -O3
>>> +
>>> +# Enable basic warnings but disable some which are accepted
>>> +CFLAGS += -Wall
>> It is possible to use $(WERROR_FLAGS), which set automatically based on
>> selected compiler. See mk/toolchain/* .
>
> Thanks, will do in v2.
>
>> And you can add extra options here, please keep in mind that there are
>> three compiler supported right now: gcc, clang and icc. You may require
>> to add compiler and version checks..
>
> I've tried to disable the driver build on ICC since we've never tested it.
I believe we don't support selective config per compiler. Currently if a
code is enabled by default, it should support compilation with all three
compilers.
> I've failed to find list of compiler versions which must/should be checked.
That list is not clear as far as I know. Mostly version related fixes
added based on reported build errors. So you can leave as it is right
now, or can test with default compiler versions of some common
distributions.
> I've tested versions which come with RHEL 7.2, Debian Jessie and Sid.
> (In v1 I've lost my fixes for clang which produce warnings because of
> unsupported -W option)
>
>>> +CFLAGS += -Wno-strict-aliasing
>>> +
>>> +# Enable extra warnings but disable some which are accepted
>>> +CFLAGS += -Wextra
>>> +CFLAGS += -Wno-empty-body
>>> +CFLAGS += -Wno-sign-compare
>>> +CFLAGS += -Wno-type-limits
>>> +CFLAGS += -Wno-unused-parameter
>> Is there a way to not disable these warnings but fix in the source code?
>> Or move to CFLAGS_BASE_DRIVER, if the reason is the base driver?
>
> Will do in v2.
>
>>> +
>>> +# More warnings not enabled by above aggregators
>>> +CFLAGS += -Waggregate-return
>>> +CFLAGS += -Wbad-function-cast
>>> +CFLAGS += -Wcast-qual
>>> +CFLAGS += -Wdisabled-optimization
>>> +CFLAGS += -Wmissing-declarations
>>> +CFLAGS += -Wmissing-prototypes
>>> +CFLAGS += -Wnested-externs
>>> +CFLAGS += -Wold-style-definition
>>> +CFLAGS += -Wpointer-arith
>>> +CFLAGS += -Wstrict-prototypes
>>> +CFLAGS += -Wundef
>>> +CFLAGS += -Wwrite-strings
>> If you believe some can be useful for everybody, please feel free to add
>> to mk/toolchain/* .
>
> I'll definitely remove duplicates which are already included in
> $(WERROR_FLAGS).
> I'd prefer to keep the rest just here for now. I think that adding it
> world-wide
> requires testing on really many compiler versions etc.
>
>>> +
>>> +EXPORT_MAP := rte_pmd_sfc_efx_version.map
>>> +
>>> +LIBABIVER := 1
>>> +
>>> +#
>>> +# all source are stored in SRCS-y
>>> +#
>>> +SRCS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += sfc_ethdev.c
>>> +SRCS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += sfc_kvargs.c
>>> +
>>> +
>>> +# this lib depends upon:
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_eal
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_kvargs
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_ether
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_mempool
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_mbuf
>>> +
>>> +include $(RTE_SDK)/mk/rte.lib.mk
>>> diff --git a/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map b/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map
>>> new file mode 100644
>>> index 0000000..1901bcb
>>> --- /dev/null
>>> +++ b/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map
>>> @@ -0,0 +1,4 @@
>>> +DPDK_16.07 {
>> Now this become 17.02
>
> Thanks, will fix in v2.
>
>>> +
>>> + local: *;
>>> +};
>>> diff --git a/drivers/net/sfc/efx/sfc.h b/drivers/net/sfc/efx/sfc.h
>>> new file mode 100644
>>> index 0000000..16fd2bb
>>> --- /dev/null
>>> +++ b/drivers/net/sfc/efx/sfc.h
>>> @@ -0,0 +1,53 @@
>> <..>
>>> +
>>> +#ifndef _SFC_H
>>> +#define _SFC_H
>> s/^I/ /
>> This also exists in other locations and files..
>
> Will fix in v2.
> I thought that DPDK prefers TAB after #define as FreeBSD does, but
> counting shows that space is really preferred.
> I think that such things should be caught by checkpatch.
>
>> <...>
>
>
^ 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