Re: [dpdk-dev] [PATCH v2] app/testpmd: support flow aging

[Ferruh Yigit <ferruh.yigit@intel.com>]

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 <dongz@mellanox.com>
>>>>>>> ---
>>>>>>> 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.
>>>
>