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 A98A4A0353; Thu, 6 Aug 2020 18:57:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8B1D237B4; Thu, 6 Aug 2020 18:57:51 +0200 (CEST) Received: from mail-il1-f194.google.com (mail-il1-f194.google.com [209.85.166.194]) by dpdk.org (Postfix) with ESMTP id 82DA237B4 for ; Thu, 6 Aug 2020 18:57:50 +0200 (CEST) Received: by mail-il1-f194.google.com with SMTP id 77so12555017ilc.5 for ; Thu, 06 Aug 2020 09:57:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=eq7o3kwNSLQCUn4KLC0w53KTuqN5YAHpYTTXS9Zby0I=; b=dZIa40PISKziw3nbdiAvzyJl3VMlHOJrbiOSCMXIDcyNiG4h4+upxq4sjdJ6LwJjdq yA+/U1mAjlIMg+oGmJpxkCnUzHXpdAa5tFLhK0/z1sveg3SM7zs5i7T3al7mucOCq3c3 RGJ4NigJWnlR7qho2IKh0Qu+E4WkG1ML/RbIahjgybvJCjPb7gNrWg2wjMreaiFp6Esp TUZMcJ7mDsN3SX/x1Enu39/TmWSoFvD71i4XtWeNJa4xjkT4ohEchs2+XG93M19Vdr9p SWpG3HQMZVYK13W48SbHchZdjUKFzUcxQR/fsfR1xaRCHWErw4+GPyftXTXPQ/wFMn0l gB6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=eq7o3kwNSLQCUn4KLC0w53KTuqN5YAHpYTTXS9Zby0I=; b=PjnDseFb1gUK3h0b530ccBPA9tFIjwh0/G02ZyXe08TvDb0RodM/OKgLhHGLmGScwQ TURdoY/mp56gvbNDhV+mRNc0WmCi20iJ9EplAN26BBeUvb0TPSr7bjZnN9YkCZS+PW/D S5kSissJWjhU7IB78MO3MHZApV8kehB6noovChI+OCUtp0o4/PgZbOHLBb9dreWLWWCa MBcXTiJrM0Ty4xs72P30MXGeVD+KRoGQ9kXKVhLKB46ZsO6HBPd5d4lg+/fTsQIzpJ+S xPra8FHB2FU3uLGH15wSgCyz1vgbPKDFwy56MFm4gv5+/wKFgwfZEJENk2Zj+iK9wa1G fDmA== X-Gm-Message-State: AOAM531I2IVHAosHHJKVo7VUulXF541IZVpCAE/GJoEDipdmcd5VGI2C TycnH76efGBPU/APc4IFXL0WKuPigGak+GAB9bs= X-Google-Smtp-Source: ABdhPJwQsu2Ra0U8RmAkF83VAQNDjlVpgDkVspPGyG7cxD5ncFqBdldXo3TKThsM1+Gp9PxODW547nbryswvBTVmc1E= X-Received: by 2002:a92:dcc8:: with SMTP id b8mr10862731ilr.60.1596733069768; Thu, 06 Aug 2020 09:57:49 -0700 (PDT) MIME-Version: 1.0 References: <20200802105137.1666-1-pbhagavatula@marvell.com> <20200803072903.1209-1-pbhagavatula@marvell.com> <20200804104153.GA1464@bricha3-MOBL.ger.corp.intel.com> <20200804092007.54bf76f3@hermes.lan> <216e9b9d-3a4e-8250-ffb6-4dc4a74c7867@ashroe.eu> <20200805175950.3888ef11@hermes.lan> In-Reply-To: <20200805175950.3888ef11@hermes.lan> From: Jerin Jacob Date: Thu, 6 Aug 2020 22:27:33 +0530 Message-ID: To: Stephen Hemminger Cc: "Kinsella, Ray" , Bruce Richardson , Pavan Nikhilesh , Jerin Jacob , Neil Horman , John McNamara , Marko Kovacevic , dpdk-dev , Thomas Monjalon , David Marchand Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v2] doc: add reserve fields to eventdev public structures 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 Thu, Aug 6, 2020 at 6:29 AM Stephen Hemminger wrote: > > On Wed, 5 Aug 2020 14:40:01 +0530 > Jerin Jacob wrote: > > > On Wed, Aug 5, 2020 at 2:16 PM Kinsella, Ray wrote: > > > > > > > > > > > > On 04/08/2020 17:20, Stephen Hemminger wrote: > > > > On Tue, 4 Aug 2020 11:41:53 +0100 > > > > Bruce Richardson wrote: > > > > > > > >> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote: > > > >>> From: Pavan Nikhilesh > > > >>> > > > >>> Add 64 byte padding at the end of event device public structure to allow > > > >>> future extensions. > > > >>> > > > >>> Signed-off-by: Pavan Nikhilesh > > > >>> Acked-by: Jerin Jacob > > > >>> --- > > > >>> v2 Changes: > > > >>> - Modify commit title. > > > >>> - Add patch reference to doc. > > > >>> > > > >>> doc/guides/rel_notes/deprecation.rst | 11 +++++++++++ > > > >>> 1 file changed, 11 insertions(+) > > > >>> > > > >>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst > > > >>> index ea4cfa7a4..ec5db68e9 100644 > > > >>> --- a/doc/guides/rel_notes/deprecation.rst > > > >>> +++ b/doc/guides/rel_notes/deprecation.rst > > > >>> @@ -151,3 +151,14 @@ Deprecation Notices > > > >>> Python 2 support will be completely removed in 20.11. > > > >>> In 20.08, explicit deprecation warnings will be displayed when running > > > >>> scripts with Python 2. > > > >>> + > > > >>> +* eventdev: A 64 byte padding is added at the end of the following structures > > > >>> + in event device library to support future extensions: > > > >>> + ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``, > > > >>> + ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``, > > > >>> + ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``, > > > >>> + ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``, > > > >>> + ``rte_event_port_conf``, ``rte_event_timer_adapter``, > > > >>> + ``rte_event_timer_adapter_data``. > > > >>> + Reference: > > > >>> + http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=* > > > >>> -- > > > >> > > > >> I don't like this idea of adding lots of padding to the ends of these > > > >> structures. For some structures, such as the public arrays for devices it > > > >> may be necessary, but for all the conf structures passed as parameters to > > > >> functions I think we can do better. Since these structures are passed by > > > >> the user to various functions, function versioning can be used to ensure > > > >> that the correct function in eventdev is always called. From there to the > > > >> individual PMDs, we can implement ABI compatibility by either: > > > >> 1. including the length of the struct as a parameter to the driver. (This is > > > >> a bit similar to my proposal for rawdev [1]) > > > >> 2. including the ABI version as a parameter to the driver. > > > >> > > > >> Regards > > > >> /Bruce > > > >> > > > >> [1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs > > > > > > > > This is a bad idea. > > > > > > > > Reserved fields won't work because nothing requires that the application > > > > zero them. You can't start using them later because the application > > > > may put uninitialized or junk data there. > > > > > > > > > > +1, to Stephens comments. > > > > Since the problem is not specific to one substem, if we need to add a > > field in config structures, > > What will the expected way of handling across the DPDK? > > If you need fields go through the normal enhancement process, and get it > reviewed and put them in a major release milestone. > Sorry, there is no free lunch by adding reserved fields. > > Look up YAGNI YAGNI is useful. But If we need to wait for one year to change the API then it is the problem. That's time frame silicon companies are making the next generation of silicon nowadays. We just tried to follow the existing scheme of things in the codebase[1]. But I am also not a great fan of the Reserved filed scheme either. Probably it is better to add the new feature as a new API or config structure with out breaking anything(as experimental) and upcoming next release, rework to adapt to subsystem API philosophy. [1] lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev_core.h: uint64_t reserved_64s[4]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev_core.h: void *reserved_ptrs[4]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev_core.h: uint64_t reserved_64s[4]; /**< Reserved for future fields */ lib/librte_ethdev/rte_ethdev_core.h: void *reserved_ptrs[4]; /**< Reserved for future fields */ lib/librte_eal/linux/eal_timer.c: uint64_t reserved0; /**< Reserved for future use. */ lib/librte_eal/linux/eal_timer.c: uint64_t reserved1; /**< Reserved for future use. */ lib/librte_eal/linux/eal_timer.c: uint64_t reserved2[25]; /**< Reserved for future use. */ lib/librte_eal/linux/eal_timer.c: uint64_t reserved3; /**< Reserved for future use. */ lib/librte_eal/linux/eal_timer.c: uint64_t reserved4; /**< Reserved for future use. */ lib/librte_eventdev/rte_eventdev.h: /**< Reserved for future use */ lib/librte_eventdev/rte_eventdev.h: uint64_t reserved_64s[4]; /**< Reserved for future fields */ lib/librte_eventdev/rte_eventdev.h: void *reserved_ptrs[4]; /**< Reserved for future fields */ lib/librte_eventdev/rte_eventdev.h: uint64_t reserved_64s[4]; /**< Reserved for future fields */ lib/librte_eventdev/rte_eventdev.h: void *reserved_ptrs[4]; /**< Reserved for future fields */