Message ID | 1708762927-14126-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 908AF43BBD; Sat, 24 Feb 2024 09:23:59 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1032F402D9; Sat, 24 Feb 2024 09:23:54 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 0AFA5402A8 for <dev@dpdk.org>; Sat, 24 Feb 2024 09:22:12 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 2CFFA20B74C0; Sat, 24 Feb 2024 00:22:11 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 2CFFA20B74C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1708762931; bh=OoK4rapXckkzpG0y5/LyvJYky2hhsGycXpdU7ZA25t4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=hOVSdp9HPoAO/v4mg18MMPovc3VEoqrf+C3p6OOmMB1bywBWxutOC/rg/4pjZEaVJ KYzh4a3wC9BmCsZCrTi6afxD0fJWr4U59c9qH75iAmEafKizC6AYgMDevL/JRkdl8Z UbFCrneulUknpcHAHmvCmiAhtU3MFT7e0ayfio4o= From: Tyler Retzlaff <roretzla@linux.microsoft.com> To: dev@dpdk.org Cc: Ajit Khaparde <ajit.khaparde@broadcom.com>, Andrew Boyer <andrew.boyer@amd.com>, Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>, Bruce Richardson <bruce.richardson@intel.com>, Chenbo Xia <chenbox@nvidia.com>, Chengwen Feng <fengchengwen@huawei.com>, Dariusz Sosnowski <dsosnowski@nvidia.com>, David Christensen <drc@linux.vnet.ibm.com>, Hyong Youb Kim <hyonkim@cisco.com>, Jerin Jacob <jerinj@marvell.com>, Jie Hai <haijie1@huawei.com>, Jingjing Wu <jingjing.wu@intel.com>, John Daley <johndale@cisco.com>, Kevin Laatz <kevin.laatz@intel.com>, Kiran Kumar K <kirankumark@marvell.com>, Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>, Maciej Czekaj <mczekaj@marvell.com>, Matan Azrad <matan@nvidia.com>, Maxime Coquelin <maxime.coquelin@redhat.com>, Nithin Dabilpuram <ndabilpuram@marvell.com>, Ori Kam <orika@nvidia.com>, Ruifeng Wang <ruifeng.wang@arm.com>, Satha Rao <skoteshwar@marvell.com>, Somnath Kotur <somnath.kotur@broadcom.com>, Suanming Mou <suanmingm@nvidia.com>, Sunil Kumar Kori <skori@marvell.com>, Viacheslav Ovsiienko <viacheslavo@nvidia.com>, Yisen Zhuang <yisen.zhuang@huawei.com>, Yuying Zhang <Yuying.Zhang@intel.com>, mb@smartsharesystems.com, Tyler Retzlaff <roretzla@linux.microsoft.com> Subject: [PATCH v5 00/22] stop using RTE_MARKER extensions Date: Sat, 24 Feb 2024 00:21:45 -0800 Message-Id: <1708762927-14126-1-git-send-email-roretzla@linux.microsoft.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <1706657173-26166-1-git-send-email-roretzla@linux.microsoft.com> References: <1706657173-26166-1-git-send-email-roretzla@linux.microsoft.com> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 |
Series |
stop using RTE_MARKER extensions
|
|
Message
Tyler Retzlaff
Feb. 24, 2024, 8:21 a.m. UTC
RTE_MARKER typedefs are a GCC extension unsupported by MSVC. This series hides the markers when building with MSVC and updates libraries and drivers to access compatibly typed pointers to the rte_mbuf struct offsets. This series, does the following. Introduces a new macro __rte_marker(type, name) which is used to conditionally expand RTE_MARKER fields empty when building with GCC. Updates existing inline functions accessing cacheline{0,1} markers in the rte_mbuf struct to stop using the markers and instead uses the mbuf fields directly. Introduces 2 new inline functions to allow drivers to access rearm_data and rx_descriptor_fields1 descriptors without using the RTE_MARKER fields. Updates all drivers to use the new inline rte_mbuf struct accessors for rearm_data and rx_descriptor_fields1. Any previous Acks on the series are considered to be reset due to amount of change. v5: * update existing cacheline{0, 1} inline functions to access actual mbuf fields. * introduce new inline functions for accessing rearm_data and rx_descriptor_fields1 descriptors. * adapt drivers to use new inline functions. prior versions not relevant due to re-work of entire series. Tyler Retzlaff (22): eal: provide macro to expand marker extensions mbuf: expand rte markers empty when building with MSVC security: expand rte markers empty when building with MSVC cryptodev: expand rte markers empty when building with MSVC mbuf: stop using mbuf cacheline marker fields mbuf: add mbuf descriptor accessors common/idpf: use mbuf descriptor accessors net/bnxt: use mbuf descriptor accessors net/cnxk: use mbuf descriptor accessors net/enic: use mbuf descriptor accessors net/fm10k: use mbuf descriptor accessors net/hns3: use mbuf descriptor accessors net/i40e: use mbuf descriptor accessors net/iavf: use mbuf descriptor accessors net/ice: use mbuf descriptor accessors net/ionic: use mbuf descriptor accessors net/ixgbe: use mbuf descriptor accessors net/mlx5: use mbuf descriptor accessors net/octeon_ep: use mbuf descriptor accessors net/sfc: use mbuf descriptor accessors net/thunderx: use mbuf descriptor accessors net/virtio: use mbuf descriptor accessors drivers/common/idpf/idpf_common_rxtx.c | 4 +-- drivers/common/idpf/idpf_common_rxtx_avx512.c | 33 ++++++++++++----------- drivers/net/bnxt/bnxt_rxtx_vec_avx2.c | 32 +++++++++++----------- drivers/net/bnxt/bnxt_rxtx_vec_common.h | 4 +-- drivers/net/bnxt/bnxt_rxtx_vec_neon.c | 16 +++++------ drivers/net/bnxt/bnxt_rxtx_vec_sse.c | 16 +++++------ drivers/net/cnxk/cn10k_rx.h | 36 ++++++++++++------------- drivers/net/cnxk/cn9k_rx.h | 20 +++++++------- drivers/net/cnxk/cnxk_ethdev.c | 2 +- drivers/net/enic/enic_main.c | 4 +-- drivers/net/enic/enic_rxtx_vec_avx2.c | 18 ++++++------- drivers/net/fm10k/fm10k_rxtx_vec.c | 19 +++++-------- drivers/net/hns3/hns3_rxtx_vec.c | 4 +-- drivers/net/hns3/hns3_rxtx_vec_neon.h | 16 +++++------ drivers/net/i40e/i40e_rxtx_vec_altivec.c | 18 +++++-------- drivers/net/i40e/i40e_rxtx_vec_avx2.c | 16 +++++------ drivers/net/i40e/i40e_rxtx_vec_avx512.c | 16 +++++------ drivers/net/i40e/i40e_rxtx_vec_common.h | 4 +-- drivers/net/i40e/i40e_rxtx_vec_neon.c | 16 +++++------ drivers/net/i40e/i40e_rxtx_vec_sse.c | 16 +++++------ drivers/net/iavf/iavf_rxtx_vec_avx2.c | 32 +++++++++++----------- drivers/net/iavf/iavf_rxtx_vec_avx512.c | 32 +++++++++++----------- drivers/net/iavf/iavf_rxtx_vec_common.h | 4 +-- drivers/net/iavf/iavf_rxtx_vec_neon.c | 16 +++++------ drivers/net/iavf/iavf_rxtx_vec_sse.c | 32 +++++++++++----------- drivers/net/ice/ice_rxtx_vec_avx2.c | 16 +++++------ drivers/net/ice/ice_rxtx_vec_avx512.c | 16 +++++------ drivers/net/ice/ice_rxtx_vec_common.h | 4 +-- drivers/net/ice/ice_rxtx_vec_sse.c | 16 +++++------ drivers/net/ionic/ionic_lif.c | 4 +-- drivers/net/ionic/ionic_rxtx_sg.c | 4 +-- drivers/net/ionic/ionic_rxtx_simple.c | 2 +- drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 4 +-- drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c | 12 ++++----- drivers/net/ixgbe/ixgbe_rxtx_vec_sse.c | 24 ++++++++--------- drivers/net/mlx5/mlx5_rxq.c | 2 +- drivers/net/mlx5/mlx5_rxtx_vec_altivec.h | 28 +++++++++---------- drivers/net/mlx5/mlx5_rxtx_vec_neon.h | 20 +++++++------- drivers/net/mlx5/mlx5_rxtx_vec_sse.h | 28 +++++++++---------- drivers/net/octeon_ep/cnxk_ep_rx.h | 2 +- drivers/net/octeon_ep/cnxk_ep_rx_avx.c | 2 +- drivers/net/octeon_ep/cnxk_ep_rx_neon.c | 8 +++--- drivers/net/octeon_ep/cnxk_ep_rx_sse.c | 8 +++--- drivers/net/octeon_ep/otx_ep_rxtx.c | 5 +--- drivers/net/sfc/sfc_ef100_rx.c | 4 +-- drivers/net/sfc/sfc_ef10_rx.c | 6 ++--- drivers/net/thunderx/nicvf_ethdev.c | 4 +-- drivers/net/thunderx/nicvf_rxtx.h | 4 +-- drivers/net/virtio/virtio_rxtx_packed_avx.h | 10 +++---- drivers/net/virtio/virtio_rxtx_packed_neon.h | 16 +++++------ drivers/net/virtio/virtio_rxtx_simple.c | 4 +-- drivers/net/virtio/virtio_rxtx_simple.h | 5 +--- drivers/net/virtio/virtio_rxtx_simple_altivec.c | 16 +++++------ drivers/net/virtio/virtio_rxtx_simple_neon.c | 24 ++++++----------- drivers/net/virtio/virtio_rxtx_simple_sse.c | 16 +++++------ lib/cryptodev/cryptodev_pmd.h | 5 ++-- lib/eal/include/rte_common.h | 8 +++++- lib/mbuf/rte_mbuf.h | 28 +++++++++++++++++-- lib/mbuf/rte_mbuf_core.h | 10 +++---- lib/security/rte_security_driver.h | 5 ++-- 60 files changed, 392 insertions(+), 404 deletions(-)
Comments
+to: Thomas > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Saturday, 24 February 2024 09.22 > > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. This series > hides the markers when building with MSVC and updates libraries and > drivers to access compatibly typed pointers to the rte_mbuf struct > offsets. > > This series, does the following. > > Introduces a new macro __rte_marker(type, name) which is used to > conditionally expand RTE_MARKER fields empty when building with GCC. Important typo: GCC -> MSVC. Correct me if I'm wrong: When building with MSVC, the RTE_MARKER fields don't exist at all. > > Updates existing inline functions accessing cacheline{0,1} markers in > the rte_mbuf struct to stop using the markers and instead uses the mbuf > fields directly. > > Introduces 2 new inline functions to allow drivers to access rearm_data > and rx_descriptor_fields1 descriptors without using the RTE_MARKER > fields. > > Updates all drivers to use the new inline rte_mbuf struct accessors for > rearm_data and rx_descriptor_fields1. > > Any previous Acks on the series are considered to be reset due to amount > of change. > > v5: > * update existing cacheline{0, 1} inline functions to access actual > mbuf fields. > * introduce new inline functions for accessing rearm_data and > rx_descriptor_fields1 descriptors. > * adapt drivers to use new inline functions. Accessor functions like these are BS for structures that are part of the public API. Nobody will use the accessor functions! When developing an application (or lib or driver), the developer will look at the structure and access the relevant fields directly; why would the developer start looking elsewhere for accessor functions? Another alternative would be to remove the rearm_data and rx_descriptor_fields1 fields from the structure, and in the drivers address the first field of the group, and preferably add some static_asserts to check the sequence of the fields they cover. I don't like this alternative, but I'm putting it out there for discussion/inspiration. I prefer the union+struct approach to visibly group the fields together. Overall, I dislike approach taken in this version of the patch series. On the surface, it has minimal changes to the mbuf structure. But underneath, some of the fields (the markers) may or may not exist, depending on compiler, and this fact is not obvious when looking at the structure. I think this will degrade future MSVC compatibility for both applications, libraries and PMDs. As Thomas stressed, we should take special care of the mbuf structure! It has to be modified for MSVC compatibility, so we have to find the best compromise. Personally, I prefer the previous approach over this version. Maybe we need to compromise on API compatibility to make this a beautiful modification. @Thomas, looking at the mbuf and eal patches, what do you think about this version of the series?
24/02/2024 11:42, Morten Brørup: > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. This series > > hides the markers when building with MSVC and updates libraries and > > drivers to access compatibly typed pointers to the rte_mbuf struct > > offsets. > > > > This series, does the following. > > > > Introduces a new macro __rte_marker(type, name) which is used to > > conditionally expand RTE_MARKER fields empty when building with GCC. > > Important typo: GCC -> MSVC. > > Correct me if I'm wrong: When building with MSVC, the RTE_MARKER fields don't exist at all. Yes it should trigger a compiler error when using an old marker with MSVC. So no need of this wrapper __rte_marker(). > > Updates existing inline functions accessing cacheline{0,1} markers in > > the rte_mbuf struct to stop using the markers and instead uses the mbuf > > fields directly. > > > > Introduces 2 new inline functions to allow drivers to access rearm_data > > and rx_descriptor_fields1 descriptors without using the RTE_MARKER > > fields. > > > > Updates all drivers to use the new inline rte_mbuf struct accessors for > > rearm_data and rx_descriptor_fields1. > > Accessor functions like these are BS for structures that are part of the public API. What is BS? > Nobody will use the accessor functions! When developing an application (or lib or driver), the developer will look at the structure and access the relevant fields directly; why would the developer start looking elsewhere for accessor functions? Yes that's why we need to reference the accessors inside the mbuf structure. Also we should check new code (with checkpatch) for not using markers anymore. > Another alternative would be to remove the rearm_data and rx_descriptor_fields1 fields from the structure, and in the drivers address the first field of the group, and preferably add some static_asserts to check the sequence of the fields they cover. I don't like this alternative, but I'm putting it out there for discussion/inspiration. > I prefer the union+struct approach to visibly group the fields together. Unions make the mbuf struct more complicate just for compatibility. > Overall, I dislike approach taken in this version of the patch series. > On the surface, it has minimal changes to the mbuf structure. > But underneath, some of the fields (the markers) may or may not exist, depending on compiler, and this fact is not obvious when looking at the structure. I think this will degrade future MSVC compatibility for both applications, libraries and PMDs. With appropriate checks, we won't use markers anymore. > As Thomas stressed, we should take special care of the mbuf structure! > It has to be modified for MSVC compatibility, so we have to find the best compromise. > Personally, I prefer the previous approach over this version. > > Maybe we need to compromise on API compatibility to make this a beautiful modification. > > @Thomas, looking at the mbuf and eal patches, what do you think about this version of the series? I prefer this series with following changes: - no __rte_marker wrapper - make sure cache line padding is effective without markers - no direct access of fields for cache line prefetch - comments in mbuf - checkpatch
> From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Saturday, 24 February 2024 12.14 > > 24/02/2024 11:42, Morten Brørup: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. This > series > > > hides the markers when building with MSVC and updates libraries and > > > drivers to access compatibly typed pointers to the rte_mbuf struct > > > offsets. > > > > > > This series, does the following. > > > > > > Introduces a new macro __rte_marker(type, name) which is used to > > > conditionally expand RTE_MARKER fields empty when building with GCC. > > > > Important typo: GCC -> MSVC. > > > > Correct me if I'm wrong: When building with MSVC, the RTE_MARKER > fields don't exist at all. > > Yes it should trigger a compiler error when using an old marker with > MSVC. > So no need of this wrapper __rte_marker(). > > > > Updates existing inline functions accessing cacheline{0,1} markers > in > > > the rte_mbuf struct to stop using the markers and instead uses the > mbuf > > > fields directly. > > > > > > Introduces 2 new inline functions to allow drivers to access > rearm_data > > > and rx_descriptor_fields1 descriptors without using the RTE_MARKER > > > fields. > > > > > > Updates all drivers to use the new inline rte_mbuf struct accessors > for > > > rearm_data and rx_descriptor_fields1. > > > > Accessor functions like these are BS for structures that are part of > the public API. > > What is BS? > > > Nobody will use the accessor functions! When developing an application > (or lib or driver), the developer will look at the structure and access > the relevant fields directly; why would the developer start looking > elsewhere for accessor functions? > > Yes that's why we need to reference the accessors inside the mbuf > structure. > Also we should check new code (with checkpatch) for not using markers > anymore. > > > Another alternative would be to remove the rearm_data and > rx_descriptor_fields1 fields from the structure, and in the drivers > address the first field of the group, and preferably add some > static_asserts to check the sequence of the fields they cover. I don't > like this alternative, but I'm putting it out there for > discussion/inspiration. > > I prefer the union+struct approach to visibly group the fields > together. > > Unions make the mbuf struct more complicate just for compatibility. > > > Overall, I dislike approach taken in this version of the patch series. > > On the surface, it has minimal changes to the mbuf structure. > > But underneath, some of the fields (the markers) may or may not exist, > depending on compiler, and this fact is not obvious when looking at the > structure. I think this will degrade future MSVC compatibility for both > applications, libraries and PMDs. > > With appropriate checks, we won't use markers anymore. > > > As Thomas stressed, we should take special care of the mbuf structure! > > It has to be modified for MSVC compatibility, so we have to find the > best compromise. > > Personally, I prefer the previous approach over this version. > > > > Maybe we need to compromise on API compatibility to make this a > beautiful modification. > > > > @Thomas, looking at the mbuf and eal patches, what do you think about > this version of the series? > > I prefer this series with following changes: > - no __rte_marker wrapper > - make sure cache line padding is effective without markers > - no direct access of fields for cache line prefetch > - comments in mbuf > - checkpatch > Thomas has provided a lot of good feedback for this version of the series, showing that the path is worth exploring further. In other words: I'm likely to be convinced. ;-)