[dpdk-dev] [PATCH v2 00/26] net/ena: new features and fixes

[dpdk-dev] [PATCH v2 00/26] net/ena: new features and fixes

[Michal Krawczyk]

The ENA driver was updated with the new features and few fixes and minor
changes are introduced.
First of all, the communication layer which is delivered by vendor was
updated - the version in the HEAD is a bit outdated now. ENA is able to
communicate with the driver through Admin queue by using admin interrupts
instead of polling.
Admin interrupts are also used for handling AENQ events, which are used for
the following new features:
  - LSC handlers
  - watchdog and device rest
  - monitoring the admin queue
  - handling ENA notifications (getting hints from device)
For the watchdog and admin queue monitoring, the timers had to be used, so
the makefile was modified to do not cut out the librte_timer.

>From other fixes and changes:
  - legacy LLQ was removed which is now deprecated API
  - Rx out of order completion was added to enable cleaning up packets out
    of order
  - Tx mbufs are now linearized if they exceed supported number of segments
  - pass information about maximum number of Tx and Rx descriptors
  - the IO queue number is now taking into consideration maximum number of
    sq and cq
  - Tx id requested for sending is now being validated and the reset is
    being triggered if it is invalid
  - branch predictioning was added for better performance
  - error checking and returned values were fixed
  - macros for allocating memory in communication layer were fixed
  - information about numa mode is now being passed to the NIC

---
v2:
* Rebased on top of dpdk-next-net
* Added link speed patch
* Added fix when allocating coherent memory in the PMD

Michal Krawczyk (11):
  net/ena: update ena_com to the newer version
  net/ena: add interrupt handler for admin queue
  net/ena: add stop and uninit routines
  net/ena: add LSC intr support and AENQ handling
  net/ena: restart only initialized queues instead of all
  net/ena: add reset routine
  mk: link librte_timer with --whole-archive
  net/ena: add watchdog and keep alive AENQ handler
  net/ena: add checking for admin queue state
  net/ena: add RX out of order completion
  net/ena: store handle when allocating coherent memory node

Rafal Kozik (15):
  net/ena: remove support of legacy LLQ
  net/ena: handle ENA notification
  net/ena: make watchdog configurable
  net/ena: linearize Tx mbuf
  net/ena: add info about max number of Tx/Rx descriptors
  net/ena: unimplemented handler error
  net/ena: rework configuration of IO queue numbers
  net/ena: validate Tx req id
  net/ena: add (un)likely statements
  net/ena: adjust error checking and cleaning
  net/ena: update numa node
  net/ena: check pointer before memset
  net/ena: change memory type
  net/ena: fix GENMASK_ULL macro
  net/ena: set link speed as none

 drivers/net/ena/base/ena_com.c                  |  717 +++++++-------
 drivers/net/ena/base/ena_com.h                  |  112 +--
 drivers/net/ena/base/ena_defs/ena_admin_defs.h  | 1164 +++++++----------------
 drivers/net/ena/base/ena_defs/ena_common_defs.h |    8 +-
 drivers/net/ena/base/ena_defs/ena_eth_io_defs.h |  758 +++++----------
 drivers/net/ena/base/ena_defs/ena_gen_info.h    |    4 +-
 drivers/net/ena/base/ena_defs/ena_includes.h    |    2 -
 drivers/net/ena/base/ena_defs/ena_regs_defs.h   |   36 +
 drivers/net/ena/base/ena_eth_com.c              |   78 +-
 drivers/net/ena/base/ena_eth_com.h              |   10 +-
 drivers/net/ena/base/ena_plat.h                 |    2 -
 drivers/net/ena/base/ena_plat_dpdk.h            |   74 +-
 drivers/net/ena/ena_ethdev.c                    |  718 +++++++++++---
 drivers/net/ena/ena_ethdev.h                    |   32 +-
 mk/rte.app.mk                                   |    1 +
 15 files changed, 1709 insertions(+), 2007 deletions(-)

-- 
2.14.1

Re: [dpdk-dev] [PATCH v2 00/26] net/ena: new features and fixes

[Ferruh Yigit]

