From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A9EE942C5C; Thu, 8 Jun 2023 10:47:29 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7DC4540A84; Thu, 8 Jun 2023 10:47:29 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id A3D9D40042 for ; Thu, 8 Jun 2023 10:47:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1686214047; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Xnxfmjk+bjDibqGEvc9/RUbNjHr12heRRZLHUsXFtmY=; b=i90SfujNrmty3JwI6UxRjFknIio/uarcA19o+oB0rW9s5fSLCxa7ZXrIahIRc7NtAzTSn6 F8kcfL9xMRMjW9/0zCKQIthsFyOlYwKgdY5Y+EsgEmO91ZdHg4TKFlLEpl0usQJTAGsrfD Ri1m0kpSRytvCxBIgcdNPPvi6dgLShQ= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-19-n7knZ67yO0amlooYJ9k7qg-1; Thu, 08 Jun 2023 04:47:24 -0400 X-MC-Unique: n7knZ67yO0amlooYJ9k7qg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9546B3825BA0; Thu, 8 Jun 2023 08:47:23 +0000 (UTC) Received: from [10.39.208.25] (unknown [10.39.208.25]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 034E59E8D; Thu, 8 Jun 2023 08:47:21 +0000 (UTC) Message-ID: <30fbed4d-9f04-9a44-d77d-156e7a6257f5@redhat.com> Date: Thu, 8 Jun 2023 10:47:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 To: "Chautru, Nicolas" , David Marchand Cc: Stephen Hemminger , "dev@dpdk.org" , "Rix, Tom" , "hemant.agrawal@nxp.com" , "Vargas, Hernan" References: <20230526021132.41413-1-nicolas.chautru@intel.com> <20230526021132.41413-2-nicolas.chautru@intel.com> <20230525204722.73635324@hermes.local> <874c2179-6dfd-caba-0a8a-75137cb1a418@redhat.com> From: Maxime Coquelin Subject: Re: [PATCH v1 1/1] doc: announce change in bbdev api related to operation extension In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 6/6/23 23:01, Chautru, Nicolas wrote: > Hi David, > >> -----Original Message----- >> From: David Marchand >> Sent: Tuesday, June 6, 2023 2:21 AM >> To: Chautru, Nicolas >> Cc: Maxime Coquelin ; Stephen Hemminger >> ; dev@dpdk.org; Rix, Tom >> ; hemant.agrawal@nxp.com; Vargas, Hernan >> >> Subject: Re: [PATCH v1 1/1] doc: announce change in bbdev api related to >> operation extension >> >> On Mon, Jun 5, 2023 at 10:08 PM Chautru, Nicolas >> wrote: >>> Wrt the MLD functions: these are new into the related serie but still the >> break the ABI since the struct rte_bbdev includes these functions hence >> causing offset changes. >>> >>> Should I then just rephrase as: >>> >>> +* bbdev: Will extend the API to support the new operation type >>> +``RTE_BBDEV_OP_MLDTS`` as per >>> + this `v1 >>> +`. This + >>> will notably introduce + new symbols for >>> ``rte_bbdev_dequeue_mldts_ops``, +``rte_bbdev_enqueue_mldts_ops`` >>> into the stuct rte_bbdev. >> >> I don't think we need this deprecation notice. >> >> >> Do you need to expose those new mldts ops in rte_bbdev? >> Can't they go to dev_ops? >> If you can't, at least moving those new ops at the end of the structure >> would avoid the breakage on rte_bbdev. > > It would probably be best to move all these ops at the end of the structure (ie. keep them together). > In that case the deprecation notice would call out that the rte_bbdev structure content is more generally modified. Probably best for the longer run. > David, Maxime, ok with that option? > > struct __rte_cache_aligned rte_bbdev { > rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; > rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; > rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; > rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; > rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; > rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; > rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; > rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; > rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; > rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; > const struct rte_bbdev_ops *dev_ops; > struct rte_bbdev_data *data; > enum rte_bbdev_state state; > struct rte_device *device; > struct rte_bbdev_cb_list list_cbs; > struct rte_intr_handle *intr_handle; > }; The best thing, as suggested by David, would be to move all the ops out of struct rte_bbdev, as these should not be visible to the application. > > > >> >> >>> >>> Pasting below the ABI results for reference >>> >>> [C] 'function rte_bbdev* rte_bbdev_allocate(const char*)' at >> rte_bbdev.c:174:1 has some indirect sub-type changes: >>> return type changed: >>> in pointed to type 'struct rte_bbdev' at rte_bbdev.h:498:1: >>> type size hasn't changed >>> 2 data member insertions: >>> 'rte_bbdev_enqueue_mldts_ops_t rte_bbdev::enqueue_mldts_ops', >> at offset 640 (in bits) at rte_bbdev.h:520:1 >>> 'rte_bbdev_dequeue_mldts_ops_t rte_bbdev::dequeue_mldts_ops', >> at offset 704 (in bits) at rte_bbdev.h:522:1 >>> 7 data member changes (9 filtered): >>> type of 'rte_bbdev_dequeue_fft_ops_t rte_bbdev::dequeue_fft_ops' >> changed: >>> underlying type 'typedef uint16_t (rte_bbdev_queue_data*, >> rte_bbdev_fft_op**, typedef uint16_t)*' changed: >>> in pointed to type 'function type typedef uint16_t >> (rte_bbdev_queue_data*, rte_bbdev_fft_op**, typedef uint16_t)': >>> parameter 2 of type 'rte_bbdev_fft_op**' has sub-type changes: >>> in pointed to type 'rte_bbdev_fft_op*': >>> in pointed to type 'struct rte_bbdev_fft_op' at >> rte_bbdev_op.h:978:1: >>> type size changed from 832 to 1664 (in bits) >>> 1 data member change: >>> type of 'rte_bbdev_op_fft rte_bbdev_fft_op::fft' changed: >>> type size changed from 640 to 1472 (in bits) >>> 6 data member insertions: >>> 'rte_bbdev_op_data >> rte_bbdev_op_fft::dewindowing_input', at offset 256 (in bits) at >> rte_bbdev_op.h:771:1 >>> 'int8_t rte_bbdev_op_fft::freq_resample_mode', at offset >> 768 (in bits) at rte_bbdev_op.h:807:1 >>> 'uint16_t rte_bbdev_op_fft::output_depadded_size', at >> offset 784 (in bits) at rte_bbdev_op.h:809:1 >>> 'uint16_t rte_bbdev_op_fft::cs_theta_0[12]', at offset 800 >> (in bits) at rte_bbdev_op.h:811:1 >>> 'uint32_t rte_bbdev_op_fft::cs_theta_d[12]', at offset 992 >> (in bits) at rte_bbdev_op.h:813:1 >>> 'int8_t rte_bbdev_op_fft::time_offset[12]', at offset 1376 >> (in bits) at rte_bbdev_op.h:815:1 >>> 17 data member changes: >>> 'rte_bbdev_op_data >> rte_bbdev_op_fft::power_meas_output' offset changed from 256 to 384 (in >> bits) (by +128 bits) >>> 'uint32_t rte_bbdev_op_fft::op_flags' offset changed from >> 384 to 512 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::input_sequence_size' offset >> changed from 416 to 544 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::input_leading_padding' >> offset changed from 432 to 560 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::output_sequence_size' offset >> changed from 448 to 576 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::output_leading_depadding' >> offset changed from 464 to 592 (in bits) (by +128 bits) >>> 'uint8_t rte_bbdev_op_fft::window_index[6]' offset >> changed from 480 to 608 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::cs_bitmap' offset changed >> from 528 to 656 (in bits) (by +128 bits) >>> 'uint8_t rte_bbdev_op_fft::num_antennas_log2' offset >> changed from 544 to 672 (in bits) (by +128 bits) >>> 'uint8_t rte_bbdev_op_fft::idft_log2' offset changed from >> 552 to 680 (in bits) (by +128 bits) >>> 'uint8_t rte_bbdev_op_fft::dft_log2' offset changed from >> 560 to 688 (in bits) (by +128 bits) >>> 'int8_t rte_bbdev_op_fft::cs_time_adjustment' offset >> changed from 568 to 696 (in bits) (by +128 bits) >>> 'int8_t rte_bbdev_op_fft::idft_shift' offset changed from >> 576 to 704 (in bits) (by +128 bits) >>> 'int8_t rte_bbdev_op_fft::dft_shift' offset changed from >> 584 to 712 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::ncs_reciprocal' offset >> changed from 592 to 720 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::power_shift' offset changed >> from 608 to 736 (in bits) (by +128 bits) >>> 'uint16_t rte_bbdev_op_fft::fp16_exp_adjust' offset >> changed from 624 to 752 (in bits) (by +128 bits) >>> 'const rte_bbdev_ops* rte_bbdev::dev_ops' offset changed from 640 >> to 768 (in bits) (by +128 bits) >>> 'rte_bbdev_data* rte_bbdev::data' offset changed from 704 to 832 >> (in bits) (by +128 bits) >>> 'rte_bbdev_state rte_bbdev::state' offset changed from 768 to 896 >> (in bits) (by +128 bits) >>> 'rte_device* rte_bbdev::device' offset changed from 832 to 960 (in >> bits) (by +128 bits) >>> 'rte_bbdev_cb_list rte_bbdev::list_cbs' offset changed from 896 to >> 1024 (in bits) (by +128 bits) >>> 'rte_intr_handle* rte_bbdev::intr_handle' offset changed >>> from 1024 to 1152 (in bits) (by +128 bits) >> >> As for the report on the rte_bbdev_op_fft structure changes: >> - wrt to its size, I think it is okay to waive it, rte_bbdev_fft_op objects are >> coming from a bbdev mempool which is created by the bbdev library itself >> (with the right element size if the application asked for RTE_BBDEV_OP_FFT >> type), >> - wrt to the fields locations, an application may have been touching those >> fields, so moving all the added fields at the end of the structure would be >> better. >> But on the other hand, an application will have to call an fft_ops >> experimental API at some point, and the application developer is already >> warned that ABI is not preserved on this part of the API, >> >> So I would waive the changes on rte_bbdev_fft_op with something like: >> >> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore index >> 3ff51509de..3cdce69418 100644 >> --- a/devtools/libabigail.abignore >> +++ b/devtools/libabigail.abignore >> @@ -36,6 +36,8 @@ >> [suppress_type] >> type_kind = enum >> changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM, >> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END >> +[suppress_type] >> + name = rte_bbdev_fft_op > > > OK I did not know about this method. Shouldn't this apply more generally to all experimental structures? > This can be added into the serie for 23.11. > > > Thanks > Nic > > > >> >> ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >> ; Temporary exceptions till next major ABI version ; >> >> >> -- >> David Marchand >