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

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

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.

> 
>  
>>>
>>> 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.
>