On 6/4/2018 1:09 PM, Michal Krawczyk wrote:
> The ENA driver was updated with the new features and few fixes and minor
> changes are introduced.
> First of all, the communication layer which is delivered by vendor was
> updated - the version in the HEAD is a bit outdated now. ENA is able to
> communicate with the driver through Admin queue by using admin interrupts
> instead of polling.
> Admin interrupts are also used for handling AENQ events, which are used for
> the following new features:
>   - LSC handlers
>   - watchdog and device rest
>   - monitoring the admin queue
>   - handling ENA notifications (getting hints from device)
> For the watchdog and admin queue monitoring, the timers had to be used, so
> the makefile was modified to do not cut out the librte_timer.
> 
> From other fixes and changes:
>   - legacy LLQ was removed which is now deprecated API
>   - Rx out of order completion was added to enable cleaning up packets out
>     of order
>   - Tx mbufs are now linearized if they exceed supported number of segments
>   - pass information about maximum number of Tx and Rx descriptors
>   - the IO queue number is now taking into consideration maximum number of
>     sq and cq
>   - Tx id requested for sending is now being validated and the reset is
>     being triggered if it is invalid
>   - branch predictioning was added for better performance
>   - error checking and returned values were fixed
>   - macros for allocating memory in communication layer were fixed
>   - information about numa mode is now being passed to the NIC
> 
> ---
> v2:
> * Rebased on top of dpdk-next-net
> * Added link speed patch
> * Added fix when allocating coherent memory in the PMD

Hi Michał,

I am getting build error for ICC [1] and shared library [2], can you please check?


[1]
.../drivers/net/ena/base/ena_com.c(323): error #592: variable "flags" is used
before its value is set
        ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
        ^
.../drivers/net/ena/base/ena_com.c(534): error #3656: variable "flags" may be
used before its value is set
                  ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
                  ^
.../drivers/net/ena/base/ena_com.c(589): error #592: variable "flags" is used
before its value is set
                ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
                ^
.../drivers/net/ena/base/ena_com.c(634): error #592: variable "flags" is used
before its value is set
        ENA_SPINLOCK_LOCK(mmio_read->lock, flags);
        ^
.../drivers/net/ena/base/ena_com.c(1297): error #592: variable "flags" is used
before its value is set
        ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
        ^
.../drivers/net/ena/base/ena_com.c(1341): error #592: variable "flags" is used
before its value is set
        ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
        ^


[2]
ena_ethdev.o: In function `eth_ena_pci_probe':
.../drivers/net/ena/ena_ethdev.c:(.text+0x6de): undefined reference to
`rte_timer_subsystem_init'
.../drivers/net/ena/ena_ethdev.c:(.text+0x6ea): undefined reference to
`rte_timer_init'
ena_ethdev.o: In function `eth_ena_pci_remove':
.../drivers/net/ena/ena_ethdev.c:(.text+0x854): undefined reference to
`rte_timer_stop_sync'
ena_ethdev.o: In function `ena_start':
.../drivers/net/ena/ena_ethdev.c:(.text+0x1ff6): undefined reference to
`rte_timer_reset'
ena_ethdev.o: In function `ena_stop':
.../drivers/net/ena/ena_ethdev.c:(.text+0x21a1): undefined reference to
`rte_timer_stop_sync'
ena_ethdev.o: In function `ena_close':
.../drivers/net/ena/ena_ethdev.c:(.text+0x21d8): undefined reference to
`rte_timer_stop_sync'

Re: [dpdk-dev] [PATCH v2 00/26] net/ena: new features and fixes

[Michał Krawczyk]

Hi Ferruh,

2018-06-05 18:42 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
>
> On 6/4/2018 1:09 PM, Michal Krawczyk wrote:
> > The ENA driver was updated with the new features and few fixes and minor
> > changes are introduced.
> > First of all, the communication layer which is delivered by vendor was
> > updated - the version in the HEAD is a bit outdated now. ENA is able to
> > communicate with the driver through Admin queue by using admin interrupts
> > instead of polling.
> > Admin interrupts are also used for handling AENQ events, which are used for
> > the following new features:
> >   - LSC handlers
> >   - watchdog and device rest
> >   - monitoring the admin queue
> >   - handling ENA notifications (getting hints from device)
> > For the watchdog and admin queue monitoring, the timers had to be used, so
> > the makefile was modified to do not cut out the librte_timer.
> >
> > From other fixes and changes:
> >   - legacy LLQ was removed which is now deprecated API
> >   - Rx out of order completion was added to enable cleaning up packets out
> >     of order
> >   - Tx mbufs are now linearized if they exceed supported number of segments
> >   - pass information about maximum number of Tx and Rx descriptors
> >   - the IO queue number is now taking into consideration maximum number of
> >     sq and cq
> >   - Tx id requested for sending is now being validated and the reset is
> >     being triggered if it is invalid
> >   - branch predictioning was added for better performance
> >   - error checking and returned values were fixed
> >   - macros for allocating memory in communication layer were fixed
> >   - information about numa mode is now being passed to the NIC
> >
> > ---
> > v2:
> > * Rebased on top of dpdk-next-net
> > * Added link speed patch
> > * Added fix when allocating coherent memory in the PMD
>
> Hi Michał,
>
> I am getting build error for ICC [1] and shared library [2], can you please check?
>
>
> [1]
> .../drivers/net/ena/base/ena_com.c(323): error #592: variable "flags" is used
> before its value is set
>         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>         ^
> .../drivers/net/ena/base/ena_com.c(534): error #3656: variable "flags" may be
> used before its value is set
>                   ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>                   ^
> .../drivers/net/ena/base/ena_com.c(589): error #592: variable "flags" is used
> before its value is set
>                 ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>                 ^
> .../drivers/net/ena/base/ena_com.c(634): error #592: variable "flags" is used
> before its value is set
>         ENA_SPINLOCK_LOCK(mmio_read->lock, flags);
>         ^
> .../drivers/net/ena/base/ena_com.c(1297): error #592: variable "flags" is used
> before its value is set
>         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>         ^
> .../drivers/net/ena/base/ena_com.c(1341): error #592: variable "flags" is used
> before its value is set
>         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>         ^
>

That should be easy to fix - I prepared the patch but I want to send
it with second fix together.

>
>
> [2]
> ena_ethdev.o: In function `eth_ena_pci_probe':
> .../drivers/net/ena/ena_ethdev.c:(.text+0x6de): undefined reference to
> `rte_timer_subsystem_init'
> .../drivers/net/ena/ena_ethdev.c:(.text+0x6ea): undefined reference to
> `rte_timer_init'
> ena_ethdev.o: In function `eth_ena_pci_remove':
> .../drivers/net/ena/ena_ethdev.c:(.text+0x854): undefined reference to
> `rte_timer_stop_sync'
> ena_ethdev.o: In function `ena_start':
> .../drivers/net/ena/ena_ethdev.c:(.text+0x1ff6): undefined reference to
> `rte_timer_reset'
> ena_ethdev.o: In function `ena_stop':
> .../drivers/net/ena/ena_ethdev.c:(.text+0x21a1): undefined reference to
> `rte_timer_stop_sync'
> ena_ethdev.o: In function `ena_close':
> .../drivers/net/ena/ena_ethdev.c:(.text+0x21d8): undefined reference to
> `rte_timer_stop_sync'
>

Strange that this happens, it should be fixed in the patch:
  mk: link librte_timer with --whole-archive
which seems to be no longer needed, because similar commit is already
applied (eb54ef42b02f94c4093556fdd6b51e2d7fd0df47). That should
resolve the issue with linking (at least for gcc it is helping) by
linking timer library with --whole-archive.
Could you, please, send me build logs so I could look closer if it is
properly built with ICC as I don't have access to it?

Thanks,
Michał

Re: [dpdk-dev] [PATCH v2 00/26] net/ena: new features and fixes

[Ferruh Yigit]

On 6/6/2018 12:46 PM, Michał Krawczyk wrote:
> Hi Ferruh,
> 
> 2018-06-05 18:42 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
>>
>> On 6/4/2018 1:09 PM, Michal Krawczyk wrote:
>>> The ENA driver was updated with the new features and few fixes and minor
>>> changes are introduced.
>>> First of all, the communication layer which is delivered by vendor was
>>> updated - the version in the HEAD is a bit outdated now. ENA is able to
>>> communicate with the driver through Admin queue by using admin interrupts
>>> instead of polling.
>>> Admin interrupts are also used for handling AENQ events, which are used for
>>> the following new features:
>>>   - LSC handlers
>>>   - watchdog and device rest
>>>   - monitoring the admin queue
>>>   - handling ENA notifications (getting hints from device)
>>> For the watchdog and admin queue monitoring, the timers had to be used, so
>>> the makefile was modified to do not cut out the librte_timer.
>>>
>>> From other fixes and changes:
>>>   - legacy LLQ was removed which is now deprecated API
>>>   - Rx out of order completion was added to enable cleaning up packets out
>>>     of order
>>>   - Tx mbufs are now linearized if they exceed supported number of segments
>>>   - pass information about maximum number of Tx and Rx descriptors
>>>   - the IO queue number is now taking into consideration maximum number of
>>>     sq and cq
>>>   - Tx id requested for sending is now being validated and the reset is
>>>     being triggered if it is invalid
>>>   - branch predictioning was added for better performance
>>>   - error checking and returned values were fixed
>>>   - macros for allocating memory in communication layer were fixed
>>>   - information about numa mode is now being passed to the NIC
>>>
>>> ---
>>> v2:
>>> * Rebased on top of dpdk-next-net
>>> * Added link speed patch
>>> * Added fix when allocating coherent memory in the PMD
>>
>> Hi Michał,
>>
>> I am getting build error for ICC [1] and shared library [2], can you please check?
>>
>>
>> [1]
>> .../drivers/net/ena/base/ena_com.c(323): error #592: variable "flags" is used
>> before its value is set
>>         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>>         ^
>> .../drivers/net/ena/base/ena_com.c(534): error #3656: variable "flags" may be
>> used before its value is set
>>                   ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>>                   ^
>> .../drivers/net/ena/base/ena_com.c(589): error #592: variable "flags" is used
>> before its value is set
>>                 ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>>                 ^
>> .../drivers/net/ena/base/ena_com.c(634): error #592: variable "flags" is used
>> before its value is set
>>         ENA_SPINLOCK_LOCK(mmio_read->lock, flags);
>>         ^
>> .../drivers/net/ena/base/ena_com.c(1297): error #592: variable "flags" is used
>> before its value is set
>>         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>>         ^
>> .../drivers/net/ena/base/ena_com.c(1341): error #592: variable "flags" is used
>> before its value is set
>>         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
>>         ^
>>
> 
> That should be easy to fix - I prepared the patch but I want to send
> it with second fix together.
> 
>>
>>
>> [2]
>> ena_ethdev.o: In function `eth_ena_pci_probe':
>> .../drivers/net/ena/ena_ethdev.c:(.text+0x6de): undefined reference to
>> `rte_timer_subsystem_init'
>> .../drivers/net/ena/ena_ethdev.c:(.text+0x6ea): undefined reference to
>> `rte_timer_init'
>> ena_ethdev.o: In function `eth_ena_pci_remove':
>> .../drivers/net/ena/ena_ethdev.c:(.text+0x854): undefined reference to
>> `rte_timer_stop_sync'
>> ena_ethdev.o: In function `ena_start':
>> .../drivers/net/ena/ena_ethdev.c:(.text+0x1ff6): undefined reference to
>> `rte_timer_reset'
>> ena_ethdev.o: In function `ena_stop':
>> .../drivers/net/ena/ena_ethdev.c:(.text+0x21a1): undefined reference to
>> `rte_timer_stop_sync'
>> ena_ethdev.o: In function `ena_close':
>> .../drivers/net/ena/ena_ethdev.c:(.text+0x21d8): undefined reference to
>> `rte_timer_stop_sync'
>>
> 
> Strange that this happens, it should be fixed in the patch:
>   mk: link librte_timer with --whole-archive
> which seems to be no longer needed, because similar commit is already
> applied (eb54ef42b02f94c4093556fdd6b51e2d7fd0df47). That should
> resolve the issue with linking (at least for gcc it is helping) by
> linking timer library with --whole-archive.
> Could you, please, send me build logs so I could look closer if it is
> properly built with ICC as I don't have access to it?

Note "--whole-archive" is related to the static library build, but the above
error is related to shared build, via "CONFIG_RTE_BUILD_SHARED_LIB=y" option.

Adding librte_timer as a dependent library should fix the issue, like:
 diff --git a/drivers/net/ena/Makefile b/drivers/net/ena/Makefile
 index 43339f3b9..ff9ce315b 100644
 --- a/drivers/net/ena/Makefile
 +++ b/drivers/net/ena/Makefile
 @@ -58,5 +58,6 @@ CFLAGS += $(INCLUDES)
  LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
  LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs
  LDLIBS += -lrte_bus_pci
 +LDLIBS += -lrte_timer

 include $(RTE_SDK)/mk/rte.lib.mk


> 
> Thanks,
> Michał
>