From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 21622A00E6 for ; Fri, 12 Jul 2019 13:10:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C25131B9A6; Fri, 12 Jul 2019 13:10:08 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 37D9514E8 for ; Fri, 12 Jul 2019 13:10:07 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Jul 2019 04:10:05 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,482,1557212400"; d="scan'208";a="189806812" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.10]) ([10.237.221.10]) by fmsmga004.fm.intel.com with ESMTP; 12 Jul 2019 04:10:04 -0700 To: Vamsi Krishna Attunuru , "dev@dpdk.org" Cc: "olivier.matz@6wind.com" , "arybchenko@solarflare.com" , Kiran Kumar Kokkilagadda References: <20190422061533.17538-1-kirankumark@marvell.com> <20190625035700.2953-1-vattunuru@marvell.com> <20190625035700.2953-5-vattunuru@marvell.com> <98bf2103-f48f-4baa-0d4a-f03f9e538519@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQkI71rKFiEE 0jZTh0IuwoTjmYHH+TPrQ98TYR8FAlznMMQFCwkIBwMFFQoJCAsFFgIDAQAACgkQ+TPrQ98T YR/B9Q//a57esjq996nfZVm7AsUl7zbvhN+Ojity25ib2gcSVVsAN2j6lcQS4hf6/OVvRj3q CgebJ4o2gXR6X12UzWBJL7NE8Xpc70MvUIe0r11ykurQ9n9jUaWMjxdSqBPF93hU+Z/MZe5M 1rW5O2VJLuTJzkDw3EYUCbHOwPjeaS8Qqj3RI0LYbGthbHBIp9CsjkgsJSjTT5GQ8AQWkE7I z+hvPx6f1rllfjxFyi4DI3jLhAI+j1Nm+l+ESyoX59HrLTHAvq4RPkLpTnGBj9gOnJ+5sVEr GE0fcffsNcuMSkpqSEoJCPAHmChoLgezskhhsy0BiU3xlSIj1Dx2XMDerUXFOK3ftlbYNRte HQy4EKubfZRB8H5Rvcpksom3fRBDcJT8zw+PTH14htRApU9f8I/RamQ7Ujks7KuaB7JX5QaG gMjfPzHGYX9PfF6KIchaFmAWLytIP1t0ht8LpJkjtvUCSQZ2VxpCXwKyUzPDIF3co3tp90o7 X07uiC5ymX0K0+Owqs6zeslLY6DMxNdt8ye+h1TVkSZ5g4dCs4C/aiEF230+luL1CnejOv/K /s1iSbXQzJNM7be3FlRUz4FdwsfKiJJF7xYALSBnSvEB04R7I2P2V9Zpudkq6DRT6HZjBeJ1 pBF2J655cdoenPBIeimjnnh4K7YZBzwOLJf2c6u76fe5Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXOcvZgUJBvIWKAAKCRD5M+tD3xNhHxhBD/9toXMIaPIVFd9w1nKsRDM1GE6gZe4jie8q MJpeHB9O+936fSXA0W2X0het60wJQQ45O8TpTcxpc9nGzcE4MTaLAI3E8TjIXAO0cPqUNLyp g0DXezmTw5BU+SKZ51+jSKOtFmzJCHOJZQaMeCHD+G3CrdUHQVQBb5AeuH3KFv9ltgDcWsc8 YO70o3+tGHwcEnyXLdrI0q05wV7ncnLdkgVo+VUN4092bNMPwYly1TZWcU3Jw5gczOUEfTY7 sgo6E/sGX3B+FzgIs5t4yi1XOweCAQ/mPnb6uFeNENEFyGKyMG1HtjwBqnftbiFO3qitEIUY xWGQH23oKscv7i9lT0gg2D+ktzZhVWwHJVY/2vWSB9aCSWChcH2BT+lWrkwSpoPhy+almM84 Qz2wF72/d4ce4L27pSrS+vOXtXHLGOOGcAn8yr9TV0kM4aR+NbGBRXGKhG6w4lY54uNd9IBa ARIPUhij5JSygxZCBaJKo+X64AHGkk5bXq+f0anwAMNuJXbYC/lz4DEdKmPgQGShOWNs1Y1a N3cI87Hun/RBVwQ0a3Tr1g6OWJ6xK8cYbMcoR8NZ7L9ALMeJeuUDQR39+fEeHg/6sQN0P0mv 0sL+//BAJphCzDk8ztbrFw+JaPtgzZpRSM6JhxnY+YMAsatJRXA0WSpYP5zzl7yu/GZJIgsv VQ== Message-ID: <285145b7-a829-b614-7971-2df171800466@intel.com> Date: Fri, 12 Jul 2019 12:10:03 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v6 4/4] kernel/linux/kni: add IOVA support in kni module X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 7/12/2019 11:38 AM, Vamsi Krishna Attunuru wrote: > > > > -------------------------------------------------------------------------------- > *From:* Ferruh Yigit > *Sent:* Thursday, July 11, 2019 10:00 PM > *To:* Vamsi Krishna Attunuru; dev@dpdk.org > *Cc:* olivier.matz@6wind.com; arybchenko@solarflare.com; Kiran Kumar Kokkilagadda > *Subject:* [EXT] Re: [PATCH v6 4/4] kernel/linux/kni: add IOVA support in kni > module >   > External Email > > ---------------------------------------------------------------------- > On 6/25/2019 4:57 AM, vattunuru@marvell.com wrote: >> From: Kiran Kumar K >> >> Patch adds support for kernel module to work in IOVA = VA mode, >> the idea is to get physical address from iova address using >> iommu_iova_to_phys API and later use phys_to_virt API to >> convert the physical address to kernel virtual address. >> >> When compared with IOVA = PA mode, there is no performance >> drop with this approach. >> >> This approach does not work with the kernel versions less >> than 4.4.0 because of API compatibility issues. >> >> Signed-off-by: Kiran Kumar K >> Signed-off-by: Vamsi Attunuru > > <...> > >> @@ -351,15 +354,56 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num, >>        strncpy(kni->name, dev_info.name, RTE_KNI_NAMESIZE); >>  >>        /* Translate user space info into kernel space info */ >> -     kni->tx_q = phys_to_virt(dev_info.tx_phys); >> -     kni->rx_q = phys_to_virt(dev_info.rx_phys); >> -     kni->alloc_q = phys_to_virt(dev_info.alloc_phys); >> -     kni->free_q = phys_to_virt(dev_info.free_phys); >> - >> -     kni->req_q = phys_to_virt(dev_info.req_phys); >> -     kni->resp_q = phys_to_virt(dev_info.resp_phys); >> -     kni->sync_va = dev_info.sync_va; >> -     kni->sync_kva = phys_to_virt(dev_info.sync_phys); >> +     if (dev_info.iova_mode) { >> +#if KERNEL_VERSION(4, 4, 0) > LINUX_VERSION_CODE > > We have "kni/compat.h" to put the version checks, please use abstracted feature > checks only in the code. > From experience this goes ugly quickly with the addition to distro kernels and > their specific versioning, so better to hide these all from the source code. > > And this version requirement needs to be documented in kni doc. > > ack > >> +             (void)pci; >> +             pr_err("Kernel version is not supported\n"); > > Can you please include 'iova_mode' condition into the message log, because this > kernel version is supported if user wants to use via 'iova_mode == 0' condition. > > ack > >> +             return -EINVAL; >> +#else >> +             pci = pci_get_device(dev_info.vendor_id, >> +                                  dev_info.device_id, NULL); >> +             while (pci) { >> +                     if ((pci->bus->number == dev_info.bus) && >> +                         (PCI_SLOT(pci->devfn) == dev_info.devid) && >> +                         (PCI_FUNC(pci->devfn) == dev_info.function)) { >> +                             domain = iommu_get_domain_for_dev(&pci->dev); >> +                             break; >> +                     } >> +                     pci = pci_get_device(dev_info.vendor_id, >> +                                          dev_info.device_id, pci); >> +             } > > What if 'pci' is NULL here? > In kni it is not required to provide a device at all. > > Ack, will add a NULL check. > other point is not clear to me, device info is absolutely required at least > for  IOVA=VA mode, since it requires to procure iommu domain details. "device info is absolutely required" *only* for IOVA=VA mode, so user may skip to provide it. > Any thoughts or ways to address this without device.? Return error if 'iova_mode' requested but device info not? But you didn't replied to passing 'iova_mode' from application, I would like hear what you are thinking about it.. > > <...> > >> @@ -186,7 +202,10 @@ kni_fifo_trans_pa2va(struct kni_dev *kni, >>                        return; >>  >>                for (i = 0; i < num_rx; i++) { >> -                     kva = pa2kva(kni->pa[i]); >> +                     if (likely(kni->iova_mode == 1)) >> +                             kva = iova2kva(kni, kni->pa[i]); >> +                     else >> +                             kva = pa2kva(kni->pa[i]); > > To reduce the churn, what about updating the 'pa2kva()' and put the > "(kni->iova_mode == 1)" check there? Does it help? (not only 'pa2kva()' but its > friends also, and if it makes more sense agree to rename the functions) > > No, in VA mode, kni->pa[i] points to iova address, pa2kva() of iova address might > crash, hence the if..else check is added. I understand that part. What I am suggestion is something like this: kva = common_function(kni, kni->pa[i]); --- common_function() { if (unlikely(kni->iova_mode == 1)) return iova2kva(kni, kni->pa[i]); return pa2kva(kni->pa[i]); } To hide the check in the function and make code more readable > > And btw, why 'likely' case is "kni->iova_mode == 1"? > > no specific case other than branch predict, will remove this if it's really > harmful to PA mode.