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 92BC2A0584; Wed, 19 Oct 2022 13:38:49 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4A232427FF; Wed, 19 Oct 2022 13:38:49 +0200 (CEST) Received: from NAM11-BN8-obe.outbound.protection.outlook.com (mail-bn8nam11on2040.outbound.protection.outlook.com [40.107.236.40]) by mails.dpdk.org (Postfix) with ESMTP id 1C511410D1 for ; Wed, 19 Oct 2022 13:38:48 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=H+LA0qMfBeswxkefVLKkRzRrJy3P9cQDHmXYf3dOH9L7mEEQ7EvgBQhN3/qTWRzFf4l4ehjAAwuF4/kTnUG4oLEtUnhLecbmtQDF6fWIHEMUG4VVEdghy9uFmYMUzr8s7Kse+DbTvcWUPjTlv6ElfCSG5JRmjJOpXcMAgrl0WczsNlOEKMK/EZjjPNUjllGB+sOciFKqHITKQxZrJ9OFan4oVJBP1kybtiNpUMuVaRiKRoPMGgsiuQXsI/7Yyb48Af0F+ubdEEv7BhtPnkTQMDcXR0sQn+Hr6EArmbMl5TJ0ayDQE2FWcLhpO1qxim+H32KrMBGHP4E5MpBQGAL4lA== 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=1OZba3sMUT2bF5ap1wGwyHg8App7qe/gSICjOUDMQRQ=; b=gxZADklsplw2uMUKvgmB/j+RdSQul8wmO0ZdtoYgvAh3nZMFJKwfY4Z0C/Xa98cMdAWN47Z3Ae2es3H7ijdn3wIQs0PHiFPz6ilYIO2rDiZGwD3uw2JfxpScRsxXFd46lExbo1016LBn0fLlT8XG5DC/7bosF504fo/AapK+mJr+0rtkOhAP3N098LyDLkUgqOdSD/pA+VX08oWXbAR17WUEb7inx+YmzjsKcgSWV8r0yvwxd/6b6OmElg7Kz+5KsISRBMKrV9PzoZYXze22DS+1W9z1ymwnn3K14+oNKpczgtgX+L3BvQpySZajXDeuAPzwCUcrahURFnrHNoqwKw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1OZba3sMUT2bF5ap1wGwyHg8App7qe/gSICjOUDMQRQ=; b=sN3LR10vhHJIRRSXhCpFQL5tVTtwAynDCIqVSxTJi/Hp2Q8GybpdReBzieGc70waV6nRjyzAEB399itpdaUgl8DVM7mixlHZljqo5zKO8b+aTkxSOXM51H8aEzEF1MA3w/O3F90Arxjuc0wisLfQ3uMDmAYcTxEe/7daMNPiiEI= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; Received: from DM6PR12MB4297.namprd12.prod.outlook.com (2603:10b6:5:211::20) by SA1PR12MB5613.namprd12.prod.outlook.com (2603:10b6:806:23e::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5723.33; Wed, 19 Oct 2022 11:38:45 +0000 Received: from DM6PR12MB4297.namprd12.prod.outlook.com ([fe80::b9fd:e732:4585:6b25]) by DM6PR12MB4297.namprd12.prod.outlook.com ([fe80::b9fd:e732:4585:6b25%7]) with mapi id 15.20.5723.033; Wed, 19 Oct 2022 11:38:45 +0000 Message-ID: Date: Wed, 19 Oct 2022 12:38:39 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Subject: Re: [PATCH v2 03/24] net/nfp: add the flow APIs of nfp PMD Content-Language: en-US To: Chaoyong He Cc: oss-drivers , Niklas Soderlund , "dev@dpdk.org" References: <1665382142-21684-1-git-send-email-chaoyong.he@corigine.com> <1665382142-21684-4-git-send-email-chaoyong.he@corigine.com> <0f2d733d-4f52-b117-9d0d-762cb890b82e@amd.com> <2c989a1e-5a95-1ae0-ea0b-e29a6b76b67a@amd.com> From: Ferruh Yigit In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-ClientProxiedBy: LO2P123CA0089.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:138::22) To DM6PR12MB4297.namprd12.prod.outlook.com (2603:10b6:5:211::20) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DM6PR12MB4297:EE_|SA1PR12MB5613:EE_ X-MS-Office365-Filtering-Correlation-Id: 4f630acc-8547-4883-1204-08dab1c67af7 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: AnUyiH7c5xuH4JNFXtRPb/PYiVyuo4pLrmSEGK9ir6kdyzhKh3slDgKoX3aguhGBayb10H2x5z+q5N0X6cTys4b8vStW7rZaigzVtXDJT08+mT3tQsw/9y5KKr+uM5yo1wOZ3agc8idB05Wx3IS2v1lEti264l3To98/lASJgDYa/29l1qG5755G4ceJHOp/TuniG6KlidBM50/8e2vBHTQ0nYeqHCsLJgstozuL/3hVHMpxnVs2Nqf+3nRa+JWNObdoKKB4zrutB162Xq60B2Z71uPs95kiyU0qXBrXoL0UPA6wbcSejMOfao+LYWvcJxDQ5ifBERyfkM4kHCX1+u268KZcWGiUOvBH74hoy7nj9jpn6tZhyCcBVLpoQIQ5OoQPMW1UpLrlhlkdMR9n6sDkPW3/xGz2Er2XGrGIYskz52PIy6RWJQKYXGhY/M6WduIjtGY3QeN3s/dP/mOHIK+tRtlrOLNqZfz32s7C5/jNyWC3XkSw281/glwEQXiJ+hEm25yaSffvHR81N0HVqHw+8gVwwH5Y4JjFoNGAp9LoSEloP7iSvU/VBwW2qmluOXptzwRra9Ub/6NPpzJWyO+Gkk6XqG4zUqHroAjYwl+7zJ7m9zJL+nv6zngr6imZsWix17l0u/TuOn8Lo70CVH3X5ocXZ5a29PV1kW2+pIqFtI31w6tsBnWlNO5kMXi2Jz0auaHTgZ12mtHemrOjNB3Y/GdVKAQZAqbUOdEkQRqJ9j9+kCnoUIjHmEaanlQuDlRMwF2cGAowzBo+YY60u7+wrS+6U6RktKzFpojs92A= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR12MB4297.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(4636009)(396003)(346002)(366004)(136003)(376002)(39860400002)(451199015)(38100700002)(316002)(2616005)(8936002)(26005)(86362001)(6512007)(66946007)(5660300002)(44832011)(36756003)(66476007)(4326008)(6666004)(8676002)(66556008)(186003)(41300700001)(53546011)(31696002)(2906002)(6506007)(6486002)(31686004)(6916009)(66574015)(54906003)(478600001)(83380400001)(45980500001)(43740500002); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RE1FckY2ZWhWRnZjc2Ntc1RISGZTL3FDL1pIM3pRNEFhRFRFZFBXL3NTTHo0?= =?utf-8?B?eVlzYy93MWhnOGtBYjBlWXp3ak0rK01zYmgwVVFUS2lJQmJna3lSZkN0Njhs?= =?utf-8?B?a0dVamVBeHVzWC91eU5ncW1LYXhsN09wKzBXeHY3Sm94UVVQMjN5Q1o5RHg3?= =?utf-8?B?YmlBM2tWQmtzRjVDMmpqdHVSLy9GNEsyb2ZHLzJJMVhqVE10Q05COVgwM2JV?= =?utf-8?B?aVpXWlM4YWFGTThVWEVUdHBYZUVacjVWbG9tRzJ6VWV5d3E2TVQ0cGVrVHBF?= =?utf-8?B?ZDNwdXdBaVAxTU1TSS9KU08vREJORXRHZ21KYzRxWmQrUjh6Q0d5UXdtK0hT?= =?utf-8?B?cDFWT0JGOStzdlArOC9NcGZuODlmMWkxTWNXYmFaTmFCSkJRNFFicmNrK2hm?= =?utf-8?B?WUFsS2xXSDRIckVHYXpaUzVDa3B3L2psNUxJMkZiakJ3NG1JeXVjaWhLUUNs?= =?utf-8?B?SU5vTDZHTnc2ZTdmNlR2UmgzZzJneHRGWWttMjAvVm4vSitxUFQvQ2lLZ1Z4?= =?utf-8?B?ME1vdWF1eEwwT0I5ZUIxZ0VWazByaWM1Wkx6VXVXRFArY1BLendQTnZMWDE2?= =?utf-8?B?U05wNHN1bDhzUGRnVFQ5aThDdDh1RmJQMW9QMG9NTGZFTjFyYzV0dHNrWnBl?= =?utf-8?B?MWdsaG9jUHVoMFJid2lzSTZUUnZISjJjQnFBNnJXdi9yNk82M0VodXpwV0sr?= =?utf-8?B?VFFKcklnMFRWTGMwQVY3T2RoMWEweFNwTDc0R2hmT2N5RWYrU1VJWityM0tD?= =?utf-8?B?R21tK2haSzBqd2JhRkFYNk9YSTJMWWZnNFd5UzVBQ3dZY2hmMkxxbFlVbTZw?= =?utf-8?B?OFFXQXErYm9jUlo4Q0x3WEJRb05Db294VXlLcC83RlhIbWVSdUlNMlF3aGh5?= =?utf-8?B?a3YxVFhrN0VORE1lZWUxMExGaEUrZnZvZ29YVk9FaU5KL1psenozS1k0SjBU?= =?utf-8?B?cGJBTVZFNktodXhMcUk4Sy9DRmFvbTQ1Q2d0cDdGdGNKdHk5cFNubS81dXcr?= =?utf-8?B?UzQxcGp2MXpLbExlT2dCNnJVNUdNQ1MxZXNhZTdMZ0FReG84Rk1hbG9HSSty?= =?utf-8?B?K0JIVllUYmpoWkg0cmtMeVYvT3JNYjRnb0Z2WWZrcDJjU2g2N1NXUU53M1Jm?= =?utf-8?B?TnhqOHZWeGU3RFZRc1lCYTRDOXc0cUd1aW42eVJqUVh0dzNZeEVkQjNkWVVV?= =?utf-8?B?b05IT2RQWk1FeU53S2NrV0lVa1hnWEM5blJ3cmQ3RU85Z1JOUnZrLzBrUU12?= =?utf-8?B?aWhxSHREckVxUGU1VTk4Z0VhR3lhSmpMbVRyNXZBNXlTZm9leFhxZm5lMFBv?= =?utf-8?B?NERnQTZ1QWpuSm5kcm50RFV3N3Znd05UZEdNZ2NBU1FSUjZaRWxJM3YyRGR3?= =?utf-8?B?QTNoVlExdEdRYWVvdHA1N1VBN1EvZGFnajg5ZW5ycUVCd2w3V2FRYTcwVG51?= =?utf-8?B?TXZBQ1NSMXBEZUR0V0Z2QUVrckdoc3hJeUZBc0R6ZWlNWVYvNHl4MGRYb3dS?= =?utf-8?B?ZnhBdWNiRS82czR3OU80elp3d0JrUEluZmNHY1l4WllTcmpma3NMMjlhM2Jy?= =?utf-8?B?TTdhbDAzcnJFMjJtSDdsSURORW5JcHFReE5RSVRmYnlHWnpDLytIbWtFa2F2?= =?utf-8?B?WFJNa2pPRUptY2hJWU1HNXNrZkhLVW5tYWM0M240UzRaR1NBeXhwUTlTdTlM?= =?utf-8?B?SGlxWUpBcSszMWpnSmtzeTZITm05VFhCN2FnWUZTQVFNb1lMM3pYT2FRMGVp?= =?utf-8?B?Rk1XRlpCVDdaOEJuNFI2MXhPd29nTEJzOE1NTlFnV203aDJpajFYSVk0Z1JK?= =?utf-8?B?SXExd3k3TU8rVzZ0Z0VLRFRUNG9CTkxkWEJiYW9XckZPZjZFNER2WmxwR3VF?= =?utf-8?B?QThMSEZqTGtuSzBUd1N5YTVZc1ZaUDRQMnBBdG50NHhKdS9hU1loZFdHOW1h?= =?utf-8?B?Mk9GU3ljUDlIZU52WmZsQmloR1NvcWw3SEU2eXJLYmRJd1ZtNkJOZ2c5SFdw?= =?utf-8?B?WVZzZWs5T2FNU3RWTGtrUFlVQ0RWeFlCRzNBSnkzdWRneTJORHNlUFNqN0RG?= =?utf-8?B?eWViM28zaWJURmY2YUxJNDFWQ3U2TDhvWUNyTWZQa08rNHlwNlN1K1cxcVNR?= =?utf-8?Q?b4uk1dfjJGmwUttm3fqaUAMyB?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4f630acc-8547-4883-1204-08dab1c67af7 X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB4297.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Oct 2022 11:38:45.3199 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: +6dQbHWlNdqjjjZD/6mgwVh+rjO7ZVRxdd34/MCr/SelMAveKDi9jOv3aiYtnCZu X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB5613 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 10/19/2022 12:30 PM, Chaoyong He wrote: >> On 10/19/2022 4:00 AM, Chaoyong He wrote: >>>> On 10/10/2022 7:08 AM, Chaoyong He wrote: >>>>> Add the flow validate/create/query/destroy/flush API of nfp PMD. >>>>> >>>>> The flow create API construct a control cmsg and send it to >>>>> firmware, then add this flow to the hash table. >>>>> >>>>> The flow query API get flow stats from the flow_priv structure. >>>>> Note there exist an rte_spin_lock to prevent the update and query >>>>> action occur at the same time. >>>>> >>>>> The flow destroy API construct a control cmsg and send it to >>>>> firmware, then adelete this flow from the hash table. >>>>> >>>>> The flow flush API just iterate the flows in hash table and call the >>>>> flow destroy API. >>>>> >>>>> Signed-off-by: Chaoyong He >>>>> Reviewed-by: Niklas Söderlund >>>> >>>> <...> >>>> >>>>> +static void >>>>> +nfp_flow_stats_get(struct rte_eth_dev *dev, >>>>> + struct rte_flow *nfp_flow, >>>>> + void *data) >>>>> +{ >>>>> + uint32_t ctx_id; >>>>> + struct rte_flow *flow; >>>>> + struct nfp_flow_priv *priv; >>>>> + struct nfp_fl_stats *stats; >>>>> + struct rte_flow_query_count *query; >>>>> + >>>>> + priv = nfp_flow_dev_to_priv(dev); >>>>> + flow = nfp_flow_table_search(priv, nfp_flow); >>>>> + if (flow == NULL) { >>>>> + PMD_DRV_LOG(ERR, "Can not find statistics for this flow."); >>>>> + return; >>>>> + } >>>>> + >>>>> + query = (struct rte_flow_query_count *)data; >>>>> + ctx_id = rte_be_to_cpu_32(nfp_flow->payload.meta->host_ctx_id); >>>>> + stats = &priv->stats[ctx_id]; >>>>> + >>>>> + rte_spinlock_lock(&priv->stats_lock); >>>>> + if (stats->pkts && stats->bytes) { >>>> >>>> Is it guaranteed that 'query' ("void *data") is zeroed out when it is >>>> provided by application? >>>> >> >> Let me clarify this comment, >> >> When "stats->pkts == 0", not taken this branch and 'query' fields are not >> updated. How caller can know if 'query' has random values or assigned values, >> won't it be good to memset query. >> > > Okay, I understand it now. I'll revise like what you said in the next version patch, thanks. > >>>>> + query->hits = stats->pkts; >>>>> + query->bytes = stats->bytes; >>>>> + query->hits_set = 1; >>>>> + query->bytes_set = 1; >>>>> + stats->pkts = 0; >>>>> + stats->bytes = 0; >>>> >>>> need to check 'reset' field of action to decide reset or not. >>>> >> >> And this one also seems not answered, to there is an attribute of action to >> request resetting stats, above code ignores it. >> > > How about like this: > ``` > if (stats->pkts != 0 && stats->bytes != 0) { > query->hits = stats->pkts; > query->bytes = stats->bytes; > query->hits_set = 1; > query->bytes_set = 1; > if (query->reset != 0) { > rte_spinlock_lock(&priv->stats_lock); > stats->pkts = 0; > stats->bytes = 0; > rte_spinlock_unlock(&priv->stats_lock); > } ack > } else { > memset(query, 0, sizeof(*query)); > } memset can go outside of the if block for simplicity, but both are OK, up to you. > ``` > > And I'm not quite sure if I should add a check about the `reserved` field like this: > ``` > query = (struct rte_flow_query_count *)data; > if (query->reserved != 0) { > PMD_DRV_LOG(ERR, "The reserved field should always be 0!"); > return; > } > ``` I don't think it is required to check reserved fields. > >>>> <...> >>>> >>>>> @@ -75,6 +101,7 @@ struct nfp_fl_stats { >>>>> >>>>> struct nfp_flow_priv { >>>>> uint32_t hash_seed; /**< Hash seed for hash tables in this >>>>> structure. */ >>>>> + uint64_t flower_version; /**< Flow version, always increase. >>>>> + */ >>>> >>>> Is this version to keep unique value per flow configuration? If so as >>>> far as I can see '.validate' is updating this value, is this expected? >>>> >>>> Also who suppose to use this value? >>> >>> Yes, it is expected. >>> >>> This value is part of the nfp_flow_meta, and which is part of the flow >>> offloaded to the firmware. >>> And the content of the flow offloaded to the firmware is the ABI of >>> the firmware, so it's can't easily change. >> >> I am not sure we are on same page. This variable is increased when a flow >> rule is added or validated by application, how this is part of ABI? >> >> Also whenever application validates a flow this 'flower_version' value is >> increased. Application is just validating the flow, not adding a flow so nothing >> changes in the HW config. >> And this increase in the 'flower_version' variable may cause driver/hw think >> that config is changed? > > Okay, I will revise to make sure the '.validate' won't change this value anymore, thanks. ack