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 05693A04AD; Fri, 1 May 2020 15:38:37 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 94F3C1D9DB; Fri, 1 May 2020 15:38:37 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id E6BBB1D9D9 for ; Fri, 1 May 2020 15:38:34 +0200 (CEST) IronPort-SDR: D82N+IHVWjv1WGWRNbX0kcWGpNAYY+8v6LU1+HDEOqn4xmWEyU93Nk3oDhzzxJTBSO0BUOIPxC 8zx8w3Y1hiaQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 May 2020 06:38:34 -0700 IronPort-SDR: 7I3aOd1xreyNg7ICsZvFPNYvwmHm5JCTAJPaXz3yx8WtgpC85LNQW5LrEw8JHfiLqPtJiC2sLB Bd5fNyYmSNfA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,339,1583222400"; d="scan'208";a="459908833" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.252.12.222]) ([10.252.12.222]) by fmsmga005.fm.intel.com with ESMTP; 01 May 2020 06:38:31 -0700 To: Matan Azrad , Bill Zhou , "wenzhuo.lu@intel.com" , "jingjing.wu@intel.com" , "bernard.iremonger@intel.com" , Ori Kam , "john.mcnamara@intel.com" , "marko.kovacevic@intel.com" Cc: "dev@dpdk.org" References: <20200424105511.13147-1-dongz@mellanox.com> <20200430155345.1384-1-dongz@mellanox.com> <5fc8bc00-e4b1-b2c4-1caf-03dea0456f4c@intel.com> From: Ferruh Yigit Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJsBBMBCgBWAhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEABQkKqZZ8FiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl6ha3sXGHZrczovL2tl eXMub3BlbnBncC5vcmcACgkQ+TPrQ98TYR8uLA//QwltuFliUWe60xwmu9sY38c1DXvX67wk UryQ1WijVdIoj4H8cf/s2KtyIBjc89R254KMEfJDao/LrXqJ69KyGKXFhFPlF3VmFLsN4XiT PSfxkx8s6kHVaB3O183p4xAqnnl/ql8nJ5ph9HuwdL8CyO5/7dC/MjZ/mc4NGq5O9zk3YRGO lvdZAp5HW9VKW4iynvy7rl3tKyEqaAE62MbGyfJDH3C/nV/4+mPc8Av5rRH2hV+DBQourwuC ci6noiDP6GCNQqTh1FHYvXaN4GPMHD9DX6LtT8Fc5mL/V9i9kEVikPohlI0WJqhE+vQHFzR2 1q5nznE+pweYsBi3LXIMYpmha9oJh03dJOdKAEhkfBr6n8BWkWQMMiwfdzg20JX0o7a/iF8H 4dshBs+dXdIKzPfJhMjHxLDFNPNH8zRQkB02JceY9ESEah3wAbzTwz+e/9qQ5OyDTQjKkVOo cxC2U7CqeNt0JZi0tmuzIWrfxjAUulVhBmnceqyMOzGpSCQIkvalb6+eXsC9V1DZ4zsHZ2Mx Hi+7pCksdraXUhKdg5bOVCt8XFmx1MX4AoV3GWy6mZ4eMMvJN2hjXcrreQgG25BdCdcxKgqp e9cMbCtF+RZax8U6LkAWueJJ1QXrav1Jk5SnG8/5xANQoBQKGz+yFiWcgEs9Tpxth15o2v59 gXK5Ag0EV9ZMvgEQAKc0Db17xNqtSwEvmfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ES YpV8QWj0xK4YM0dLxnDU2IYxjEshSB1TqAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4Ai bPtrHuIXWQOBECcVZTTOdZYGAzaYzxiAONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxD UQljeNvKYt1lZE/gAUUxNLWsYyTT+22/vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/ 3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35piVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVj sM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQI3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdc q9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYHfVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH7 1PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZqw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFB VOQOxCvwRG2QCgcJ/UTn5vlivul+cThi6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI 8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJlRr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYC GwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNhHwUCXqFrngUJCKxSYAAKCRD5M+tD3xNhH3YWD/9b cUiWaHJasX+OpiuZ1Li5GG3m9aw4lR/k2lET0UPRer2Jy1JsL+uqzdkxGvPqzFTBXgx/6Byz EMa2mt6R9BCyR286s3lxVS5Bgr5JGB3EkpPcoJT3A7QOYMV95jBiiJTy78Qdzi5LrIu4tW6H o0MWUjpjdbR01cnj6EagKrDx9kAsqQTfvz4ff5JIFyKSKEHQMaz1YGHyCWhsTwqONhs0G7V2 0taQS1bGiaWND0dIBJ/u0pU998XZhmMzn765H+/MqXsyDXwoHv1rcaX/kcZIcN3sLUVcbdxA WHXOktGTQemQfEpCNuf2jeeJlp8sHmAQmV3dLS1R49h0q7hH4qOPEIvXjQebJGs5W7s2vxbA 5u5nLujmMkkfg1XHsds0u7Zdp2n200VC4GQf8vsUp6CSMgjedHeF9zKv1W4lYXpHp576ZV7T GgsEsvveAE1xvHnpV9d7ZehPuZfYlP4qgo2iutA1c0AXZLn5LPcDBgZ+KQZTzm05RU1gkx7n gL9CdTzVrYFy7Y5R+TrE9HFUnsaXaGsJwOB/emByGPQEKrupz8CZFi9pkqPuAPwjN6Wonokv ChAewHXPUadcJmCTj78Oeg9uXR6yjpxyFjx3vdijQIYgi5TEGpeTQBymLANOYxYWYOjXk+ae dYuOYKR9nbPv+2zK9pwwQ2NXbUBystaGyQ== Message-ID: <4fb5f2ba-a3c1-4d39-f121-b197ab6bc589@intel.com> Date: Fri, 1 May 2020 14:38:31 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: support flow aging 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 5/1/2020 1:45 PM, Matan Azrad wrote: > > > From: Ferruh Yigit: >> On 5/1/2020 12:28 PM, Matan Azrad wrote: >>> >>> Hi Ferruh >>> >>> From: Ferruh Yigit: >>>> On 5/1/2020 7:51 AM, Matan Azrad wrote: >>>>> Hi Ferruh >>>>> >>>>> From: Ferruh Yigit >>>>>> On 4/30/2020 4:53 PM, Bill Zhou wrote: >>>>>>> Currently, there is no way to check the aging event or to get the >>>>>>> current aged flows in testpmd, this patch include those >>>>>>> implements, it's >>>>>> included: >>>>>>> - Registering aging event when the testpmd application start, add new >>>>>>> command to control if the event expose to the applications. If it's be >>>>>>> set, when new flow be checked age out, there will be one-line >>>>>>> output >>>> log. >>>>>>> - Add new command to list all aged flows, meanwhile, we can set >>>>>> parameter >>>>>>> to destroy it. >>>>>>> >>>>>>> Signed-off-by: Bill Zhou >>>>>>> --- >>>>>>> v2: Update the way of registering aging event, add new command to >>>>>>> control if the event need be print or not. Update the output of >>>>>>> the delete aged flow command format. >>>>>> >>>>>> <...> >>>>>> >>>>>>> @@ -19387,6 +19394,44 @@ cmdline_parse_inst_t >>>> cmd_showport_macs = >>>>>> { >>>>>>> }, >>>>>>> }; >>>>>>> >>>>>>> +/* Enable/Disable flow aging event log */ struct >>>>>>> +cmd_set_aged_flow_event_log_result { >>>>>>> + cmdline_fixed_string_t set; >>>>>>> + cmdline_fixed_string_t keyword; >>>>>>> + cmdline_fixed_string_t enable; >>>>>>> +}; >>>>>>> +cmdline_parse_token_string_t cmd_set_aged_flow_event_log_set >> = >>>>>>> + TOKEN_STRING_INITIALIZER(struct >>>>>> cmd_set_aged_flow_event_log_result, >>>>>>> + set, "set"); >>>>>>> +cmdline_parse_token_string_t >>>> cmd_set_aged_flow_event_log_keyword >>>>>> = >>>>>>> + TOKEN_STRING_INITIALIZER(struct >>>>>> cmd_set_aged_flow_event_log_result, >>>>>>> + keyword, "aged_flow_event_log"); >> cmdline_parse_token_string_t >>>> cmd_set_aged_flow_event_log_enable = >>>>>>> + TOKEN_STRING_INITIALIZER(struct >>>>>> cmd_set_aged_flow_event_log_result, >>>>>>> + enable, "on#off"); >>>>>>> + >>>>>>> +static void >>>>>>> +cmd_set_aged_flow_event_log_parsed(void *parsed_result, >>>>>>> + __rte_unused struct cmdline *cl, >>>>>>> + __rte_unused void *data) >>>>>>> +{ >>>>>>> + struct cmd_set_aged_flow_event_log_result *res = >> parsed_result; >>>>>>> + int enable = (strcmp(res->enable, "on") == 0) ? 1 : 0; >>>>>>> + update_aging_event_log_status(enable); >>>>>>> +} >>>>>>> + >>>>>>> +cmdline_parse_inst_t cmd_set_aged_flow_event_log = { >>>>>>> + .f = cmd_set_aged_flow_event_log_parsed, >>>>>>> + .data = NULL, >>>>>>> + .help_str = "set aged_flow_event_log on|off", >>>>>> >>>>>> What do you think "set aged_flow_verbose on|off" to be more similar >>>>>> to existing verbose command? >>>>> >>>>> Please see my comments below regard it. >>>>> >>>>>> <...> >>>>>> >>>>>>> --- a/app/test-pmd/testpmd.c >>>>>>> +++ b/app/test-pmd/testpmd.c >>>>>>> @@ -3068,6 +3068,21 @@ rmv_port_callback(void *arg) >>>>>>> start_packet_forwarding(0); >>>>>>> } >>>>>>> >>>>>>> +static int aged_flow_event_enable; >>>>>> >>>>>> Other global config values are at the beginning of the file, with a >>>>>> comment on them, can you do same with variable? >>>>> >>>>> +1 >>>>> >>>>>>> +void update_aging_event_log_status(int enable) { >>>>>>> + aged_flow_event_enable = enable; } >>>>>>> + >>>>>>> +int aging_event_output(uint16_t portid) >>>>>> >>>>>> This can be a 'static' function. >>>>> >>>>> +1 >>>>> >>>>>>> +{ >>>>>>> + if (aged_flow_event_enable) { >>>>>>> + printf("port %u RTE_ETH_EVENT_FLOW_AGED >> triggered\n", >>>>>> portid); >>>>>>> + fflush(stdout); >>>>>>> + } >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> /* This function is used by the interrupt thread */ static int >>>>>>> eth_event_callback(portid_t port_id, enum rte_eth_event_type >> type, >>>>>>> void *param, @@ -3098,6 +3113,8 @@ eth_event_callback(portid_t >>>>>> port_id, enum rte_eth_event_type type, void *param, >>>>>>> rmv_port_callback, (void >>>>>> *)(intptr_t)port_id)) >>>>>>> fprintf(stderr, "Could not set up deferred device >>>>>> removal\n"); >>>>>>> break; >>>>>>> + case RTE_ETH_EVENT_FLOW_AGED: >>>>>>> + aging_event_output(port_id); >>>>>> >>>>>> Can't this provide more information than port_id, like flow id etc, >>>>>> what event_process function provides? can we print it too? >>>>>> And what is the intended usage in real application, when flow aged >>>>>> event occurred, should application destroy the flow? So it should >>>>>> know the flow, right? >>>>> >>>>> Probably Yes Ferruh, I will add details, maybe it will be clearer: >>>>> >>>>> As described well in rte_flow_get_aged_flows API description, there >>>>> are 2 >>>> suggested options for the application: >>>>> >>>>> 1. To call rte_flow_get_aged_flows from the AGED event callback in >>>>> order >>>> to get the aged flow contexts of the triggered port. >>>>> 2. To call rte_flow_get_aged_flows in any time application wants. >>>> >>>> I see, for the testpmd implementation what do you think getting the >>>> aged flow list and print them in event callback, this can be good as >>>> sample of the intended usage? >>> >>> Yes, we thought on it and I understand you, The issue with it is that >>> we need to synchronize all the flow management in Testpmd and to >> protect any flow operation with a lock. >>> Because the callback is probably coming from the host thread while other >> flow operations from different Testpmd thread(command line thread). >>> >>> It will do the patch very intrusive. >>> >>> The current approach(like the second suggestion) moves the >> synchronization to the testpmd user to access flows only from the command >> line thread while hinting to the user when a port holds some aged-out flows. >>> I think it is better to stay the implementation simple. >> >> OK >> >>> >>>>> >>>>> It is probably depends in the way the application wants to >>>>> synchronize flow >>>> APIs calls. >>>>> >>>>> The application just gets the information of the aged-out flows >>>>> context >>>> from the above API and can do any flow operation for it, and yes, the >>>> most expected case is to destroy the flows. >>>>> >>>>> Bill added 2 testpmd commands: >>>>> >>>>> 1. To use rte_flow_get_aged_flows and to print a list of all the >>>>> aged-out >>>> flows (with option to destroy them directly by the command). >>>>> 2. A Boolean to indicate the application whether to notify the >>>>> testpmd user >>>> about new aged-out flows(by print). >>>>> >>>>> By this way, the testpmd user can use the 2 approaches with minimum >>>> testpmd flow management change. >>>>> >>>>> So, the Boolean var is more like "trigger testpmd user >>>>> notification", not like >>>> regular verbose options. >>>> >>>> As you already know, event always registered and callback always >>>> called, this command only defines to print a log in the callback or >>>> not, so I believe 'verbose' suits here, main concern is there are >>>> many testpmd commands and it is hard to remember them (usage is not >>>> intuitive, I had need to check source code to find a command many >>>> times), trying to make them as consistent as possible to help usage. >>>> >>>> But I think as see your concern, "set aged_flow_verbose on|off" >>>> command can be confused as if changing "flow aged #" command >> verbosity level. >>>> >>>> What do you think about a more generic "set event_verbose on|off" >>>> command, which can control to logging for all event handlers, but >>>> right now only for aged events? >>> >>> Looks good to me, but maybe instead of on | off it is better to use bitmap >> in order to indicate the event? >> >> No objection but not sure that level fine grain needed now, or later, why not >> add the basic on/off now and add the bitmap when we need to control for >> each event. > > It will be good to the user to reduce logs of non-interested events (maybe even more often) which makes his log bigger. Right now, is there any other event that logs? And how much log do they produce? > > >> >>> >>> >>>>> >>>>> Matan >>>>> >>>>> >>>>> >>>>> >>>>>> >>>>>> <...> >>>>>> >>>>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>>>>> @@ -1062,6 +1062,21 @@ Where: >>>>>>> >>>>>>> Check the NIC Datasheet for hardware limits. >>>>>>> >>>>>>> +aged flow event log set >>>>>>> +~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>> + >>>>>>> +Currently, the flow aging event is registered when the testpmd >>>>>>> +application start, if new flow be checked age out, there will be >>>>>>> +one output log for it. But some applications do not interest this >>>>>>> +event, use this >>>>>> command can set if the event expose to the applications:: >>>>>>> + >>>>>>> + testpmd> set aged_flow_event_log (on|off) >>>>>>> + >>>>>>> +For examine:: >>>>>>> + >>>>>>> + testpmd> set aged_flow_event_log on >>>>>>> + testpmd> port 0 RTE_ETH_EVENT_FLOW_AGED triggered >>>>>>> + testpmd> set aged_flow_event_log off >>>>>>> + >>>>>> >>>>>> This can be moved below the "set verbose" command since they are in >>>>>> similar group. >>> >