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 1ECCBA0526; Tue, 10 Nov 2020 10:44:08 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7D823F64; Tue, 10 Nov 2020 10:44:06 +0100 (CET) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id CB938DED for ; Tue, 10 Nov 2020 10:44:04 +0100 (CET) IronPort-SDR: FrfFAWiiN+FuaAcjDtkucNuckUojq1arSI9U8WqVIrOHLkBDYvDuoui4hefDz7qEFfpgP4kPFb 13D61xzfPQcQ== X-IronPort-AV: E=McAfee;i="6000,8403,9800"; a="167363290" X-IronPort-AV: E=Sophos;i="5.77,466,1596524400"; d="scan'208";a="167363290" 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/ECDHE-RSA-AES256-GCM-SHA384; 10 Nov 2020 01:44:02 -0800 IronPort-SDR: rS+kB51QqvLmCqmh1GI3MfN0pib4+7T46X7Tp3UkJkRHFAtrzJouxW+EKaq1GdVAhARNn442iP S7ErjTXtCLmQ== X-IronPort-AV: E=Sophos;i="5.77,466,1596524400"; d="scan'208";a="541262881" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.0.179]) ([10.252.0.179]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Nov 2020 01:44:00 -0800 To: Ori Kam , Matan Azrad , Wenzhuo Lu , Beilei Xing , Bernard Iremonger Cc: "dev@dpdk.org" References: <1604252927-213452-1-git-send-email-matan@nvidia.com> <1604611973-64970-1-git-send-email-matan@nvidia.com> <970eeb8a-b18b-fc89-7659-02fa6020d19b@intel.com> <410fbc60-93f7-c810-3d2e-0fd2216f34a5@intel.com> From: Ferruh Yigit Message-ID: <8859b571-ffb8-4dc5-d0c4-3dc455ac6a40@intel.com> Date: Tue, 10 Nov 2020 09:43:56 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: support age shared action context 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 11/10/2020 8:30 AM, Ori Kam wrote: > Hi, > Ferruh and Matan, > >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Monday, November 9, 2020 1:13 PM >> To: Matan Azrad ; Wenzhuo Lu ; >> Beilei Xing ; Bernard Iremonger >> ; Ori Kam >> Cc: dev@dpdk.org >> Subject: Re: [PATCH v2] app/testpmd: support age shared action context >> >> On 11/9/2020 10:38 AM, Matan Azrad wrote: >>> >>> >>> From: Ferruh Yigit >>>> On 11/7/2020 5:30 PM, Matan Azrad wrote: >>>>> Hi Ferruh >>>>> >>>>> From: Ferruh Yigit >>>>>> On 11/5/2020 9:32 PM, Matan Azrad wrote: >>>>>>> When an age action becomes aged-out the rte_flow_get_aged_flows >>>>>>> should return the action context supplied by the configuration structure. >>>>>>> >>>>>>> In case the age action created by the shared action API, the shared >>>>>>> action context of the Testpmd application was not set. >>>>>>> >>>>>>> In addition, the application handler of the contexts returned by the >>>>>>> rte_flow_get_aged_flows API didn't consider the fact that the action >>>>>>> could be set by the shared action API and considered it as regular >>>>>>> flow context. >>>>>>> >>>>>>> This caused a crash in Testpmd when the context is parsed. >>>>>>> >>>>>>> This patch set context type in the flow and shared action context >>>>>>> and uses it to parse the aged-out contexts correctly. >>>>>>> >>>>>>> Signed-off-by: Matan Azrad >>>>>>> Acked-by: Dekel Peled >>>>>>> --- >>>>>>> app/test-pmd/config.c | 119 >>>>>>> ++++++++++++++++++++++++++++++++++------- >>>>>> -------- >>>>>>> app/test-pmd/testpmd.h | 5 +++ >>>>>>> 2 files changed, 87 insertions(+), 37 deletions(-) >>>>>>> >>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index >>>>>>> 755d1df..00a7dd1 100644 >>>>>>> --- a/app/test-pmd/config.c >>>>>>> +++ b/app/test-pmd/config.c >>>>>>> @@ -1763,6 +1763,33 @@ void port_flow_tunnel_create(portid_t >>>>>>> port_id, >>>>>> const struct tunnel_ops *ops) >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> +#define AGE_ACTION_TYPE_MASK 0x3u >>>>>>> + >>>>>>> +static void >>>>>>> +set_age_action_context(void **ctx, enum action_age_context_type >>>>>>> +type, void *obj) { >>>>>>> + uintptr_t value = (uintptr_t)obj; >>>>>>> + >>>>>>> + /* >>>>>>> + * obj is allocated by malloc\calloc which must return an address >>>>>>> + * aligned to 8. >>>>>>> + * Use the last 2 bits for the age context type. >>>>>>> + */ >>>>>>> + value |= (uintptr_t)type & AGE_ACTION_TYPE_MASK; >>>>>>> + *ctx = (void *)value; >>>>>> >>>>>> Thanks Matan, I think this is much clear. But I though the 'id' will >>>>>> be used, not the pointer itself, like "uintptr_t value = id | (type * MASK)" >>>>>> OR the address pointer and type seems error prone, although you >>>>>> comment you rely on the alignment. >>>>> >>>>> I understand your concern, that's why the context value management is >>>> wrapped well by dedicated functions for set and parse. >>>>> Also it's very optimized way for memory and time especially when we are >>>> talking about big scale(see below). >>>>> >>>>>> The testpmd usage also kind of sample usage for the applications, I >>>>>> am for not suggesting this for the user applications. >>>>> >>>>> >>>>>> Reserving the two bit of the 'id' reduces the usable 'id' to 30 bits, >>>>>> but it looks still big enough, what do you think? >>>>> >>>>> >>>>> Yes, it is big enough. >>>>> The problem with the id is the latency to get the pointer from it. >>>>> Since both the flows and the shared actions are saved in a list we need to >>>> traverse all the list in order to get the pointer and the needed information. >>>>> >>>> >>>> Using 'id' was your idea. >>> >>> Yes, Now I suggest even better one 😊 >>> >>>> >>>> OK, what about back to previous suggestion, adding a new data struct for >> both >>>> pointers and type? >>>> Your concern there was the memory consumption, yes although it will >> require >>>> more memory the amount is not unreasonable. >>> >>> Think about big scale. >>> It is not only memory (malloc overhead + ~16B) but also time >> consuming(malloc). >>> >>> If we have solution that no need malloc and can do things faster, why not to >> take it? >>> I don't see here a bug - malloc alignment is a known topic - it should be at >> least the size of the biggest primitive type. >>> >> >> I can see there is a cost, either with malloc/free, or traverse the list to find >> 'id', but updating memory pointer that you will use later to carry more >> metadata >> looks hack and error prone to me. >> >> One can do these kind of tricks on their application, but for testpmd which is >> for testing and reference I didn't like the idea. > > I know I'm jumping a bit late, I have a different suggestion. > What about creating new struct > Struct context_type { > uint8_t type; /**< holds the type of the context can be enum. */ > } > This struct should be added to both shared context and flow. > When adding context to the aging action we add the pointer to this struct. > > Then when getting the context pointer we cast it to struct context_type, > get the value, and based on the type we use parentof function to get to the > pointer of the original structure. > > The advantages: > 1. Just adding one uint8_t to each struct. (not wasting space). > 2. Very fast lookup since we just add if on the type and then > use parentof. > 3. No major changes in structs. > 4. Not an hack. > 5. save extra allocation. > 6. can be expended to support future types. > > What do you think? > Hi Ori, Matan, Looks good to me, this is more like first version of the patch but with 'parentof' usage it is safer I think.