Re: [dpdk-dev] 17.11.4 patches review and test

[Yongseok Koh <yskoh@mellanox.com>]

On Aug 24, 2018, at 8:00 AM, Alejandro Lucero <alejandro.lucero@netronome.com<mailto:alejandro.lucero@netronome.com>> wrote:



On Fri, Aug 24, 2018 at 4:31 PM, Yongseok Koh <yskoh@mellanox.com<mailto:yskoh@mellanox.com>> wrote:

> On Aug 24, 2018, at 1:51 AM, Alejandro Lucero <alejandro.lucero@netronome.com<mailto:alejandro.lucero@netronome.com>> wrote:
>
>
>
> On Thu, Aug 23, 2018 at 6:18 PM, Yongseok Koh <yskoh@mellanox.com<mailto:yskoh@mellanox.com>> wrote:
>
> > On Aug 22, 2018, at 5:19 PM, Yongseok Koh <yskoh@mellanox.com<mailto:yskoh@mellanox.com>> wrote:
> >
> > On Tue, Aug 21, 2018 at 12:07:49PM +0200, Alejandro Lucero wrote:
> >> Hi Yonngseok,
> >>
> >> There is a patchset aimed at 17.11.x:
> >>
> >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatches.dpdk.org%2Fcover%2F42741%2F&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Cb1b0e3eff71c499ff3fb08d6088e1ede%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636705803846548967&amp;sdata=8f12c1IuUe4mw2EaTZ18vVTuLTjXOD2cSe%2B%2B7f6OFfk%3D&amp;reserved=0<https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatches.dpdk.org%2Fcover%2F42741%2F&data=02%7C01%7Cyskoh%40mellanox.com%7Ce378358ad14e4cd9fa8008d609d24f74%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636707196241797155&sdata=wMVAaMMvZJ5UYUPkcwAhvko%2F8MorOTtz6S8VCqeVgZw%3D&reserved=0>
> >>
> >> It was not accepted for master because the memory code has changed a lot
> >> since 17.11, and I'm working on another patchset for adjusting to the those
> >> changes.
> >>
> >> I wonder if there is any issue with adding this patchset to stable 17.11.4.
> >> Note that this makes unlikely a known limitation with emulated IOMMU inside
> >> VMs.
> >
> > This patchset seems quite large for stable release and need to be well verified
> > before GA. In -rc1 stage, we don't usually take such a large patchset as people
> > have already started verification. And we don't usually release -rc2. If you're
> > trying to solve a very critical issue with this patchset, I have to release -rc2
> > and ask people to verify again. How critical is your issue?
> >
> > For the patchset,
> > - "mem: add function for checking memsegs IOVAs addresses"
> >  This is adding a new API, so I don't expect any API/ABI breakage, but want to
> >  double-confirm with Thomas. Thomas?
> >
> > - "bus/pci: use IOVAs check when setting IOVA mode"
> >  All the patches got ack except for this one but from looking at the threads in
> >  dev mailing list, it looks okay. I have a question though.
> >
> >> @@ -640,13 +643,17 @@
> >> {
> >>        struct rte_pci_device *dev = NULL;
> >>        struct rte_pci_driver *drv = NULL;
> >> +       int iommu_dma_mask_check_done = 0;
> >>
> >>        FOREACH_DRIVER_ON_PCIBUS(drv) {
> >>                FOREACH_DEVICE_ON_PCIBUS(dev) {
> >>                        if (!rte_pci_match(drv, dev))
> >>                                continue;
> >> -                       if (!pci_one_device_iommu_support_va(dev))
> >> -                               return false;
> >> +                       if (!iommu_dma_mask_check_done) {
> >> +                               if (pci_one_device_iommu_support_va(dev) < 0)
> >
> > pci_one_device_iommu_support_va() returns true/false(1/0), then why do you
> > expect to see a negative return value in case of failure?
>
>
> Emulated IOMMU has a 39 bits addressing limitation in some QEMU versions. With pci_one_device_iommu_support_va this is checked out, and if it does exist, IOMMU with VA is not supported.
>
> This patch avoids such coarse check using dma mask code added for allowing IOMMU with VA if allocated memory is below the addressing limitation. This is going to help for using IOMMU with VA is most of the systems out there, and even with systems with more than 512GB as long as the DPDK allocated memory is below that limit.

I was asking about this change:

from,
> >> -                       if (!pci_one_device_iommu_support_va(dev))

to,
> >> +                               if (pci_one_device_iommu_support_va(dev) < 0)


The original code checks zero but you changed it to check negative value.
But it looks pci_one_device_iommu_support_va() doesn't return negative value, right?

I thought this is buggy, please let me know.


Yes, you are right. I remember I initially changed pci_one_device_iommu_support_va for returning an int instead of boolean, but I did leave it as boolean at the end. It seems I forgot to modify the call. I will send another version.

Is it OK if I send it just to stable@dpdk.org<mailto:stable@dpdk.org> tagging fix for 17.11?

Yes, it is.

Thanks
Yongseok


Thanks

Thanks,
Yongseok

>
> Alejandro,
>
> As I will release -rc2, I can integrate your patchset but this should be
> addressed. Please let me know.
>
> Thanks,
> Yongseok
>
> >> +                                       return false;
> >> +                               iommu_dma_mask_check_done  = 1;
> >> +                       }
> >>                }
> >>        }
> >>        return true;
> >>
> >>
> >>
> >> Thanks
> >>
> >> On Thu, Aug 16, 2018 at 8:18 PM, Yongseok Koh <yskoh@mellanox.com<mailto:yskoh@mellanox.com>> wrote:
> >>
> >>> Hi all,
> >>>
> >>> Here is a list of patches targeted for LTS release 17.11.4. Please help
> >>> review
> >>> and test. The planned date for the final release is August 23. Before that,
> >>> please shout if anyone has objections with these patches being applied.
> >>>
> >>> Also for the companies committed to running regression tests, please run
> >>> the
> >>> tests and report any issue before the release date.
> >>>
> >>> A release candidate tarball can be found at:
> >>>
> >>>    https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdpdk.org%2Fbrowse%2Fdpdk-stable%2Ftag%2F%3Fid%3Dv17.11.4-rc1&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Cb1b0e3eff71c499ff3fb08d6088e1ede%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636705803846548967&amp;sdata=AMgyJMFIs512o5zfZ4aNSy1Ptp%2BhEIMUCVZ6HaL2F40%3D&amp;reserved=0<https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdpdk.org%2Fbrowse%2Fdpdk-stable%2Ftag%2F%3Fid%3Dv17.11.4-rc1&data=02%7C01%7Cyskoh%40mellanox.com%7Ce378358ad14e4cd9fa8008d609d24f74%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636707196241797155&sdata=HpDJXL3fMREfoXAFc3B9Mdorrdn5juINJNLXp1wdHmM%3D&reserved=0>
> >>>
> >>> These patches are located at branch 17.11 of dpdk-stable repo:
> >>>    https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdpdk.org%2Fbrowse%2Fdpdk-stable%2F&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Cb1b0e3eff71c499ff3fb08d6088e1ede%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636705803846548967&amp;sdata=hvOhEk502vVzboCbRbCZXqJXcsiI3DTtgQypQJi0Aro%3D&amp;reserved=0<https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdpdk.org%2Fbrowse%2Fdpdk-stable%2F&data=02%7C01%7Cyskoh%40mellanox.com%7Ce378358ad14e4cd9fa8008d609d24f74%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636707196241797155&sdata=AAzBustIYRpjP67zVmFECFuAL3bcOrLEjoN1npVdY3A%3D&reserved=0>
> >>>
> >>> Thanks,
> >>> Yongseok
> >>>
> >>> ---
> >>> Adrien Mazarguil (2):
> >>>      maintainers: update for Mellanox PMDs
> >>>      net/mlx4: fix minor resource leak during init
> >>>
> >>> Ajit Khaparde (7):
> >>>      net/bnxt: fix HW Tx checksum offload check
> >>>      net/bnxt: fix set MTU
> >>>      net/bnxt: fix Rx ring count limitation
> >>>      net/bnxt: fix memory leaks in NVM commands
> >>>      net/bnxt: fix lock release on NVM write failure
> >>>      net/bnxt: check access denied for HWRM commands
> >>>      net/bnxt: fix RETA size
> >>>
> >>> Alejandro Lucero (1):
> >>>      net/nfp: fix field initialization in Tx descriptor
> >>>
> >>> Alok Makhariya (1):
> >>>      bus/dpaa: fix phandle support for Linux 4.16
> >>>
> >>> Anatoly Burakov (8):
> >>>      eal/linux: fix invalid syntax in interrupts
> >>>      eal/linux: fix uninitialized value
> >>>      test: fix EAL flags autotest on FreeBSD
> >>>      test: fix result printing
> >>>      test: fix code on report
> >>>      test: make autotest runner python 2/3 compliant
> >>>      test: print autotest categories
> >>>      test: improve filtering
> >>>
> >>> Andrew Rybchenko (2):
> >>>      net/sfc: cut non VLAN ID bits from TCI
> >>>      net/sfc: fix assert in set multicast address list
> >>>
> >>> Andy Green (1):
> >>>      ring: fix sign conversion warning
> >>>
> >>> Beilei Xing (3):
> >>>      net/i40e: fix shifts of 32-bit value
> >>>      net/i40e: fix packet type parsing with DDP
> >>>      net/i40e: fix setting TPID with AQ command
> >>>
> >>> Bruce Richardson (2):
> >>>      examples/exception_path: fix out-of-bounds read
> >>>      mk: fix permissions when using make install
> >>>
> >>> Chas Williams (2):
> >>>      net/bonding: always update bonding link status
> >>>      net/bonding: do not clear active slave count
> >>>
> >>> Dan Gora (1):
> >>>      kni: fix crash with null name
> >>>
> >>> Daria Kolistratova (1):
> >>>      net/ena: fix SIGFPE with 0 Rx queue
> >>>
> >>> Dariusz Stojaczyk (1):
> >>>      eal: fix return codes on thread naming failure
> >>>
> >>> David Marchand (1):
> >>>      net/bnxt: add missing ids in xstats
> >>>
> >>> Drocula Lambda (1):
> >>>      kni: fix build on RHEL 7.5
> >>>
> >>> Ferruh Yigit (2):
> >>>      kni: fix build with gcc 8.1
> >>>      net/thunderx: fix build with gcc optimization on
> >>>
> >>> Gavin Hu (3):
> >>>      mk: fix cross build
> >>>      net/dpaa2: remove loop for unused pool entries
> >>>      maintainers: claim maintainership for ARM v7 and v8
> >>>
> >>> Haiyue Wang (1):
> >>>      net/i40e: workaround performance degradation
> >>>
> >>> Harry van Haaren (1):
> >>>      event: fix ring init failure handling
> >>>
> >>> Hemant Agrawal (2):
> >>>      test/crypto: fix device id when stopping port
> >>>      bus/dpaa: fix buffer offset setting in FMAN
> >>>
> >>> Hyong Youb Kim (1):
> >>>      net/enic: do not overwrite admin Tx queue limit
> >>>
> >>> Ido Goshen (1):
> >>>      net/pcap: fix multiple queues
> >>>
> >>> Jananee Parthasarathy (1):
> >>>      mk: update targets for classified tests
> >>>
> >>> Jay Ding (1):
> >>>      net/bnxt: check for invalid vNIC id
> >>>
> >>> Jerin Jacob (2):
> >>>      ethdev: fix queue statistics mapping documentation
> >>>      eal: fix bitmap documentation
> >>>
> >>> Kiran Kumar (2):
> >>>      net/bonding: fix MAC address reset
> >>>      net/thunderx: avoid sq door bell write on zero packet
> >>>
> >>> Konstantin Ananyev (3):
> >>>      examples/ipsec-secgw: fix IPv4 checksum at Tx
> >>>      examples/ipsec-secgw: fix bypass rule processing
> >>>      app/testpmd: fix DCB config
> >>>
> >>> Maxime Coquelin (4):
> >>>      vhost: improve dirty pages logging performance
> >>>      vhost: fix missing increment of log cache count
> >>>      vhost: flush IOTLB cache on new mem table handling
> >>>      vhost: retranslate vring addr when memory table changes
> >>>
> >>> Moti Haimovsky (2):
> >>>      net/mlx5: fix build with old kernels
> >>>      net/mlx4: check RSS queues number limitation
> >>>
> >>> Nelio Laranjeiro (1):
> >>>      net/mlx5: fix TCI mask filter
> >>>
> >>> Nikhil Rao (5):
> >>>      eventdev: fix port in Rx adapter internal function
> >>>      eventdev: fix missing update to Rx adaper WRR position
> >>>      eventdev: add event buffer flush in Rx adapter
> >>>      eventdev: fix internal port logic in Rx adapter
> >>>      eventdev: fix Rx SW adapter stop
> >>>
> >>> Nithin Dabilpuram (1):
> >>>      app/testpmd: fix buffer leak in TM command
> >>>
> >>> Ophir Munk (1):
> >>>      net/mlx5: fix secondary process resource leakage
> >>>
> >>> Pablo de Lara (7):
> >>>      examples/l2fwd-crypto: fix digest with AEAD algo
> >>>      examples/l2fwd-crypto: check return value on IV size check
> >>>      examples/l2fwd-crypto: skip device not supporting operation
> >>>      test/hash: fix multiwriter with non consecutive cores
> >>>      test/hash: fix potential memory leak
> >>>      app/crypto-perf: fix auth IV offset
> >>>      hash: fix doxygen of return values
> >>>
> >>> Pavan Nikhilesh (2):
> >>>      event/octeontx: remove unnecessary port start and stop
> >>>      net/octeontx: fix stop clearing Rx/Tx functions
> >>>
> >>> Qi Zhang (1):
> >>>      vfio: fix PCI address comparison
> >>>
> >>> Radu Nicolau (3):
> >>>      security: fix crash on destroy null session
> >>>      test: fix uninitialized port configuration
> >>>      net/bonding: fix race condition
> >>>
> >>> Rafal Kozik (4):
> >>>      net/ena: fix GENMASK_ULL macro
> >>>      net/ena: set link speed as none
> >>>      net/ena: check pointer before memset
> >>>      net/ena: change memory type
> >>>
> >>> Rahul Lakkireddy (1):
> >>>      net/cxgbe: fix init failure due to new flash parts
> >>>
> >>> Rami Rosen (2):
> >>>      examples/l3fwd: remove useless include
> >>>      ethdev: fix a doxygen comment for port allocation
> >>>
> >>> Rasesh Mody (9):
> >>>      net/qede: fix VF MTU update
> >>>      net/qede: remove primary MAC removal
> >>>      net/qede: fix for devargs
> >>>      net/qede: fix default extended VLAN offload config
> >>>      doc: update qede management firmware guide
> >>>      net/qede/base: fix GRC attention callback
> >>>      net/bnx2x: fix FW command timeout during stop
> >>>      net/bnx2x: fix poll link status
> >>>      net/qede/base: fix to clear HW indication
> >>>
> >>> Remy Horton (4):
> >>>      bitrate: add sanity check on parameters
> >>>      metrics: add check for invalid key
> >>>      metrics: do not fail silently when uninitialised
> >>>      metrics: disallow null as metric name
> >>>
> >>> Reshma Pattan (2):
> >>>      test/flow_classify: fix return types
> >>>      mk: remove unnecessary test rules
> >>>
> >>> Rosen Xu (1):
> >>>      examples/flow_filtering: add flow director config for i40e
> >>>
> >>> Shahaf Shuler (1):
> >>>      net/mlx5: fix compilation for rdma-core v19
> >>>
> >>> Shahed Shaikh (7):
> >>>      net/qede: fix link change event notification
> >>>      net/qede: fix legacy interrupt mode
> >>>      net/qede: fix incorrect link status update
> >>>      net/qede: fix unicast MAC address handling in VF
> >>>      net/qede: fix interrupt handler unregister
> >>>      net/qede: fix MAC address removal failure message
> >>>      net/qede: fix ntuple filter configuration
> >>>
> >>> Shreyansh Jain (1):
> >>>      doc: fix bonding command in testpmd
> >>>
> >>> Somnath Kotur (3):
> >>>      net/bnxt: fix to move a flow to a different queue
> >>>      net/bnxt: use correct flags during VLAN configuration
> >>>      net/bnxt: fix filter freeing
> >>>
> >>> Thomas Monjalon (1):
> >>>      bus/dpaa: fix build
> >>>
> >>> Tomasz Duszynski (1):
> >>>      net/mvpp2: check pointer before using it
> >>>
> >>> Wei Zhao (7):
> >>>      net/ixgbe: add support for VLAN in IP mode FDIR
> >>>      net/ixgbe: fix tunnel id format error for FDIR
> >>>      net/ixgbe: fix tunnel type set error for FDIR
> >>>      net/ixgbe: fix mask bits register set error for FDIR
> >>>      app/testpmd: fix VLAN TCI mask set error for FDIR
> >>>      net/i40e: fix check of flow director programming status
> >>>      net/i40e: revert fix of flow director check
> >>>
> >>> Xiaoyun Li (1):
> >>>      net/i40e: fix link speed
> >>>
> >>> Xueming Li (1):
> >>>      net/mlx5: fix crash in device probe
> >>>
> >>> Yipeng Wang (3):
> >>>      hash: fix multiwriter lock memory allocation
> >>>      hash: fix a multi-writer race condition
> >>>      hash: fix key slot size accuracy
> >>>
> >>> Yongseok Koh (7):
> >>>      net/mlx5: fix Rx buffer replenishment threshold
> >>>      net/mlx5: add missing sanity checks for Tx completion queue
> >>>      net/mlx5: fix assert for Tx completion queue count
> >>>      net/mlx5: fix queue rollback when starting device
> >>>      net/mlx5: fix error number handling
> >>>      net/mlx5: preserve promiscuous flag for flow isolation mode
> >>>      net/mlx5: preserve allmulticast flag for flow isolation mode
> >>>
>
>