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 3698AA00C2; Tue, 27 Sep 2022 11:43:35 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1C16541133; Tue, 27 Sep 2022 11:43:35 +0200 (CEST) Received: from NAM12-BN8-obe.outbound.protection.outlook.com (mail-bn8nam12on2060.outbound.protection.outlook.com [40.107.237.60]) by mails.dpdk.org (Postfix) with ESMTP id 782CB410D0 for ; Tue, 27 Sep 2022 11:43:34 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Y0/n1bIuEjs1bJuYdVV/U4I6cdRs07KOhI0FyPqCQInTFbvEJP9Uh4XO4LUNhOLEYdm0pk6wPuTgg4CW0yquo5mMGnnl1KHjOuEAZbr/fE61ZP5R3DPNg5OBmiospBCVPyRC4GuVHjJdH7l/Fn8SrkJoZbEd8vpELbd2W8XFKgLCuyMDvaJSHuKc7Escdp2tMsTdlBh+fGQvTUnLGKtoPdwKoye5m2EBrH1btAzrqW3AjuXQ/BUE23Bjyqet2/F+8jUuk96E/N51zni2gtLm9FeaZ6rVgI1wVwQjwbjAdSCsS2qSUjj4plllwivadVXxHf9iXgJcUYd2NjZckuIj0Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=agjFxT3HKt+/nsOKZIjKk93mUgWBtxiRofBBhLDb68Q=; b=dQBSKmABmZvQAHKwnbjXTDv/3kZzgBOMBpy/MJPMBG3Im1awfQgvHaqEgq0wyvoRy69QolJEGC7Oe3CUE7p3Z570csUXdMcA03H8whcqsttyzFl0voklXdgB4USDii5OyqHYbhSui8rxQOO21zGUzOD9H5M/dm1OMG0aaXKDdNbZvGhajisXJDuFaHY+/HBTL6meI0Ol6BLecf6Rxgvepf1+2W6/wYW1y5AyStMPW+vscmCBX2uetasHYNaY7ig2pluHn1X1LB/rIzPgi+2rQjpdxeW0dmcYRCRQn1Rav3NMchTMdE0Q0hjiAj7WAuLdM0XwePRb+yHXcu5fZD5D9A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=softfail (sender ip is 149.199.80.198) smtp.rcpttodomain=intel.com smtp.mailfrom=amd.com; dmarc=fail (p=quarantine sp=quarantine pct=100) action=quarantine header.from=amd.com; dkim=none (message not signed); arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=xilinx.onmicrosoft.com; s=selector2-xilinx-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=agjFxT3HKt+/nsOKZIjKk93mUgWBtxiRofBBhLDb68Q=; b=Ur8Mnz05+pG7Tboa9dnLcS1yoB4EyIwO09di+37b27OKYMn7H6REzyNC/oNeCF8/F7SIrzC2farurEuR3/xFxtM3FT+JVqawwPN0aXFF/kVGEwhvaIOvm0T87rRv066ZWbI4szLVPIJ1v5vAaungo3QHlG+tvw9R+HFqw/N2AYo= Received: from BN0PR10CA0018.namprd10.prod.outlook.com (2603:10b6:408:143::29) by SA0PR02MB7497.namprd02.prod.outlook.com (2603:10b6:806:e0::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5676.15; Tue, 27 Sep 2022 09:43:33 +0000 Received: from BN1NAM02FT037.eop-nam02.prod.protection.outlook.com (2603:10b6:408:143:cafe::35) by BN0PR10CA0018.outlook.office365.com (2603:10b6:408:143::29) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5654.26 via Frontend Transport; Tue, 27 Sep 2022 09:43:32 +0000 X-MS-Exchange-Authentication-Results: spf=softfail (sender IP is 149.199.80.198) smtp.mailfrom=amd.com; dkim=none (message not signed) header.d=none;dmarc=fail action=quarantine header.from=amd.com; Received-SPF: SoftFail (protection.outlook.com: domain of transitioning amd.com discourages use of 149.199.80.198 as permitted sender) Received: from xir-pvapexch02.xlnx.xilinx.com (149.199.80.198) by BN1NAM02FT037.mail.protection.outlook.com (10.13.2.148) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.5654.14 via Frontend Transport; Tue, 27 Sep 2022 09:43:32 +0000 Received: from xir-pvapexch02.xlnx.xilinx.com (172.21.17.17) by xir-pvapexch02.xlnx.xilinx.com (172.21.17.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 27 Sep 2022 10:43:31 +0100 Received: from smtp.xilinx.com (172.21.105.198) by xir-pvapexch02.xlnx.xilinx.com (172.21.17.17) with Microsoft SMTP Server id 15.1.2375.24 via Frontend Transport; Tue, 27 Sep 2022 10:43:31 +0100 Envelope-to: ferruh.yigit@xilinx.com, nicolas.chautru@intel.com, gakhil@marvell.com, dev@dpdk.org, maxime.coquelin@redhat.com, mdr@ashroe.eu, thomas@monjalon.net, trix@redhat.com, bruce.richardson@intel.com, david.marchand@redhat.com, stephen@networkplumber.org, mingshan.zhang@intel.com, hemant.agrawal@nxp.com Received: from [10.71.194.74] (port=64609) by smtp.xilinx.com with esmtp (Exim 4.90) (envelope-from ) id 1od77j-0005Wi-J9; Tue, 27 Sep 2022 10:43:31 +0100 Message-ID: <17bacfdd-1326-36e2-5644-bf9dc1938c42@amd.com> Date: Tue, 27 Sep 2022 10:43:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and status information Content-Language: en-US To: "Chautru, Nicolas" , Akhil Goyal , "dev@dpdk.org" , Maxime Coquelin , "ferruh.yigit@xilinx.com" , Ray Kinsella CC: "thomas@monjalon.net" , "trix@redhat.com" , "Richardson, Bruce" , "david.marchand@redhat.com" , "stephen@networkplumber.org" , "Zhang, Mingshan" , "hemant.agrawal@nxp.com" References: <1655491040-183649-6-git-send-email-nicolas.chautru@intel.com> <1661796438-204861-1-git-send-email-nicolas.chautru@intel.com> <1661796438-204861-7-git-send-email-nicolas.chautru@intel.com> <88a06267-0cf3-a6cd-0785-b5b2a419fc02@amd.com> From: Ferruh Yigit In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BN1NAM02FT037:EE_|SA0PR02MB7497:EE_ X-MS-Office365-Filtering-Correlation-Id: d7266da0-24e5-4a8f-29ec-08daa06cbdd2 X-MS-Exchange-SenderADCheck: 2 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: i+2S1noAFXsDlqupuzEeQuBsTLUC8w2FWpd8aamwPDWgbvQfb1yvlquqkI03Pg4VnyUKd19UQK0Jv/xkE+Gtok/5Xz46iqgy/YlvOi0Rv7Ee2I7yz+bvxH8nt6y5yjlwioSaqCvq7gjonEk24dvVqx/N6wefKIAEiUPxm952Kj2FQ51dUSvNb3CPumDlS2K6Z9vh+iYPWlvIvtQDKeOq7m0vf7zHEuklFM/4wRQK6fFL+Kom1mJ00C31pvDrR7ggpaW6zra+rvIXml9KbwiB+9FdEZiTgCzYl1TLU5Sw244O/QF3Cz6glXpJsolwoo8JVl3zKxB6ELUHJybpo5nFTarDSpmS02lSaM7oaIusmQYf4utFs8/fZhjLzpqbpHcRY8Vsyy2QE76gjKPGEBxsy8CrQfAGpepNn/HoPCNOny91Iewl+LbbBQ1fq1/FHp920eFHWYzNiu4jU+dz2pZ3yAOrkvzjxD9/Ib7aXDQGawTvDl3kBqslw3HEy1KZU//ayM5bqIXsukj+9lzws+bddy7lT99uf23Du6/VZ9OVV8J7K9IW1tfLCB9aSA4uGD2Sb/s+OBvT2OJ24B/vhH5hOD2bgqZIQ+1xChmH6mUtmzh9vrW6GsKPZYmlZ+GpCYa/VCwFGs56LQ/79ZGTYsOszC3QHTHxVDCc7C2goDbzkoZ/EwhVRevWMNCDcX4+xKdFiDmoIJNFi3Zp+wtm+a1b1IfMjWJo4FeBbjrkHs+7VUUGCSc/tutdlme/7BBUcyBheNPxvFsBvGBXLf0E60h2xjwLq7QI+XhYPALTFZgEsAlHhlaXLGwqaY19DOtO5vICz8eefQVmjbEa2TfZ5tpKeQ== X-Forefront-Antispam-Report: CIP:149.199.80.198; CTRY:IE; LANG:en; SCL:1; SRV:; IPV:CAL; SFV:NSPM; H:xir-pvapexch02.xlnx.xilinx.com; PTR:unknown-80-198.xilinx.com; CAT:NONE; SFS:(13230022)(4636009)(396003)(39860400002)(136003)(376002)(346002)(451199015)(40470700004)(46966006)(31686004)(35950700001)(8676002)(70586007)(83380400001)(44832011)(26005)(70206006)(47076005)(4326008)(53546011)(7416002)(5660300002)(316002)(40480700001)(36756003)(498600001)(8936002)(54906003)(336012)(2906002)(31696002)(110136005)(9786002)(356005)(86362001)(82740400003)(7636003)(82310400005)(2616005)(40460700003)(41300700001)(50156003)(43740500002); DIR:OUT; SFP:1101; X-OriginatorOrg: xilinx.onmicrosoft.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Sep 2022 09:43:32.6207 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: d7266da0-24e5-4a8f-29ec-08daa06cbdd2 X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c; Ip=[149.199.80.198]; Helo=[xir-pvapexch02.xlnx.xilinx.com] X-MS-Exchange-CrossTenant-AuthSource: BN1NAM02FT037.eop-nam02.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR02MB7497 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 9/24/2022 5:34 PM, Chautru, Nicolas wrote: > Hi Ferruh, Ray, Akhil, > > >>> -----Original Message----- >>> From: Ferruh Yigit >>> Sent: Friday, September 23, 2022 4:28 PM >>> To: Akhil Goyal ; Nicolas Chautru >>> ; dev@dpdk.org; thomas@monjalon.net; >>> hemant.agrawal@nxp.com; Ray Kinsella >>> Cc: maxime.coquelin@redhat.com; trix@redhat.com; >>> bruce.richardson@intel.com; david.marchand@redhat.com; >>> stephen@networkplumber.org; mingshan.zhang@intel.com >>> Subject: Re: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and >>> status information >>> >>> On 9/21/2022 8:21 PM, Akhil Goyal wrote: >>>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index >>>>> ed528b8..b7ecf94 100644 >>>>> --- a/lib/bbdev/rte_bbdev.h >>>>> +++ b/lib/bbdev/rte_bbdev.h >>>>> @@ -224,6 +224,19 @@ struct rte_bbdev_queue_conf { >>>>> rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id); >>>>> >>>>> /** >>>>> + * Flags indicate the reason why a previous enqueue may not have >>>>> + * consumed all requested operations >>>>> + * In case of multiple reasons the latter superdes a previous one >>>> Spell check - supersedes. >>>> >>>>> + */ >>>>> +enum rte_bbdev_enqueue_status { >>>>> + RTE_BBDEV_ENQ_STATUS_NONE, /**< Nothing to report */ >>>>> + RTE_BBDEV_ENQ_STATUS_QUEUE_FULL, /**< Not enough room >> in >>>>> queue */ >>>>> + RTE_BBDEV_ENQ_STATUS_RING_FULL, /**< Not enough room >> in >>>>> ring */ >>>>> + RTE_BBDEV_ENQ_STATUS_INVALID_OP, /**< Operation was >>>>> rejected as invalid */ >>>>> + RTE_BBDEV_ENQ_STATUS_PADDED_MAX = 6, /**< Maximum enq >>>>> status number including padding */ >>>> >>>> Are we ok to have this kind of padding across DPDK for all the enums >>>> to avoid >>> ABI issues? >>>> @Ray, @Thomas: any thoughts? >>>> >>>> >>> >>> This kind of usage can prevent ABI tool warning, and can fix issues >>> caused by application using returned enum as index [1]. >>> >>> But I think it is still problematic in case application tries to walk >>> through till MAX, that is a common usage [2], user may miss that this >>> is PADDED. > > Hi Ferruh, > I don’t believe that case can happen here. Even if application was using an undefined index, the related functions are protected for that : > See rte_bbdev_enqueue_status_str(enum rte_bbdev_enqueue_status status) It doesn't have to use undefined index for DPDK API, application can iterate until MAX enum for application specific array or function. > The reason for having padded max, is that the application may use this for array sizing if required without concern, we will never return a value that would exceeds this. > In the other direction that is not a problem either since application (even it needs to store thigs in array) can used the padded version. > Note that this discussed with Ray notably as a BKM. > I can see usage is more for size, that is why I think it is better to have a #define that has SIZE_MAX in name, instead of enum with PADDED_MAX name. As said above, since you will never return value exceeds PADDED_MAX it solves the issue that application using returned enum as index [1]. So, this usage is not that problematic. But my concern was if user assumes all values valid until PADDED_MAX and tries to iterate array until that value [2]. >>> >>> Overall exchanging enum values between library and application is >>> possible trouble for binary compatibility. If application and library >>> uses enum values independently, this is OK. >>> Since enum cases not deleted but added in new version of the >>> libraries, more problematic usage is passing enum value from library >>> to application, and bbdev seems doing this in a few places. > > I don’t see a case where it is a genuine issue. > An enum is being reported from library, even if due to future enum insertion there is a new enum reported between 2 ABI changes, that would still be within bounds. > Passing enum case from library to application has problem [1], other issue can be application may miss that library added new enum cases and application code needs updating. Overall I think it is not good idea for library to exchange enum values in APIs, specially if there is an intention to have ABI compatibility. >>> >>> With current approach PADDED_MAX usage is more like #define usage, it >>> is not dynamic but a hardcoded value that is used as array size value. >>> >>> Not providing a MAX enum case restricts the application, application >>> can't use it as size of an array or can't use it walk through related >>> array, usage reduces to if/switch comparisons. > > It can use the padded_max to size application array. Even if application was walking through these, there is no adverse effect. > >>> Although this may not be most convenient for application, it can >>> provide safer usage for binary compatibility. >>> >>> >>> @Nic, what do you think provide these PADDED_MAX as #define SIZE_MAX >>> macros? >>> With this application still can allocate a relevant array with correct >>> size, or know the size of library provided array, but can't use it to >>> iterate on these arrays. >>> > > That would be back to how it was done before which made things very inflexible and prevented to change these enums between ABIs versions. > It would be same, the problem was MAX enum value changing. Removing MAX enum but introduce #define SIZE_MAX will be same for application. Only it would be more clear. > This change was highlighted and discussed many months ago and flagged in the deprecation notice in previous release for that very reason. > As far as I remember the deprecation notice was to remove MAX enum values, now you are introducing PADDED_MAX enum value. > Ray, can you please chime in since you know best. > I think this PADDED_MAX solution is not too problematic, but I prefer SIZE_MAX define, I hope my reasoning above is clear. This is a comment from me, not a blocker, I am OK to go with whatever consensus is. > Thanks Ferruh, > Nic > > > >>> >>> >>> >>> [1] >>> --------------- library old version ---------------------------- enum >>> type { >>> CASE_A, >>> CASE_B, >>> CASE_MAX, >>> }; >>> >>> struct data { >>> enum type type; >>> union { >>> type specific struct >>> }; >>> }; >>> >>> int api_get(struct data *data); >>> >>> >>> --------------- application ---------------------------- >>> >>> struct data data; >>> int array[CASE_MAX]; >>> >>> api_get(&data); >>> foo(array[data.type]); >>> >>> >>> --------------- library NEW version ---------------------------- >>> >>> enum type { >>> CASE_A, >>> CASE_B, >>> CASE_C, >>> CASE_D, >>> CASE_MAX, >>> }; >>> >>> >>> When application is NOT recompiled but using new version of the >>> library, values 'CASE_C' & 'CASE_D' will crash application, so this >>> will create a ABI compatibility issue. >>> >>> Note: In the past I have claimed that application should check >>> 'CASE_MAX', instead of using returned value directly as index, but >>> this is refused by argument that we can't control the application and >>> should play safe assuming application behaves wrong. >>> >>> >>> >>> >>> [2] >>> >>> --------------- library ---------------------------- >>> >>> enum type { >>> CASE_NONE, >>> CASE_A, >>> CASE_B, >>> CASE_C, >>> CASE_D, >>> CASE_PADDED_MAX = 666, >>> }; >>> >>> --------------- application ---------------------------- >>> >>> for (int i = CASE_NONE; i < CASE_PADDED_MAX; i++) >>> fragile_init(i); >>> >>> --- >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >