Message ID | cover.1559147228.git.anatoly.burakov@intel.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A5E731B949; Wed, 29 May 2019 18:31:15 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 789313576 for <dev@dpdk.org>; Wed, 29 May 2019 18:31:14 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 May 2019 09:31:13 -0700 X-ExtLoop1: 1 Received: from silpixa00399498.ir.intel.com (HELO silpixa00399498.ger.corp.intel.com) ([10.237.223.125]) by orsmga005.jf.intel.com with ESMTP; 29 May 2019 09:31:11 -0700 From: Anatoly Burakov <anatoly.burakov@intel.com> To: dev@dpdk.org Cc: stephen@networkplumber.org, thomas@monjalon.net, david.marchand@redhat.com Date: Wed, 29 May 2019 17:30:46 +0100 Message-Id: <cover.1559147228.git.anatoly.burakov@intel.com> X-Mailer: git-send-email 2.17.1 Subject: [dpdk-dev] [PATCH 00/25] Make shared memory config non-public X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Series |
Make shared memory config non-public
|
|
Message
Anatoly Burakov
May 29, 2019, 4:30 p.m. UTC
This patchset removes the shared memory config from public API, and replaces all usages of said config with new API calls. The patchset is mostly a search-and-replace job and should be pretty easy to review. However, the changes to ENA driver are of particular interest, because they're using the shared memory config in a way that i find confusing. I tried to implement the equivalent changes as well as i could, but since the code doesn't make any sense to me, i would really like to request help from ENA maintainers. Everything else should be pretty straightforward. Anatoly Burakov (25): eal: add API to lock/unlock memory hotplug bus/fslmc: use new memory locking API net/mlx4: use new memory locking API net/mlx5: use new memory locking API net/virtio: use new memory locking API mem: use new memory locking API malloc: use new memory locking API vfio: use new memory locking API eal: add EAL tailq list lock/unlock API acl: use new tailq locking API distributor: use new tailq locking API efd: use new tailq locking API eventdev: use new tailq locking API hash: use new tailq locking API lpm: use new tailq locking API member: use new tailq locking API mempool: use new tailq locking API reorder: use new tailq locking API ring: use new tailq locking API stack: use new tailq locking API eal: add new API to lock/unlock mempool list mempool: use new mempool list locking API eal: remove unused macros net/ena: fix direct access to shared memory config eal: hide shared memory config app/test/test_memzone.c | 1 + app/test/test_tailq.c | 1 + drivers/bus/fslmc/fslmc_vfio.c | 8 +- drivers/bus/pci/linux/pci_vfio.c | 1 + drivers/net/ena/ena_ethdev.c | 18 +-- drivers/net/mlx4/mlx4_mr.c | 11 +- drivers/net/mlx5/mlx5_mr.c | 11 +- .../net/virtio/virtio_user/virtio_user_dev.c | 7 +- lib/librte_acl/rte_acl.c | 20 +-- lib/librte_distributor/rte_distributor.c | 5 +- lib/librte_distributor/rte_distributor_v20.c | 5 +- lib/librte_eal/common/eal_common_memory.c | 128 +++++++++++++--- lib/librte_eal/common/eal_common_memzone.c | 1 + lib/librte_eal/common/eal_common_tailqs.c | 1 + lib/librte_eal/common/eal_memcfg.h | 75 +++++++++ lib/librte_eal/common/include/rte_eal.h | 10 -- .../common/include/rte_eal_memconfig.h | 143 ++++++++++-------- lib/librte_eal/common/malloc_heap.c | 16 +- lib/librte_eal/common/malloc_mp.c | 1 + lib/librte_eal/common/rte_malloc.c | 33 ++-- lib/librte_eal/freebsd/eal/eal_memory.c | 1 + lib/librte_eal/linux/eal/eal.c | 1 + lib/librte_eal/linux/eal/eal_memalloc.c | 1 + lib/librte_eal/linux/eal/eal_memory.c | 1 + lib/librte_eal/linux/eal/eal_vfio.c | 17 +-- lib/librte_eal/rte_eal_version.map | 18 +++ lib/librte_efd/rte_efd.c | 15 +- lib/librte_eventdev/rte_event_ring.c | 16 +- lib/librte_hash/rte_cuckoo_hash.c | 17 ++- lib/librte_hash/rte_fbk_hash.c | 15 +- lib/librte_kni/rte_kni.c | 16 +- lib/librte_lpm/rte_lpm.c | 25 +-- lib/librte_lpm/rte_lpm6.c | 15 +- lib/librte_member/rte_member.c | 17 ++- lib/librte_mempool/rte_mempool.c | 27 ++-- lib/librte_reorder/rte_reorder.c | 15 +- lib/librte_ring/rte_ring.c | 19 +-- lib/librte_stack/rte_stack.c | 18 +-- 38 files changed, 460 insertions(+), 290 deletions(-) create mode 100644 lib/librte_eal/common/eal_memcfg.h
Comments
On Wed, May 29, 2019 at 6:31 PM Anatoly Burakov <anatoly.burakov@intel.com> wrote: > This patchset removes the shared memory config from public > API, and replaces all usages of said config with new API > calls. > > The patchset is mostly a search-and-replace job and should > be pretty easy to review. However, the changes to ENA > I went and did the same job with some scripts. Not sure you really need to split in all those patches. We are not going to backport this. Some changes are mixed, the kni changes are in the hash: patch. I spotted a missed qlock in : lib/librte_eal/common/eal_common_tailqs.c: rte_rwlock_read_lock(&mcfg->qlock); lib/librte_eal/common/eal_common_tailqs.c: rte_rwlock_read_unlock(&mcfg->qlock); On the names of the functions, could we have something shorter ? The prefix rte_eal_mcfg_ is not necessary from my pov. driver are of particular interest, because they're using > the shared memory config in a way that i find confusing. > I thought the same when I looked at it before. Hopefully the ena maintainers will enlight us :-). I tried to implement the equivalent changes as well as > i could, but since the code doesn't make any sense to me, > i would really like to request help from ENA maintainers. > > Everything else should be pretty straightforward. > We are missing the deprecation notice removal at the end of the series and a note in the release notes. Thanks Anatoly!
On 29-May-19 9:11 PM, David Marchand wrote: > On Wed, May 29, 2019 at 6:31 PM Anatoly Burakov <anatoly.burakov@intel.com> > wrote: > >> This patchset removes the shared memory config from public >> API, and replaces all usages of said config with new API >> calls. >> >> The patchset is mostly a search-and-replace job and should >> be pretty easy to review. However, the changes to ENA >> > > I went and did the same job with some scripts. > > Not sure you really need to split in all those patches. > We are not going to backport this. The "separate commits" thing is made for the benefit of reviewers, not backporters. In my experience it's much easier to get a maintainer to review a smaller patch than it is to look through a wall of irrelevant changes. That said, for trivial changes such as these, maybe this is indeed unnecessary. > Some changes are mixed, the kni changes are in the hash: patch. Oops, will fix, thanks for pointing it out! > > > I spotted a missed qlock in : > lib/librte_eal/common/eal_common_tailqs.c: > rte_rwlock_read_lock(&mcfg->qlock); > lib/librte_eal/common/eal_common_tailqs.c: > rte_rwlock_read_unlock(&mcfg->qlock); > > > On the names of the functions, could we have something shorter ? > The prefix rte_eal_mcfg_ is not necessary from my pov. I can drop the mcfg, but IMO all of these locking functions should be kept under one namespace, and rte_eal_ is too broad. > > > driver are of particular interest, because they're using >> the shared memory config in a way that i find confusing. >> > > I thought the same when I looked at it before. > Hopefully the ena maintainers will enlight us :-). > > > I tried to implement the equivalent changes as well as >> i could, but since the code doesn't make any sense to me, >> i would really like to request help from ENA maintainers. >> >> Everything else should be pretty straightforward. >> > > We are missing the deprecation notice removal at the end of the series and > a note in the release notes. Will add. Making into V1 deadline was higher priority :D > > Thanks Anatoly! > >
On Thu, May 30, 2019 at 09:07:44AM +0100, Burakov, Anatoly wrote: > On 29-May-19 9:11 PM, David Marchand wrote: > > On Wed, May 29, 2019 at 6:31 PM Anatoly Burakov <anatoly.burakov@intel.com> > > wrote: > > > > > This patchset removes the shared memory config from public > > > API, and replaces all usages of said config with new API > > > calls. > > > > > > The patchset is mostly a search-and-replace job and should > > > be pretty easy to review. However, the changes to ENA > > > > > > > I went and did the same job with some scripts. > > > > Not sure you really need to split in all those patches. > > We are not going to backport this. > > The "separate commits" thing is made for the benefit of reviewers, not > backporters. In my experience it's much easier to get a maintainer to review > a smaller patch than it is to look through a wall of irrelevant changes. > > That said, for trivial changes such as these, maybe this is indeed > unnecessary. > > > Some changes are mixed, the kni changes are in the hash: patch. > > Oops, will fix, thanks for pointing it out! > > > > > > > I spotted a missed qlock in : > > lib/librte_eal/common/eal_common_tailqs.c: > > rte_rwlock_read_lock(&mcfg->qlock); > > lib/librte_eal/common/eal_common_tailqs.c: > > rte_rwlock_read_unlock(&mcfg->qlock); > > > > > > On the names of the functions, could we have something shorter ? > > The prefix rte_eal_mcfg_ is not necessary from my pov. > > I can drop the mcfg, but IMO all of these locking functions should be kept > under one namespace, and rte_eal_ is too broad. > I think most/all developers are aware that memory is part of eal, so rte_mcfg_ prefix (or rte_memcfg) might work.
30/05/2019 12:15, Bruce Richardson: > On Thu, May 30, 2019 at 09:07:44AM +0100, Burakov, Anatoly wrote: > > On 29-May-19 9:11 PM, David Marchand wrote: > > > On the names of the functions, could we have something shorter ? > > > The prefix rte_eal_mcfg_ is not necessary from my pov. > > > > I can drop the mcfg, but IMO all of these locking functions should be kept > > under one namespace, and rte_eal_ is too broad. > > I think most/all developers are aware that memory is part of eal, so > rte_mcfg_ prefix (or rte_memcfg) might work. Why not being explicit with "rte_mem_config_", same as the structure?